Add inlining of property load on ARM...

Add inlining of property load on ARM

Generate inlined named property load for in-object properties. This uses the same mechanism as on the Intel platforms with the map check and load instruction of the inlined code being	patched by the inline cache code. The map check is patched through the normal constant pool patching and the load instruction is patched in place.

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

[Mads Ager (chromium), 2010-04-21 14:59:00]

LGTM

http://codereview.chromium.org/1715003/diff/9001/10006
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/1715003/diff/9001/10006#newcode359
src/arm/assembler-arm.cc:359: return ((instr & Imm24Mask) << 8) >> 6;
Maybe you should add a comment with the explanation that you just gave me
offline: this takes the 24-bit immediate and makes sure to preserve its sign
(the reason for shifting it all the way to the left and then to the right again)
and multiply it by 4 to get the offset in bytes instead of words.

http://codereview.chromium.org/1715003/diff/9001/10002
File src/arm/assembler-arm.h (right):

http://codereview.chromium.org/1715003/diff/9001/10002#newcode968
src/arm/assembler-arm.h:968: static bool IsLDRRegisterImmediate(Instr instr);
LDR -> Ldr?

http://codereview.chromium.org/1715003/diff/9001/10003
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1715003/diff/9001/10003#newcode4982
src/arm/codegen-arm.cc:4982: { // NOLINT
Will the linter allow 

{ Assembler::Block...;

?

http://codereview.chromium.org/1715003/diff/9001/10003#newcode4999
src/arm/codegen-arm.cc:4999: // Make the initial (invalid) offset load the map.
Comment not understood. 

Load the inobject property. Initially use invalid offset which will be patched
by the inline cache code?

http://codereview.chromium.org/1715003/diff/9001/10009
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/1715003/diff/9001/10009#newcode577
src/arm/ic-arm.cc:577: // located just after the call to the is in the deferred
code handling the
is -> ic

[Søren Thygesen Gjesse, 2010-04-22 07:16:28]

http://codereview.chromium.org/1715003/diff/9001/10006
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/1715003/diff/9001/10006#newcode359
src/arm/assembler-arm.cc:359: return ((instr & Imm24Mask) << 8) >> 6;
On 2010/04/21 14:59:00, Mads Ager wrote:
> Maybe you should add a comment with the explanation that you just gave me
> offline: this takes the 24-bit immediate and makes sure to preserve its sign
> (the reason for shifting it all the way to the left and then to the right
again)
> and multiply it by 4 to get the offset in bytes instead of words.

Done.

http://codereview.chromium.org/1715003/diff/9001/10002
File src/arm/assembler-arm.h (right):

http://codereview.chromium.org/1715003/diff/9001/10002#newcode968
src/arm/assembler-arm.h:968: static bool IsLDRRegisterImmediate(Instr instr);
On 2010/04/21 14:59:00, Mads Ager wrote:
> LDR -> Ldr?

Done.

http://codereview.chromium.org/1715003/diff/9001/10003
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1715003/diff/9001/10003#newcode4982
src/arm/codegen-arm.cc:4982: { // NOLINT
On 2010/04/21 14:59:00, Mads Ager wrote:
> Will the linter allow 
> 
> { Assembler::Block...;
> 
> ?

Yes, if there is no empty line just after - fixed all // NOLINT like this.

http://codereview.chromium.org/1715003/diff/9001/10003#newcode4999
src/arm/codegen-arm.cc:4999: // Make the initial (invalid) offset load the map.
On 2010/04/21 14:59:00, Mads Ager wrote:
> Comment not understood. 
> 
> Load the inobject property. Initially use invalid offset which will be patched
> by the inline cache code?

Fixed comment (I used to have HeapObject::kMapOffset instead of 0 - hence the
comment).

http://codereview.chromium.org/1715003/diff/9001/10009
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/1715003/diff/9001/10009#newcode577
src/arm/ic-arm.cc:577: // located just after the call to the is in the deferred
code handling the
On 2010/04/21 14:59:00, Mads Ager wrote:
> is -> ic

Done.

[Erik Corry, 2010-04-22 20:10:40]

I'm a little late with this, but the last point seems worth thinking about.

http://codereview.chromium.org/1715003/diff/9001/10006
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/1715003/diff/9001/10006#newcode352
src/arm/assembler-arm.cc:352: bool Assembler::IsB(Instr instr) {
Can we call this IsBranch?  The instruction is called b, but this name is a
little terse.

http://codereview.chromium.org/1715003/diff/9001/10006#newcode357
src/arm/assembler-arm.cc:357: int Assembler::GetBOffset(Instr instr) {
And here

http://codereview.chromium.org/1715003/diff/9001/10005
File src/arm/codegen-arm.h (right):

http://codereview.chromium.org/1715003/diff/9001/10005#newcode291
src/arm/codegen-arm.h:291: // stack, and remain there.
remain -> remains

http://codereview.chromium.org/1715003/diff/23002/27009
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/1715003/diff/23002/27009#newcode596
src/arm/ic-arm.cc:596: ldr_property_instr, offset - kHeapObjectTag);
What if the offset doesn't fit in the immediate field that the instruction
provides?

[Søren Thygesen Gjesse, 2010-04-22 21:44:45]

On 2010/04/22 20:10:40, Erik Corry wrote:
> I'm a little late with this, but the last point seems worth thinking about.
> 
> http://codereview.chromium.org/1715003/diff/9001/10006
> File src/arm/assembler-arm.cc (right):
> 
> http://codereview.chromium.org/1715003/diff/9001/10006#newcode352
> src/arm/assembler-arm.cc:352: bool Assembler::IsB(Instr instr) {
> Can we call this IsBranch?  The instruction is called b, but this name is a
> little terse.
> 
> http://codereview.chromium.org/1715003/diff/9001/10006#newcode357
> src/arm/assembler-arm.cc:357: int Assembler::GetBOffset(Instr instr) {
> And here
> 
> http://codereview.chromium.org/1715003/diff/9001/10005
> File src/arm/codegen-arm.h (right):
> 
> http://codereview.chromium.org/1715003/diff/9001/10005#newcode291
> src/arm/codegen-arm.h:291: // stack, and remain there.
> remain -> remains
> 
> http://codereview.chromium.org/1715003/diff/23002/27009
> File src/arm/ic-arm.cc (right):
> 
> http://codereview.chromium.org/1715003/diff/23002/27009#newcode596
> src/arm/ic-arm.cc:596: ldr_property_instr, offset - kHeapObjectTag);
> What if the offset doesn't fit in the immediate field that the instruction
> provides?

Thanks for the comments Erik. They have been addressed as part of
http://codereview.chromium.org/1758003.