Implement crankshaft support for pixel array stores.

Implement crankshaft support for pixel array stores.

[danno, 2011-02-15 16:31:27]

Please review. Thanks!

[Kevin Millikin (Chromium), 2011-02-16 08:51:25]

LGTM, very small suggestions below.

http://codereview.chromium.org/6528013/diff/5001/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

http://codereview.chromium.org/6528013/diff/5001/src/arm/lithium-arm.h#newcod...
src/arm/lithium-arm.h:1169: class LStorePixelArrayElement: public
LTemplateInstruction<0, 3, 1> {
You should just delete this instruction because it's not used yet on the
backend.

http://codereview.chromium.org/6528013/diff/5001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

http://codereview.chromium.org/6528013/diff/5001/src/hydrogen-instructions.cc...
src/hydrogen-instructions.cc:61: case kNumRepresentations:
Thanks for removing the default.

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

http://codereview.chromium.org/6528013/diff/5001/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:3195: HValue* external_pointer() const { return
OperandAt(0); }
I would just use return operands_[0] (OperandAt is virtual).

http://codereview.chromium.org/6528013/diff/5001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/6528013/diff/5001/src/hydrogen.cc#newcode3773
src/hydrogen.cc:3773: HInstruction* length = AddInstruction(new
HPixelArrayLength(elements));
Wow, AddInstruction returns a value and we usually ignore it?  Who knew.

I think I'd rather just make it have a side effect and no retrun value (as a
separate change).  Return values that are normally ignored seems like a bad
smell.

Could you write this statement as two separate ones:

HInstruction* length = new HPixelArrayLength(elements);
AddInstruction(length);

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

http://codereview.chromium.org/6528013/diff/5001/src/ia32/lithium-ia32.cc#new...
src/ia32/lithium-ia32.cc:1845: LOperand* clamped = FixedTemp(eax);
Maybe a simple comment that this has to be a byte register, choice of eax is
arbitrary.

[danno, 2011-02-16 13:54:22]

Addressed and committed.

http://codereview.chromium.org/6528013/diff/5001/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

http://codereview.chromium.org/6528013/diff/5001/src/arm/lithium-arm.h#newcod...
src/arm/lithium-arm.h:1169: class LStorePixelArrayElement: public
LTemplateInstruction<0, 3, 1> {
On 2011/02/16 08:51:25, Kevin Millikin wrote:
> You should just delete this instruction because it's not used yet on the
> backend.

Done.

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

http://codereview.chromium.org/6528013/diff/5001/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:3195: HValue* external_pointer() const { return
OperandAt(0); }
On 2011/02/16 08:51:25, Kevin Millikin wrote:
> I would just use return operands_[0] (OperandAt is virtual).

Done.

http://codereview.chromium.org/6528013/diff/5001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/6528013/diff/5001/src/hydrogen.cc#newcode3773
src/hydrogen.cc:3773: HInstruction* length = AddInstruction(new
HPixelArrayLength(elements));
On 2011/02/16 08:51:25, Kevin Millikin wrote:
> Wow, AddInstruction returns a value and we usually ignore it?  Who knew.
> 
> I think I'd rather just make it have a side effect and no retrun value (as a
> separate change).  Return values that are normally ignored seems like a bad
> smell.
> 
> Could you write this statement as two separate ones:
> 
> HInstruction* length = new HPixelArrayLength(elements);
> AddInstruction(length);

Done.

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

http://codereview.chromium.org/6528013/diff/5001/src/ia32/lithium-ia32.cc#new...
src/ia32/lithium-ia32.cc:1845: LOperand* clamped = FixedTemp(eax);
On 2011/02/16 08:51:25, Kevin Millikin wrote:
> Maybe a simple comment that this has to be a byte register, choice of eax is
> arbitrary.

Done.