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

src/hydrogen.cc

Index: src/hydrogen.cc
diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index e34688051d53f9fa75691d6db382ddcc18795c6c..4380f96440abde1fa972ded9e524ab9288868eb4 100644
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -5462,6 +5462,21 @@ HInstruction* HOptimizedGraphBuilder::BuildLoadKeyedGeneric(HValue* object,
 }
 
 
+LoadKeyedHoleMode HOptimizedGraphBuilder::GetKeyedHoleMode(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) &&
+      isolate()->IsFastArrayConstructorPrototypeChainIntact()) {
+    Handle<JSObject> prototype(JSObject::cast(map->prototype()), isolate());
+    Handle<JSObject> object_prototype = isolate()->initial_object_prototype();
+    Add<HCheckPrototypeMaps>(prototype, object_prototype, zone(), top_info());
+    load_mode = ALLOW_RETURN_HOLE;
+    graph()->MarkDependsOnEmptyArrayProtoElements();
+  }
+  return load_mode;
+}
+
+
 HInstruction* HOptimizedGraphBuilder::BuildMonomorphicElementAccess(
     HValue* object,
     HValue* key,
@@ -5477,21 +5492,11 @@ HInstruction* HOptimizedGraphBuilder::BuildMonomorphicElementAccess(
     mapcheck->ClearGVNFlag(kDependsOnElementsKind);
   }
 
-  // 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) &&
-      isolate()->IsFastArrayConstructorPrototypeChainIntact()) {
-    Handle<JSObject> prototype(JSObject::cast(map->prototype()), isolate());
-    Handle<JSObject> object_prototype = isolate()->initial_object_prototype();
-    Add<HCheckPrototypeMaps>(prototype, object_prototype, zone(), top_info());
-    load_mode = ALLOW_RETURN_HOLE;
-    graph()->MarkDependsOnEmptyArrayProtoElements();
-  }
-
   return BuildUncheckedMonomorphicElementAccess(
       object, key, val,
       mapcheck, map->instance_type() == JS_ARRAY_TYPE,
-      map->elements_kind(), is_store, load_mode, store_mode);
+      map->elements_kind(), is_store,
+      GetKeyedHoleMode(map), store_mode);
 }
 
 
@@ -5652,22 +5657,13 @@ HValue* HOptimizedGraphBuilder::HandlePolymorphicElementAccess(
     HInstruction* checked_key = NULL;
     HInstruction* access = NULL;
     if (IsFastElementsKind(elements_kind)) {
-      if (is_store && !IsFastDoubleElementsKind(elements_kind)) {
-        AddInstruction(HCheckMaps::New(
-            elements, isolate()->factory()->fixed_array_map(),
-            zone(), top_info(), mapcompare));
-      }
-      if (map->instance_type() == JS_ARRAY_TYPE) {
-        HInstruction* length = AddLoad(
-            object, HObjectAccess::ForArrayLength(elements_kind), mapcompare);
-        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);
+      access = BuildUncheckedMonomorphicElementAccess(
+          object, key, val,
+          NULL,

[Jakob Kummerow, 2013/07/31 09:43:00]

I'm pretty sure that you need to pass |mapcompare| here. Otherwise the length
load can be hoisted past the dispatch, which can crash (when the code tries to
load a JSArray's length from an object that isn't a JSArray).

+          map->instance_type() == JS_ARRAY_TYPE,
+          elements_kind, is_store,
+          GetKeyedHoleMode(map),
+          store_mode);
     } else if (IsDictionaryElementsKind(elements_kind)) {
       if (is_store) {
         access = AddInstruction(BuildStoreKeyedGeneric(object, key, val));