Consolidated all the key store/load classes in the Hydrogen and Lithium

src/hydrogen-instructions.h

Index: src/hydrogen-instructions.h
diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h
index 9e6344cd5f5e08035f65ae5e702558629090238b..5b4f4fa9962bbd07f9366c37fc7118f2e27e1b65 100644
--- a/src/hydrogen-instructions.h
+++ b/src/hydrogen-instructions.h
@@ -133,10 +133,8 @@ class LChunkBuilder;
   V(LoadFunctionPrototype)                     \
   V(LoadGlobalCell)                            \
   V(LoadGlobalGeneric)                         \
-  V(LoadKeyedFastDoubleElement)                \
-  V(LoadKeyedFastElement)                      \
+  V(LoadKeyed)                                 \
   V(LoadKeyedGeneric)                          \
-  V(LoadKeyedSpecializedArrayElement)          \
   V(LoadNamedField)                            \
   V(LoadNamedFieldPolymorphic)                 \
   V(LoadNamedGeneric)                          \
@@ -163,10 +161,8 @@ class LChunkBuilder;
   V(StoreContextSlot)                          \
   V(StoreGlobalCell)                           \
   V(StoreGlobalGeneric)                        \
-  V(StoreKeyedFastDoubleElement)               \
-  V(StoreKeyedFastElement)                     \
+  V(StoreKeyed)                                \
   V(StoreKeyedGeneric)                         \
-  V(StoreKeyedSpecializedArrayElement)         \
   V(StoreNamedField)                           \
   V(StoreNamedGeneric)                         \
   V(StringAdd)                                 \
@@ -4243,29 +4239,66 @@ class ArrayInstructionInterface {
   virtual ~ArrayInstructionInterface() { };
 };
 
-class HLoadKeyedFastElement
+
+class HLoadKeyed
     : public HTemplateInstruction<3>, public ArrayInstructionInterface {
  public:
-  HLoadKeyedFastElement(HValue* obj,
-                        HValue* key,
-                        HValue* dependency,
-                        ElementsKind elements_kind = FAST_ELEMENTS)
+  HLoadKeyed(HValue* obj,
+             HValue* key,
+             HValue* dependency,
+             ElementsKind elements_kind,
+             bool is_external)
       : bit_field_(0) {
-    ASSERT(IsFastSmiOrObjectElementsKind(elements_kind));
     bit_field_ = ElementsKindField::encode(elements_kind);
-    if (IsFastSmiElementsKind(elements_kind) &&
-        IsFastPackedElementsKind(elements_kind)) {
-      set_type(HType::Smi());
-    }
+
     SetOperandAt(0, obj);
     SetOperandAt(1, key);
     SetOperandAt(2, dependency);
-    set_representation(Representation::Tagged());
-    SetGVNFlag(kDependsOnArrayElements);
+
+    if (!is_external) {

[danno, 2012/10/21 20:44:41]

use IsExternalArrayElementsKind here

+      // I can detect the case between storing double (holey and fast) and
+      // smi/object by looking at elements_kind_.
+      ASSERT(IsFastSmiOrObjectElementsKind(elements_kind) ||
+             IsFastDoubleElementsKind(elements_kind));
+
+      if (IsFastSmiOrObjectElementsKind(elements_kind)) {
+        if (IsFastSmiElementsKind(elements_kind) &&
+            IsFastPackedElementsKind(elements_kind)) {
+          set_type(HType::Smi());
+        }
+
+        set_representation(Representation::Tagged());
+        SetGVNFlag(kDependsOnArrayElements);
+      } else {
+        set_representation(Representation::Double());
+        SetGVNFlag(kDependsOnDoubleArrayElements);
+      }
+    } else {
+      if (elements_kind == EXTERNAL_FLOAT_ELEMENTS ||
+          elements_kind == EXTERNAL_DOUBLE_ELEMENTS) {
+        set_representation(Representation::Double());
+      } else {
+        set_representation(Representation::Integer32());
+      }
+
+      SetGVNFlag(kDependsOnSpecializedArrayElements);
+      // Native code could change the specialized array.
+      SetGVNFlag(kDependsOnCalls);
+    }
+
     SetFlag(kUseGVN);
   }
 
-  HValue* object() { return OperandAt(0); }
+  bool is_external() const { return

[danno, 2012/10/21 20:44:41]

use IsExternalArrayElementsKind(elements_kind()) to calculate the result

+        CheckGVNFlag(kDependsOnSpecializedArrayElements); }
+  HValue* object() {
+    ASSERT(!is_external());
+    return OperandAt(0);
+  }
+  HValue* external_pointer() {
+    ASSERT(is_external());
+    return OperandAt(0);
+  }
   HValue* key() { return OperandAt(1); }
   HValue* dependency() { return OperandAt(2); }
   uint32_t index_offset() { return IndexOffsetField::decode(bit_field_); }
@@ -4274,6 +4307,7 @@ class HLoadKeyedFastElement
   }
   HValue* GetKey() { return key(); }
   void SetKey(HValue* key) { SetOperandAt(1, key); }
+
   bool IsDehoisted() { return IsDehoistedField::decode(bit_field_); }
   void SetDehoisted(bool is_dehoisted) {
     bit_field_ = IsDehoistedField::update(bit_field_, is_dehoisted);
@@ -4283,8 +4317,12 @@ class HLoadKeyedFastElement
   }
 
   virtual Representation RequiredInputRepresentation(int index) {
+    // <tagged | external>[int] (dependency = none)
     // The key is supposed to be Integer32.
-    if (index == 0) return Representation::Tagged();
+    if (index == 0) {
+      return is_external() ? Representation::External()
+          : Representation::Tagged();
+    }
     if (index == 1) return Representation::Integer32();
     return Representation::None();
   }
@@ -4293,157 +4331,43 @@ class HLoadKeyedFastElement
 
   bool RequiresHoleCheck() const;
 
-  DECLARE_CONCRETE_INSTRUCTION(LoadKeyedFastElement)
+  virtual Range* InferRange(Zone* zone);
+
+  DECLARE_CONCRETE_INSTRUCTION(LoadKeyed)
 
  protected:
   virtual bool DataEquals(HValue* other) {
-    if (!other->IsLoadKeyedFastElement()) return false;
-    HLoadKeyedFastElement* other_load = HLoadKeyedFastElement::cast(other);
+    if (!other->IsLoadKeyed()) return false;
+    HLoadKeyed* other_load = HLoadKeyed::cast(other);
+
+    // TODO(mvstanton): doing a special check for external here, because
+    // external didn't care about hoisting state and index offset.
+    // Is this nessecary at all?

[danno, 2012/10/21 20:44:41]

We should be able to treat all array accesses uniformly, including optimizing
away bounds checks on external array, so this code shouldn't be necessary. BTW:
You should make sure that array bounds check elimination works for external
arrays after your changes.

[mvstanton, 2012/10/23 23:44:20]

Done.

+    if (is_external() && other_load->is_external()) {
+      return elements_kind() == other_load->elements_kind();
+    }
+
     if (IsDehoisted() && index_offset() != other_load->index_offset())
       return false;
     return elements_kind() == other_load->elements_kind();
   }
 
  private:
-  virtual bool IsDeletable() const { return !RequiresHoleCheck(); }
+  virtual bool IsDeletable() const {
+    return is_external() ? true : !RequiresHoleCheck();

[danno, 2012/10/21 20:44:41]

RequiresHoleCheck should return false for external arrays, so IsDeletable should
just return !RequiresHoleCheck()

[mvstanton, 2012/10/23 23:44:20]

Done.

+  }
 
-  class ElementsKindField:  public BitField<ElementsKind, 0, 4> {};
-  class IndexOffsetField:   public BitField<uint32_t, 4, 27> {};
+  // TODO(mvstanton) - to deal with some of the external element
+  // kinds (PIXEL_ARRAY) I gave the ElementsKindField another bit,
+  // taking it from the index offset.
+  // Is this okay?
+  class ElementsKindField:  public BitField<ElementsKind, 0, 5> {};
+  class IndexOffsetField:   public BitField<uint32_t, 5, 26> {};
   class IsDehoistedField:   public BitField<bool, 31, 1> {};
   uint32_t bit_field_;
 };
 
 
-enum HoleCheckMode { PERFORM_HOLE_CHECK, OMIT_HOLE_CHECK };
-
-
-class HLoadKeyedFastDoubleElement
-    : public HTemplateInstruction<3>, public ArrayInstructionInterface {
- public:
-  HLoadKeyedFastDoubleElement(
-    HValue* elements,
-    HValue* key,
-    HValue* dependency,
-    HoleCheckMode hole_check_mode = PERFORM_HOLE_CHECK)
-      : index_offset_(0),
-        is_dehoisted_(false),
-        hole_check_mode_(hole_check_mode) {
-    SetOperandAt(0, elements);
-    SetOperandAt(1, key);
-    SetOperandAt(2, dependency);
-    set_representation(Representation::Double());
-    SetGVNFlag(kDependsOnDoubleArrayElements);
-    SetFlag(kUseGVN);
-  }
-
-  HValue* elements() { return OperandAt(0); }
-  HValue* key() { return OperandAt(1); }
-  HValue* dependency() { return OperandAt(2); }
-  uint32_t index_offset() { return index_offset_; }
-  void SetIndexOffset(uint32_t index_offset) { index_offset_ = index_offset; }
-  HValue* GetKey() { return key(); }
-  void SetKey(HValue* key) { SetOperandAt(1, key); }
-  bool IsDehoisted() { return is_dehoisted_; }
-  void SetDehoisted(bool is_dehoisted) { is_dehoisted_ = is_dehoisted; }
-
-  virtual Representation RequiredInputRepresentation(int index) {
-    // The key is supposed to be Integer32.
-    if (index == 0) return Representation::Tagged();
-    if (index == 1) return Representation::Integer32();
-    return Representation::None();
-  }
-
-  bool RequiresHoleCheck() const {
-    return hole_check_mode_ == PERFORM_HOLE_CHECK;
-  }
-
-  virtual void PrintDataTo(StringStream* stream);
-
-  DECLARE_CONCRETE_INSTRUCTION(LoadKeyedFastDoubleElement)
-
- protected:
-  virtual bool DataEquals(HValue* other) {
-    if (!other->IsLoadKeyedFastDoubleElement()) return false;
-    HLoadKeyedFastDoubleElement* other_load =
-        HLoadKeyedFastDoubleElement::cast(other);
-    return hole_check_mode_ == other_load->hole_check_mode_;
-  }
-
- private:
-  virtual bool IsDeletable() const { return !RequiresHoleCheck(); }
-
-  uint32_t index_offset_;
-  bool is_dehoisted_;
-  HoleCheckMode hole_check_mode_;
-};
-
-
-class HLoadKeyedSpecializedArrayElement
-    : public HTemplateInstruction<3>, public ArrayInstructionInterface {
- public:
-  HLoadKeyedSpecializedArrayElement(HValue* external_elements,
-                                    HValue* key,
-                                    HValue* dependency,
-                                    ElementsKind elements_kind)
-      :  elements_kind_(elements_kind),
-         index_offset_(0),
-         is_dehoisted_(false) {
-    SetOperandAt(0, external_elements);
-    SetOperandAt(1, key);
-    SetOperandAt(2, dependency);
-    if (elements_kind == EXTERNAL_FLOAT_ELEMENTS ||
-        elements_kind == EXTERNAL_DOUBLE_ELEMENTS) {
-      set_representation(Representation::Double());
-    } else {
-      set_representation(Representation::Integer32());
-    }
-    SetGVNFlag(kDependsOnSpecializedArrayElements);
-    // Native code could change the specialized array.
-    SetGVNFlag(kDependsOnCalls);
-    SetFlag(kUseGVN);
-  }
-
-  virtual void PrintDataTo(StringStream* stream);
-
-  virtual Representation RequiredInputRepresentation(int index) {
-    // The key is supposed to be Integer32.
-    if (index == 0) return Representation::External();
-    if (index == 1) return Representation::Integer32();
-    return Representation::None();
-  }
-
-  HValue* external_pointer() { return OperandAt(0); }
-  HValue* key() { return OperandAt(1); }
-  HValue* dependency() { return OperandAt(2); }
-  ElementsKind elements_kind() const { return elements_kind_; }
-  uint32_t index_offset() { return index_offset_; }
-  void SetIndexOffset(uint32_t index_offset) { index_offset_ = index_offset; }
-  HValue* GetKey() { return key(); }
-  void SetKey(HValue* key) { SetOperandAt(1, key); }
-  bool IsDehoisted() { return is_dehoisted_; }
-  void SetDehoisted(bool is_dehoisted) { is_dehoisted_ = is_dehoisted; }
-
-  virtual Range* InferRange(Zone* zone);
-
-  DECLARE_CONCRETE_INSTRUCTION(LoadKeyedSpecializedArrayElement)
-
- protected:
-  virtual bool DataEquals(HValue* other) {
-    if (!other->IsLoadKeyedSpecializedArrayElement()) return false;
-    HLoadKeyedSpecializedArrayElement* cast_other =
-        HLoadKeyedSpecializedArrayElement::cast(other);
-    return elements_kind_ == cast_other->elements_kind();
-  }
-
- private:
-  virtual bool IsDeletable() const { return true; }
-
-  ElementsKind elements_kind_;
-  uint32_t index_offset_;
-  bool is_dehoisted_;
-};
-
-
 class HLoadKeyedGeneric: public HTemplateInstruction<3> {
  public:
   HLoadKeyedGeneric(HValue* context, HValue* obj, HValue* key) {
@@ -4566,31 +4490,65 @@ class HStoreNamedGeneric: public HTemplateInstruction<3> {
 };
 
 
-class HStoreKeyedFastElement
+class HStoreKeyed
     : public HTemplateInstruction<3>, public ArrayInstructionInterface {
  public:
-  HStoreKeyedFastElement(HValue* obj, HValue* key, HValue* val,
-                         ElementsKind elements_kind = FAST_ELEMENTS)
+  HStoreKeyed(HValue* obj, HValue* key, HValue* val,
+              ElementsKind elements_kind, bool is_external)

[danno, 2012/10/21 20:44:41]

Same feedback here about is_external as in the load case... it should be
computed from elements_kind

[mvstanton, 2012/10/23 23:44:20]

Done.

       : elements_kind_(elements_kind), index_offset_(0), is_dehoisted_(false) {
     SetOperandAt(0, obj);
     SetOperandAt(1, key);
     SetOperandAt(2, val);
-    SetGVNFlag(kChangesArrayElements);
+
+    if (is_external) {
+      SetGVNFlag(kChangesSpecializedArrayElements);
+    } else if (!IsFastDoubleElementsKind(elements_kind)) {
+      SetGVNFlag(kChangesArrayElements);
+    } else {
+      SetFlag(kDeoptimizeOnUndefined);
+      SetGVNFlag(kChangesDoubleArrayElements);
+    }
   }
 
   virtual Representation RequiredInputRepresentation(int index) {
     // The key is supposed to be Integer32.
-    return index == 1
-        ? Representation::Integer32()
-        : Representation::Tagged();
+    // (<tagged> | <external>>[int32] = <tagged> | double | int

[danno, 2012/10/21 20:44:41]

nit: (<tagged> | <external>)[int32] = (<tagged> | double | int)

[mvstanton, 2012/10/23 23:44:20]

Done.

+    if (index == 0) {
+      return is_external() ? Representation::External()
+                           : Representation::Tagged();
+    } else if (index == 1) {
+      return Representation::Integer32();
+    }
+
+    ASSERT_EQ(index, 2);

[danno, 2012/10/21 20:44:41]

Perhaps add a new predicate in elements-kind.h that detects all floating/double
types?

e.g. create:

inline bool IsDoubleOrFloatElementsKind(ElementsKind elements_kind) {
  return IsFastDoubleElementsKind(elements_kind) ||
   elements_kind == EXTERNAL_DOUBLE_ELEMENTS ||
   elements_kind == EXTERNAL_FLOAT_ELEMENTS;
}

then the rest of this function become:

if (IsDoubleOrFloatElementsKind()) return Representation::Double();
return is_external() ? Representation::Tagged() : Representation::Integer32();

[mvstanton, 2012/10/23 23:44:20]

Wow, that was quite a knot, thanks!

Actually, having spent an hour tracking down a bug, I found it here :D. Your
proposal should say
return is_external() ? Representation::INTEGER32() :   
Representation::TAGGED(); (swapped)

Damn, making me read instead of copying and pasting :D.

+    if (is_external()) {
+        bool float_or_double_elements =
+            elements_kind() == EXTERNAL_FLOAT_ELEMENTS ||
+            elements_kind() == EXTERNAL_DOUBLE_ELEMENTS;
+      return float_or_double_elements ? Representation::Double()
+                                      : Representation::Integer32();
+    }
+
+    return IsFastDoubleElementsKind(elements_kind()) ? Representation::Double()
+                                                     : Representation::Tagged();
   }
 
-  HValue* object() { return OperandAt(0); }
+  bool is_external() const { return
+        CheckGVNFlag(kChangesSpecializedArrayElements); }
+  HValue* external_pointer() {
+    ASSERT(is_external());
+    return OperandAt(0);
+  }
+  HValue* object() {
+    ASSERT(!is_external());
+    return OperandAt(0);
+  }
   HValue* key() { return OperandAt(1); }
   HValue* value() { return OperandAt(2); }
-  bool value_is_smi() {
+  bool value_is_smi() const {
     return IsFastSmiElementsKind(elements_kind_);
   }
+  ElementsKind elements_kind() const { return elements_kind_; }
   uint32_t index_offset() { return index_offset_; }
   void SetIndexOffset(uint32_t index_offset) { index_offset_ = index_offset; }
   HValue* GetKey() { return key(); }
@@ -4606,110 +4564,11 @@ class HStoreKeyedFastElement
     }
   }
 
-  virtual void PrintDataTo(StringStream* stream);
-
-  DECLARE_CONCRETE_INSTRUCTION(StoreKeyedFastElement)
-
- private:
-  ElementsKind elements_kind_;
-  uint32_t index_offset_;
-  bool is_dehoisted_;
-};
-
-
-class HStoreKeyedFastDoubleElement
-    : public HTemplateInstruction<3>, public ArrayInstructionInterface {
- public:
-  HStoreKeyedFastDoubleElement(HValue* elements,
-                               HValue* key,
-                               HValue* val)
-      : index_offset_(0), is_dehoisted_(false) {
-    SetOperandAt(0, elements);
-    SetOperandAt(1, key);
-    SetOperandAt(2, val);
-    SetFlag(kDeoptimizeOnUndefined);
-    SetGVNFlag(kChangesDoubleArrayElements);
-  }
-
-  virtual Representation RequiredInputRepresentation(int index) {
-    if (index == 1) {
-      return Representation::Integer32();
-    } else if (index == 2) {
-      return Representation::Double();
-    } else {
-      return Representation::Tagged();
-    }
-  }
-
-  HValue* elements() { return OperandAt(0); }
-  HValue* key() { return OperandAt(1); }
-  HValue* value() { return OperandAt(2); }
-  uint32_t index_offset() { return index_offset_; }
-  void SetIndexOffset(uint32_t index_offset) { index_offset_ = index_offset; }
-  HValue* GetKey() { return key(); }
-  void SetKey(HValue* key) { SetOperandAt(1, key); }
-  bool IsDehoisted() { return is_dehoisted_; }
-  void SetDehoisted(bool is_dehoisted) { is_dehoisted_ = is_dehoisted; }
-
-  bool NeedsWriteBarrier() {
-    return StoringValueNeedsWriteBarrier(value());
-  }
-
   bool NeedsCanonicalization();
 
   virtual void PrintDataTo(StringStream* stream);
 
-  DECLARE_CONCRETE_INSTRUCTION(StoreKeyedFastDoubleElement)
-
- private:
-  uint32_t index_offset_;
-  bool is_dehoisted_;
-};
-
-
-class HStoreKeyedSpecializedArrayElement
-    : public HTemplateInstruction<3>, public ArrayInstructionInterface {
- public:
-  HStoreKeyedSpecializedArrayElement(HValue* external_elements,
-                                     HValue* key,
-                                     HValue* val,
-                                     ElementsKind elements_kind)
-      : elements_kind_(elements_kind), index_offset_(0), is_dehoisted_(false) {
-    SetGVNFlag(kChangesSpecializedArrayElements);
-    SetOperandAt(0, external_elements);
-    SetOperandAt(1, key);
-    SetOperandAt(2, val);
-  }
-
-  virtual void PrintDataTo(StringStream* stream);
-
-  virtual Representation RequiredInputRepresentation(int index) {
-    if (index == 0) {
-      return Representation::External();
-    } else {
-      bool float_or_double_elements =
-          elements_kind() == EXTERNAL_FLOAT_ELEMENTS ||
-          elements_kind() == EXTERNAL_DOUBLE_ELEMENTS;
-      if (index == 2 && float_or_double_elements) {
-        return Representation::Double();
-      } else {
-        return Representation::Integer32();
-      }
-    }
-  }
-
-  HValue* external_pointer() { return OperandAt(0); }
-  HValue* key() { return OperandAt(1); }
-  HValue* value() { return OperandAt(2); }
-  ElementsKind elements_kind() const { return elements_kind_; }
-  uint32_t index_offset() { return index_offset_; }
-  void SetIndexOffset(uint32_t index_offset) { index_offset_ = index_offset; }
-  HValue* GetKey() { return key(); }
-  void SetKey(HValue* key) { SetOperandAt(1, key); }
-  bool IsDehoisted() { return is_dehoisted_; }
-  void SetDehoisted(bool is_dehoisted) { is_dehoisted_ = is_dehoisted; }
-
-  DECLARE_CONCRETE_INSTRUCTION(StoreKeyedSpecializedArrayElement)
+  DECLARE_CONCRETE_INSTRUCTION(StoreKeyed)
 
  private:
   ElementsKind elements_kind_;
@@ -4740,6 +4599,7 @@ class HStoreKeyedGeneric: public HTemplateInstruction<4> {
   StrictModeFlag strict_mode_flag() { return strict_mode_flag_; }
 
   virtual Representation RequiredInputRepresentation(int index) {
+    // <tagged>[<tagged>] = <tagged>
     return Representation::Tagged();
   }