I took the liberty of addressing comments to get this committed.
Committed: http://code.google.com/p/v8/source/detail?r=7014 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); On 2011/02/28 09:54:32, Søren Gjesse wrote:
Please change this label to obj_is_not_smi.
Done. 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. On 2011/02/28 09:54:32, Søren Gjesse wrote:
in -> into
Done. 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.. On 2011/02/28 09:54:32, Søren Gjesse wrote:
.. -> .
Done. 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)); On 2011/02/28 09:54:32, Søren Gjesse wrote:
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.
Done. 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 On 2011/02/28 09:54:32, Søren Gjesse wrote:
20 -> 0?
Done. 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); On 2011/02/28 09:54:32, Søren Gjesse wrote:
12 -> 32 - HeapNumber::kMantissaBitsInTopWord
Done. http://codereview.chromium.org/6594009/diff/1/src/arm/code-stubs-arm.cc#newcode3318 src/arm/code-stubs-arm.cc:3318: On 2011/02/28 09:54:32, Søren Gjesse wrote:
Maybe add a comment here that r0 and r1 are preserved for the runtime
call. Done. 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. On 2011/02/28 09:54:32, Søren Gjesse wrote:
Add "Preserve r0 and r1 for the runtime call to the comment".
Done. 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 On 2011/02/28 09:54:32, Søren Gjesse wrote:
Can we fix WriteInt32ToHeapNumberStub to avoid a runtime call here?
Postponed. 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)) { On 2011/02/28 09:54:32, Søren Gjesse wrote:
No need to actually enter VFP3 scope here, as the code is not using
VFP3
instructions.
Done. http://codereview.chromium.org/6594009/diff/1/src/arm/code-stubs-arm.cc#newcode3557 src/arm/code-stubs-arm.cc:3557: On 2011/02/28 09:54:32, Søren Gjesse wrote:
I see no jump to this label - dead code?
Removed. 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, On 2011/02/28 09:54:32, Søren Gjesse wrote:
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.
Changed to always use kRoundToZero. Use of inexact flag in the non-truncating case postponed. 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); On 2011/02/28 09:54:32, Søren Gjesse wrote:
ASSERT lsb >= 0 ASSERT on with and with + lsb as well?
Done. 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. On 2011/02/28 09:54:32, Søren Gjesse wrote:
not -> but not
The but was there in the first place. http://codereview.chromium.org/6594009/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
