Implement crankshaft support for pixel array loads.

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 108 matching lines @@
   V(ClassOfTest)                               \
   V(LeaveInlined)                              \
   V(LoadContextSlot)                           \
   V(LoadElements)                              \
   V(LoadFunctionPrototype)                     \
   V(LoadGlobal)                                \
   V(LoadKeyedFastElement)                      \
   V(LoadKeyedGeneric)                          \
   V(LoadNamedField)                            \
   V(LoadNamedGeneric)                          \
+  V(LoadPixelArrayElement)                     \
+  V(LoadPixelArrayExternalPointer)             \
   V(Mod)                                       \
   V(Mul)                                       \
   V(ObjectLiteral)                             \
   V(OsrEntry)                                  \
   V(OuterContext)                              \
   V(Parameter)                                 \
+  V(PixelArrayLength)                          \
   V(Power)                                     \
   V(PushArgument)                              \
   V(RegExpLiteral)                             \
   V(Return)                                    \
   V(Sar)                                       \
   V(Shl)                                       \
   V(Shr)                                       \
   V(Simulate)                                  \
   V(StackCheck)                                \
   V(StoreContextSlot)                          \
@@ skipping 1181 matching lines @@
     return Representation::Tagged();
   }
 
   DECLARE_CONCRETE_INSTRUCTION(FixedArrayLength, "fixed_array_length")
 
  protected:
   virtual bool DataEquals(HValue* other) const { return true; }
 };
 
 
+class HPixelArrayLength: public HUnaryOperation {
+ public:
+  explicit HPixelArrayLength(HValue* value) : HUnaryOperation(value) {
+    set_representation(Representation::Integer32());
+    SetFlag(kDependsOnArrayLengths);

[Mads Ager (chromium), 2011/02/07 11:36:06]

Can you ever change the length of a pixel array? I hope you can't? In that case
you probably don't need the dependence on ArrayLengths. If the input is the same
elements then the length will be the same?

[danno, 2011/02/08 09:25:57]

Done.

+    SetFlag(kUseGVN);
+  }
+
+  virtual Representation RequiredInputRepresentation(int index) const {
+    return Representation::Tagged();
+  }
+
+  DECLARE_CONCRETE_INSTRUCTION(PixelArrayLength, "pixel_array_length")
+
+ protected:
+  virtual bool DataEquals(HValue* other) const { return true; }
+};
+
+
 class HBitNot: public HUnaryOperation {
  public:
   explicit HBitNot(HValue* value) : HUnaryOperation(value) {
     set_representation(Representation::Integer32());
     SetFlag(kUseGVN);
     SetFlag(kTruncatingToInt32);
   }
 
   virtual Representation RequiredInputRepresentation(int index) const {
     return Representation::Integer32();
@@ skipping 98 matching lines @@
     return Representation::Tagged();
   }
 
   DECLARE_CONCRETE_INSTRUCTION(LoadElements, "load-elements")
 
  protected:
   virtual bool DataEquals(HValue* other) const { return true; }
 };
 
 
+class HLoadPixelArrayExternalPointer: public HUnaryOperation {
+ public:
+  explicit HLoadPixelArrayExternalPointer(HValue* value)
+      : HUnaryOperation(value) {
+    set_representation(Representation::Tagged());

[danno, 2011/02/07 09:47:27]

This isn't right: This pointer isn't tagged, it's a naked pointer to external
storage. However, if I mark it as Representation::None(), the code deoptimizes.
Need to look into why.

[Mads Ager (chromium), 2011/02/07 11:36:06]

Doesn't this have to depend on something? You shouldn't move this past something
that changes maps at least.

[Mads Ager (chromium), 2011/02/07 11:36:06]

I don't think we have raw C++ pointers in the system anywhere else. Therefore,
this really should be a new representation type, shouldn't it? It should have a
result representation that is not None).

[Kevin Millikin (Chromium), 2011/02/07 12:39:08]

This is almost right.  We have a raw pointer to an object outside V8's heap when
we work with the arguments object in optimized code.

We don't materialize the object, but we instead use a raw pointer into the
stack.

In that case we say that it's an int32.  We can do two things: (1) change the
name Integer32 and make it represent arbitrar untagged 32-bit values, (2)
introduce a new representation type.

I prefer (2).

[danno, 2011/02/08 09:25:57]

I don't think this needs to depend on anything. It's like the pixel array
length, it can only be set when the PixelArray elements is allocated for the
first time, then it is idempotent given the same input (elements) object.
On 2011/02/07 11:36:06, Mads Ager wrote:
Done.

[danno, 2011/02/08 09:25:57]

After discussion with Kevin, I implemented 2)

On 2011/02/07 12:39:08, Kevin Millikin wrote:

+    SetFlag(kUseGVN);
+  }
+
+  virtual Representation RequiredInputRepresentation(int index) const {
+    return Representation::Tagged();
+  }
+
+  DECLARE_CONCRETE_INSTRUCTION(LoadPixelArrayExternalPointer,
+                               "load-pixel-array-external-pointer")
+
+ protected:
+  virtual bool DataEquals(HValue* other) const { return true; }
+};
+
+
 class HCheckMap: public HUnaryOperation {
  public:
   HCheckMap(HValue* value, Handle<Map> map)
       : HUnaryOperation(value), map_(map) {
     set_representation(Representation::Tagged());
     SetFlag(kUseGVN);
     SetFlag(kDependsOnMaps);
   }
 
   virtual bool IsCheckInstruction() const { return true; }
@@ skipping 1358 matching lines @@
   }
 
   DECLARE_CONCRETE_INSTRUCTION(LoadKeyedFastElement,
                                "load_keyed_fast_element")
 
  protected:
   virtual bool DataEquals(HValue* other) const { return true; }
 };
 
 
+class HLoadPixelArrayElement: public HBinaryOperation {
+ public:
+  HLoadPixelArrayElement(HValue* external_elements, HValue* key)
+      : HBinaryOperation(external_elements, key) {
+    set_representation(Representation::Integer32());
+  }
+
+  virtual void PrintDataTo(StringStream* stream) const;
+
+  virtual Representation RequiredInputRepresentation(int index) const {
+    // The key is supposed to be Integer32, but the base pointer
+    // for the element load is a naked pointer.
+    return (index == 1) ? Representation::Integer32()
+        : Representation::None();

[danno, 2011/02/07 09:47:27]

None() isn't consistent with HLoadPixelArrayExternalPointer's type. See my other
comment above. 

[Mads Ager (chromium), 2011/02/07 11:36:06]

Here it is a requirement and therefore None tells us that we don't care. It
really should be RawPointer as should the one above?

[danno, 2011/02/08 09:25:57]

Done.

+  }
+
+  HValue* external_pointer() const { return OperandAt(0); }
+  HValue* key() const { return OperandAt(1); }
+
+  DECLARE_CONCRETE_INSTRUCTION(LoadPixelArrayElement,
+                               "load_pixel_array_element")
+
+ protected:
+  virtual bool DataEquals(HValue* other) const { return true; }

[Mads Ager (chromium), 2011/02/07 11:36:06]

Since you are not using GVN you should not implement this one. Shouldn't you use
GVN and actually implement data equals so that only if elements and key are the
same will you reuse the result?

[Kevin Millikin (Chromium), 2011/02/07 12:39:08]

I think it's safe to use GVN for this.  Right now, since you don't store to
pixel arrays in optimized code, the element would be changed by calls (only, as
far as I can see).

DataEquals doesn't have to compare the two input operands, they'll be compared
by Equals, so in this case "return true" is the correct override.

[danno, 2011/02/08 09:25:57]

Whoops. Forgot the kUseGVN. These should be GVNed.
On 2011/02/07 11:36:06, Mads Ager wrote:

[danno, 2011/02/08 09:25:57]

I added a flag kDependsOnPixelArrayElements. Later the pa stores can trigger a
ChangesPixelArrayElements. 
On 2011/02/07 12:39:08, Kevin Millikin wrote:

+};
+
+
 class HLoadKeyedGeneric: public HLoadKeyed {
  public:
   HLoadKeyedGeneric(HValue* obj, HValue* key) : HLoadKeyed(obj, key) {
     SetAllSideEffects();
   }
 
   DECLARE_CONCRETE_INSTRUCTION(LoadKeyedGeneric, "load_keyed_generic")
 };
 
 
@@ skipping 335 matching lines @@
   HValue* object() const { return left(); }
   HValue* key() const { return right(); }
 };
 
 #undef DECLARE_INSTRUCTION
 #undef DECLARE_CONCRETE_INSTRUCTION
 
 } }  // namespace v8::internal
 
 #endif  // V8_HYDROGEN_INSTRUCTIONS_H_