ARM: Implement Math.abs in lithium code generator for the integer and

ARM: Implement Math.abs in lithium code generator for the integer and
tagged case. I couldn't find a way to test the double case yet so I'm
leaving that unimplemented for now.

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

[Alexandre, 2011-01-19 16:34:45]

Here are a few comments.
I'll add full support for vabs when this lands.

Regards,

Alexandre

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

http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2365: __ mov(tmp, Operand(input));
In LCodeGen::DoMathAbs we know that input and result use the same register:
2428   ASSERT(instr->input()->Equals(instr->result()));
There is no need to overwrite it in the safepoint slot in this case.

http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2371: __ LoadRoot(scratch,
Heap::kHeapNumberMapRootIndex);
Since we save all registers on the stack, we could use an additional scratch
register to preserve the value HeapNumberMap loaded before and spare a load
here.

http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2388: __ ldr(tmp2, FieldMemOperand(input,
HeapNumber::kExponentOffset));
Same as before, we could use a scratch register to preserve this value from the
earlier load.
We may need to restore it after the callruntime though if it was clobbered.

http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2389: __ and_(tmp2, tmp2,
Operand(~HeapNumber::kSignMask));
The bic (Bit Clear) instruction would be clearer.

http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2394: __ bind(&done);
Can be moved after the next store, as input and output are the same register.

http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2396: __ str(tmp, MemOperand(sp,
SpIndexForPushAll(input) * kPointerSize));
You can use 
MemOperand MacroAssembler::SafepointRegisterSlot(Register reg)
(introduced after you uploaded the patch)

http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2407: __ rsb(input, input, Operand(0), SetCC);
We could remove the is_positive label and the branch and use a conditional
rsb(..., pl) instead. (cmp will clear the V flag so it is safe.)

[Mads Ager (chromium), 2011-01-20 07:32:17]

Thanks for your comments Alexandre!

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

http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2365: __ mov(tmp, Operand(input));
On 2011/01/19 16:34:45, Alexandre wrote:
> In LCodeGen::DoMathAbs we know that input and result use the same register:
> 2428   ASSERT(instr->input()->Equals(instr->result()));
> There is no need to overwrite it in the safepoint slot in this case.

Good catch. And that applies on ia32 as well. Change done for both platforms.

http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2371: __ LoadRoot(scratch,
Heap::kHeapNumberMapRootIndex);
On 2011/01/19 16:34:45, Alexandre wrote:
> Since we save all registers on the stack, we could use an additional scratch
> register to preserve the value HeapNumberMap loaded before and spare a load
> here.

For this one it is not so clear because the heap number map is loaded (into ip)
*before* we push all registers.

http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2388: __ ldr(tmp2, FieldMemOperand(input,
HeapNumber::kExponentOffset));
On 2011/01/19 16:34:45, Alexandre wrote:
> Same as before, we could use a scratch register to preserve this value from
the
> earlier load.
> We may need to restore it after the callruntime though if it was clobbered.

I'd rather not do that because of the runtime call in between.

http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2389: __ and_(tmp2, tmp2,
Operand(~HeapNumber::kSignMask));
On 2011/01/19 16:34:45, Alexandre wrote:
> The bic (Bit Clear) instruction would be clearer.

Done.

http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2394: __ bind(&done);
On 2011/01/19 16:34:45, Alexandre wrote:
> Can be moved after the next store, as input and output are the same register.

Done.

http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2396: __ str(tmp, MemOperand(sp,
SpIndexForPushAll(input) * kPointerSize));
On 2011/01/19 16:34:45, Alexandre wrote:
> You can use 
> MemOperand MacroAssembler::SafepointRegisterSlot(Register reg)
> (introduced after you uploaded the patch)

Thanks, I'll do that. I have changed kInstrSize to kPointerSize in the
implementation though. SafepointRegisterSlot should multiply by kPointerSize and
not kInstrSize.

http://codereview.chromium.org/6347007/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2407: __ rsb(input, input, Operand(0), SetCC);
On 2011/01/19 16:34:45, Alexandre wrote:
> We could remove the is_positive label and the branch and use a conditional
> rsb(..., pl) instead. (cmp will clear the V flag so it is safe.)

I find that a little too indirect for my liking. Do you think it will be much
faster? I wouldn't think so and I would like to keep the code clearer by having
it explicit.

[Søren Thygesen Gjesse, 2011-01-20 08:03:22]

LGTM

http://codereview.chromium.org/6347007/diff/1/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

http://codereview.chromium.org/6347007/diff/1/src/arm/assembler-arm.h#newcode377
src/arm/assembler-arm.h:377: inline int SpIndexForPushAll(Register reg) {
We have SafepointRegisterSlot in macro assembler which returns a MemOperand. I
think we should stick to that, so that "Safepoint" is part of the name instead
of "PushAll" which is inherited from IA32.

http://codereview.chromium.org/6347007/diff/11001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/6347007/diff/11001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2479: __ jmp(&done);
Can't you move the PushSafepointRegisters() to here and don't Push/Pop for
positive numbers? That is jump to the exit_label() if positive?

[Mads Ager (chromium), 2011-01-20 08:07:31]

http://codereview.chromium.org/6347007/diff/11001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/6347007/diff/11001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2479: __ jmp(&done);
On 2011/01/20 08:03:22, Søren Gjesse wrote:
> Can't you move the PushSafepointRegisters() to here and don't Push/Pop for
> positive numbers? That is jump to the exit_label() if positive?

Great! Yes I can!

[Alexandre, 2011-01-20 10:45:34]

http://codereview.chromium.org/6347007/diff/2002/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/6347007/diff/2002/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:2476: __ jmp(&done);
b(eq, &done);
would be enough.
"done" could be renamed to "positive" if that makes it clearer.