Added fast support for Math.pow. This simply calculates the result using the...

Added fast support for Math.pow. This simply calculates the result using the
same method as the old powi version in runtime.cc and also checks if
the exponent is 0.5 or -0.5 in which case we calculate the square root or 
reciprocal value of the square root.



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

[Rico, 2010-02-25 07:48:19]

Please look at this. A few notes:
I had to update the amount of available xmm registers in assembler-ia32.h to 4
from 2 - I do not know if this has any other implications.

[Lasse Reichstein, 2010-02-25 10:36:33]

LGTM if bugs fixed.

http://codereview.chromium.org/660072/diff/1028/1032
File src/ia32/assembler-ia32.h (right):

http://codereview.chromium.org/660072/diff/1028/1032#newcode96
src/ia32/assembler-ia32.h:96: bool is_valid() const  { return 0 <= code_ &&
code_ < 4; }  // currently
This looks odd. I think you should just set it to 8.
Otherwise we create a lot of invalid registers below.

If something uses XMMRegister.is_valid for anything except checking that the
register is a valid argument to an assembler instruction, they are misusing it
(but we should check that it doesn't happen ofcourse).

I don't think it's used anywhere except in the code() method. Try making the
is_valid() method private and see if it still compiles (if not, it'll tell us
where else it's used)

http://codereview.chromium.org/660072/diff/1028/1029
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/660072/diff/1028/1029#newcode5290
src/ia32/codegen-ia32.cc:5290: Result p = allocator()->Allocate(eax);
Why does it have to be eax? Could you just allocate any register?

http://codereview.chromium.org/660072/diff/1028/1029#newcode5293
src/ia32/codegen-ia32.cc:5293: if (!(p.is_valid() && p.reg().is(eax)))
Always put braces around then-branch if on separate line.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5294
src/ia32/codegen-ia32.cc:5294: __ jmp(&go_runtime);
In this case, you can just skip all the code up to bind(&go_runtime). It will
never be executed anyway.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5302
src/ia32/codegen-ia32.cc:5302: Label Y_or_both_is_double;
Y->y

http://codereview.chromium.org/660072/diff/1028/1029#newcode5336
src/ia32/codegen-ia32.cc:5336: Label no_infinity;
Seems you only need to test this in the case where y is negative.
Also, you check that x * y < 999, but what you really need to know is whether
exponent(x) * y < 1022 (i.e., count the number of bits of the result). You have
x in a register, so you should be able to extract the exponent by bit-fiddling.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5343
src/ia32/codegen-ia32.cc:5343: __ mov(p.reg(), Immediate(999));
Give the constant a meaningful name (if one exists).

http://codereview.chromium.org/660072/diff/1028/1029#newcode5351
src/ia32/codegen-ia32.cc:5351: __ cvtsi2sd(xmm1, Operand(p.reg()));
This is the second time you load 1 into xmm1. Could you have 1 stored in another
register for later use?

http://codereview.chromium.org/660072/diff/1028/1029#newcode5360
src/ia32/codegen-ia32.cc:5360: __ sar(y.reg(), 1);
Perhaps use logic shift so the reader won't have to think to see that it will
always terminate :)

http://codereview.chromium.org/660072/diff/1028/1029#newcode5361
src/ia32/codegen-ia32.cc:5361: __ test(y.reg(), Immediate(~0));
Just use test_(y.reg(), y.reg()) to check if it's zero.
In this case, the shift operation sets the Z flag, so you don't need any further
tests.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5369
src/ia32/codegen-ia32.cc:5369: __ cmp(x.reg(), 0);
To test for negativity, use 
 test(x.reg(), x.reg()); 
 j(positive, ...)
It saves the 32-bit immediate in the cmp operation.
(General rule: Never compare to 0, use test with self instead, it sets exactly
the same flags).

http://codereview.chromium.org/660072/diff/1028/1029#newcode5372
src/ia32/codegen-ia32.cc:5372: __ cvtsi2sd(xmm0, Operand(x.reg()));
Here we load 1 again.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5373
src/ia32/codegen-ia32.cc:5373: __ divsd(xmm0, xmm1);
Amazing how SSE has RCPSS but not RCPSD. 
And RSQRTSS, but not RSQRTSD.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5374
src/ia32/codegen-ia32.cc:5374: __ AllocateHeapNumber(p.reg(), y.reg(), x.reg(),
&go_runtime);
You have two versions of AllocateHeapNumber here. You could just move xmm0 to
xmm1 and reuse the latter one.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5384
src/ia32/codegen-ia32.cc:5384: __ bind(&Y_or_both_is_double);
Better name for label is (something like) y_nonsmi.
It's confusing that you are at a label that says that y is a double, and then
start out by testing whether it really is a HeapNumber.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5390
src/ia32/codegen-ia32.cc:5390: __ cmp(Operand(p.reg()), Immediate(0xfff00000));
As discussed offline, this doesn't work, and it's not needed either (since we go
to runtime in any case if y isn't +/-0.5).

http://codereview.chromium.org/660072/diff/1028/1029#newcode5393
src/ia32/codegen-ia32.cc:5393: // Y must be a double.
How about checking whether y is +/-0.5 at this point (it's likely to be not the
case, and then you can go directly to runtime).
+/- 0.5 has the bit pattern: 0x3fe0000000000000 + optional sign bit. Maybe it's
easier to test directly on the heap number value's exponent/mantissa halves.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5407
src/ia32/codegen-ia32.cc:5407: __ mov(p.reg(), FieldOperand(x.reg(),
HeapNumber::kExponentOffset));
To test for NaN/Infinity, you should and p.reg() with HeapNumber::kExponentMask
before comparing it (and compare it to kExponentMask as well, to use a named
constant).

http://codereview.chromium.org/660072/diff/1028/1029#newcode5416
src/ia32/codegen-ia32.cc:5416: // Load xmm2 with -0.5.
How about doing:
 mov(p.reg(), "single precission representation of -0.5");
 movd(xmm2, p.reg());
 cvtss2sd(xmm2, xmm2);
Seems like it should be faster than doing the div.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5463
src/ia32/codegen-ia32.cc:5463: __ bind(&return_preg);
Could you allocate the heap number here instead, and always store xmm1 into it.
Then all the places above wouldn't need to duplicate the heap allocation code
(just before they jump here).
You would ofcourse have to jump accros the allocation from the runtime call
result.

[Rico, 2010-02-26 08:01:36]

I addressed the issues and made a few extra adjustments making it more simple -
could you please have another look to see if everything seems reasonable.

http://codereview.chromium.org/660072/diff/1028/1032
File src/ia32/assembler-ia32.h (right):

http://codereview.chromium.org/660072/diff/1028/1032#newcode96
src/ia32/assembler-ia32.h:96: bool is_valid() const  { return 0 <= code_ &&
code_ < 4; }  // currently
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> This looks odd. I think you should just set it to 8.
> Otherwise we create a lot of invalid registers below.
> 
> If something uses XMMRegister.is_valid for anything except checking that the
> register is a valid argument to an assembler instruction, they are misusing it
> (but we should check that it doesn't happen ofcourse).
> 
> I don't think it's used anywhere except in the code() method. Try making the
> is_valid() method private and see if it still compiles (if not, it'll tell us
> where else it's used)

I changed it to 8

http://codereview.chromium.org/660072/diff/1028/1029
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/660072/diff/1028/1029#newcode5290
src/ia32/codegen-ia32.cc:5290: Result p = allocator()->Allocate(eax);
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> Why does it have to be eax? Could you just allocate any register?
If we bail out to runtime the result is in eax - so we might as well use this
one.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5293
src/ia32/codegen-ia32.cc:5293: if (!(p.is_valid() && p.reg().is(eax)))
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> Always put braces around then-branch if on separate line.

Done.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5294
src/ia32/codegen-ia32.cc:5294: __ jmp(&go_runtime);
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> In this case, you can just skip all the code up to bind(&go_runtime). It will
> never be executed anyway.
> 
> 

Done.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5302
src/ia32/codegen-ia32.cc:5302: Label Y_or_both_is_double;
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> Y->y

Done.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5336
src/ia32/codegen-ia32.cc:5336: Label no_infinity;
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> Seems you only need to test this in the case where y is negative.
> Also, you check that x * y < 999, but what you really need to know is whether
> exponent(x) * y < 1022 (i.e., count the number of bits of the result). You
have
> x in a register, so you should be able to extract the exponent by
bit-fiddling.

I changed this to only check after we have calculated the result and if y is
negative (i.e., if y <0 and xmm1=infinity   we go to runtime)

http://codereview.chromium.org/660072/diff/1028/1029#newcode5343
src/ia32/codegen-ia32.cc:5343: __ mov(p.reg(), Immediate(999));
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> Give the constant a meaningful name (if one exists).
See comment above

http://codereview.chromium.org/660072/diff/1028/1029#newcode5351
src/ia32/codegen-ia32.cc:5351: __ cvtsi2sd(xmm1, Operand(p.reg()));
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> This is the second time you load 1 into xmm1. Could you have 1 stored in
another
> register for later use?

Done.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5360
src/ia32/codegen-ia32.cc:5360: __ sar(y.reg(), 1);
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> Perhaps use logic shift so the reader won't have to think to see that it will
> always terminate :)

Done.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5361
src/ia32/codegen-ia32.cc:5361: __ test(y.reg(), Immediate(~0));
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> Just use test_(y.reg(), y.reg()) to check if it's zero.
> In this case, the shift operation sets the Z flag, so you don't need any
further
> tests.

Done.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5369
src/ia32/codegen-ia32.cc:5369: __ cmp(x.reg(), 0);
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> To test for negativity, use 
>  test(x.reg(), x.reg()); 
>  j(positive, ...)
> It saves the 32-bit immediate in the cmp operation.
> (General rule: Never compare to 0, use test with self instead, it sets exactly
> the same flags).

Done.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5372
src/ia32/codegen-ia32.cc:5372: __ cvtsi2sd(xmm0, Operand(x.reg()));
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> Here we load 1 again.

Done.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5373
src/ia32/codegen-ia32.cc:5373: __ divsd(xmm0, xmm1);
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> Amazing how SSE has RCPSS but not RCPSD. 
> And RSQRTSS, but not RSQRTSD.
> 
Yes

http://codereview.chromium.org/660072/diff/1028/1029#newcode5374
src/ia32/codegen-ia32.cc:5374: __ AllocateHeapNumber(p.reg(), y.reg(), x.reg(),
&go_runtime);
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> You have two versions of AllocateHeapNumber here. You could just move xmm0 to
> xmm1 and reuse the latter one.
> 
Moved all allocation to the allocate_and_return label and jump to there from all
allocations. At that point we assume that the right value is in xmm1.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5384
src/ia32/codegen-ia32.cc:5384: __ bind(&Y_or_both_is_double);
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> Better name for label is (something like) y_nonsmi.
> It's confusing that you are at a label that says that y is a double, and then
> start out by testing whether it really is a HeapNumber.

Done.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5390
src/ia32/codegen-ia32.cc:5390: __ cmp(Operand(p.reg()), Immediate(0xfff00000));
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> As discussed offline, this doesn't work, and it's not needed either (since we
go
> to runtime in any case if y isn't +/-0.5).

Done.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5393
src/ia32/codegen-ia32.cc:5393: // Y must be a double.
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> How about checking whether y is +/-0.5 at this point (it's likely to be not
the
> case, and then you can go directly to runtime).
> +/- 0.5 has the bit pattern: 0x3fe0000000000000 + optional sign bit. Maybe
it's
> easier to test directly on the heap number value's exponent/mantissa halves.
After changing the special case handling below I think the extra check here
might not be worthwhile - but if you think it is still better I will put it in (
I do not think that we will get here very often in any case -
Math.pow(something, float) is probably very uncommon in any case - except those
people using it for sqrt and 1/sqrt - and not many people do this anyway)

http://codereview.chromium.org/660072/diff/1028/1029#newcode5407
src/ia32/codegen-ia32.cc:5407: __ mov(p.reg(), FieldOperand(x.reg(),
HeapNumber::kExponentOffset));
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> To test for NaN/Infinity, you should and p.reg() with
HeapNumber::kExponentMask
> before comparing it (and compare it to kExponentMask as well, to use a named
> constant).

Done.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5416
src/ia32/codegen-ia32.cc:5416: // Load xmm2 with -0.5.
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> How about doing:
>  mov(p.reg(), "single precission representation of -0.5");
>  movd(xmm2, p.reg());
>  cvtss2sd(xmm2, xmm2);
> Seems like it should be faster than doing the div.

Done.

http://codereview.chromium.org/660072/diff/1028/1029#newcode5463
src/ia32/codegen-ia32.cc:5463: __ bind(&return_preg);
On 2010/02/25 10:36:33, Lasse Reichstein wrote:
> Could you allocate the heap number here instead, and always store xmm1 into
it.
> Then all the places above wouldn't need to duplicate the heap allocation code
> (just before they jump here).
> You would ofcourse have to jump accros the allocation from the runtime call
> result.
Done - but I put it before unusing x,y,p so I still have those registers to use
as scratch registers.

[Lasse Reichstein, 2010-02-26 09:06:19]

LGTM with suggestions.

http://codereview.chromium.org/660072/diff/1062/1063
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/660072/diff/1062/1063#newcode5345
src/ia32/codegen-ia32.cc:5345: __ test(y.reg(), Immediate(1));
If you shr(y.reg(),1) here, then the bit that was shifted out is in the carry
flag. I.e.:
shr(y.reg(), 1);
j(not_carry, &no_multiply);
mulsd(xmm1, xmm0);
bind(&no_multiply);

http://codereview.chromium.org/660072/diff/1062/1063#newcode5351
src/ia32/codegen-ia32.cc:5351: __ j(zero, &powi_done);
Just do:
 mulsd(xmm0, xmm0);
 j(not_zero, &while_true);  // mulsd doesn't affect flags.

It does one more mulsd than necessary, but on, e.g., a core2 chip, that's just
one cycle, and it saves an instruction on each iteration of the loop.

This and he previous comment should reduce the loop to four operations. That's
probably only a space saving, since some of the multiplications are dependent
and have a five cycle latency (and probably want to use the same ALU).

http://codereview.chromium.org/660072/diff/1062/1063#newcode5360
src/ia32/codegen-ia32.cc:5360: __ mov(p.reg(), Immediate(0x7FF00000));
I don't think this is single-precission infinity.
Single precission floats only have eight exponent bits.
Positive infinity would be 0x7f800000.
(And 0x7ff00000 is a quiet NaN with payload).

Do we need a better test for this case? :)

http://codereview.chromium.org/660072/diff/1062/1063#newcode5363
src/ia32/codegen-ia32.cc:5363: __ comisd(xmm0, xmm1);
Use ucomisd instead of comisd.

http://codereview.chromium.org/660072/diff/1062/1063#newcode5376
src/ia32/codegen-ia32.cc:5376: __ comisd(xmm1, xmm1);
ucomisd

http://codereview.chromium.org/660072/diff/1062/1063#newcode5395
src/ia32/codegen-ia32.cc:5395: __ cmp(Operand(p.reg()),
Immediate(HeapNumber::kExponentMask));
Add comment here saying something like "p is NaN or +/-Infinity"

http://codereview.chromium.org/660072/diff/1062/1063#newcode5409
src/ia32/codegen-ia32.cc:5409: __ comisd(xmm2, xmm1);
ucomisd

http://codereview.chromium.org/660072/diff/1062/1063#newcode5413
src/ia32/codegen-ia32.cc:5413: __ sqrt(xmm0, xmm0);
Why isn't sqrt called sqrtsd?

http://codereview.chromium.org/660072/diff/1062/1063#newcode5415
src/ia32/codegen-ia32.cc:5415: __ movsd(xmm1, xmm3);
Move sqrt to the end (i.e., sqrt(xmm1,xmm1)) since it'shas a higher latency than
movsd.
(And perhaps comment that 1/sqrt(x) = sqrt(1/x)).

http://codereview.chromium.org/660072/diff/1062/1063#newcode5421
src/ia32/codegen-ia32.cc:5421: __ mov(p.reg(), Immediate(0x3F000000));
You have -0.5 in xmm2 already, and 1.0 in xmm3, so just do
addsd(xmm2, xmm3) to get +0.5.

http://codereview.chromium.org/660072/diff/1062/1063#newcode5425
src/ia32/codegen-ia32.cc:5425: __ comisd(xmm2, xmm1);
ucomisd

http://codereview.chromium.org/660072/diff/1062/1063#newcode5428
src/ia32/codegen-ia32.cc:5428: __ sqrt(xmm0, xmm0);
do 
 movsd(xmm1, xmm0);
 sqrt(xmm1, xmm1);
instead, to allow the latency of the sqrt operation to be spent during the heap
allocation code, instead of stalling on the move.

[Rico, 2010-03-04 06:39:29]

http://codereview.chromium.org/660072/diff/1062/1063
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/660072/diff/1062/1063#newcode5345
src/ia32/codegen-ia32.cc:5345: __ test(y.reg(), Immediate(1));
On 2010/02/26 09:06:20, Lasse Reichstein wrote:
> If you shr(y.reg(),1) here, then the bit that was shifted out is in the carry
> flag. I.e.:
> shr(y.reg(), 1);
> j(not_carry, &no_multiply);
> mulsd(xmm1, xmm0);
> bind(&no_multiply);

Done.

http://codereview.chromium.org/660072/diff/1062/1063#newcode5351
src/ia32/codegen-ia32.cc:5351: __ j(zero, &powi_done);
On 2010/02/26 09:06:20, Lasse Reichstein wrote:
> Just do:
>  mulsd(xmm0, xmm0);
>  j(not_zero, &while_true);  // mulsd doesn't affect flags.
> 
> It does one more mulsd than necessary, but on, e.g., a core2 chip, that's just
> one cycle, and it saves an instruction on each iteration of the loop.
> 
> This and he previous comment should reduce the loop to four operations. That's
> probably only a space saving, since some of the multiplications are dependent
> and have a five cycle latency (and probably want to use the same ALU).

Done.

http://codereview.chromium.org/660072/diff/1062/1063#newcode5360
src/ia32/codegen-ia32.cc:5360: __ mov(p.reg(), Immediate(0x7FF00000));
On 2010/02/26 09:06:20, Lasse Reichstein wrote:
> I don't think this is single-precission infinity.
> Single precission floats only have eight exponent bits.
> Positive infinity would be 0x7f800000.
> (And 0x7ff00000 is a quiet NaN with payload).
> 
> Do we need a better test for this case? :)
As discussed offline this will always branch to runtime because of the way
comisd works, I have changed it so that it only goes to runtime if it is
actually infinity.
We already have tests for this (but they where not hit since runtime executes
correctly and we always went there if y is an negative integer).

http://codereview.chromium.org/660072/diff/1062/1063#newcode5363
src/ia32/codegen-ia32.cc:5363: __ comisd(xmm0, xmm1);
On 2010/02/26 09:06:20, Lasse Reichstein wrote:
> Use ucomisd instead of comisd.
> 

Done - added ucomisd to assembler-ia32

http://codereview.chromium.org/660072/diff/1062/1063#newcode5376
src/ia32/codegen-ia32.cc:5376: __ comisd(xmm1, xmm1);
On 2010/02/26 09:06:20, Lasse Reichstein wrote:
> ucomisd

Done.

http://codereview.chromium.org/660072/diff/1062/1063#newcode5395
src/ia32/codegen-ia32.cc:5395: __ cmp(Operand(p.reg()),
Immediate(HeapNumber::kExponentMask));
On 2010/02/26 09:06:20, Lasse Reichstein wrote:
> Add comment here saying something like "p is NaN or +/-Infinity"

Done.

http://codereview.chromium.org/660072/diff/1062/1063#newcode5409
src/ia32/codegen-ia32.cc:5409: __ comisd(xmm2, xmm1);
On 2010/02/26 09:06:20, Lasse Reichstein wrote:
> ucomisd
> 

Done.

http://codereview.chromium.org/660072/diff/1062/1063#newcode5413
src/ia32/codegen-ia32.cc:5413: __ sqrt(xmm0, xmm0);
On 2010/02/26 09:06:20, Lasse Reichstein wrote:
> Why isn't sqrt called sqrtsd?
Changed name and moved down to sse2 instructions.

http://codereview.chromium.org/660072/diff/1062/1063#newcode5415
src/ia32/codegen-ia32.cc:5415: __ movsd(xmm1, xmm3);
On 2010/02/26 09:06:20, Lasse Reichstein wrote:
> Move sqrt to the end (i.e., sqrt(xmm1,xmm1)) since it'shas a higher latency
than
> movsd.
> (And perhaps comment that 1/sqrt(x) = sqrt(1/x)).
> 

Done.

http://codereview.chromium.org/660072/diff/1062/1063#newcode5421
src/ia32/codegen-ia32.cc:5421: __ mov(p.reg(), Immediate(0x3F000000));
On 2010/02/26 09:06:20, Lasse Reichstein wrote:
> You have -0.5 in xmm2 already, and 1.0 in xmm3, so just do
> addsd(xmm2, xmm3) to get +0.5.
> 

Done.

http://codereview.chromium.org/660072/diff/1062/1063#newcode5425
src/ia32/codegen-ia32.cc:5425: __ comisd(xmm2, xmm1);
On 2010/02/26 09:06:20, Lasse Reichstein wrote:
> ucomisd
> 

Done.

http://codereview.chromium.org/660072/diff/1062/1063#newcode5428
src/ia32/codegen-ia32.cc:5428: __ sqrt(xmm0, xmm0);
On 2010/02/26 09:06:20, Lasse Reichstein wrote:
> do 
>  movsd(xmm1, xmm0);
>  sqrt(xmm1, xmm1);
> instead, to allow the latency of the sqrt operation to be spent during the
heap
> allocation code, instead of stalling on the move.

Done.