LGTM.
http://codereview.chromium.org/3054047/diff/1/3 File src/codegen.h (right): http://codereview.chromium.org/3054047/diff/1/3#newcode324 src/codegen.h:324: virtual bool AutoSaveAndRestore() { return true; } I know it's not widespread in V8, but please add comments saying what the functions do. http://codereview.chromium.org/3054047/diff/1/6 File src/ia32/codegen-ia32.cc (right): http://codereview.chromium.org/3054047/diff/1/6#newcode1255 src/ia32/codegen-ia32.cc:1255: __ JumpIfNotInt32(left_, dst_, left_info_, entry_label()); The JumpIfNotInt32 name seems incorrect if it accepts fractions (and if it doesn't, the next cvt doesn't need to be truncating). Does it check that the value is <2^31 and >=-2^31? http://codereview.chromium.org/3054047/diff/1/6#newcode1260 src/ia32/codegen-ia32.cc:1260: CpuFeatures::Scope use_sse2(SSE2); What if SSE2 is not supported? (It was tested in the case above, so if it can't happen here, make a comment saying why). http://codereview.chromium.org/3054047/diff/1/6#newcode1340 src/ia32/codegen-ia32.cc:1340: __ push(left_); // 0 or 0x80000000. Make comment and/or ASSERT saying that left_ must be a smi. http://codereview.chromium.org/3054047/diff/1/8 File src/ia32/macro-assembler-ia32.cc (right): http://codereview.chromium.org/3054047/diff/1/8#newcode1540 src/ia32/macro-assembler-ia32.cc:1540: j(greater, on_not_int32); Is this faster than using SSE2 cvttsd2si and checking against 0x8000000? Even if it is, using cvttsd2si would also give the 32-bit number as a result immediately, instead of first doing this test and then converting afterwards, as it seemed was the typical use of this function. E.g., I'm suggesting a: NumberToInt32(Register dst, Register src, TypeInfo info, Label* on_not_int32) http://codereview.chromium.org/3054047/diff/1/9 File src/ia32/macro-assembler-ia32.h (right): http://codereview.chromium.org/3054047/diff/1/9#newcode229 src/ia32/macro-assembler-ia32.h:229: void SmiUntagAndBranchOnNonSmi(Register reg, TypeInfo info, Label* non_smi) { In X64 I've been naming functions like this just SmiUntag (or SmiToInteger32), letting the failure label parameter explain failure behavior. http://codereview.chromium.org/3054047/diff/1/9#newcode229 src/ia32/macro-assembler-ia32.h:229: void SmiUntagAndBranchOnNonSmi(Register reg, TypeInfo info, Label* non_smi) { Do comment that this function is destructive. http://codereview.chromium.org/3054047/diff/1/9#newcode249 src/ia32/macro-assembler-ia32.h:249: // on the min negative int32. Ignores frational parts. As stated otherwhere, the name doesn't match the functionality since it ignores fractions. Not that I have a better suggestion. http://codereview.chromium.org/3054047/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
