ARM: Use the vsqrt instruction when available...

ARM: Use the vsqrt instruction when available

vsqrt is used to calculate Math.sqrt(x), Math.pow(x, 0.5) and Math.pow(x, -0.5). Code size doesn't matter, as %_MathSqrt and %_MathPow are only called in one place each.

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

[Erik Corry, 2010-06-28 12:38:32]

LGTM

http://codereview.chromium.org/2885002/diff/19001/20001
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/2885002/diff/19001/20001#newcode2115
src/arm/assembler-arm.cc:2115: 
1 blank line too many

http://codereview.chromium.org/2885002/diff/26001/24004
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/2885002/diff/26001/24004#newcode4294
src/arm/codegen-arm.cc:4294: Label not_minus_half, allocate_return, runtime;
If you use JumpTarget here instead of Label then you shouldn't have to handle
the virtual frame manually below.

http://codereview.chromium.org/2885002/diff/26001/24004#newcode4300
src/arm/codegen-arm.cc:4300: frame_->SpillAllButCopyTOSToR1R0();
I think you could just use PopToRegister here and not force a particular
ordering on the two arguments.  That means you have to push them again where you
call the runtime, but since they remain untouched up to that point that
shouldn't be so hard.

http://codereview.chromium.org/2885002/diff/26001/24004#newcode4311
src/arm/codegen-arm.cc:4311: __ LoadRoot(r6, Heap::kHeapNumberMapRootIndex);
Here you should use heap_number_map instead of r6.

http://codereview.chromium.org/2885002/diff/26001/24004#newcode4322
src/arm/codegen-arm.cc:4322: __ b(ne, &not_minus_half);
Might as well go straight to runtime here.

http://codereview.chromium.org/2885002/diff/26001/24004#newcode4334
src/arm/codegen-arm.cc:4334: __ mov(scratch2, Operand(0x3ff00000));
for a tiny microoptimization reverse the order of these two 'mov's.

http://codereview.chromium.org/2885002/diff/26001/24004#newcode4347
src/arm/codegen-arm.cc:4347: __ b(ne, &runtime);
You can remove this test if you went straight to runtime above.

http://codereview.chromium.org/2885002/diff/26001/24004#newcode4395
src/arm/codegen-arm.cc:4395: frame()->Dup();
I think you can just push it at the point where you call the runtime.

http://codereview.chromium.org/2885002/diff/26001/24006
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/2885002/diff/26001/24006#newcode1400
src/arm/macro-assembler-arm.cc:1400: cmp(scratch1,
Operand(HeapNumber::kExponentMask));
You can do Sbfx and then compare with -1 here.  That avoids two loads to ip.

http://codereview.chromium.org/2885002/diff/26001/24007
File src/arm/macro-assembler-arm.h (right):

http://codereview.chromium.org/2885002/diff/26001/24007#newcode77
src/arm/macro-assembler-arm.h:77: AVOID_NANS_AND_INFINITIES = 1 << 2
This should probably be 1 << 1.

http://codereview.chromium.org/2885002/diff/26001/24007#newcode498
src/arm/macro-assembler-arm.h:498: Register scratch2,
I think it's rather confusing that the order of the scratch and heap_number_map
registers is different here than it is in AllocateHeapNumberWithValue.

http://codereview.chromium.org/2885002/diff/26001/24008
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/2885002/diff/26001/24008#newcode2291
src/arm/simulator-arm.cc:2291: double dd_value = sqrt(dm_value);
You need to include math.h for sqrt.

[Rico, 2010-06-28 12:43:59]

Drive by comment.

http://codereview.chromium.org/2885002/diff/26001/24004
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/2885002/diff/26001/24004#newcode4282
src/arm/codegen-arm.cc:4282: // Generates the Math.pow method - currently just
calls runtime.
Remove text after hyphen in comment above

[Søren Thygesen Gjesse, 2010-06-29 09:35:26]

http://codereview.chromium.org/2885002/diff/26001/24004
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/2885002/diff/26001/24004#newcode4282
src/arm/codegen-arm.cc:4282: // Generates the Math.pow method - currently just
calls runtime.
On 2010/06/28 12:44:00, Rico wrote:
> Remove text after hyphen in comment above 

Done.

http://codereview.chromium.org/2885002/diff/26001/24004#newcode4294
src/arm/codegen-arm.cc:4294: Label not_minus_half, allocate_return, runtime;
On 2010/06/28 12:38:32, Erik Corry wrote:
> If you use JumpTarget here instead of Label then you shouldn't have to handle
> the virtual frame manually below.

Changed runtime to a jump target. Kept not_minus_half and allocate_return as
labels.

http://codereview.chromium.org/2885002/diff/26001/24004#newcode4300
src/arm/codegen-arm.cc:4300: frame_->SpillAllButCopyTOSToR1R0();
On 2010/06/28 12:38:32, Erik Corry wrote:
> I think you could just use PopToRegister here and not force a particular
> ordering on the two arguments.  That means you have to push them again where
you
> call the runtime, but since they remain untouched up to that point that
> shouldn't be so hard.

Done.

http://codereview.chromium.org/2885002/diff/26001/24004#newcode4311
src/arm/codegen-arm.cc:4311: __ LoadRoot(r6, Heap::kHeapNumberMapRootIndex);
On 2010/06/28 12:38:32, Erik Corry wrote:
> Here you should use heap_number_map instead of r6.

Done.

http://codereview.chromium.org/2885002/diff/26001/24004#newcode4322
src/arm/codegen-arm.cc:4322: __ b(ne, &not_minus_half);
On 2010/06/28 12:38:32, Erik Corry wrote:
> Might as well go straight to runtime here.

Done.

http://codereview.chromium.org/2885002/diff/26001/24004#newcode4334
src/arm/codegen-arm.cc:4334: __ mov(scratch2, Operand(0x3ff00000));
On 2010/06/28 12:38:32, Erik Corry wrote:
> for a tiny microoptimization reverse the order of these two 'mov's.

Done.

http://codereview.chromium.org/2885002/diff/26001/24004#newcode4347
src/arm/codegen-arm.cc:4347: __ b(ne, &runtime);
On 2010/06/28 12:38:32, Erik Corry wrote:
> You can remove this test if you went straight to runtime above.

Done.

http://codereview.chromium.org/2885002/diff/26001/24004#newcode4395
src/arm/codegen-arm.cc:4395: frame()->Dup();
On 2010/06/28 12:38:32, Erik Corry wrote:
> I think you can just push it at the point where you call the runtime.

Done.

http://codereview.chromium.org/2885002/diff/26001/24006
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/2885002/diff/26001/24006#newcode1400
src/arm/macro-assembler-arm.cc:1400: cmp(scratch1,
Operand(HeapNumber::kExponentMask));
On 2010/06/28 12:38:32, Erik Corry wrote:
> You can do Sbfx and then compare with -1 here.  That avoids two loads to ip.

Done.

http://codereview.chromium.org/2885002/diff/26001/24007
File src/arm/macro-assembler-arm.h (right):

http://codereview.chromium.org/2885002/diff/26001/24007#newcode77
src/arm/macro-assembler-arm.h:77: AVOID_NANS_AND_INFINITIES = 1 << 2
On 2010/06/28 12:38:32, Erik Corry wrote:
> This should probably be 1 << 1.

Done.

http://codereview.chromium.org/2885002/diff/26001/24007#newcode498
src/arm/macro-assembler-arm.h:498: Register scratch2,
On 2010/06/28 12:38:32, Erik Corry wrote:
> I think it's rather confusing that the order of the scratch and
heap_number_map
> registers is different here than it is in AllocateHeapNumberWithValue.

Done.

http://codereview.chromium.org/2885002/diff/26001/24008
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/2885002/diff/26001/24008#newcode2291
src/arm/simulator-arm.cc:2291: double dd_value = sqrt(dm_value);
On 2010/06/28 12:38:32, Erik Corry wrote:
> You need to include math.h for sqrt.

Done.