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

src/hydrogen.cc

 // Copyright 2013 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 5444 matching lines @@
 }
 
 
 HInstruction* HOptimizedGraphBuilder::BuildLoadKeyedGeneric(HValue* object,
                                                             HValue* key) {
   HValue* context = environment()->LookupContext();
   return new(zone()) HLoadKeyedGeneric(context, object, key);
 }
 
 
+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,
     HValue* val,
     HValue* dependency,
     Handle<Map> map,
     bool is_store,
     KeyedAccessStoreMode store_mode) {
   HCheckMaps* mapcheck = HCheckMaps::New(
       object, map, zone(), top_info(), dependency);
   AddInstruction(mapcheck);
   if (dependency) {
     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);
 }
 
 
 HInstruction* HOptimizedGraphBuilder::TryBuildConsolidatedElementLoad(
     HValue* object,
     HValue* key,
     HValue* val,
     SmallMapList* maps) {
   // For polymorphic loads of similar elements kinds (i.e. all tagged or all
   // double), always use the "worst case" code without a transition.  This is
@@ skipping 140 matching lines @@
     HBasicBlock* this_map = graph()->CreateBasicBlock();
     HBasicBlock* other_map = graph()->CreateBasicBlock();
     HCompareMap* mapcompare =
         new(zone()) HCompareMap(object, map, this_map, other_map);
     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)) {
-        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));
       } else {
         access = AddInstruction(BuildLoadKeyedGeneric(object, key));
       }
     } else {
       ASSERT(IsExternalArrayElementsKind(elements_kind));

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

AFAICS BuildUncheckedMonomorphicElementAccess can handle external elements too.
So let's remove some more duplication :-)

if (IsDictionaryElementsKind(elements_kind)) {
  if (is_store) access = ... else ...
} else {
  ASSERT(IsFastElementsKind(...) || IsExternalArrayElementsKind(...));
  access = BuildUncheckedMonomorphicElementAccess(...);
}

       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);
     }
     *has_side_effects |= access->HasObservableSideEffects();
     // The caller will use has_side_effects and add a correct Simulate.
@@ skipping 4223 matching lines @@
   if (ShouldProduceTraceOutput()) {
     isolate()->GetHTracer()->TraceHydrogen(name(), graph_);
   }
 
 #ifdef DEBUG
   graph_->Verify(false);  // No full verify.
 #endif
 }
 
 } }  // namespace v8::internal