ARM: Implement correct rounding in the lithium codegenerator.

ARM: Implement correct rounding in the lithium codegenerator.

This patch provides testing for values that have a fraction part of 0.5 and uses a different rounding method for them. The original method of VFPTruncating the value with round-to-nearest is not correct because it does not round to the larger number in case of a tie.

BUG=http://code.google.com/p/v8/issues/detail?id=958

[Lasse Reichstein, 2011-04-14 19:09:04]

LGTM, but I think it can be simplified in the general case.

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

http://codereview.chromium.org/6840051/diff/7001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:2885: __ cmp(scratch3, Operand(1024));
You can deoptimize for all exponents above or equal to 31 (or 32, if you don't
want to miss -(2^31)). The value won't fit in an int32 in any case.

http://codereview.chromium.org/6840051/diff/7001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:2897: __ b(lt, &adjust_sign_on_zero);
You can jump to after the zero-test at adjust_sign_on_zero.
Or just jump directly to done if we don't care about -0's.

http://codereview.chromium.org/6840051/diff/7001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:2901: __ cmp(scratch3,
Operand(HeapNumber::kMantissaBits));
If you bailed out for exponent >= 31, this isn't needed.

http://codereview.chromium.org/6840051/diff/7001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:2905: // Check if the bit we want to test is in
the upper or lower word.
The bit we need to test is the one with value 0.5, right?
(I.e., more explanation of what we are trying to do here)

http://codereview.chromium.org/6840051/diff/7001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:2906: __ cmp(scratch3,
Operand(HeapNumber::kMantissaBitsInTopWord - 1));
You might be able to save a few instructions here by doing:
rsb(scratch3, scratch3, Operand(HeapNumber::kMantissaBitsInTopWord - 1))
mov(scratch1, scratch2, LeaveCC, ge);
b(ge, &test);
// lower_word here.
add(scratch3, scratch3, Operand(HeapNumber::kMantissaBits -
HeapNumber::kMantissaBitsInTopWord));

http://codereview.chromium.org/6840051/diff/7001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:2939: // the normal case.
If you are going to add 0.5 and round in half the cases, you might as well just
add 0.5 and round down for all numbers except [-0.5..0.5[.

The only cases where adding 0.5 and rounding down does not work is when the 0.5
bit is just outside the range of bits in the double, so adding it will lose
precision (i.e., (+/-) 0.49999999999999994 and [2^52..2^53-1], and the latter is
outside the int32 range we are aiming for).
I.e., you only need the special case for -0.5 and for exponent < -1 (the
negative ones round to -0 and the positive ones doesn't all work with
floor(x+0.5)).

[Søren Thygesen Gjesse, 2011-04-15 07:55:34]

LGMT

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

http://codereview.chromium.org/6840051/diff/7001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:2904: // Exponent is greater or equal zero and
below HeapNumber::kMantissaBits.
equal zero -> equal to zero

[Karl Klose, 2011-04-15 09:24:17]

I have uploaded a shorter version based on Lasse's comments. Lasse, can you have
a look?

[Rodolph Perfetta, 2011-04-15 09:37:54]

drive by comments

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

http://codereview.chromium.org/6840051/diff/11001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2893: // Construct and add 0.5 to the input.
there is a vmov immediate instruction which for 0.5 will translate into one VFP
instruction.

[Lasse Reichstein, 2011-04-15 09:42:04]

LGTM

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

http://codereview.chromium.org/6840051/diff/11001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2902: __ and_(scratch1, scratch1,
Operand(HeapNumber::kSignMask));
Just eor scratch3 with scratch1 and make sure it sets the flags - if the result
is negative, the sign changed.

[Karl Klose, 2011-04-15 11:26:06]

Thank you for the review. Using the immediate vmov also frees a register, which
means that we don't need a temp register for the instruction.

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

http://codereview.chromium.org/6840051/diff/11001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2893: // Construct and add 0.5 to the input.
On 2011/04/15 09:37:54, Rodolph Perfetta wrote:
> there is a vmov immediate instruction which for 0.5 will translate into one
VFP
> instruction.

Done.

http://codereview.chromium.org/6840051/diff/11001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2902: __ and_(scratch1, scratch1,
Operand(HeapNumber::kSignMask));
On 2011/04/15 09:42:04, Lasse Reichstein wrote:
> Just eor scratch3 with scratch1 and make sure it sets the flags - if the
result
> is negative, the sign changed.

Done.