Support conversion of clamped double values for pixel arrays in Crankshaft.

Support conversion of clamped double values for pixel arrays in Crankshaft.

BUG=1313
TEST=test/mjsunit/external-array.js

Committed: http://code.google.com/p/v8/source/detail?r=7901

[danno, 2011-05-13 08:04:52]

Please review, thanks!

[Lasse Reichstein, 2011-05-13 13:17:23]

Drive-by x64 comments.

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2593: Set(result_reg, 0);
How about setting result to 0 prior to the compare, and just jump directly to
done if below_equal. Should give less jumping.

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2596: uint64_t max_clamp = BitCast<uint64_t,
double>(255.0);
How about not capping at 255 until after you have converted to integer?

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2598: movq(temp_xmm_reg, temp_reg);
Use movsd here. Movq makes the CPU think the XMM register holds an integer, not
a double. There is a latency for switching between interpretations.

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2606: roundsd(temp_xmm_reg, input_reg,
kRoundToNearest);
Roundsd's round-to-nearest rounds to even numbers in case of a tie. The code in
the other branch rounds up. Which one is correct?

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2610: movq(temp_xmm_reg, temp_reg);
Movsd.

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2611: addsd(temp_xmm_reg, input_reg);
Adding 0.5 and truncating doesn't work perfectly as round-to-nearet.
It doesn't round correctly for 0.49999999999999994. Adding 0.5 incorrectly
rounds this up to 1, but round to nearest must round down.

You can instead first compare to 0.5, which you already have, and if below, just
return 0.

(There's also a roundoff problem at 2^53-1, but that doesn't matter here, since
both the actual and the correct result will be capped to 255 anyway).

http://codereview.chromium.org/7014033/diff/4001/test/mjsunit/external-array.js
File test/mjsunit/external-array.js (right):

http://codereview.chromium.org/7014033/diff/4001/test/mjsunit/external-array....
test/mjsunit/external-array.js:155: 
Test 0.49999999999999994, please :).

And both 1.5 and 2.5.

[Lasse Reichstein, 2011-05-14 09:34:38]

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2592: j(above, &above_zero, Label::kNear);
I believe NaN should also be converted to zero (i.e., if the P flag is set after
ucomisd)

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2606: roundsd(temp_xmm_reg, input_reg,
kRoundToNearest);
I tried to look up a spec for typed arrays, and if the one I found was correct,
we should just round towards zero, i.e., truncate.

[danno, 2011-05-15 05:50:27]

Updated with feedback addressed.

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2592: j(above, &above_zero, Label::kNear);
NaN does get set to zero, there is an explicit test for it in the
external_arrays.js. Above jumps when CF=0 and ZF=0, which is not the case for
comparison for NaN (for unordered comparisons ZF,PF,CF are all set to 111), so
the code falls through and return zero path is taken.
On 2011/05/14 09:34:38, Lasse Reichstein wrote:
> I believe NaN should also be converted to zero (i.e., if the P flag is set
after
> ucomisd)

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2593: Set(result_reg, 0);
On 2011/05/13 13:17:24, Lasse Reichstein wrote:
> How about setting result to 0 prior to the compare, and just jump directly to
> done if below_equal. Should give less jumping.

Done.

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2596: uint64_t max_clamp = BitCast<uint64_t,
double>(255.0);
On 2011/05/13 13:17:24, Lasse Reichstein wrote:
> How about not capping at 255 until after you have converted to integer?

Done.

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2598: movq(temp_xmm_reg, temp_reg);
On 2011/05/13 13:17:24, Lasse Reichstein wrote:
> Use movsd here. Movq makes the CPU think the XMM register holds an integer,
not
> a double. There is a latency for switching between interpretations.

Done.

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2606: roundsd(temp_xmm_reg, input_reg,
kRoundToNearest);
On 2011/05/13 13:17:24, Lasse Reichstein wrote:
> Roundsd's round-to-nearest rounds to even numbers in case of a tie. The code
in
> the other branch rounds up. Which one is correct?

Done.

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2606: roundsd(temp_xmm_reg, input_reg,
kRoundToNearest);
This code is specifically for CanvasPixelArrays (which use the special clamping
semantics that include rounding, look at http://www.w3.org/TR/2dcontext/ and
search for "clamp": "use the value from the nearest edge pixel,"), unlike typed
arrays (which use ToInt32 semantics, which we currently don't implement
correctly, but that's another issue and another CL for another day). 
On 2011/05/14 09:34:38, Lasse Reichstein wrote:
> I tried to look up a spec for typed arrays, and if the one I found was
correct,
> we should just round towards zero, i.e., truncate.

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2610: movq(temp_xmm_reg, temp_reg);
On 2011/05/13 13:17:24, Lasse Reichstein wrote:
> Movsd.

Done.

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2611: addsd(temp_xmm_reg, input_reg);
I am aware of this imprecision, but this is exactly the roudning algorithm used
in the "slow" case implementation for pixel arrays in objects.cc SetProperty,
and the semantic should be the same. I'll file a separate bug to fix the overall
behavior, but that's beyond the scope of this CL.
On 2011/05/13 13:17:24, Lasse Reichstein wrote:
> Adding 0.5 and truncating doesn't work perfectly as round-to-nearet.
> It doesn't round correctly for 0.49999999999999994. Adding 0.5 incorrectly
> rounds this up to 1, but round to nearest must round down.
> 
> You can instead first compare to 0.5, which you already have, and if below,
just
> return 0.
> 
> (There's also a roundoff problem at 2^53-1, but that doesn't matter here,
since
> both the actual and the correct result will be capped to 255 anyway).

http://codereview.chromium.org/7014033/diff/4001/test/mjsunit/external-array.js
File test/mjsunit/external-array.js (right):

http://codereview.chromium.org/7014033/diff/4001/test/mjsunit/external-array....
test/mjsunit/external-array.js:155: 
I'm not so sure we should add the test case, since the rest of the
implementation for CanvasPixelArrays uses the incorrect +.5 semantics and
already did "before my time". :-) I suggest staying consistent until it gets
fixed everywhere, and I don't think it's a good idea to bake in an incorrect
test case.

I promise to add the test case when I fix the bug (issue 1392). :-)

On 2011/05/13 13:17:24, Lasse Reichstein wrote:
> Test 0.49999999999999994, please :).
> 
> And both 1.5 and 2.5.

[Lasse Reichstein, 2011-05-15 10:18:50]

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2592: j(above, &above_zero, Label::kNear);
Ack, yes. I didn't consider that "above" would also match NaN (which is great
:).

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2606: roundsd(temp_xmm_reg, input_reg,
kRoundToNearest);
I read that as clamping positions to the edge of the available image when a
filtering algorithm needs surrounding pixels that aren't in the image.
I don't see anything about rounding double values to octets for the pixel array
(but I can see that Firefox is using round-to-even on my machine, so they are
probably using roundsd).

http://codereview.chromium.org/7014033/diff/4001/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:2611: addsd(temp_xmm_reg, input_reg);
Good :)

http://codereview.chromium.org/7014033/diff/4001/test/mjsunit/external-array.js
File test/mjsunit/external-array.js (right):

http://codereview.chromium.org/7014033/diff/4001/test/mjsunit/external-array....
test/mjsunit/external-array.js:155: 
That's fine.
If you have a test that is known to fail for now, you can add a test file in
mjsunit/bugs. It'll default to expecting to fail, and can be moved to
mjsunit/regress when the bug is fixed.

[Kevin Millikin (Chromium), 2011-05-16 07:06:34]

I have some naming suggestions, and beefed up comments/asserts.  Suggestions in
the ARM files apply sometimes to all platforms.

LGTM with comments adressed.

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

http://codereview.chromium.org/7014033/diff/4001/src/arm/lithium-arm.h#newcode77
src/arm/lithium-arm.h:77: V(ClampIToUint8)                              \
It's strange that there is "Double", "I", and "Tagged" :)  I suggest "D", "I",
and "T" so the class names match the printed instruction names.

http://codereview.chromium.org/7014033/diff/4001/src/arm/lithium-arm.h#newcod...
src/arm/lithium-arm.h:1935: explicit LClampDoubleToUint8(LOperand* value,
LOperand* temp) {
No need for explicit here.

http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-arm.cc#newco...
src/arm/lithium-arm.cc:1756: Representation rep = value->representation();
Can we rename this to input_rep and assert that instr->representation is int32?

http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-arm.cc#newco...
src/arm/lithium-arm.cc:1759: return DefineAsRegister(new
LClampDoubleToUint8(reg, FixedTemp(d1)));
Probably needs a comment that the fixed register is because register allocator
does not (yet) support allocation of double temporaries.  I'd be puzzled if I
read this code again in a month.

http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:3356: __ Usat(value, 8, Operand(value));
Should this be removed?

http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:4052: __ b(al, &done);
There is __ jmp(&done) for ARM, which just emits __b(al, &done).  I find it
clearer when reading ARM code but I'm not sure if everyone agrees with that.

http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:4057: __ vldr(double_scratch0(), scratch0(),
HeapNumber::kValueOffset);
Someone has kindly implemented vldr to take a MemOperand!  So you should be able
to get rid of the sub and the scratch0 register use and write:

__ vldr(double_scratch0(), FieldMemOperand(input_reg,
HeapNumber::kValueOffset));

I guess this code was copied from somewhere, so you could change it there too.

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

http://codereview.chromium.org/7014033/diff/1025/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:1061: SetFlag(kFlexibleRepresentation);
Everywhere else we have kFlexibleRepresentation, we also have
set_representation(Representation::Tagged()) to establish a default
representation.  I don't know that the representation inference algorithm
depends on it, but maybe it should be here for safety should somebody later
assume it.

http://codereview.chromium.org/7014033/diff/1025/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/7014033/diff/1025/src/x64/lithium-codegen-x64....
src/x64/lithium-codegen-x64.cc:3188: {  // Clamp the value to [0..255].
Can we remove this clamping?

[danno, 2011-05-16 14:13:43]

Review feedback addressed, final version committed.

http://codereview.chromium.org/7014033/diff/4001/test/mjsunit/external-array.js
File test/mjsunit/external-array.js (right):

http://codereview.chromium.org/7014033/diff/4001/test/mjsunit/external-array....
test/mjsunit/external-array.js:155: 
I'll write and submit the test in a separate CL.
 On 2011/05/13 13:17:24, Lasse Reichstein wrote:
> Test 0.49999999999999994, please :).
> 
> And both 1.5 and 2.5.

http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-arm.cc#newco...
src/arm/lithium-arm.cc:1756: Representation rep = value->representation();
On 2011/05/16 07:06:35, Kevin Millikin wrote:
> Can we rename this to input_rep and assert that instr->representation is
int32?

Done.

http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-arm.cc#newco...
src/arm/lithium-arm.cc:1759: return DefineAsRegister(new
LClampDoubleToUint8(reg, FixedTemp(d1)));
On 2011/05/16 07:06:35, Kevin Millikin wrote:
> Probably needs a comment that the fixed register is because register allocator
> does not (yet) support allocation of double temporaries.  I'd be puzzled if I
> read this code again in a month.

Done.

http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:3356: __ Usat(value, 8, Operand(value));
On 2011/05/16 07:06:35, Kevin Millikin wrote:
> Should this be removed?

Done.

http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:4052: __ b(al, &done);
On 2011/05/16 07:06:35, Kevin Millikin wrote:
> There is __ jmp(&done) for ARM, which just emits __b(al, &done).  I find it
> clearer when reading ARM code but I'm not sure if everyone agrees with that.

Done.

http://codereview.chromium.org/7014033/diff/1025/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:4057: __ vldr(double_scratch0(), scratch0(),
HeapNumber::kValueOffset);
On 2011/05/16 07:06:35, Kevin Millikin wrote:
> Someone has kindly implemented vldr to take a MemOperand!  So you should be
able
> to get rid of the sub and the scratch0 register use and write:
> 
> __ vldr(double_scratch0(), FieldMemOperand(input_reg,
> HeapNumber::kValueOffset));
> 
> I guess this code was copied from somewhere, so you could change it there too.

Done.

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

http://codereview.chromium.org/7014033/diff/1025/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:1061: SetFlag(kFlexibleRepresentation);
On 2011/05/16 07:06:35, Kevin Millikin wrote:
> Everywhere else we have kFlexibleRepresentation, we also have
> set_representation(Representation::Tagged()) to establish a default
> representation.  I don't know that the representation inference algorithm
> depends on it, but maybe it should be here for safety should somebody later
> assume it.

Done.

http://codereview.chromium.org/7014033/diff/1025/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/7014033/diff/1025/src/x64/lithium-codegen-x64....
src/x64/lithium-codegen-x64.cc:3188: {  // Clamp the value to [0..255].
On 2011/05/16 07:06:35, Kevin Millikin wrote:
> Can we remove this clamping?

Done.