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.
http://codereview.chromium.org/660072
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
