Store mode for keyed stores should be passed in from type feedback

regardless of the map used in polymorphic stores.

BUG=
R=jkummerow@chromium.org, verwaest@chromium.org

Review URL: https://codereview.chromium.org/21058003

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@16323 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/code-stubs-hydrogen.cc b/src/code-stubs-hydrogen.cc
index 9bbab9a..3e18138 100644 (file)
--- a/src/code-stubs-hydrogen.cc
+++ b/src/code-stubs-hydrogen.cc
@@ -495,7 +495,7 @@ Handle<Code> CreateAllocationSiteStub::GenerateCode() {
 template <>
 HValue* CodeStubGraphBuilder<KeyedLoadFastElementStub>::BuildCodeStub() {
   HInstruction* load = BuildUncheckedMonomorphicElementAccess(
-      GetParameter(0), GetParameter(1), NULL, NULL,
+      GetParameter(0), GetParameter(1), NULL,
       casted_stub()->is_js_array(), casted_stub()->elements_kind(),
       false, NEVER_RETURN_HOLE, STANDARD_STORE);
   return load;
@@ -540,7 +540,7 @@ Handle<Code> KeyedLoadFieldStub::GenerateCode() {
 template <>
 HValue* CodeStubGraphBuilder<KeyedStoreFastElementStub>::BuildCodeStub() {
   BuildUncheckedMonomorphicElementAccess(
-      GetParameter(0), GetParameter(1), GetParameter(2), NULL,
+      GetParameter(0), GetParameter(1), GetParameter(2),
       casted_stub()->is_js_array(), casted_stub()->elements_kind(),
       true, NEVER_RETURN_HOLE, casted_stub()->store_mode());
 
@@ -888,11 +888,11 @@ HValue* CodeStubGraphBuilder<ElementsTransitionAndStoreStub>::BuildCodeStub() {
                                 casted_stub()->to_kind(),
                                 casted_stub()->is_jsarray());
 
-    BuildUncheckedMonomorphicElementAccess(object, key, value, NULL,
-                                          casted_stub()->is_jsarray(),
-                                          casted_stub()->to_kind(),
-                                          true, ALLOW_RETURN_HOLE,
-                                          casted_stub()->store_mode());
+    BuildUncheckedMonomorphicElementAccess(object, key, value,
+                                           casted_stub()->is_jsarray(),
+                                           casted_stub()->to_kind(),
+                                           true, ALLOW_RETURN_HOLE,
+                                           casted_stub()->store_mode());
   }
 
   return value;

diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index fd4cc35..95090d5 100644 (file)
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -1219,10 +1219,9 @@ void HGraphBuilder::BuildTransitionElementsKind(HValue* object,
 
 
 HInstruction* HGraphBuilder::BuildUncheckedMonomorphicElementAccess(
-    HValue* object,
+    HValue* checked_object,
     HValue* key,
     HValue* val,
-    HCheckMaps* checked_object,
     bool is_js_array,
     ElementsKind elements_kind,
     bool is_store,
@@ -1237,14 +1236,12 @@ HInstruction* HGraphBuilder::BuildUncheckedMonomorphicElementAccess(
   // generated store code.
   if ((elements_kind == FAST_HOLEY_ELEMENTS) ||
       (elements_kind == FAST_ELEMENTS && is_store)) {
-    if (checked_object != NULL) {
-      checked_object->ClearGVNFlag(kDependsOnElementsKind);
-    }
+    checked_object->ClearGVNFlag(kDependsOnElementsKind);
   }
-  if (checked_object != NULL) object = checked_object;
+
   bool fast_smi_only_elements = IsFastSmiElementsKind(elements_kind);
   bool fast_elements = IsFastObjectElementsKind(elements_kind);
-  HValue* elements = AddLoadElements(object);
+  HValue* elements = AddLoadElements(checked_object);
   if (is_store && (fast_elements || fast_smi_only_elements) &&
       store_mode != STORE_NO_TRANSITION_HANDLE_COW) {
     HCheckMaps* check_cow_map = Add<HCheckMaps>(
@@ -1254,7 +1251,7 @@ HInstruction* HGraphBuilder::BuildUncheckedMonomorphicElementAccess(
   HInstruction* length = NULL;
   if (is_js_array) {
     length = Add<HLoadNamedField>(
-        object, HObjectAccess::ForArrayLength(elements_kind));
+        checked_object, HObjectAccess::ForArrayLength(elements_kind));
   } else {
     length = AddLoadFixedArrayLength(elements);
   }
@@ -1301,8 +1298,9 @@ HInstruction* HGraphBuilder::BuildUncheckedMonomorphicElementAccess(
 
   if (IsGrowStoreMode(store_mode)) {
     NoObservableSideEffectsScope no_effects(this);
-    elements = BuildCheckForCapacityGrow(object, elements, elements_kind,
-                                         length, key, is_js_array);
+    elements = BuildCheckForCapacityGrow(checked_object, elements,
+                                         elements_kind, length, key,
+                                         is_js_array);
     checked_key = key;
   } else {
     checked_key = Add<HBoundsCheck>(key, length);
@@ -1310,9 +1308,8 @@ HInstruction* HGraphBuilder::BuildUncheckedMonomorphicElementAccess(
     if (is_store && (fast_elements || fast_smi_only_elements)) {
       if (store_mode == STORE_NO_TRANSITION_HANDLE_COW) {
         NoObservableSideEffectsScope no_effects(this);
-
-        elements = BuildCopyElementsOnWrite(object, elements, elements_kind,
-                                            length);
+        elements = BuildCopyElementsOnWrite(checked_object, elements,
+                                            elements_kind, length);
       } else {
         HCheckMaps* check_cow_map = Add<HCheckMaps>(
             elements, isolate()->factory()->fixed_array_map(),
@@ -5501,19 +5498,7 @@ HInstruction* HOptimizedGraphBuilder::BuildLoadKeyedGeneric(HValue* object,
 }
 
 
-HInstruction* HOptimizedGraphBuilder::BuildMonomorphicElementAccess(
-    HValue* object,
-    HValue* key,
-    HValue* val,
-    HValue* dependency,
-    Handle<Map> map,
-    bool is_store,
-    KeyedAccessStoreMode store_mode) {
-  HCheckMaps* mapcheck = Add<HCheckMaps>(object, map, top_info(), dependency);
-  if (dependency) {
-    mapcheck->ClearGVNFlag(kDependsOnElementsKind);
-  }
-
+LoadKeyedHoleMode HOptimizedGraphBuilder::BuildKeyedHoleMode(Handle<Map> map) {
   // Loads from a "stock" fast holey double arrays can elide the hole check.
   LoadKeyedHoleMode load_mode = NEVER_RETURN_HOLE;
   if (*map == isolate()->get_initial_js_array_map(FAST_HOLEY_DOUBLE_ELEMENTS) &&
@@ -5525,10 +5510,30 @@ HInstruction* HOptimizedGraphBuilder::BuildMonomorphicElementAccess(
     graph()->MarkDependsOnEmptyArrayProtoElements();
   }
 
+  return load_mode;
+}
+
+
+HInstruction* HOptimizedGraphBuilder::BuildMonomorphicElementAccess(
+    HValue* object,
+    HValue* key,
+    HValue* val,
+    HValue* dependency,
+    Handle<Map> map,
+    bool is_store,
+    KeyedAccessStoreMode store_mode) {
+  HCheckMaps* checked_object = Add<HCheckMaps>(object, map, top_info(),
+                                               dependency);
+  if (dependency) {
+    checked_object->ClearGVNFlag(kDependsOnElementsKind);
+  }
+
+  LoadKeyedHoleMode load_mode = BuildKeyedHoleMode(map);
   return BuildUncheckedMonomorphicElementAccess(
-      object, key, val,
-      mapcheck, map->instance_type() == JS_ARRAY_TYPE,
-      map->elements_kind(), is_store, load_mode, store_mode);
+      checked_object, key, val,
+      map->instance_type() == JS_ARRAY_TYPE,
+      map->elements_kind(), is_store,
+      load_mode, store_mode);
 }
 
 
@@ -5582,14 +5587,14 @@ HInstruction* HOptimizedGraphBuilder::TryBuildConsolidatedElementLoad(
   }
   if (!has_double_maps && !has_smi_or_object_maps) return NULL;
 
-  HCheckMaps* check_maps = Add<HCheckMaps>(object, maps);
+  HCheckMaps* checked_object = Add<HCheckMaps>(object, maps);
   // FAST_ELEMENTS is considered more general than FAST_HOLEY_SMI_ELEMENTS.
   // If we've seen both, the consolidated load must use FAST_HOLEY_ELEMENTS.
   ElementsKind consolidated_elements_kind = has_seen_holey_elements
       ? GetHoleyElementsKind(most_general_consolidated_map->elements_kind())
       : most_general_consolidated_map->elements_kind();
   HInstruction* instr = BuildUncheckedMonomorphicElementAccess(
-      object, key, val, check_maps,
+      checked_object, key, val,
       most_general_consolidated_map->instance_type() == JS_ARRAY_TYPE,
       consolidated_elements_kind,
       false, NEVER_RETURN_HOLE, STANDARD_STORE);
@@ -5678,12 +5683,8 @@ HValue* HOptimizedGraphBuilder::HandlePolymorphicElementAccess(
     return is_store ? NULL : instr;
   }
 
-  HInstruction* checked_object =
-      AddInstruction(HCheckInstanceType::NewIsSpecObject(object, zone()));
   HBasicBlock* join = graph()->CreateBasicBlock();
 
-  HInstruction* elements = AddLoadElements(checked_object);
-
   for (int i = 0; i < untransitionable_maps.length(); ++i) {
     Handle<Map> map = untransitionable_maps[i];
     ElementsKind elements_kind = map->elements_kind();
@@ -5694,40 +5695,22 @@ HValue* HOptimizedGraphBuilder::HandlePolymorphicElementAccess(
     current_block()->Finish(mapcompare);
 
     set_current_block(this_map);
-    HInstruction* checked_key = NULL;
     HInstruction* access = NULL;
-    if (IsFastElementsKind(elements_kind)) {
-      if (is_store && !IsFastDoubleElementsKind(elements_kind)) {
-        Add<HCheckMaps>(
-            elements, isolate()->factory()->fixed_array_map(),
-            top_info(), mapcompare);
-      }
-      if (map->instance_type() == JS_ARRAY_TYPE) {
-        HInstruction* length = Add<HLoadNamedField>(
-            mapcompare, HObjectAccess::ForArrayLength(elements_kind));
-        checked_key = Add<HBoundsCheck>(key, length);
-      } else {
-        HInstruction* length = AddLoadFixedArrayLength(elements);
-        checked_key = Add<HBoundsCheck>(key, length);
-      }
-      access = AddFastElementAccess(
-          elements, checked_key, val, mapcompare,
-          elements_kind, is_store, NEVER_RETURN_HOLE, STANDARD_STORE);
-    } else if (IsDictionaryElementsKind(elements_kind)) {
-      if (is_store) {
-        access = AddInstruction(BuildStoreKeyedGeneric(object, key, val));
-      } else {
-        access = AddInstruction(BuildLoadKeyedGeneric(object, key));
-      }
+    if (IsDictionaryElementsKind(elements_kind)) {
+      access = is_store
+          ? AddInstruction(BuildStoreKeyedGeneric(object, key, val))
+          : AddInstruction(BuildLoadKeyedGeneric(object, key));
     } else {
-      ASSERT(IsExternalArrayElementsKind(elements_kind));
-      HInstruction* length = AddLoadFixedArrayLength(elements);
-      checked_key = Add<HBoundsCheck>(key, length);
-      HLoadExternalArrayPointer* external_elements =
-          Add<HLoadExternalArrayPointer>(elements);
-      access = AddExternalArrayElementAccess(
-          external_elements, checked_key, val,
-          mapcompare, elements_kind, is_store);
+      ASSERT(IsFastElementsKind(elements_kind) ||
+             IsExternalArrayElementsKind(elements_kind));
+      LoadKeyedHoleMode load_mode = BuildKeyedHoleMode(map);
+      // Happily, mapcompare is a checked object.
+      access = BuildUncheckedMonomorphicElementAccess(
+          mapcompare, key, val,
+          map->instance_type() == JS_ARRAY_TYPE,
+          elements_kind, is_store,
+          load_mode,
+          store_mode);
     }
     *has_side_effects |= access->HasObservableSideEffects();
     // The caller will use has_side_effects and add a correct Simulate.

diff --git a/src/hydrogen.h b/src/hydrogen.h
index 6128942..2b9fd96 100644 (file)
--- a/src/hydrogen.h
+++ b/src/hydrogen.h
@@ -1225,10 +1225,9 @@ class HGraphBuilder {
                                    bool is_jsarray);
 
   HInstruction* BuildUncheckedMonomorphicElementAccess(
-      HValue* object,
+      HValue* checked_object,
       HValue* key,
       HValue* val,
-      HCheckMaps* mapcheck,
       bool is_js_array,
       ElementsKind elements_kind,
       bool is_store,
@@ -1982,6 +1981,8 @@ class HOptimizedGraphBuilder V8_FINAL
                                                 HValue* val,
                                                 SmallMapList* maps);
 
+  LoadKeyedHoleMode BuildKeyedHoleMode(Handle<Map> map);
+
   HInstruction* BuildMonomorphicElementAccess(HValue* object,
                                               HValue* key,
                                               HValue* val,

diff --git a/test/mjsunit/array-store-and-grow.js b/test/mjsunit/array-store-and-grow.js
index a03a753..5ac95e9 100644 (file)
--- a/test/mjsunit/array-store-and-grow.js
+++ b/test/mjsunit/array-store-and-grow.js
@@ -187,6 +187,7 @@ array_store_1(a, 0, 0.5);
 assertEquals(0.5, a[0]);
 assertEquals(0.5, array_store_1([], 0, 0.5));
 
+
 // Verify that a grow store will deoptimize if the max gap (difference between
 // the end of an array capacity and a new index) is passed. The wrapper is to
 // make sure array_store_10 isn't inlined.
@@ -207,3 +208,49 @@ assertEquals(0.5, array_store_1([], 0, 0.5));
   %ClearFunctionTypeFeedback(grow_store);
 })();
 
+
+// Verify that a polymorphic store and grow IC when crankshafted is still
+// a grow IC (earlier it would revert to a standard store in the polymorphic
+// case).
+(function() {
+  function f(o, k, v) {
+    o[k] = v;
+  }
+
+  a = [3.5];
+  f(a, 1, "hi");  // DOUBLE packed array -> tagged packed grow
+  a = {};
+  a.p = "property";
+  a[0] = 1;
+  f(a, 0, 5.4);
+
+  %OptimizeFunctionOnNextCall(f);
+  // Should be a polymorphic grow stub. If not a grow stub it will deopt.
+  f(new Array("hi"), 1, 3);
+  assertOptimized(f);
+  %ClearFunctionTypeFeedback(f);
+})();
+
+
+// Now verify that a polymorphic store (non-growing) IC when crankshafted WILL
+// deopt if you pass an element out of bounds.
+(function() {
+  function f(o, k, v) {
+    o[k] = v;
+  }
+
+  a = [3.5];
+  f(a, 0, "hi");  // DOUBLE packed array -> tagged packed grow
+  a = {};
+  a.p = "property";
+  a[0] = 1;
+  f(a, 0, 5.4);
+
+  %OptimizeFunctionOnNextCall(f);
+  f(new Array("hi"), 0, 3);
+  assertOptimized(f);
+  // An attempt to grow should cause deopt
+  f(new Array("hi"), 1, 3);
+  assertUnoptimized(f);
+  %ClearFunctionTypeFeedback(f);
+})();