ARM: Implement DoPower in the lithium code generator.

ARM: Implement DoPower in the lithium code generator.

[Søren Thygesen Gjesse, 2011-02-17 09:59:56]

LGTM

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

http://codereview.chromium.org/6532020/diff/1/src/arm/lithium-arm.cc#newcode1431
src/arm/lithium-arm.cc:1431: return MarkAsCall(DefineFixedDouble(result, d3),
instr,
Please do either
MarkAsCall(
    DefineFixedDouble(result, d3), instr, CAN_DEOPTIMIZE_EAGERLY);

or

MarkAsCall(DefineFixedDouble(result, d3),
           instr,
           CAN_DEOPTIMIZE_EAGERLY);

I think the first will fit.

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

http://codereview.chromium.org/6532020/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2872: __ stm(db_w, sp, r0.bit() | r1.bit() |
r2.bit() | r3.bit());
As discussed off-line this saving is not needed.

http://codereview.chromium.org/6532020/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2880: __ stm(db_w, sp, r0.bit() | r1.bit() |
r2.bit() | r3.bit());
Ditto.

http://codereview.chromium.org/6532020/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2888: ASSERT(CpuFeatures::IsSupported(VFP3));
Crankshaft implies VFP3, and we are already in a VFP3 scope, so this ASSERT is
not necessary.

http://codereview.chromium.org/6532020/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2893: Label non_smi, call;
Please use JumpIfNotSmi.

http://codereview.chromium.org/6532020/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2911: __ add(scratch, right_reg,
Operand(value_offset), LeaveCC);
LeaveCC is default for add.

http://codereview.chromium.org/6532020/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2922: // Store the result in the result register
and restore r0-r3.
We need to support non USE_ARM_EABI, where fp result is returned in cr8, See
code-atubs-arm.cc.

Maybe we should abstract this to a macro assembler GetCFunctionDoubleResult
instruction to avoid the #ifdef/#endif spreading.

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

http://codereview.chromium.org/6532020/diff/1/src/arm/simulator-arm.cc#newcod...
src/arm/simulator-arm.cc:319: PrintF("%3s: %f \n",
Accidental edit?

[Karl Klose, 2011-02-21 13:34:58]

Please have a look again. This version still only works for ARM EABI, but it
will fail with UNIMPLEMENTED instead of producing wrong results.

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

http://codereview.chromium.org/6532020/diff/1/src/arm/lithium-arm.cc#newcode1431
src/arm/lithium-arm.cc:1431: return MarkAsCall(DefineFixedDouble(result, d3),
instr,
On 2011/02/17 09:59:56, Søren Gjesse wrote:
> Please do either
> MarkAsCall(
>     DefineFixedDouble(result, d3), instr, CAN_DEOPTIMIZE_EAGERLY);
> 
> or
> 
> MarkAsCall(DefineFixedDouble(result, d3),
>            instr,
>            CAN_DEOPTIMIZE_EAGERLY);
> 
> I think the first will fit.

Done.

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

http://codereview.chromium.org/6532020/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2872: __ stm(db_w, sp, r0.bit() | r1.bit() |
r2.bit() | r3.bit());
On 2011/02/17 09:59:56, Søren Gjesse wrote:
> As discussed off-line this saving is not needed.

Done.

http://codereview.chromium.org/6532020/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2880: __ stm(db_w, sp, r0.bit() | r1.bit() |
r2.bit() | r3.bit());
On 2011/02/17 09:59:56, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/6532020/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2888: ASSERT(CpuFeatures::IsSupported(VFP3));
On 2011/02/17 09:59:56, Søren Gjesse wrote:
> Crankshaft implies VFP3, and we are already in a VFP3 scope, so this ASSERT is
> not necessary.

Done.

http://codereview.chromium.org/6532020/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2893: Label non_smi, call;
On 2011/02/17 09:59:56, Søren Gjesse wrote:
> Please use JumpIfNotSmi.

Done.

http://codereview.chromium.org/6532020/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2911: __ add(scratch, right_reg,
Operand(value_offset), LeaveCC);
On 2011/02/17 09:59:56, Søren Gjesse wrote:
> LeaveCC is default for add.

Done.

http://codereview.chromium.org/6532020/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2922: // Store the result in the result register
and restore r0-r3.
Implemented MacroAssembler::GetCFunctionDoubleResult and used it here and in

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

http://codereview.chromium.org/6532020/diff/1/src/arm/simulator-arm.cc#newcod...
src/arm/simulator-arm.cc:319: PrintF("%3s: %f \n",
Yes, removed.

[Søren Thygesen Gjesse, 2011-02-23 09:35:01]

LGTM

http://codereview.chromium.org/6532020/diff/4001/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/6532020/diff/4001/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:750: UNIMPLEMENTED();
UNIMPLEMENTED -> UNREACHABLE

I think we should force FLAG_crankshaft to off if USE_ARM_EABI is not defined.