LGTM, with comments addressed This is great work. Thanks for working on this.
http://codereview.chromium.org/6594009/diff/1/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/6594009/diff/1/src/arm/code-stubs-arm.cc#newcode716 src/arm/code-stubs-arm.cc:716: __ bind(&obj_is_heap_number); Please change this label to obj_is_not_smi. http://codereview.chromium.org/6594009/diff/1/src/arm/code-stubs-arm.cc#newcode785 src/arm/code-stubs-arm.cc:785: // Untag the object in the destination register. in -> into http://codereview.chromium.org/6594009/diff/1/src/arm/code-stubs-arm.cc#newcode819 src/arm/code-stubs-arm.cc:819: // Load the double value in the destination registers.. .. -> . http://codereview.chromium.org/6594009/diff/1/src/arm/code-stubs-arm.cc#newcode875 src/arm/code-stubs-arm.cc:875: __ tst(src1, Operand(HeapNumber::kSignMask)); Maybe add a bit more commenting here would be nice (I had to use quite a few brain cycles to run these three instructions if different cases). __ cmp(scratch, Operand(30), eq); // Executed for positive. If exponent is 30 the gt condition will be "correct" and the next instruction will be skipped. __ cmp(scratch, Operand(31), ne); // Executed for negative and positive where exponent is not 30. http://codereview.chromium.org/6594009/diff/1/src/arm/code-stubs-arm.cc#newcode886 src/arm/code-stubs-arm.cc:886: // Because bits [21:20] are null, we can check instead that the 20 -> 0? http://codereview.chromium.org/6594009/diff/1/src/arm/code-stubs-arm.cc#newcode890 src/arm/code-stubs-arm.cc:890: __ Ubfx(dst, src2, HeapNumber::kMantissaBitsInTopWord, 12); 12 -> 32 - HeapNumber::kMantissaBitsInTopWord http://codereview.chromium.org/6594009/diff/1/src/arm/code-stubs-arm.cc#newcode3318 src/arm/code-stubs-arm.cc:3318: Maybe add a comment here that r0 and r1 are preserved for the runtime call. http://codereview.chromium.org/6594009/diff/1/src/arm/code-stubs-arm.cc#newcode3443 src/arm/code-stubs-arm.cc:3443: // Convert operands to 32-bit integers. Right in r2 and left in r3. Add "Preserve r0 and r1 for the runtime call to the comment". http://codereview.chromium.org/6594009/diff/1/src/arm/code-stubs-arm.cc#newcode3486 src/arm/code-stubs-arm.cc:3486: // The non vfp3 code does not support this special case, so jump to Can we fix WriteInt32ToHeapNumberStub to avoid a runtime call here? http://codereview.chromium.org/6594009/diff/1/src/arm/code-stubs-arm.cc#newcode3488 src/arm/code-stubs-arm.cc:3488: if (CpuFeatures::IsSupported(VFP3)) { No need to actually enter VFP3 scope here, as the code is not using VFP3 instructions. http://codereview.chromium.org/6594009/diff/1/src/arm/code-stubs-arm.cc#newcode3557 src/arm/code-stubs-arm.cc:3557: I see no jump to this label - dead code? http://codereview.chromium.org/6594009/diff/1/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): http://codereview.chromium.org/6594009/diff/1/src/arm/lithium-codegen-arm.cc#newcode3342 src/arm/lithium-codegen-arm.cc:3342: __ EmitVFPTruncate(rounding_mode, r6951 changed this to always use kRoundToZero. Is there a reason not to keep that, or is it a svn merge error? With the inexact flag we can probably get rid of converting back in the non-truncating case. http://codereview.chromium.org/6594009/diff/1/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6594009/diff/1/src/arm/macro-assembler-arm.cc#newcode280 src/arm/macro-assembler-arm.cc:280: ASSERT(lsb < 32); ASSERT lsb >= 0 ASSERT on with and with + lsb as well? http://codereview.chromium.org/6594009/diff/1/src/arm/macro-assembler-arm.h File src/arm/macro-assembler-arm.h (right): http://codereview.chromium.org/6594009/diff/1/src/arm/macro-assembler-arm.h#newcode126 src/arm/macro-assembler-arm.h:126: // not the same as dst. not -> but not http://codereview.chromium.org/6594009/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
