ARM: Implement inline conversion of heap numbers to int32 values for bitoperations.

ARM: Implement inline conversion of heap numbers to int32 values for bitoperations.

For bit operations, values are truncated and converted to the range -2^31 through 2^31 - 1. This patch provides inlined handling of this conversion for all heap numbers.

Before, we were calling runtime for every heap number that did not fit into a signed int32.

[Søren Thygesen Gjesse, 2011-03-10 14:20:30]

LGTM

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:406: static void LoadNumberAsInteger(MacroAssembler*
masm,
Maybe this function should have a name that indicate that it will do the
truncate. Also the comment "Floating point value in the 32-bit integer range
will be rounded to an integer." is not totally accurate any more.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:638: CpuFeatures::Scope scope(VFP3);
Should we not support non-vfp here? VFP instructions are only used for loading
the two words of the float as far as I can see.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:646: __ sub(dst, dst, Operand(1023));
1023 -> HeapNumber::kExponentBias

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:652: __ sub(dst, dst, Operand(31));
You could combine this sub with the one before subtracting the bias.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:653: __ cmp(dst, Operand(53));
We have HeapNumber::kMantissaBits which is 52.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:693: __ bind(&neg_shift);
neg_shift -> shift_done?

[Rodolph Perfetta, 2011-03-10 14:36:44]

drive by comments.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:639: __ vldr(double_scratch, FieldMemOperand(object,
HeapNumber::kValueOffset));
Use Ldrd, it will be faster (no transfer VFP-ARM) and you don't need to require
VFP3.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:643: // sign, exponent and mantissa.
the and and mov could be rewritten as a Ubfx

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:678: // scratch3 = 32 - scratch3.
__ rsb(scratch3, scratch3, Operand(32), SetCC) can replace all the instruction
below

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:684: // Negate scratch3.
__ rsb(scratch3, scratch3, Operand(0)) will neagete scratch3

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:696: // Restore sign if necessary.
__ cmp(sign, Operand(0));

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:697: __ tst(sign, sign);
rsb again here.

[Karl Klose, 2011-03-11 09:27:39]

Thank you for the comments, Soeren and Rodolph. I have uploaded a new version,
please have another look.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:406: static void LoadNumberAsInteger(MacroAssembler*
masm,
Done. I renamed the function to ConvertNumberToInt32 and updated the comment,
including a reference to the specification.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:638: CpuFeatures::Scope scope(VFP3);
Done, see comment below.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:639: __ vldr(double_scratch, FieldMemOperand(object,
HeapNumber::kValueOffset));
I decided to use two ldr instructions to avoid putting constraints on the
scratch registers. If this is a major performance issue, I can change it to
ldrd. 

This way the code is also safe for big-endian floating points, although that may
be less relevant.

Either way the function does not require VFP anymore.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:643: // sign, exponent and mantissa.
On 2011/03/10 14:36:44, Rodolph Perfetta wrote:
> the and and mov could be rewritten as a Ubfx

Done.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:646: __ sub(dst, dst, Operand(1023));
On 2011/03/10 14:20:30, Søren Gjesse wrote:
> 1023 -> HeapNumber::kExponentBias

Done.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:652: __ sub(dst, dst, Operand(31));
On 2011/03/10 14:20:30, Søren Gjesse wrote:
> You could combine this sub with the one before subtracting the bias.

Done.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:653: __ cmp(dst, Operand(53));
Done.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:678: // scratch3 = 32 - scratch3.
On 2011/03/10 14:36:44, Rodolph Perfetta wrote:
> __ rsb(scratch3, scratch3, Operand(32), SetCC) can replace all the instruction
> below

Done.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:684: // Negate scratch3.
On 2011/03/10 14:36:44, Rodolph Perfetta wrote:
> __ rsb(scratch3, scratch3, Operand(0)) will neagete scratch3

Done.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:693: __ bind(&neg_shift);
On 2011/03/10 14:20:30, Søren Gjesse wrote:
> neg_shift -> shift_done?

Done.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:696: // Restore sign if necessary.
On 2011/03/10 14:36:44, Rodolph Perfetta wrote:
> __ cmp(sign, Operand(0));

Done.

http://codereview.chromium.org/6658034/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:697: __ tst(sign, sign);
On 2011/03/10 14:36:44, Rodolph Perfetta wrote:
> rsb again here.

Done.

[Søren Thygesen Gjesse, 2011-03-11 10:06:24]

LGTM

http://codereview.chromium.org/6658034/diff/6001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6658034/diff/6001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:637: __ bind(&not_in_int32_range);
There is a macro assembler instruction Ldrd which will eiterh use ldrd or two
ldr instructions.

http://codereview.chromium.org/6658034/diff/6001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:664: // Put implicit 1 before the mantissa part in
scratch2 and
The "and shift the mantissa bits to the correct position." part of the comment
is repeated below.

http://codereview.chromium.org/6658034/diff/6001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:674: //  Shift mantisssa bits in high word.
Two spaces after //

[Alexandre, 2011-03-11 11:08:34]

A suggestion to rename a function and move it to the MacroAssembler.

I implemented the same ecma truncation for bitwise operations using the vfp, but
seeing (and as you suggested) this code manual handling will probably be faster
for non trivial cases.

I'll make a few tests and use this code if it behaves better than the vfp.

http://codereview.chromium.org/6658034/diff/6001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6658034/diff/6001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:403: static void ConvertNumberToInt32(MacroAssembler*
masm,
As mentioned before by Søren, I suggest renaming this to indicate this is a
truncation.
TruncateNumberToInt32?
Maybe even mention that it is for ECMA bitwise operations.

I also suggest moving this to the MacroAssembler.

[Rodolph Perfetta, 2011-03-11 11:09:54]

LGTM.