Avoid a call to the runtime system when doing binary fp ops on ARM...

Avoid a call to the runtime system when doing binary fp ops on ARM
(at the moment only if we do not need to allocate a heap number).
Find a few more oportunities to avoid heap number allocation on IA32.
Add some infrastructure to test coverage of generated ARM code in our
tests.

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

[Kasper Lund, 2009-04-15 07:40:36]

LG. I hope we have good coverage of the generated code. You should try
sprinkling __ stop(...) at various places and make sure you hit them - if not we
need more tests. Does it lint?

http://codereview.chromium.org/67163/diff/1/14
File src/assembler.cc (right):

http://codereview.chromium.org/67163/diff/1/14#newcode613
Line 613: break;
How about a default clause that triggers an UNREACHABLE?

http://codereview.chromium.org/67163/diff/1/10
File src/assembler.h (right):

http://codereview.chromium.org/67163/diff/1/10#newcode420
Line 420: static ExternalReference double_fp_operation(char operation);
Why is the operation a char? Why not a Token::Value or something like that? ...
and it seems a dodgy to have this on IA32 where we don't need it at all.

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

http://codereview.chromium.org/67163/diff/1/11#newcode4516
Line 4516: char operation,
Token::Value operation?

http://codereview.chromium.org/67163/diff/1/11#newcode4543
Line 4543: // Get type of r1 into r3
Terminate comment with .

http://codereview.chromium.org/67163/diff/1/6
File src/codegen-ia32.h (right):

http://codereview.chromium.org/67163/diff/1/6#newcode435
Line 435: const OverwriteMode overwrite_mode);
I wonder why this is const?

http://codereview.chromium.org/67163/diff/1/9
File src/simulator-arm.cc (right):

http://codereview.chromium.org/67163/diff/1/9#newcode433
Line 433: int sixty_four_bits[2];
Shouldn't this be int32 or something like that?

http://codereview.chromium.org/67163/diff/1/9#newcode447
Line 447: int sixty_four_bits[2];
int32?

http://codereview.chromium.org/67163/diff/1/9#newcode458
Line 458: registers_[2] = 0x50Bad4U;
We have a recognizable zap value you could use here: kZapValue.

http://codereview.chromium.org/67163/diff/1/9#newcode937
Line 937: double z = (swi == simulator_fp_add) ?
This looks nasty. Why not write it as a series of if-else-if?

http://codereview.chromium.org/67163/diff/1/3
File test/mjsunit/number-limits.js (right):

http://codereview.chromium.org/67163/diff/1/3#newcode36
Line 36: print("i = " + i + ", 1/eps = " + (1/eps) + ", mulAboveMax = " +
mulAboveMax + ", Number.MAX_VALUE = " + Number.MAX_VALUE + ", Infinity = " +
Infinity);
Lines too long.

[Kevin Millikin (Chromium), 2009-04-15 08:01:04]

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

http://codereview.chromium.org/67163/diff/1/11#newcode688
Line 688: // Minor key encoding in 16 bits FOOOOOOOOOOOOOMM.
This comment is wrong.  There is no "F" here.

http://codereview.chromium.org/67163/diff/1/11#newcode4550
Line 4550: __ ldr(r2, MemOperand(r0, HeapNumber::kValueOffset -
kHeapObjectTag));
On IA32 there is a FieldOperand abstraction (in macro-assembler-ia32.h) that
does the subtract kHeapObjectTag thing.  You might consider the same for ARm and
using it here.

[iposva, 2009-04-15 23:25:21]

Comments from the side to help cleanup the code...

-Ivan

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

http://codereview.chromium.org/67163/diff/1/11#newcode4550
Line 4550: __ ldr(r2, MemOperand(r0, HeapNumber::kValueOffset -
kHeapObjectTag));
On 2009/04/15 08:01:04, Kevin Millikin wrote:
> On IA32 there is a FieldOperand abstraction (in macro-assembler-ia32.h) that
> does the subtract kHeapObjectTag thing.  You might consider the same for ARm
and
> using it here.  

There is a FieldMemOperand(Register object, int offset) in
macro-assembler-arm.cc which should be used here and in many other places in
this new code.

http://codereview.chromium.org/67163/diff/1/9
File src/simulator-arm.cc (right):

http://codereview.chromium.org/67163/diff/1/9#newcode435
Line 435: sixty_four_bits[0] = registers_[0];
There is an accessor for this: get_register(r0)

http://codereview.chromium.org/67163/diff/1/9#newcode438
Line 438: memcpy(x, buffer, sizeof(sixty_four_bits));
Can you please explain why you copy registers->sixty_four_bits->buffer->x?
This seems a very round-about way of doing something like this:
int32_t* xpointer = reinterpret_cast<int32_t>(x);
xpointer[0] = registers_[0];
xpointer[1] = registers_[1];

If this does not work, then please explain in comments exactly why these
multiple steps are necessary.

http://codereview.chromium.org/67163/diff/1/9#newcode451
Line 451: registers_[0] = sixty_four_bits[0];
There is an accessor for this: set_register(r0, value).

http://codereview.chromium.org/67163/diff/1/9#newcode452
Line 452: registers_[1] = sixty_four_bits[1];
Again multiple copies. Why?

http://codereview.chromium.org/67163/diff/1/9#newcode456
Line 456: void Simulator::TrashCalleeSaveRegisters() {
You probably mean to trash the CallerSaved registers. CalleeSavedRegisters
should be preserved across the call and not trashed.

Also this method seems to be not used. How about just removing it for now?

http://codereview.chromium.org/67163/diff/1/9#newcode936
Line 936: GetFpArgs(&x, &y);
How about making the Simulator API a bit more generic?
x = GetDoubleValue(r0, r1);
y = GetDoubleValue(r2, r3);

http://codereview.chromium.org/67163/diff/1/9#newcode937
Line 937: double z = (swi == simulator_fp_add) ?
On 2009/04/15 07:40:36, Kasper Lund wrote:
> This looks nasty. Why not write it as a series of if-else-if?

Yes, please!