Not done. Looks good so far
http://codereview.chromium.org/506052/diff/1/2 File src/ia32/codegen-ia32.cc (right): http://codereview.chromium.org/506052/diff/1/2#newcode7172 src/ia32/codegen-ia32.cc:7172: __ mov(ecx, Immediate(0)); Use xor to zero. http://codereview.chromium.org/506052/diff/1/2#newcode7173 src/ia32/codegen-ia32.cc:7173: // Check whether the exponent matches a 32 bit signed int that is not a Smi. "is not a Smi" -> "cannot be represented as a smi". We have non-smis that could be smis. http://codereview.chromium.org/506052/diff/1/2#newcode7174 src/ia32/codegen-ia32.cc:7174: // A non-Smi integer is 1.xxx * 2^30 so the exponent is 30 (biased). This is "A non-smi 32-bit integer is ..." http://codereview.chromium.org/506052/diff/1/2#newcode7192 src/ia32/codegen-ia32.cc:7192: // We have the big exponent from >>>. ... or from some other operation that gives a result in the range 2^31..2^32-1. http://codereview.chromium.org/506052/diff/1/2#newcode7201 src/ia32/codegen-ia32.cc:7201: // distance. Change last sentence to (something like): We have kExponentShift+1 significant bits in the low end of the word. Shift them to to the top bits. http://codereview.chromium.org/506052/diff/1/2#newcode7206 src/ia32/codegen-ia32.cc:7206: // Shift down 21 bits to get the last 11 bits. Explain, or replace, the constants: 21 = big_shift_distance 11 = 32 - big_shift_distance http://codereview.chromium.org/506052/diff/1/2#newcode7213 src/ia32/codegen-ia32.cc:7213: __ neg(ecx); Slightly shorter: test(scratch, scratch) j(positive, &done) neg(ecx) http://codereview.chromium.org/506052/diff/1/2#newcode7216 src/ia32/codegen-ia32.cc:7216: // End of handling for big exponent. You could wrap the entire big-exponent section in a statement block. It gives scope to the variables and groups and indents the code for easier reading. http://codereview.chromium.org/506052/diff/1/2#newcode7224 src/ia32/codegen-ia32.cc:7224: (HeapNumber::kExponentBias + 0) << HeapNumber::kExponentShift; I think (0 + HeapNumber::kExponentBias) is more readable (but that's obviuously a matter of taste). http://codereview.chromium.org/506052/diff/1/2#newcode7232 src/ia32/codegen-ia32.cc:7232: __ sub(ecx, Operand(scratch2)); Complex rewrite: If you make ecx one larger here (loading immediate 31), and then below increase shift_distance by one as well to shift out the sign bit out, then the shr_cl further down will shift in a zero sign bit. This allows you to drop the and_ with kMantissaMask. (I think.) http://codereview.chromium.org/506052/diff/1/2#newcode7242 src/ia32/codegen-ia32.cc:7242: // we want to leave the sign bit 0 so we subtract 2 bits from the shift "leave the sign bit 0" -> "leave a zero sign bit" http://codereview.chromium.org/506052/diff/1/2#newcode7248 src/ia32/codegen-ia32.cc:7248: // it's probably slower to test than just to do it. True. The load is probably from the first level cache. http://codereview.chromium.org/506052/diff/1/2#newcode7250 src/ia32/codegen-ia32.cc:7250: // Shift down 22 bits to get the last 10 bits. Explain or replace constants. http://codereview.chromium.org/506052/diff/1/2#newcode7267 src/ia32/codegen-ia32.cc:7267: Only reviewed until here. http://codereview.chromium.org/506052 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
