First part of lithium ARM port.

First part of lithium ARM port.

Implement LoadNamedField, CallConstantFunction, CmpMapAndBranch, JSArrayLength, BoundsCheck, IsNull, CallFunction, and CallStub in the ARM lithium codegenerator.


BUG=
TEST=

[Kevin Millikin (Chromium), 2011-01-04 09:17:19]

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-arm.h#newcod...
src/arm/lithium-arm.h:1146: Handle<Map> map_;
FYI you shouldn't need any of the fields map_, true_block_id_, or
false_block_id_.  You can keep the accessors and just delegate them to the
corresponding hydrogen instruction:

Handle<Map> map() const { return hydrogen()->map(); }
int true_block_id() const { return hydrogen()->FirstSuccessor()->block_id(); }

Though HCompareMapAndBranch::FirstSuccessor is virtual so you may prefer
duplicating the block ID in the Lithium instruction or else exposing a
non-virtual accessor on HCompareMapAndBranch.

You will need the macro invocation

DECLARE_HYDROGEN_ACCESSOR(CompareMapAndBranch)

in this class.  I realize this is just ported from the ia32 code, but you could
also feel free to clean it up at the same time you do the porting---a hidden
benefit of writing the port.

[Alexandre, 2011-01-04 09:50:37]

I guess I know why you removed the StoreNamedField code.
I implemented the same stubs and had raytrace fail.
The bug does not come from StoreNamedfield; I identified it and I'll have a fix
ready soon (tomorrow probably).

Cheers,

Alexandre

On 2011/01/04 09:17:19, Kevin Millikin wrote:
> http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-arm.h
> File src/arm/lithium-arm.h (right):
> 
>
http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-arm.h#newcod...
> src/arm/lithium-arm.h:1146: Handle<Map> map_;
> FYI you shouldn't need any of the fields map_, true_block_id_, or
> false_block_id_.  You can keep the accessors and just delegate them to the
> corresponding hydrogen instruction:
> 
> Handle<Map> map() const { return hydrogen()->map(); }
> int true_block_id() const { return hydrogen()->FirstSuccessor()->block_id(); }
> 
> Though HCompareMapAndBranch::FirstSuccessor is virtual so you may prefer
> duplicating the block ID in the Lithium instruction or else exposing a
> non-virtual accessor on HCompareMapAndBranch.
> 
> You will need the macro invocation
> 
> DECLARE_HYDROGEN_ACCESSOR(CompareMapAndBranch)
> 
> in this class.  I realize this is just ported from the ia32 code, but you
could
> also feel free to clean it up at the same time you do the porting---a hidden
> benefit of writing the port.

[Søren Thygesen Gjesse, 2011-01-04 11:23:34]

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

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1222: __ cmp(reg,
Operand(Factory::null_value()));
ou should use the root array for reading these fixed objects.

  __ LoadRoot(ip, Heap::kNullValueRootIndex);
  __ cmp(reg, ip);

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1223: if (instr->is_strict()) {
Two conditional LoadRoot should work here.

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1224: __ mov(result,
Operand(Handle<Object>(Heap::true_value())));
Use root array.

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1227: __ mov(result,
Operand(Handle<Object>(Heap::false_value())));
Use root array.

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1232: __ cmp(reg,
Operand(Factory::undefined_value()));
Use root array.

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1239: __ ldr(scratch, FieldMemOperand(reg,
HeapObject::kMapOffset));
Code in comment.

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1245: __ mov(result,
Operand(Handle<Object>(Heap::false_value())));
Use root array.

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1248: __ mov(result,
Operand(Handle<Object>(Heap::true_value())));
Use root array.

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1794: __ cmp(ToRegister(instr->index()),
ToOperand(instr->length()));
To much indention.

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:2149: void
LCodeGen::DoCheckPrototypeMaps(LCheckPrototypeMaps* instr) {
reg -> temp1, temp -> temp2?

[Karl Klose, 2011-01-04 12:32:18]

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-arm.h#newcod...
src/arm/lithium-arm.h:1146: Handle<Map> map_;
On 2011/01/04 09:17:19, Kevin Millikin wrote:
> FYI you shouldn't need any of the fields map_, true_block_id_, or
> false_block_id_.  You can keep the accessors and just delegate them to the
> corresponding hydrogen instruction:
> 
> Handle<Map> map() const { return hydrogen()->map(); }
> int true_block_id() const { return hydrogen()->FirstSuccessor()->block_id(); }
> 
> Though HCompareMapAndBranch::FirstSuccessor is virtual so you may prefer
> duplicating the block ID in the Lithium instruction or else exposing a
> non-virtual accessor on HCompareMapAndBranch.
> 
> You will need the macro invocation
> 
> DECLARE_HYDROGEN_ACCESSOR(CompareMapAndBranch)
> 
> in this class.  I realize this is just ported from the ia32 code, but you
could
> also feel free to clean it up at the same time you do the porting---a hidden
> benefit of writing the port.

Done.

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

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1222: __ cmp(reg,
Operand(Factory::null_value()));
On 2011/01/04 11:23:34, Søren Gjesse wrote:
> ou should use the root array for reading these fixed objects.
> 
>   __ LoadRoot(ip, Heap::kNullValueRootIndex);
>   __ cmp(reg, ip);

Done.

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1223: if (instr->is_strict()) {
On 2011/01/04 11:23:34, Søren Gjesse wrote:
> Two conditional LoadRoot should work here.

Done.

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1224: __ mov(result,
Operand(Handle<Object>(Heap::true_value())));
On 2011/01/04 11:23:34, Søren Gjesse wrote:
> Use root array.

Done.

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1227: __ mov(result,
Operand(Handle<Object>(Heap::false_value())));
On 2011/01/04 11:23:34, Søren Gjesse wrote:
> Use root array.

Done.

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1232: __ cmp(reg,
Operand(Factory::undefined_value()));
On 2011/01/04 11:23:34, Søren Gjesse wrote:
> Use root array.

Done.

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1239: __ ldr(scratch, FieldMemOperand(reg,
HeapObject::kMapOffset));
On 2011/01/04 11:23:34, Søren Gjesse wrote:
> Code in comment.

Done.

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1245: __ mov(result,
Operand(Handle<Object>(Heap::false_value())));
On 2011/01/04 11:23:34, Søren Gjesse wrote:
> Use root array.

Done.

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1248: __ mov(result,
Operand(Handle<Object>(Heap::true_value())));
On 2011/01/04 11:23:34, Søren Gjesse wrote:
> Use root array.

Done.

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1794: __ cmp(ToRegister(instr->index()),
ToOperand(instr->length()));
On 2011/01/04 11:23:34, Søren Gjesse wrote:
> To much indention.

Done.

http://codereview.chromium.org/6069010/diff/4001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:2149: void
LCodeGen::DoCheckPrototypeMaps(LCheckPrototypeMaps* instr) {
On 2011/01/04 11:23:34, Søren Gjesse wrote:
> reg -> temp1, temp -> temp2?

Done.

[Søren Thygesen Gjesse, 2011-01-04 12:44:19]

LGTM