Inline keyed load on ARM...

Inline keyed load on ARM

This uses the same infrastructure as is used by the inlining of named property load. The code patching if the inlined code is simpler as the key is provided in a register os the only patching required is the map check directing the inlined code to the deferred code block or not.

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

[antonm, 2010-04-23 10:37:40]

drive by question

http://codereview.chromium.org/1735007/diff/15001/16001
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5389
src/arm/codegen-arm.cc:5389: __ tst(r0, Operand(kSmiTagMask));
r0 is an object, no?  Should it be r1 and check that key is smi?

And sorry for being pushy but there is BranchOnSmi

[Mads Ager (chromium), 2010-04-23 12:21:24]

We should add a test case that bails out of the inlined keyed load because of a
non-smi key.

http://codereview.chromium.org/1735007/diff/15001/16001
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5282
src/arm/codegen-arm.cc:5282: // Setup the name register and call load IC.
All arguments to keyed load ic are on the stack.

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5363
src/arm/codegen-arm.cc:5363: // to be fixed. Therefore the instruction pool is
blocked when generating
instruction -> constant

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5388
src/arm/codegen-arm.cc:5388: // Check that the key is a heap object.
Check that the key is a smi. And as Anton comments you should check r1. Looks
like we need another test case if we are not hitting this with our current
tests.

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5396
src/arm/codegen-arm.cc:5396: __ mov(r4, Operand(Factory::fixed_array_map()));
Use root array?

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5406
src/arm/codegen-arm.cc:5406: __ mov(r3, Operand(Factory::the_hole_value()));
Use root array?

[Erik Corry, 2010-04-23 14:56:47]

LGTM

http://codereview.chromium.org/1735007/diff/15001/16001
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5286
src/arm/codegen-arm.cc:5286: // kayed load has been inlined.
kayed -> keyed.

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5362
src/arm/codegen-arm.cc:5362: // Parts of this code is patched, so the exact
instructions generated needs
Parts ... is -> Parts ... are
instructions ... needs -> instructions ... need

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5389
src/arm/codegen-arm.cc:5389: __ tst(r0, Operand(kSmiTagMask));
On 2010/04/23 10:37:41, antonm wrote:
> r0 is an object, no?  Should it be r1 and check that key is smi?
> 
> And sorry for being pushy but there is BranchOnSmi

I don't think it works for deferred.  Perhaps we should have BranchOnSmi on
deferred.

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5393
src/arm/codegen-arm.cc:5393: // is not a dictionary.
Can it become a dictionary without changing the map?

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5410
src/arm/codegen-arm.cc:5410: __ cmp(r1, r3);
Surely you should be comparing r3 against r0 here?

[Søren Thygesen Gjesse, 2010-04-26 12:23:34]

Some further testing revealed that this change still have bugs. So it will not
be committed just now.

http://codereview.chromium.org/1735007/diff/15001/16001
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5282
src/arm/codegen-arm.cc:5282: // Setup the name register and call load IC.
On 2010/04/23 12:21:24, Mads Ager wrote:
> All arguments to keyed load ic are on the stack.

Done.

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5286
src/arm/codegen-arm.cc:5286: // kayed load has been inlined.
On 2010/04/23 14:56:47, Erik Corry wrote:
> kayed -> keyed.

Done.

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5362
src/arm/codegen-arm.cc:5362: // Parts of this code is patched, so the exact
instructions generated needs
On 2010/04/23 14:56:47, Erik Corry wrote:
> Parts ... is -> Parts ... are
> instructions ... needs -> instructions ... need

Done.

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5363
src/arm/codegen-arm.cc:5363: // to be fixed. Therefore the instruction pool is
blocked when generating
On 2010/04/23 12:21:24, Mads Ager wrote:
> instruction -> constant

Done.

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5388
src/arm/codegen-arm.cc:5388: // Check that the key is a heap object.
On 2010/04/23 12:21:24, Mads Ager wrote:
> Check that the key is a smi. And as Anton comments you should check r1. Looks
> like we need another test case if we are not hitting this with our current
> tests.

Done.

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5389
src/arm/codegen-arm.cc:5389: __ tst(r0, Operand(kSmiTagMask));
On 2010/04/23 10:37:41, antonm wrote:
> r0 is an object, no?  Should it be r1 and check that key is smi?
> 
> And sorry for being pushy but there is BranchOnSmi

Done.

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5389
src/arm/codegen-arm.cc:5389: __ tst(r0, Operand(kSmiTagMask));
On 2010/04/23 14:56:47, Erik Corry wrote:
> On 2010/04/23 10:37:41, antonm wrote:
> > r0 is an object, no?  Should it be r1 and check that key is smi?
> > 
> > And sorry for being pushy but there is BranchOnSmi
> 
> I don't think it works for deferred.  Perhaps we should have BranchOnSmi on
> deferred.
> 
> 

There is no special branch methods for deferred code.

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5393
src/arm/codegen-arm.cc:5393: // is not a dictionary.
On 2010/04/23 14:56:47, Erik Corry wrote:
> Can it become a dictionary without changing the map?

I don't think changing the representation of the elements is reflected in the
map. Thats only for properties.

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5396
src/arm/codegen-arm.cc:5396: __ mov(r4, Operand(Factory::fixed_array_map()));
On 2010/04/23 12:21:24, Mads Ager wrote:
> Use root array?

Done.

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5406
src/arm/codegen-arm.cc:5406: __ mov(r3, Operand(Factory::the_hole_value()));
On 2010/04/23 12:21:24, Mads Ager wrote:
> Use root array?

Done.

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5410
src/arm/codegen-arm.cc:5410: __ cmp(r1, r3);
On 2010/04/23 14:56:47, Erik Corry wrote:
> Surely you should be comparing r3 against r0 here?

Absolutely.

[antonm, 2010-04-26 15:07:30]

http://codereview.chromium.org/1735007/diff/15001/16001
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5389
src/arm/codegen-arm.cc:5389: __ tst(r0, Operand(kSmiTagMask));
On 2010/04/23 14:56:47, Erik Corry wrote:
> On 2010/04/23 10:37:41, antonm wrote:
> > r0 is an object, no?  Should it be r1 and check that key is smi?
> > 
> > And sorry for being pushy but there is BranchOnSmi
> 
> I don't think it works for deferred.  Perhaps we should have BranchOnSmi on
> deferred.
> 
> 

You're right, Erik.  Let me add one not to sound silly next time :)

[antonm, 2010-04-26 15:15:04]

http://codereview.chromium.org/1735007/diff/15001/16001
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1735007/diff/15001/16001#newcode5389
src/arm/codegen-arm.cc:5389: __ tst(r0, Operand(kSmiTagMask));
On 2010/04/26 15:07:31, antonm wrote:
> On 2010/04/23 14:56:47, Erik Corry wrote:
> > On 2010/04/23 10:37:41, antonm wrote:
> > > r0 is an object, no?  Should it be r1 and check that key is smi?
> > > 
> > > And sorry for being pushy but there is BranchOnSmi
> > 
> > I don't think it works for deferred.  Perhaps we should have BranchOnSmi on
> > deferred.
> > 
> > 
> 
> You're right, Erik.  Let me add one not to sound silly next time :)

Wait, won't __ BranchOnSmi(r1, deferred->entry_label()); do the trick?