Fix opmitized external array access for compound assignments

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 3196 matching lines @@
       instr = BuildStoreNamedGeneric(object, name, value);
     }
 
   } else {
     // Keyed store.
     VISIT_FOR_VALUE(prop->key());
     VISIT_FOR_VALUE(expr->value());
     value = Pop();
     HValue* key = Pop();
     HValue* object = Pop();
-
-    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()) {
-        instr = BuildStoreKeyedSpecializedArrayElement(object,
-                                                       key,
-                                                       value,
-                                                       expr);
-      } else if (receiver_type->has_fast_elements()) {
-        instr = BuildStoreKeyedFastElement(object, key, value, expr);
-      }
-    }
-    if (instr == NULL) {
-      instr = BuildStoreKeyedGeneric(object, key, value);
-    }
+    instr = BuildStoreKeyed(object,

[Mads Ager (chromium), 2011/04/06 16:45:15]

Looks like this will not fit on one line?

+                            key,
+                            value,
+                            expr);
   }
-
   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
 // superclass of Assignment and CountOperation, we cannot just pass the
@@ skipping 43 matching lines @@
       HStoreContextSlot* instr = new HStoreContextSlot(context, index, Top());
       AddInstruction(instr);
       if (instr->HasSideEffects()) AddSimulate(expr->AssignmentId());
     } else {
       BAILOUT("compound assignment to lookup slot");
     }
     ast_context()->ReturnValue(Pop());
 
   } else if (prop != NULL) {
     prop->RecordTypeFeedback(oracle());
+    expr->RecordTypeFeedback(oracle());

[Mads Ager (chromium), 2011/04/06 16:45:15]

Maybe move this to the only branch where it is needed?

 
     if (prop->key()->IsPropertyName()) {
       // Named property.
       VISIT_FOR_VALUE(prop->obj());
       HValue* obj = Top();
 
       HInstruction* load = NULL;
       if (prop->IsMonomorphic()) {
         Handle<String> name = prop->key()->AsLiteral()->AsPropertyName();
         Handle<Map> map = prop->GetReceiverTypes()->first();
@@ skipping 20 matching lines @@
       if (store->HasSideEffects()) AddSimulate(expr->AssignmentId());
       ast_context()->ReturnValue(Pop());
 
     } else {
       // Keyed property.
       VISIT_FOR_VALUE(prop->obj());
       VISIT_FOR_VALUE(prop->key());
       HValue* obj = environment()->ExpressionStackAt(1);
       HValue* key = environment()->ExpressionStackAt(0);
 
-      bool is_fast_elements = prop->IsMonomorphic() &&
-          prop->GetMonomorphicReceiverType()->has_fast_elements();
-      HInstruction* load = is_fast_elements
-          ? BuildLoadKeyedFastElement(obj, key, prop)
-          : BuildLoadKeyedGeneric(obj, key);
+      HInstruction* load = BuildLoadKeyed(obj, key, prop);
       PushAndAdd(load);
       if (load->HasSideEffects()) AddSimulate(expr->CompoundLoadId());
 
       VISIT_FOR_VALUE(expr->value());
       HValue* right = Pop();
       HValue* left = Pop();
 
       HInstruction* instr = BuildBinaryOperation(operation, left, right);
       PushAndAdd(instr);
       if (instr->HasSideEffects()) AddSimulate(operation->id());
 
-      HInstruction* store = is_fast_elements
-          ? BuildStoreKeyedFastElement(obj, key, instr, prop)
-          : BuildStoreKeyedGeneric(obj, key, instr);
+      HInstruction* store = BuildStoreKeyed(obj, key, instr, expr);
       AddInstruction(store);
       // Drop the simulated receiver, key, and value.  Return the value.
       Drop(3);
       Push(instr);
       if (store->HasSideEffects()) AddSimulate(expr->AssignmentId());
       ast_context()->ReturnValue(Pop());
     }
 
   } else {
     BAILOUT("invalid lhs in compound assignment");
@@ skipping 182 matching lines @@
   AddInstruction(elements);
   HInstruction* length = new HExternalArrayLength(elements);
   AddInstruction(length);
   AddInstruction(new HBoundsCheck(key, length));
   HLoadExternalArrayPointer* external_elements =
       new HLoadExternalArrayPointer(elements);
   AddInstruction(external_elements);
   HLoadKeyedSpecializedArrayElement* pixel_array_value =
       new HLoadKeyedSpecializedArrayElement(external_elements,
                                             key,
-                                            expr->GetExternalArrayType());
+                                            expr->external_array_type());
   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);
+}
+
+
 HInstruction* HGraphBuilder::BuildStoreKeyedGeneric(HValue* object,
                                                     HValue* key,
                                                     HValue* value) {
   HContext* context = new HContext;
   AddInstruction(context);
   return new HStoreKeyedGeneric(context, object, key, value);
 }
 
 
-HInstruction* HGraphBuilder::BuildStoreKeyedFastElement(HValue* object,
-                                                        HValue* key,
-                                                        HValue* val,
-                                                        Expression* expr) {
+HInstruction* HGraphBuilder::BuildStoreKeyedFastElement(
+    HValue* object,
+    HValue* key,
+    HValue* val,
+    Expression* expr) {
   ASSERT(expr->IsMonomorphic());
   AddInstruction(new HCheckNonSmi(object));
   Handle<Map> map = expr->GetMonomorphicReceiverType();
   ASSERT(map->has_fast_elements());
   AddInstruction(new HCheckMap(object, map));
   HInstruction* elements = AddInstruction(new HLoadElements(object));
   AddInstruction(new 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 HJSArrayLength(object));
   } else {
     length = AddInstruction(new HFixedArrayLength(elements));
   }
   AddInstruction(new HBoundsCheck(key, length));
   return new HStoreKeyedFastElement(elements, key, val);
 }
 
 
 HInstruction* HGraphBuilder::BuildStoreKeyedSpecializedArrayElement(
     HValue* object,
     HValue* key,
     HValue* val,
-    Assignment* expr) {
+    Expression* expr) {
   ASSERT(expr->IsMonomorphic());
   AddInstruction(new HCheckNonSmi(object));
   Handle<Map> map = expr->GetMonomorphicReceiverType();
   ASSERT(!map->has_fast_elements());
   ASSERT(map->has_external_array_elements());
   AddInstruction(new HCheckMap(object, map));
   HLoadElements* elements = new HLoadElements(object);
   AddInstruction(elements);
   HInstruction* length = AddInstruction(new HExternalArrayLength(elements));
   AddInstruction(new HBoundsCheck(key, length));
   HLoadExternalArrayPointer* external_elements =
       new HLoadExternalArrayPointer(elements);
   AddInstruction(external_elements);
-  return new HStoreKeyedSpecializedArrayElement(
-      external_elements,
-      key,
-      val,
-      expr->GetExternalArrayType());
+  return new HStoreKeyedSpecializedArrayElement(external_elements,
+                                                key,
+                                                val,
+                                                expr->external_array_type());
 }
 
 
+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;
   }
 
   HInstruction* result = NULL;
   if (expr->key()->IsPropertyName()) {
@@ skipping 64 matching lines @@
       instr = new HLoadNamedFieldPolymorphic(obj, types, name);
     } else {
       instr = BuildLoadNamedGeneric(obj, expr);
     }
 
   } else {
     VISIT_FOR_VALUE(expr->key());
 
     HValue* key = Pop();
     HValue* obj = Pop();
-
-    if (expr->IsMonomorphic()) {
-      Handle<Map> receiver_type(expr->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()) {
-        instr = BuildLoadKeyedSpecializedArrayElement(obj, key, expr);
-      } else if (receiver_type->has_fast_elements()) {
-        instr = BuildLoadKeyedFastElement(obj, key, expr);
-      }
-    }
-    if (instr == NULL) {
-      instr = BuildLoadKeyedGeneric(obj, key);
-    }
+    instr = BuildLoadKeyed(obj, key, expr);
   }
   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 836 matching lines @@
       HStoreContextSlot* instr = new HStoreContextSlot(context, index, after);
       AddInstruction(instr);
       if (instr->HasSideEffects()) AddSimulate(expr->AssignmentId());
     } else {
       BAILOUT("lookup variable in count operation");
     }
     Drop(has_extra ? 2 : 1);
     ast_context()->ReturnValue(expr->is_postfix() ? before : after);
 
   } else if (prop != NULL) {
-    prop->RecordTypeFeedback(oracle());
+     prop->RecordTypeFeedback(oracle());
+     expr->RecordTypeFeedback(oracle());

[Mads Ager (chromium), 2011/04/06 16:45:15]

Move to where it is needed?

 
     if (prop->key()->IsPropertyName()) {
       // Named property.
 
       // Match the full code generator stack by simulating an extra stack
       // element for postfix operations in a non-effect context.
       bool has_extra = expr->is_postfix() && !ast_context()->IsEffect();
       if (has_extra) Push(graph_->GetConstantUndefined());
 
       VISIT_FOR_VALUE(prop->obj());
@@ skipping 35 matching lines @@
       // Match the full code generator stack by simulate an extra stack element
       // for postfix operations in a non-effect context.
       bool has_extra = expr->is_postfix() && !ast_context()->IsEffect();
       if (has_extra) Push(graph_->GetConstantUndefined());
 
       VISIT_FOR_VALUE(prop->obj());
       VISIT_FOR_VALUE(prop->key());
       HValue* obj = environment()->ExpressionStackAt(1);
       HValue* key = environment()->ExpressionStackAt(0);
 
-      bool is_fast_elements = prop->IsMonomorphic() &&
-          prop->GetMonomorphicReceiverType()->has_fast_elements();
-
-      HInstruction* load = is_fast_elements
-          ? BuildLoadKeyedFastElement(obj, key, prop)
-          : BuildLoadKeyedGeneric(obj, key);
+      HInstruction* load = BuildLoadKeyed(obj, key, prop);
       PushAndAdd(load);
       if (load->HasSideEffects()) AddSimulate(increment->id());
 
       HValue* before = Pop();
       // There is no deoptimization to after the increment, so we don't need
       // to simulate the expression stack after this instruction.
       HInstruction* after = BuildIncrement(before, inc);
       AddInstruction(after);
 
-      HInstruction* store = is_fast_elements
-          ? BuildStoreKeyedFastElement(obj, key, after, prop)
-          : BuildStoreKeyedGeneric(obj, key, after);
+      HInstruction* store = BuildStoreKeyed(obj, key, after, expr);
       AddInstruction(store);
 
       // 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 (has_extra) environment()->SetExpressionStackAt(1, before);
       if (store->HasSideEffects()) AddSimulate(expr->AssignmentId());
       Drop(has_extra ? 2 : 1);
@@ skipping 1227 matching lines @@
     }
   }
 
 #ifdef DEBUG
   if (graph_ != NULL) graph_->Verify();
   if (allocator_ != NULL) allocator_->Verify();
 #endif
 }
 
 } }  // namespace v8::internal