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