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

src/hydrogen-instructions.h

 // 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 1650 matching lines @@
   }
 
   DECLARE_CONCRETE_INSTRUCTION(CallRuntime)
 
  private:
   const Runtime::Function* c_function_;
   Handle<String> name_;
 };
 
 
-class HJSArrayLength: public HUnaryOperation {
- public:
-  explicit HJSArrayLength(HValue* value) : HUnaryOperation(value) {
-    // The length of an array is stored as a tagged value in the array
-    // object. It is guaranteed to be 32 bit integer, but it can be
-    // represented as either a smi or heap number.
-    set_representation(Representation::Tagged());
-    SetFlag(kUseGVN);
-    SetFlag(kDependsOnArrayLengths);
-    SetFlag(kDependsOnMaps);
-  }
-
-  virtual Representation RequiredInputRepresentation(int index) const {
-    return Representation::Tagged();
-  }
-
-  DECLARE_CONCRETE_INSTRUCTION(JSArrayLength)
-
- protected:
-  virtual bool DataEquals(HValue* other) { return true; }
-};
-
-
 class HFixedArrayLength: public HUnaryOperation {
  public:
   explicit HFixedArrayLength(HValue* value) : HUnaryOperation(value) {
     set_representation(Representation::Tagged());
     SetFlag(kUseGVN);
     SetFlag(kDependsOnArrayLengths);
   }
 
   virtual Representation RequiredInputRepresentation(int index) const {
     return Representation::Tagged();
@@ skipping 137 matching lines @@
   virtual bool DataEquals(HValue* other) {
     HUnaryMathOperation* b = HUnaryMathOperation::cast(other);
     return op_ == b->op();
   }
 
  private:
   BuiltinFunctionId op_;
 };
 
 
-class HLoadElements: public HUnaryOperation {
- public:
-  explicit HLoadElements(HValue* value) : HUnaryOperation(value) {
-    set_representation(Representation::Tagged());
-    SetFlag(kUseGVN);
-    SetFlag(kDependsOnMaps);
-  }
-
-  virtual Representation RequiredInputRepresentation(int index) const {
-    return Representation::Tagged();
-  }
-
-  DECLARE_CONCRETE_INSTRUCTION(LoadElements)
-
- protected:
-  virtual bool DataEquals(HValue* other) { return true; }
-};
-
-
 class HLoadExternalArrayPointer: public HUnaryOperation {
  public:
   explicit HLoadExternalArrayPointer(HValue* value)
       : HUnaryOperation(value) {
     set_representation(Representation::External());
     // The result of this instruction is idempotent as long as its inputs don't
     // change.  The external array of a specialized array elements object cannot
     // change once set, so it's no necessary to introduce any additional
     // dependencies on top of the inputs.
     SetFlag(kUseGVN);
@@ skipping 18 matching lines @@
     SetFlag(kUseGVN);
     SetFlag(kDependsOnMaps);
   }
 
   virtual Representation RequiredInputRepresentation(int index) const {
     return Representation::Tagged();
   }
   virtual void PrintDataTo(StringStream* stream);
   virtual HType CalculateInferredType();
 
-#ifdef DEBUG
-  virtual void Verify();
-#endif
-
   Handle<Map> map() const { return map_; }
 
   DECLARE_CONCRETE_INSTRUCTION(CheckMap)
 
  protected:
   virtual bool DataEquals(HValue* other) {
     HCheckMap* b = HCheckMap::cast(other);
     return map_.is_identical_to(b->map());
   }
 
@@ skipping 47 matching lines @@
     return new HCheckInstanceType(value, IS_STRING);
   }
   static HCheckInstanceType* NewIsSymbol(HValue* value) {
     return new HCheckInstanceType(value, IS_SYMBOL);
   }
 
   virtual Representation RequiredInputRepresentation(int index) const {
     return Representation::Tagged();
   }
 
-#ifdef DEBUG
-  virtual void Verify();
-#endif
-
   virtual HValue* Canonicalize() {
     if (!value()->type().IsUninitialized() &&
         value()->type().IsString() &&
         check_ == IS_STRING) {
       return NULL;
     }
     return this;
   }
 
   bool is_interval_check() const { return check_ <= LAST_INTERVAL_CHECK; }
@@ skipping 446 matching lines @@
   HValue* arguments() { return OperandAt(0); }
   HValue* length() { return OperandAt(1); }
   HValue* index() { return OperandAt(2); }
 
   DECLARE_CONCRETE_INSTRUCTION(AccessArgumentsAt)
 
   virtual bool DataEquals(HValue* other) { return true; }
 };
 
 
+class HJSArrayLength: public HBinaryOperation {
+ public:
+  explicit HJSArrayLength(HValue* value, HValue* typecheck)

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

No need for explicit.

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

Done.

+      : HBinaryOperation(value, typecheck) {
+    // The length of an array is stored as a tagged value in the array
+    // object. It is guaranteed to be 32 bit integer, but it can be
+    // represented as either a smi or heap number.
+    set_representation(Representation::Tagged());
+    SetFlag(kUseGVN);
+    SetFlag(kDependsOnArrayLengths);
+    SetFlag(kDependsOnMaps);

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

I'm not positive we need this flag anymore, because it depends only on the map
that it's checking.

Everything should be made to work if we remove this flag.  That would enable
moving/eliminating this instruction in the case that some unrelated map has
changed.

The code as written is safe, so leave it as is and we can talk about changing it
when everyone gets back from vacation.

+  }
+
+  virtual Representation RequiredInputRepresentation(int index) const {
+    return Representation::Tagged();
+  }
+
+  HValue* value() { return OperandAt(0); }
+
+  DECLARE_CONCRETE_INSTRUCTION(JSArrayLength)
+
+ protected:
+  virtual bool DataEquals(HValue* other) { return true; }
+};
+
+
 class HBoundsCheck: public HBinaryOperation {
  public:
   HBoundsCheck(HValue* index, HValue* length)
       : HBinaryOperation(index, length) {
     set_representation(Representation::Integer32());
     SetFlag(kUseGVN);
   }
 
   virtual Representation RequiredInputRepresentation(int index) const {
     return Representation::Integer32();
   }
 
-#ifdef DEBUG
-  virtual void Verify();
-#endif
-
   HValue* index() { return left(); }
   HValue* length() { return right(); }
 
   DECLARE_CONCRETE_INSTRUCTION(BoundsCheck)
 
  protected:
   virtual bool DataEquals(HValue* other) { return true; }
 };
 
 
@@ skipping 979 matching lines @@
     return Representation::Tagged();
   }
 
   DECLARE_CONCRETE_INSTRUCTION(LoadFunctionPrototype)
 
  protected:
   virtual bool DataEquals(HValue* other) { return true; }
 };
 
 
+class HLoadElements: public HBinaryOperation {
+ public:
+  explicit HLoadElements(HValue* value, HValue* typecheck)

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

Same, no need for explicit.

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

Obsolete (I undid the changes to HLoadElements).

+      : HBinaryOperation(value, typecheck) {
+    set_representation(Representation::Tagged());
+    SetFlag(kUseGVN);
+    SetFlag(kDependsOnMaps);
+  }
+
+  virtual Representation RequiredInputRepresentation(int index) const {
+    return Representation::Tagged();
+  }
+
+  HValue* value() { return OperandAt(0); }
+
+  DECLARE_CONCRETE_INSTRUCTION(LoadElements)
+
+ protected:
+  virtual bool DataEquals(HValue* other) { return true; }
+};
+
+
 class HLoadKeyedFastElement: public HBinaryOperation {
  public:
   HLoadKeyedFastElement(HValue* obj, HValue* key) : HBinaryOperation(obj, key) {
     set_representation(Representation::Tagged());
     SetFlag(kDependsOnArrayElements);
     SetFlag(kUseGVN);
   }
 
   HValue* object() { return OperandAt(0); }
   HValue* key() { return OperandAt(1); }
@@ skipping 585 matching lines @@
 
   DECLARE_CONCRETE_INSTRUCTION(In)
 };
 
 #undef DECLARE_INSTRUCTION
 #undef DECLARE_CONCRETE_INSTRUCTION
 
 } }  // namespace v8::internal
 
 #endif  // V8_HYDROGEN_INSTRUCTIONS_H_