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.cc#newcode2592 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.cc#newcode2593 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.cc#newcode2596 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.cc#newcode2598 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.cc#newcode2606 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.cc#newcode2606 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.cc#newcode2610 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.cc#newcode2611 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.js#newcode155 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.
http://codereview.chromium.org/7014033/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
