Add fake data dependencies (instead of disabling GVN) to fix code motion

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 3246 matching lines @@
     // If the subexpression is a literal or a simple materialized literal it
     // is already set in the cloned array.
     if (CompileTimeValue::IsCompileTimeValue(subexpr)) continue;
 
     CHECK_ALIVE(VisitForValue(subexpr));
     HValue* value = Pop();
     if (!Smi::IsValid(i)) return Bailout("Non-smi key in array literal");
 
     // Load the elements array before the first store.
     if (elements == NULL)  {
-     elements = new(zone()) HLoadElements(literal);
+     elements = new(zone()) HLoadElements(literal, literal);

[Kevin Millikin (Chromium), 2011/07/11 15:03:54]

The second occurrence of literal is used as a token to indicate that you've
discharged your obligation to check this map ("trust me, I know what I'm
doing").  I wonder if the undefined value or something is better to use here?

[Jakob Kummerow, 2011/07/19 14:55:44]

Obsolete (I undid the changes to HLoadElements).

      AddInstruction(elements);
     }
 
     HValue* key = AddInstruction(
         new(zone()) HConstant(Handle<Object>(Smi::FromInt(i)),
                               Representation::Integer32()));
     AddInstruction(new(zone()) HStoreKeyedFastElement(elements, key, value));
     AddSimulate(expr->GetIdForElement(i));
   }
   return ast_context()->ReturnValue(Pop());
@@ skipping 610 matching lines @@
                                                            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()) {
     return is_store ? BuildStoreKeyedGeneric(object, key, val)
                     : BuildLoadKeyedGeneric(object, key);
   }
   AddInstruction(new(zone()) HCheckNonSmi(object));
-  AddInstruction(new(zone()) HCheckMap(object, map));
-  HInstruction* elements = new(zone()) HLoadElements(object);
+  HInstruction* mapcheck = AddInstruction(new(zone()) HCheckMap(object, map));
+  HInstruction* elements = new(zone()) HLoadElements(object, mapcheck);
   HInstruction* length = NULL;
   HInstruction* checked_key = NULL;
   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);
     return BuildExternalArrayElementAccess(external_elements, checked_key,
                                            val, map->elements_kind(), is_store);
   }
   ASSERT(map->has_fast_elements());
   if (map->instance_type() == JS_ARRAY_TYPE) {
-    length = AddInstruction(new(zone()) HJSArrayLength(object));
+    length = AddInstruction(new(zone()) HJSArrayLength(object, mapcheck));
     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));
   }
   if (is_store) {
     return new(zone()) HStoreKeyedFastElement(elements, checked_key, val);
   } else {
@@ skipping 30 matching lines @@
       todo_external_array = true;
     }
   }
   // Support for FAST_DOUBLE_ELEMENTS isn't implemented yet, so we deopt.
   type_todo[JSObject::FAST_DOUBLE_ELEMENTS] = false;
 
   HBasicBlock* join = graph()->CreateBasicBlock();
 
   HInstruction* elements_kind_instr =
       AddInstruction(new(zone()) HElementsKind(object));
+  HCompareConstantEqAndBranch* elements_kind_branch = NULL;
   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)) {
     // After having handled FAST_ELEMENTS and DICTIONARY_ELEMENTS, we
     // need to add some code that's executed for all external array cases.
     STATIC_ASSERT(JSObject::LAST_EXTERNAL_ARRAY_ELEMENTS_KIND ==
                   JSObject::LAST_ELEMENTS_KIND);
     if (elements_kind == JSObject::FIRST_EXTERNAL_ARRAY_ELEMENTS_KIND
         && todo_external_array) {
-      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);
+      elements = AddInstruction(
+          new(zone()) HLoadElements(object, elements_kind_branch));

[Kevin Millikin (Chromium), 2011/07/11 15:03:54]

Should we assert the check instruction is not NULL here?

[Jakob Kummerow, 2011/07/19 14:55:44]

Good catch! Depending on the JS code being executed, the check instruction
actually could be NULL at this point. While investigating, I found that hoisting
prevention for HLoadElements no longer seems to be necessary, so I removed it.

       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();
-      HCompareConstantEqAndBranch* compare =
-          new(zone()) HCompareConstantEqAndBranch(elements_kind_instr,
-                                                  elements_kind,
-                                                  Token::EQ_STRICT);
-      compare->SetSuccessorAt(0, if_true);
-      compare->SetSuccessorAt(1, if_false);
-      current_block()->Finish(compare);
+      elements_kind_branch = new(zone()) HCompareConstantEqAndBranch(
+          elements_kind_instr, elements_kind, Token::EQ_STRICT);
+      elements_kind_branch->SetSuccessorAt(0, if_true);
+      elements_kind_branch->SetSuccessorAt(1, if_false);
+      current_block()->Finish(elements_kind_branch);
 
       set_current_block(if_true);
       HInstruction* access;
       if (elements_kind == JSObject::FAST_ELEMENTS) {
         HBasicBlock* if_jsarray = graph()->CreateBasicBlock();
         HBasicBlock* if_fastobject = graph()->CreateBasicBlock();
         HHasInstanceTypeAndBranch* typecheck =
             new(zone()) HHasInstanceTypeAndBranch(object, JS_ARRAY_TYPE);
         typecheck->SetSuccessorAt(0, if_jsarray);
         typecheck->SetSuccessorAt(1, if_fastobject);
         current_block()->Finish(typecheck);
 
         set_current_block(if_jsarray);
-        HInstruction* length = new(zone()) HJSArrayLength(object);
+        HInstruction* length = new(zone()) HJSArrayLength(object, typecheck);
         AddInstruction(length);
-        length->ClearFlag(HValue::kUseGVN);
         checked_key = AddInstruction(new(zone()) HBoundsCheck(key, length));
-        elements = AddInstruction(new(zone()) HLoadElements(object));
-        elements->ClearFlag(HValue::kUseGVN);
+        elements =
+            AddInstruction(new(zone()) HLoadElements(object, checked_key));
         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);
+        elements = AddInstruction(new(zone()) HLoadElements(object, typecheck));
         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 if (elements_kind == JSObject::DICTIONARY_ELEMENTS) {
@@ skipping 112 matching lines @@
   expr->RecordTypeFeedback(oracle());
 
   if (TryArgumentsAccess(expr)) return;
 
   CHECK_ALIVE(VisitForValue(expr->obj()));
 
   HInstruction* instr = NULL;
   if (expr->IsArrayLength()) {
     HValue* array = Pop();
     AddInstruction(new(zone()) HCheckNonSmi(array));
-    AddInstruction(HCheckInstanceType::NewIsJSArray(array));
-    instr = new(zone()) HJSArrayLength(array);
+    HInstruction* mapcheck =
+        AddInstruction(HCheckInstanceType::NewIsJSArray(array));
+    instr = new(zone()) HJSArrayLength(array, mapcheck);
 
   } else if (expr->IsStringLength()) {
     HValue* string = Pop();
     AddInstruction(new(zone()) HCheckNonSmi(string));
     AddInstruction(HCheckInstanceType::NewIsString(string));
     instr = new(zone()) HStringLength(string);
   } else if (expr->IsStringAccess()) {
     CHECK_ALIVE(VisitForValue(expr->key()));
     HValue* index = Pop();
     HValue* string = Pop();
@@ skipping 2471 matching lines @@
     }
   }
 
 #ifdef DEBUG
   if (graph_ != NULL) graph_->Verify();
   if (allocator_ != NULL) allocator_->Verify();
 #endif
 }
 
 } }  // namespace v8::internal