Implement int32 TypeRecordingBinaryOp on ARM.

Implement int32 TypeRecordingBinaryOp on ARM.

TEST=none
BUG=none

Committed: http://code.google.com/p/v8/source/detail?r=7014

[Søren Thygesen Gjesse, 2011-02-28 09:54:32]

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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#...
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#...
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#n...
src/arm/macro-assembler-arm.h:126: // not the same as dst.
not -> but not

[Søren Thygesen Gjesse, 2011-03-02 09:33:08]

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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#...
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#...
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#n...
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.