Implement crankshaft support for pixel array loads.

Implement crankshaft support for pixel array loads.

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

Work in progress, but I'd appreciate feedback. ia32 pixel loads get optimized,
but please see my comment/question about the Representation.

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:1480: set_representation(Representation::Tagged());
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.

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:2887: : Representation::None();
None() isn't consistent with HLoadPixelArrayExternalPointer's type. See my other
comment above.

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

First round of comments. I think you need a new representation type for this.

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:1343: SetFlag(kDependsOnArrayLengths);
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?

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:1480: set_representation(Representation::Tagged());
Doesn't this have to depend on something? You shouldn't move this past something
that changes maps at least.

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:1480: set_representation(Representation::Tagged());
On 2011/02/07 09:47:27, danno wrote:
> 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.

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).

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:2887: : Representation::None();
On 2011/02/07 09:47:27, danno wrote:
> None() isn't consistent with HLoadPixelArrayExternalPointer's type. See my
other
> comment above. 

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?

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:2897: virtual bool DataEquals(HValue* other) const {
return true; }
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?

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen.cc#newcode3753
src/hydrogen.cc:3753: HLoadPixelArrayElement* pixel_array_value = new
HLoadPixelArrayElement(
Move everything to a new line?

http://codereview.chromium.org/6410112/diff/2028/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/6410112/diff/2028/src/ia32/lithium-ia32.cc#new...
src/ia32/lithium-ia32.cc:1782: LOperand* pa =
UseRegisterAtStart(instr->external_pointer());
pa -> external_pointer?

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

First round of comments from me.

http://codereview.chromium.org/6410112/diff/2028/src/ast.h
File src/ast.h (right):

http://codereview.chromium.org/6410112/diff/2028/src/ast.h#newcode1211
src/ast.h:1211: has_pixel_array_elements_(false),
This field seems to be unused.

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:1480: set_representation(Representation::Tagged());
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).

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:2897: virtual bool DataEquals(HValue* other) const {
return true; }
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.

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen.cc#newcode3869
src/hydrogen.cc:3869: if (receiver_type->has_pixel_array_elements()) {
I guess from the assert above that it's the case that an object only had fast
elements or pixed array elements, never both?  A short comment here might remind
the reader.

http://codereview.chromium.org/6410112/diff/2028/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/6410112/diff/2028/src/ia32/lithium-ia32.cc#new...
src/ia32/lithium-ia32.cc:1763: return DefineSameAsFirst(new
LLoadPixelArrayExternalPointer(input));
It's not obvious why this is DefineSameAsFirst and DoPixelArrayLength is
DefineAsRegister.

http://codereview.chromium.org/6410112/diff/2028/src/ia32/lithium-ia32.cc#new...
src/ia32/lithium-ia32.cc:1785: return
AssignEnvironment(DefineSameAsFirst(result));
Does this need the environment?

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

addressed feedback, added x64 and arm port

http://codereview.chromium.org/6410112/diff/2028/src/ast.h
File src/ast.h (right):

http://codereview.chromium.org/6410112/diff/2028/src/ast.h#newcode1211
src/ast.h:1211: has_pixel_array_elements_(false),
On 2011/02/07 12:39:08, Kevin Millikin wrote:
> This field seems to be unused.

Done.

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:1343: SetFlag(kDependsOnArrayLengths);
On 2011/02/07 11:36:06, Mads Ager wrote:
> 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?

Done.

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:1480: set_representation(Representation::Tagged());
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:
> Doesn't this have to depend on something? You shouldn't move this past
something
> that changes maps at least.

Done.

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:1480: set_representation(Representation::Tagged());
After discussion with Kevin, I implemented 2)

On 2011/02/07 12:39:08, Kevin Millikin wrote:
> 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).

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:2887: : Representation::None();
On 2011/02/07 11:36:06, Mads Ager wrote:
> On 2011/02/07 09:47:27, danno wrote:
> > None() isn't consistent with HLoadPixelArrayExternalPointer's type. See my
> other
> > comment above. 
> 
> 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?

Done.

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:2897: virtual bool DataEquals(HValue* other) const {
return true; }
Whoops. Forgot the kUseGVN. These should be GVNed.
On 2011/02/07 11:36:06, Mads Ager wrote:
> 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?

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:2897: virtual bool DataEquals(HValue* other) const {
return true; }
I added a flag kDependsOnPixelArrayElements. Later the pa stores can trigger a
ChangesPixelArrayElements. 
On 2011/02/07 12:39:08, Kevin Millikin wrote:
> 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.

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen.cc#newcode3753
src/hydrogen.cc:3753: HLoadPixelArrayElement* pixel_array_value = new
HLoadPixelArrayElement(
On 2011/02/07 11:36:06, Mads Ager wrote:
> Move everything to a new line?

Done.

http://codereview.chromium.org/6410112/diff/2028/src/hydrogen.cc#newcode3869
src/hydrogen.cc:3869: if (receiver_type->has_pixel_array_elements()) {
On 2011/02/07 12:39:08, Kevin Millikin wrote:
> I guess from the assert above that it's the case that an object only had fast
> elements or pixed array elements, never both?  A short comment here might
remind
> the reader.

Done.

http://codereview.chromium.org/6410112/diff/2028/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/6410112/diff/2028/src/ia32/lithium-ia32.cc#new...
src/ia32/lithium-ia32.cc:1763: return DefineSameAsFirst(new
LLoadPixelArrayExternalPointer(input));
On 2011/02/07 12:39:08, Kevin Millikin wrote:
> It's not obvious why this is DefineSameAsFirst and DoPixelArrayLength is
> DefineAsRegister.

Done.

http://codereview.chromium.org/6410112/diff/2028/src/ia32/lithium-ia32.cc#new...
src/ia32/lithium-ia32.cc:1782: LOperand* pa =
UseRegisterAtStart(instr->external_pointer());
On 2011/02/07 11:36:06, Mads Ager wrote:
> pa -> external_pointer?

Done.

http://codereview.chromium.org/6410112/diff/2028/src/ia32/lithium-ia32.cc#new...
src/ia32/lithium-ia32.cc:1785: return
AssignEnvironment(DefineSameAsFirst(result));
On 2011/02/07 12:39:08, Kevin Millikin wrote:
> Does this need the environment?

Done.

[Mads Ager (chromium), 2011-02-08 17:26:31]

LGTM

http://codereview.chromium.org/6410112/diff/8001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/6410112/diff/8001/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:1347: SetFlag(kUseGVN);
I get a little worried when I see kUseGVN without a kDependsOnSomething. Since
the length cannot change of a pixel array and the map changes when the pixel
array is changed I can see why this is the case (because it takes as input the
output from loading the elements which does depend on maps). Can we add a
comment here?

http://codereview.chromium.org/6410112/diff/8001/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:1484: SetFlag(kUseGVN);
Same comment here. I see why we don't need a kDependsOn... here, but a comment
would be nice.

http://codereview.chromium.org/6410112/diff/8001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/6410112/diff/8001/src/hydrogen.cc#newcode3754
src/hydrogen.cc:3754: new HLoadPixelArrayElement(external_elements,
Move the second parameter to this line?