A bunch of changes to speed up math on ARM....

A bunch of changes to speed up math on ARM.
* Identify heap numbers that contain non-Smi int32s and do bit
ops on them without calling the fp hardware or emulation.
* Identify results that are non-Smi int32s and write them into
heap numbers without calling the fp hardware or emulation.
* Do unary minus on heap numbers without going into the runtime
system.
* On add, sub and mul if we have both Smi and heapnumber inputs
to the same operation then convert the Smi to a double and do
the op without going into runtime system.  This also applies
if we have two Smi inputs but the result is not Smi.

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

[Erik Corry, 2009-06-05 22:28:07]

http://codereview.chromium.org/119241/diff/1/6
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/119241/diff/1/6#newcode4403
Line 4403: // Takes a Smi and converts to an IEEE 664 bit floating point value
in two
Oops.  Should be 64, not 664.

[iposva, 2009-06-08 06:45:29]

Erik, first set of comments. Apparently I did not hit "Publish" on my work
machine on Friday. Sorry!

-Ivan

http://codereview.chromium.org/119241/diff/1/6
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/119241/diff/1/6#newcode4369
Line 4369: // of 32).
Yet, you still seem to be calling it in all cases. In my opinion the comment at
this point should explain what this function intends to do.

The comment should move down to the check for the ARM5 architecture and it
should properly note that the clz instruction is only supported for ARM5 and
above.

http://codereview.chromium.org/119241/diff/1/6#newcode4398
Line 4398: __ add(zeros, zeros, Operand(1), LeaveCC, eq);
You could add another shift by one and tst, add sequence to fix the known bug,
but maybe you know that this is never called with a zero value. At which point
you should add a comment to this effect and maybe even generate a runtime
assertion in DEBUG.

http://codereview.chromium.org/119241/diff/1/4
File src/objects.h (right):

http://codereview.chromium.org/119241/diff/1/4#newcode1165
Line 1165: // IEEE doubles are two words.  The first is just mantissa, the
second is a
What size words? Word is too overloaded. In x86 thinking it is 32-bits, but this
is a portable header...

http://codereview.chromium.org/119241/diff/1/4#newcode1168
Line 1168: static const int kExponentOffset = kValueOffset + 4;
This is only true on little-endian.

[Erik Corry, 2009-06-08 07:34:58]

http://codereview.chromium.org/119241/diff/1/6
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/119241/diff/1/6#newcode4369
Line 4369: // of 32).
On 2009/06/08 06:45:29, iposva wrote:
> Yet, you still seem to be calling it in all cases. In my opinion the comment
at
> this point should explain what this function intends to do.
> 
> The comment should move down to the check for the ARM5 architecture and it
> should properly note that the clz instruction is only supported for ARM5 and
> above.

Fixed it so I don't run this code for the case where it doesn't work (instead of
running but ignoring).  Added comment explaining what it does.  Didn't move the
comment down about not working for input of zero since it's part of the
description of the function, not just an implementation detail.

http://codereview.chromium.org/119241/diff/1/6#newcode4398
Line 4398: __ add(zeros, zeros, Operand(1), LeaveCC, eq);
On 2009/06/08 06:45:29, iposva wrote:
> You could add another shift by one and tst, add sequence to fix the known bug,

Yes, but this wouldn't win us anything since we have to special-case an input of
0 anyway.

> but maybe you know that this is never called with a zero value. At which point
> you should add a comment to this effect and maybe even generate a runtime
> assertion in DEBUG.

I don't like generating different code in DEBUG mode and I think there are
already enough comments about this.

http://codereview.chromium.org/119241/diff/1/4
File src/objects.h (right):

http://codereview.chromium.org/119241/diff/1/4#newcode1165
Line 1165: // IEEE doubles are two words.  The first is just mantissa, the
second is a
On 2009/06/08 06:45:29, iposva wrote:
> What size words? Word is too overloaded. In x86 thinking it is 32-bits, but
this
> is a portable header...

Fixed.

http://codereview.chromium.org/119241/diff/1/4#newcode1168
Line 1168: static const int kExponentOffset = kValueOffset + 4;
On 2009/06/08 06:45:29, iposva wrote:
> This is only true on little-endian.

Comment added.  IEEE doubles exist in little, big and mixed endian variants but
right now we support only little.

[iposva, 2009-06-08 22:51:55]

Next set of comments.

-Ivan

http://codereview.chromium.org/119241/diff/14/1014
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/119241/diff/14/1014#newcode4455
Line 4455: __ and_(result1_, source_, Operand(0x80000000), SetCC);
Why not use kSignMask directly instead of magic number?

http://codereview.chromium.org/119241/diff/14/1014#newcode4464
Line 4464: __ orr(result1_, result1_, Operand(1023 << 20), LeaveCC, ne);
Both 1023 and 20 are magic numbers, unless of course you dream in IEEE floating
point. Please use the constants you define in objects.h.

http://codereview.chromium.org/119241/diff/14/1014#newcode4475
Line 4475: __ rsb(result2_, zeros_, Operand(31 + 1023));
magic number, please use constant

http://codereview.chromium.org/119241/diff/14/1014#newcode4476
Line 4476: __ orr(result1_, result1_, Operand(result2_, LSL, 20));
ditto. Actually you might want to add the constant kExponentShift so that you do
not confuse the reader with kMantissaBitsInTopWord.

http://codereview.chromium.org/119241/diff/14/1014#newcode4482
Line 4482: __ mov(result2_, Operand(source_, LSL, 20));
ditto

http://codereview.chromium.org/119241/diff/14/1014#newcode4483
Line 4483: // And the top (top 20 bits).
ditto

http://codereview.chromium.org/119241/diff/14/1014#newcode4484
Line 4484: __ orr(result1_, result1_, Operand(source_, LSR, 12));
ditto

http://codereview.chromium.org/119241/diff/14/1014#newcode4634
Line 4634: AllocateHeapNumber(masm, &slow, r5, r6, r7);
You should give the motivation for the absolute register numbers here. In other
areas of the code we use a comment indicating what values are expected to reside
in which registers and we update that description as the code proceeds. It takes
far too long to identify why you are assuming r0 and r1 contain the values, and
why you are moving them into r3 and r2 or r1 and r0 respectively.

http://codereview.chromium.org/119241/diff/14/1014#newcode4710
Line 4710: __ bind(&do_the_call);
Here I would expect something along these lines:
// r0: left value (mantissa)
// r1: left value (exponent)
// r2: right value(mantissa)
// r3: right value(exponent)
// r4: I have not figured out if anything is kept in r4 during this code. It
feels like it because it is not being used, but what for?
// r5: target heap number object to be used for result of operation

http://codereview.chromium.org/119241/diff/14/1014#newcode4741
Line 4741: static void GetInt32(MacroAssembler* masm,
Please add comment what this function tries to do.

[Erik Corry, 2009-06-09 08:45:56]

New version uploaded.

http://codereview.chromium.org/119241/diff/14/1014
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/119241/diff/14/1014#newcode4455
Line 4455: __ and_(result1_, source_, Operand(0x80000000), SetCC);
On 2009/06/08 22:51:55, iposva wrote:
> Why not use kSignMask directly instead of magic number?

I will do that, but then I have to add a comment to point out that this only
works because the sign bit of the exponent word of a double is in the same place
as the sign bit of a Smi.  Using the constant was intended to indicate this to
the reader :-).

http://codereview.chromium.org/119241/diff/14/1014#newcode4464
Line 4464: __ orr(result1_, result1_, Operand(1023 << 20), LeaveCC, ne);
On 2009/06/08 22:51:55, iposva wrote:
> Both 1023 and 20 are magic numbers, unless of course you dream in IEEE
floating
> point. Please use the constants you define in objects.h.

Actually I do dream in IEEE floating point.  But I'm trying to stop.

http://codereview.chromium.org/119241/diff/14/1014#newcode4475
Line 4475: __ rsb(result2_, zeros_, Operand(31 + 1023));
On 2009/06/08 22:51:55, iposva wrote:
> magic number, please use constant

Used constant for 1023.  I'm not going to add a kNumberOfBitsInAnInt32 = 32. 
That doesn't seem to add anything.

http://codereview.chromium.org/119241/diff/14/1014#newcode4476
Line 4476: __ orr(result1_, result1_, Operand(result2_, LSL, 20));
On 2009/06/08 22:51:55, iposva wrote:
> ditto. Actually you might want to add the constant kExponentShift so that you
do
> not confuse the reader with kMantissaBitsInTopWord.

Done.

http://codereview.chromium.org/119241/diff/14/1014#newcode4483
Line 4483: // And the top (top 20 bits).
On 2009/06/08 22:51:55, iposva wrote:
> ditto

I prefer the real numbers rather than the symbol in the comments.

[iposva, 2009-06-10 04:30:48]

One small comment about an oversight, otherwise LGTM.

-Ivan

PS. Feeling sorry for you about dreaming in IEEE floating point format.

http://codereview.chromium.org/119241/diff/18/1023
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/119241/diff/18/1023#newcode4480
Line 4480: __ orr(result1_, result1_, Operand(result2_, LSL, 20));
Please use kExponentShift, which you added to the header here.