Crankshaft support for polymorphic array handling

src/hydrogen.cc

 // Copyright 2011 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 3431 matching lines @@
       instr = BuildStoreNamedGeneric(object, name, value);
     }
 
   } else {
     // Keyed store.
     CHECK_ALIVE(VisitForValue(prop->key()));
     CHECK_ALIVE(VisitForValue(expr->value()));
     value = Pop();
     HValue* key = Pop();
     HValue* object = Pop();
-    instr = BuildStoreKeyed(object, key, value, expr);
+    bool has_side_effects = false;
+    HandleKeyedElementAccess(object, key, value, expr, expr->AssignmentId(),
+                             expr->position(),
+                             true,  // is_store
+                             &has_side_effects);
+    Push(value);
+    if (has_side_effects) AddSimulate(expr->AssignmentId());
+    ast_context()->ReturnValue(Pop());
+    return;
   }
   Push(value);
   instr->set_position(expr->position());
   AddInstruction(instr);
   if (instr->HasSideEffects()) AddSimulate(expr->AssignmentId());
   ast_context()->ReturnValue(Pop());
 }
 
 
 // Because not every expression has a position and there is not common
@@ skipping 103 matching lines @@
       if (store->HasSideEffects()) AddSimulate(expr->AssignmentId());
       ast_context()->ReturnValue(Pop());
 
     } else {
       // Keyed property.
       CHECK_ALIVE(VisitForValue(prop->obj()));
       CHECK_ALIVE(VisitForValue(prop->key()));
       HValue* obj = environment()->ExpressionStackAt(1);
       HValue* key = environment()->ExpressionStackAt(0);
 
-      HInstruction* load = BuildLoadKeyed(obj, key, prop);
-      PushAndAdd(load);
-      if (load->HasSideEffects()) AddSimulate(expr->CompoundLoadId());
+      bool has_side_effects = false;
+      HValue* load = HandleKeyedElementAccess(
+          obj, key, NULL, prop, expr->CompoundLoadId(), -1,
+          false,  // is_store
+          &has_side_effects);
+      Push(load);
+      if (has_side_effects) AddSimulate(expr->CompoundLoadId());
+
 
       CHECK_ALIVE(VisitForValue(expr->value()));
       HValue* right = Pop();
       HValue* left = Pop();
 
       HInstruction* instr = BuildBinaryOperation(operation, left, right);
       PushAndAdd(instr);
       if (instr->HasSideEffects()) AddSimulate(operation->id());
 
       expr->RecordTypeFeedback(oracle());
-      HInstruction* store = BuildStoreKeyed(obj, key, instr, expr);
-      AddInstruction(store);
+      HandleKeyedElementAccess(obj, key, instr, expr, expr->AssignmentId(), -1,
+                               true,  // is_store
+                               &has_side_effects);
+
       // Drop the simulated receiver, key, and value.  Return the value.
       Drop(3);
       Push(instr);
-      if (store->HasSideEffects()) AddSimulate(expr->AssignmentId());
+      if (has_side_effects) AddSimulate(expr->AssignmentId());
       ast_context()->ReturnValue(Pop());
     }
 
   } else {
     return Bailout("invalid lhs in compound assignment");
   }
 }
 
 
 void HGraphBuilder::VisitAssignment(Assignment* expr) {
@@ skipping 143 matching lines @@
 }
 
 
 HInstruction* HGraphBuilder::BuildLoadKeyedGeneric(HValue* object,
                                                    HValue* key) {
   HValue* context = environment()->LookupContext();
   return new(zone()) HLoadKeyedGeneric(context, object, key);
 }
 
 
-HInstruction* HGraphBuilder::BuildLoadKeyedFastElement(HValue* object,
-                                                       HValue* key,
-                                                       Property* expr) {
-  ASSERT(!expr->key()->IsPropertyName() && expr->IsMonomorphic());
+HInstruction* HGraphBuilder::BuildMonomorphicElementAccess(HValue* object,
+                                                           HValue* key,
+                                                           HValue* val,
+                                                           Expression* expr,
+                                                           bool is_store) {
+  ASSERT(expr->IsMonomorphic());
+  Handle<Map> map = expr->GetMonomorphicReceiverType();
+  if (!map->has_fast_elements() && !map->has_external_array_elements()) {
+    if (is_store) {

[fschneider, 2011/06/16 13:21:28]

maybe shorten to:

return is_store ? BuildStoreKeyedGeneric(object, key, val) :
BuildLoadKeyedGeneric(object, key);

[Jakob Kummerow, 2011/06/17 08:06:09]

Done.

+      return BuildStoreKeyedGeneric(object, key, val);
+    } else {
+      return BuildLoadKeyedGeneric(object, key);
+    }
+  }
   AddInstruction(new(zone()) HCheckNonSmi(object));
-  Handle<Map> map = expr->GetMonomorphicReceiverType();
-  ASSERT(map->has_fast_elements());
   AddInstruction(new(zone()) HCheckMap(object, map));
-  bool is_array = (map->instance_type() == JS_ARRAY_TYPE);
-  HLoadElements* elements = new(zone()) HLoadElements(object);
+  HInstruction* elements = new(zone()) HLoadElements(object);
   HInstruction* length = NULL;
   HInstruction* checked_key = NULL;
-  if (is_array) {
+  if (map->has_external_array_elements()) {
+    AddInstruction(elements);
+    length = AddInstruction(new(zone()) HExternalArrayLength(elements));
+    checked_key = AddInstruction(new(zone()) HBoundsCheck(key, length));
+    HLoadExternalArrayPointer* external_elements =
+        new(zone()) HLoadExternalArrayPointer(elements);
+    AddInstruction(external_elements);
+    JSObject::ElementsKind elements_kind = map->elements_kind();
+    if (is_store) {
+      ASSERT(val != NULL);
+      switch (elements_kind) {
+        case JSObject::EXTERNAL_PIXEL_ELEMENTS: {
+          HClampToUint8* clamp = new(zone()) HClampToUint8(val);
+          AddInstruction(clamp);
+          val = clamp;
+          break;
+        }
+        case JSObject::EXTERNAL_BYTE_ELEMENTS:
+        case JSObject::EXTERNAL_UNSIGNED_BYTE_ELEMENTS:
+        case JSObject::EXTERNAL_SHORT_ELEMENTS:
+        case JSObject::EXTERNAL_UNSIGNED_SHORT_ELEMENTS:
+        case JSObject::EXTERNAL_INT_ELEMENTS:
+        case JSObject::EXTERNAL_UNSIGNED_INT_ELEMENTS: {
+          HToInt32* floor_val = new(zone()) HToInt32(val);
+          AddInstruction(floor_val);
+          val = floor_val;
+          break;
+        }
+        case JSObject::EXTERNAL_FLOAT_ELEMENTS:
+        case JSObject::EXTERNAL_DOUBLE_ELEMENTS:
+          break;
+        case JSObject::FAST_ELEMENTS:
+        case JSObject::FAST_DOUBLE_ELEMENTS:
+        case JSObject::DICTIONARY_ELEMENTS:
+          UNREACHABLE();
+          break;
+      }
+      return new(zone()) HStoreKeyedSpecializedArrayElement(
+          external_elements, checked_key, val, elements_kind);
+    } else {
+      return new(zone()) HLoadKeyedSpecializedArrayElement(
+          external_elements, checked_key, elements_kind);
+    }
+  }
+  ASSERT(map->has_fast_elements());
+  if (map->instance_type() == JS_ARRAY_TYPE) {
     length = AddInstruction(new(zone()) HJSArrayLength(object));
     checked_key = AddInstruction(new(zone()) HBoundsCheck(key, length));
     AddInstruction(elements);
   } else {
     AddInstruction(elements);
     length = AddInstruction(new(zone()) HFixedArrayLength(elements));
     checked_key = AddInstruction(new(zone()) HBoundsCheck(key, length));
   }
-  return new(zone()) HLoadKeyedFastElement(elements, checked_key);
-}
-
-
-HInstruction* HGraphBuilder::BuildLoadKeyedSpecializedArrayElement(
-    HValue* object,
-    HValue* key,
-    Property* expr) {
-  ASSERT(!expr->key()->IsPropertyName() && expr->IsMonomorphic());
+  if (is_store) {
+    return new(zone()) HStoreKeyedFastElement(elements, checked_key, val);
+  } else {
+    return new(zone()) HLoadKeyedFastElement(elements, checked_key);
+  }
+}
+
+
+HValue* HGraphBuilder::HandlePolymorphicElementAccess(HValue* object,
+                                                   HValue* key,

[fschneider, 2011/06/16 13:21:28]

Align parameters in it fits 80 chars or else newline before the first
parameters.

[Jakob Kummerow, 2011/06/17 08:06:09]

Done.

+                                                   HValue* val,
+                                                   Expression* prop,
+                                                   int ast_id,
+                                                   int position,
+                                                   bool is_store,
+                                                   bool* has_side_effects) {
+  *has_side_effects = false;
   AddInstruction(new(zone()) HCheckNonSmi(object));
-  Handle<Map> map = expr->GetMonomorphicReceiverType();
-  ASSERT(!map->has_fast_elements());
-  ASSERT(map->has_external_array_elements());
-  AddInstruction(new(zone()) HCheckMap(object, map));
-  HLoadElements* elements = new(zone()) HLoadElements(object);
-  AddInstruction(elements);
-  HInstruction* length = new(zone()) HExternalArrayLength(elements);
-  AddInstruction(length);
-  HInstruction* checked_key =
-      AddInstruction(new(zone()) HBoundsCheck(key, length));
-  HLoadExternalArrayPointer* external_elements =
-      new(zone()) HLoadExternalArrayPointer(elements);
-  AddInstruction(external_elements);
-  HLoadKeyedSpecializedArrayElement* pixel_array_value =
-      new(zone()) HLoadKeyedSpecializedArrayElement(
-          external_elements, checked_key, map->elements_kind());
-  return pixel_array_value;
-}
-
-
-HInstruction* HGraphBuilder::BuildLoadKeyed(HValue* obj,
-                                            HValue* key,
-                                            Property* prop) {
-  if (prop->IsMonomorphic()) {
-    Handle<Map> receiver_type(prop->GetMonomorphicReceiverType());
-    // An object has either fast elements or pixel array elements, but never
-    // both. Pixel array maps that are assigned to pixel array elements are
-    // always created with the fast elements flag cleared.
-    if (receiver_type->has_external_array_elements()) {
-      return BuildLoadKeyedSpecializedArrayElement(obj, key, prop);
-    } else if (receiver_type->has_fast_elements()) {
-      return BuildLoadKeyedFastElement(obj, key, prop);
-    }
-  }
-  return BuildLoadKeyedGeneric(obj, key);
+  AddInstruction(HCheckInstanceType::NewIsSpecObject(object));
+  ZoneMapList* maps = prop->GetReceiverTypes();
+  bool todo_external_array = false;
+
+  static const int kNumElementTypes = JSObject::kElementsKindCount;
+  bool type_todo[kNumElementTypes];
+  for (int i = 0; i < kNumElementTypes; ++i) {
+    type_todo[i] = false;
+  }
+
+  for (int i = 0; i < maps->length(); ++i) {
+    ASSERT(maps->at(i)->IsMap());
+    type_todo[maps->at(i)->elements_kind()] = true;
+    if (maps->at(i)->elements_kind()
+        >= JSObject::FIRST_EXTERNAL_ARRAY_ELEMENTS_KIND) {
+      todo_external_array = true;
+    }
+  }
+  // We can't treat dictionary elements here (need to deopt instead).
+  type_todo[JSObject::DICTIONARY_ELEMENTS] = false;
+
+  HBasicBlock* join = graph()->CreateBasicBlock();
+
+  HInstruction* elements_kind_instr =
+      AddInstruction(new(zone()) HElementsKind(object));
+  HInstruction* elements = NULL;
+  HLoadExternalArrayPointer* external_elements = NULL;
+  HInstruction* checked_key = NULL;
+
+  // FAST_ELEMENTS is assumed to be the first case.
+  STATIC_ASSERT(JSObject::FAST_ELEMENTS == 0);
+
+  for (JSObject::ElementsKind elements_kind = JSObject::FAST_ELEMENTS;
+       elements_kind <= JSObject::LAST_ELEMENTS_KIND;
+       elements_kind = JSObject::ElementsKind(elements_kind + 1)) {

[fschneider, 2011/06/16 13:21:28]

Too bad that ++ doesn't work ;)

[Jakob Kummerow, 2011/06/17 08:06:09]

Yeah...

+    // After having handled FAST_ELEMENTS in the first run of the loop, we
+    // need to add some code that's executed for all other cases.
+    if (elements_kind == 1 && todo_external_array) {

[fschneider, 2011/06/16 13:21:28]

Maybe write 

if (elements_kind > JSObject::FAST_ELEMENTS && todo_external_array)

[Jakob Kummerow, 2011/06/17 08:06:09]

No, because I only want to execute this if-block once, after FAST_ELEMENTS have
been handled and before all the different EXTERNAL_*_ELEMENTS are handled.
What I could do is "if (elements_kind == FAST_ELEMENTS + 1 && ...)"; or set
todo_external_array=false inside the if-block, but I don't think that'd be more
intuitive.

+      elements = AddInstruction(new(zone()) HLoadElements(object));
+      // We need to forcibly prevent some ElementsKind-dependent instructions
+      // from being hoisted out of any loops they might occur in, because
+      // the current loop-invariant-code-motion algorithm isn't clever enough
+      // to deal with them properly.
+      // There's some performance to be gained by developing a smarter
+      // solution for this.
+      elements->ClearFlag(HValue::kUseGVN);
+      HInstruction* length =
+          AddInstruction(new(zone()) HExternalArrayLength(elements));
+      checked_key = AddInstruction(new(zone()) HBoundsCheck(key, length));
+      external_elements = new(zone()) HLoadExternalArrayPointer(elements);
+      AddInstruction(external_elements);
+    }
+    if (type_todo[elements_kind]) {
+      HBasicBlock* if_true = graph()->CreateBasicBlock();
+      HBasicBlock* if_false = graph()->CreateBasicBlock();
+      HCompareWithConstant* compare = new(zone()) HCompareWithConstant(
+          elements_kind_instr,
+          elements_kind,
+          Token::EQ);
+      AddInstruction(compare);
+      HTest* branch = new(zone()) HTest(compare, if_true, if_false);
+      current_block()->Finish(branch);
+
+      set_current_block(if_true);
+      HInstruction* access;
+      if (elements_kind == JSObject::FAST_ELEMENTS) {
+        HBasicBlock* if_jsarray = graph()->CreateBasicBlock();
+        HBasicBlock* if_fastobject = graph()->CreateBasicBlock();
+        HInstruction* typecheck =
+            AddInstruction(new(zone()) HHasInstanceType(object, JS_ARRAY_TYPE));
+        HTest* test = new(zone()) HTest(typecheck, if_jsarray, if_fastobject);
+        current_block()->Finish(test);
+
+        set_current_block(if_jsarray);
+        HInstruction* length = new(zone()) HJSArrayLength(object);
+        AddInstruction(length);
+        length->ClearFlag(HValue::kUseGVN);
+        checked_key = AddInstruction(new(zone()) HBoundsCheck(key, length));
+        elements = AddInstruction(new(zone()) HLoadElements(object));
+        elements->ClearFlag(HValue::kUseGVN);
+        if (is_store) {
+          access = AddInstruction(
+              new(zone()) HStoreKeyedFastElement(elements, checked_key, val));
+        } else {
+          access = AddInstruction(
+              new(zone()) HLoadKeyedFastElement(elements, checked_key));
+          Push(access);
+        }
+        *has_side_effects |= access->HasSideEffects();
+        if (position != -1) {
+          access->set_position(position);
+        }
+        if_jsarray->Goto(join);
+
+        set_current_block(if_fastobject);
+        elements = AddInstruction(new(zone()) HLoadElements(object));
+        elements->ClearFlag(HValue::kUseGVN);
+        length = AddInstruction(new(zone()) HFixedArrayLength(elements));
+        checked_key = AddInstruction(new(zone()) HBoundsCheck(key, length));
+        if (is_store) {
+          access = AddInstruction(
+              new(zone()) HStoreKeyedFastElement(elements, checked_key, val));
+        } else {
+          access = AddInstruction(
+              new(zone()) HLoadKeyedFastElement(elements, checked_key));
+        }
+      } else {

[fschneider, 2011/06/16 13:21:28]

Do you handle FAST_DOUBLE_ELEMENTS at all? For now you could exclude then and
deopt on those like for DICTIONARY_ELEMENTS.

[Jakob Kummerow, 2011/06/17 08:06:09]

Uhm... no, I don't handle them, because Hydrogen hasn't handled them before this
CL. I don't think it's a problem (support for them is a work in progress
anyway), but I've put in an explicit deopt anyway, just to be safe.

+        if (is_store) {
+          ASSERT(val != NULL);
+          switch (elements_kind) {

[fschneider, 2011/06/16 13:21:28]

Could you have a helper inserting the conversion instructions since this switch
looks like a duplicate from the monomorphic load code?

[Jakob Kummerow, 2011/06/17 08:06:09]

Good idea. Done.

+            case JSObject::EXTERNAL_PIXEL_ELEMENTS: {
+              HClampToUint8* clamp = new(zone()) HClampToUint8(val);
+              AddInstruction(clamp);
+              val = clamp;
+              break;
+            }
+            case JSObject::EXTERNAL_BYTE_ELEMENTS:
+            case JSObject::EXTERNAL_UNSIGNED_BYTE_ELEMENTS:
+            case JSObject::EXTERNAL_SHORT_ELEMENTS:
+            case JSObject::EXTERNAL_UNSIGNED_SHORT_ELEMENTS:
+            case JSObject::EXTERNAL_INT_ELEMENTS:
+            case JSObject::EXTERNAL_UNSIGNED_INT_ELEMENTS: {
+              HToInt32* floor_val = new(zone()) HToInt32(val);
+              AddInstruction(floor_val);
+              val = floor_val;
+              break;
+            }
+            case JSObject::EXTERNAL_FLOAT_ELEMENTS:
+            case JSObject::EXTERNAL_DOUBLE_ELEMENTS:
+              break;
+            case JSObject::FAST_ELEMENTS:
+            case JSObject::FAST_DOUBLE_ELEMENTS:
+            case JSObject::DICTIONARY_ELEMENTS:
+              UNREACHABLE();
+              break;
+          }
+          access = AddInstruction(
+              new(zone()) HStoreKeyedSpecializedArrayElement(
+                  external_elements, checked_key, val, elements_kind));
+        } else {
+          access = AddInstruction(
+              new(zone()) HLoadKeyedSpecializedArrayElement(
+                  external_elements, checked_key, elements_kind));
+        }
+      }
+      *has_side_effects |= access->HasSideEffects();

[fschneider, 2011/06/16 13:21:28]

Isn't it always the case that (*has_side_effects == is_store)?

This would simplify the code here and in some other places.

[Jakob Kummerow, 2011/06/17 08:06:09]

That assumption does indeed seem to be true in all cases this method handles,
but it's not always true when HandleKeyedElementAccess() is called, so we can't
live without the additional method parameter.
Also, I think it makes sense to keep asking the instruction whether it has side
effects or not, instead of hardcoding an assumption here. If we add an ASSERT to
protect the assumption, the gain in simplicity would be nullified.

+      if (position != -1) {
+        access->set_position(position);
+      }
+      if (!is_store) {
+        Push(access);
+      }
+      current_block()->Goto(join);
+      set_current_block(if_false);
+    }
+  }
+
+  // Deopt if none of the cases matched.
+  current_block()->FinishExitWithDeoptimization(HDeoptimize::kNoUses);
+  join->SetJoinId(ast_id);
+  set_current_block(join);
+  if (!is_store) {

[fschneider, 2011/06/16 13:21:28]

Maybe shorten this to:

return is_store ? NULL : Pop();

[Jakob Kummerow, 2011/06/17 08:06:09]

Done.

+    return Pop();
+  }
+  return NULL;
+}
+
+
+HValue* HGraphBuilder::HandleKeyedElementAccess(HValue* obj,
+                                                HValue* key,
+                                                HValue* val,
+                                                Expression* expr,
+                                                int ast_id,
+                                                int position,
+                                                bool is_store,
+                                                bool* has_side_effects) {
+  ASSERT(!expr->IsPropertyName());
+  HInstruction* instr = NULL;
+  if (expr->IsMonomorphic()) {
+    instr = BuildMonomorphicElementAccess(obj, key, val, expr, is_store);
+  } else if (expr->GetReceiverTypes() != NULL &&
+             !expr->GetReceiverTypes()->is_empty()) {
+    return HandlePolymorphicElementAccess(
+        obj, key, val, expr, ast_id, position, is_store, has_side_effects);
+  } else {
+    if (is_store) {
+      instr = BuildStoreKeyedGeneric(obj, key, val);
+    } else {
+      instr = BuildLoadKeyedGeneric(obj, key);
+    }
+  }
+  if (position != -1) {

[fschneider, 2011/06/16 13:21:28]

Not sure, that you actually need to check for -1 (==kNoPosition) here. The
existing code just set the position in any case.

Does it work, if you just always assign a position?

[Jakob Kummerow, 2011/06/17 08:06:09]

Done. Since kNoPosition is the default value anyway, that works.

+    instr->set_position(position);
+  }
+  AddInstruction(instr);
+  *has_side_effects = instr->HasSideEffects();
+  return instr;
 }
 
 
 HInstruction* HGraphBuilder::BuildStoreKeyedGeneric(HValue* object,
                                                     HValue* key,
                                                     HValue* value) {
   HValue* context = environment()->LookupContext();
   return new(zone()) HStoreKeyedGeneric(
                          context,
                          object,
                          key,
                          value,
                          function_strict_mode());
 }
 
-
-HInstruction* HGraphBuilder::BuildStoreKeyedFastElement(HValue* object,
-                                                        HValue* key,
-                                                        HValue* val,
-                                                        Expression* expr) {
-  ASSERT(expr->IsMonomorphic());
-  AddInstruction(new(zone()) HCheckNonSmi(object));
-  Handle<Map> map = expr->GetMonomorphicReceiverType();
-  ASSERT(map->has_fast_elements());
-  AddInstruction(new(zone()) HCheckMap(object, map));
-  HInstruction* elements = AddInstruction(new(zone()) HLoadElements(object));
-  AddInstruction(new(zone()) HCheckMap(
-      elements, isolate()->factory()->fixed_array_map()));
-  bool is_array = (map->instance_type() == JS_ARRAY_TYPE);
-  HInstruction* length = NULL;
-  if (is_array) {
-    length = AddInstruction(new(zone()) HJSArrayLength(object));
-  } else {
-    length = AddInstruction(new(zone()) HFixedArrayLength(elements));
-  }
-  HInstruction* checked_key =
-      AddInstruction(new(zone()) HBoundsCheck(key, length));
-  return new(zone()) HStoreKeyedFastElement(elements, checked_key, val);
-}
-
-
-HInstruction* HGraphBuilder::BuildStoreKeyedSpecializedArrayElement(
-    HValue* object,
-    HValue* key,
-    HValue* val,
-    Expression* expr) {
-  ASSERT(expr->IsMonomorphic());
-  AddInstruction(new(zone()) HCheckNonSmi(object));
-  Handle<Map> map = expr->GetMonomorphicReceiverType();
-  ASSERT(!map->has_fast_elements());
-  ASSERT(map->has_external_array_elements());
-  AddInstruction(new(zone()) HCheckMap(object, map));
-  HLoadElements* elements = new(zone()) HLoadElements(object);
-  AddInstruction(elements);
-  HInstruction* length = AddInstruction(
-      new(zone()) HExternalArrayLength(elements));
-  HInstruction* checked_key =
-      AddInstruction(new(zone()) HBoundsCheck(key, length));
-  HLoadExternalArrayPointer* external_elements =
-      new(zone()) HLoadExternalArrayPointer(elements);
-  AddInstruction(external_elements);
-  JSObject::ElementsKind elements_kind = map->elements_kind();
-  switch (elements_kind) {
-    case JSObject::EXTERNAL_PIXEL_ELEMENTS: {
-      HClampToUint8* clamp = new(zone()) HClampToUint8(val);
-      AddInstruction(clamp);
-      val = clamp;
-      break;
-    }
-    case JSObject::EXTERNAL_BYTE_ELEMENTS:
-    case JSObject::EXTERNAL_UNSIGNED_BYTE_ELEMENTS:
-    case JSObject::EXTERNAL_SHORT_ELEMENTS:
-    case JSObject::EXTERNAL_UNSIGNED_SHORT_ELEMENTS:
-    case JSObject::EXTERNAL_INT_ELEMENTS:
-    case JSObject::EXTERNAL_UNSIGNED_INT_ELEMENTS: {
-      HToInt32* floor_val = new(zone()) HToInt32(val);
-      AddInstruction(floor_val);
-      val = floor_val;
-      break;
-    }
-    case JSObject::EXTERNAL_FLOAT_ELEMENTS:
-    case JSObject::EXTERNAL_DOUBLE_ELEMENTS:
-      break;
-
-    case JSObject::FAST_ELEMENTS:
-    case JSObject::FAST_DOUBLE_ELEMENTS:
-    case JSObject::DICTIONARY_ELEMENTS:
-      UNREACHABLE();
-      break;
-  }
-  return new(zone()) HStoreKeyedSpecializedArrayElement(
-      external_elements,
-      checked_key,
-      val,
-      map->elements_kind());
-}
-
-
-HInstruction* HGraphBuilder::BuildStoreKeyed(HValue* object,
-                                             HValue* key,
-                                             HValue* value,
-                                             Expression* expr) {
-  if (expr->IsMonomorphic()) {
-    Handle<Map> receiver_type(expr->GetMonomorphicReceiverType());
-    // An object has either fast elements or external array elements, but
-    // never both. Pixel array maps that are assigned to pixel array elements
-    // are always created with the fast elements flag cleared.
-    if (receiver_type->has_external_array_elements()) {
-      return BuildStoreKeyedSpecializedArrayElement(object,
-                                                    key,
-                                                    value,
-                                                    expr);
-    } else if (receiver_type->has_fast_elements()) {
-      return BuildStoreKeyedFastElement(object, key, value, expr);
-    }
-  }
-  return BuildStoreKeyedGeneric(object, key, value);
-}
-
-
 bool HGraphBuilder::TryArgumentsAccess(Property* expr) {
   VariableProxy* proxy = expr->obj()->AsVariableProxy();
   if (proxy == NULL) return false;
   if (!proxy->var()->IsStackAllocated()) return false;
   if (!environment()->Lookup(proxy->var())->CheckFlag(HValue::kIsArguments)) {
     return false;
   }
 
   // Our implementation of arguments (based on this stack frame or an
   // adapter below it) does not work for inlined functions.
@@ skipping 73 matching lines @@
       instr = new(zone()) HLoadNamedFieldPolymorphic(obj, types, name);
     } else {
       instr = BuildLoadNamedGeneric(obj, expr);
     }
 
   } else {
     CHECK_ALIVE(VisitForValue(expr->key()));
 
     HValue* key = Pop();
     HValue* obj = Pop();
-    instr = BuildLoadKeyed(obj, key, expr);
+
+    bool has_side_effects = false;
+    HValue* load = HandleKeyedElementAccess(
+        obj, key, NULL, expr, expr->id(), expr->position(),
+        false,  // is_store,

[fschneider, 2011/06/16 13:21:28]

This comment looks like commented out code. Maybe reformat or re-word to avoid
confusion,

[Jakob Kummerow, 2011/06/17 08:06:09]

I see your point. I've removed the comma. Is that enough? I think the best
convention for comments such as this one is to cite the parameter's name, but
that necessarily means that it looks like a variable name.

+        &has_side_effects);
+    if (has_side_effects) {
+      if (ast_context()->IsEffect()) {
+        AddSimulate(expr->id());
+      } else {
+        Push(load);
+        AddSimulate(expr->id());
+        Drop(1);
+      }
+    }
+    ast_context()->ReturnValue(load);
+    return;
   }
   instr->set_position(expr->position());
   ast_context()->ReturnInstruction(instr, expr->id());
 }
 
 
 void HGraphBuilder::AddCheckConstantFunction(Call* expr,
                                              HValue* receiver,
                                              Handle<Map> receiver_map,
                                              bool smi_and_map_check) {
@@ skipping 989 matching lines @@
 
     } else {
       // Keyed property.
       if (returns_original_input) Push(graph_->GetConstantUndefined());
 
       CHECK_ALIVE(VisitForValue(prop->obj()));
       CHECK_ALIVE(VisitForValue(prop->key()));
       HValue* obj = environment()->ExpressionStackAt(1);
       HValue* key = environment()->ExpressionStackAt(0);
 
-      HInstruction* load = BuildLoadKeyed(obj, key, prop);
-      PushAndAdd(load);
-      if (load->HasSideEffects()) AddSimulate(expr->CountId());
+      bool has_side_effects = false;
+      HValue* load = HandleKeyedElementAccess(
+          obj, key, NULL, prop, expr->CountId(), -1,
+          false,  // is_store
+          &has_side_effects);
+      Push(load);
+      if (has_side_effects) AddSimulate(expr->CountId());
 
       after = BuildIncrement(returns_original_input, expr);
       input = Pop();
 
       expr->RecordTypeFeedback(oracle());
-      HInstruction* store = BuildStoreKeyed(obj, key, after, expr);
-      AddInstruction(store);
+      HandleKeyedElementAccess(obj, key, after, expr, expr->AssignmentId(), -1,
+                               true,  // is_store
+                               &has_side_effects);

[fschneider, 2011/06/16 13:21:28]

Stores will always have a side effect.

[Jakob Kummerow, 2011/06/17 08:06:09]

Done.

 
       // Drop the key from the bailout environment.  Overwrite the receiver
       // with the result of the operation, and the placeholder with the
       // original value if necessary.
       Drop(1);
       environment()->SetExpressionStackAt(0, after);
       if (returns_original_input) environment()->SetExpressionStackAt(1, input);
-      if (store->HasSideEffects()) AddSimulate(expr->AssignmentId());
+      if (has_side_effects) AddSimulate(expr->AssignmentId());
     }
   }
 
   Drop(returns_original_input ? 2 : 1);
   ast_context()->ReturnValue(expr->is_postfix() ? input : after);
 }
 
 
 HCompareSymbolEq* HGraphBuilder::BuildSymbolCompare(HValue* left,
                                                     HValue* right,
@@ skipping 1332 matching lines @@
     }
   }
 
 #ifdef DEBUG
   if (graph_ != NULL) graph_->Verify();
   if (allocator_ != NULL) allocator_->Verify();
 #endif
 }
 
 } }  // namespace v8::internal