Implement crankshaft support for pixel array loads.

src/hydrogen-instructions.h

Index: src/hydrogen-instructions.h
diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h
index f1093a004c6c340131307d8fe18ed6baf868cef1..a558823f2eb33405a233b29ab0c67a1bcd66c558 100644
--- a/src/hydrogen-instructions.h
+++ b/src/hydrogen-instructions.h
@@ -126,12 +126,15 @@ class LChunkBuilder;
   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)                             \
@@ -1333,6 +1336,25 @@ class HFixedArrayLength: public HUnaryOperation {
 };
 
 
+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) {
@@ -1451,6 +1473,26 @@ class HLoadElements: public HUnaryOperation {
 };
 
 
+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)
@@ -2829,6 +2871,33 @@ class HLoadKeyedFastElement: public HLoadKeyed {
 };
 
 
+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) {