ARM: Implement DoInstanceOfKnownGlobal stub

ARM: Implement DoInstanceOfKnownGlobal stub

BUG=none
TEST=none

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

[Mads Ager (chromium), 2011-01-13 11:04:01]

Redirecting to Søren.

[Søren Thygesen Gjesse, 2011-01-13 15:14:36]

http://codereview.chromium.org/6248004/diff/6001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6248004/diff/6001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:2893: // Uses registers r0 to r4. Expected input is
Please update this comment to include information on the possible patching, the
structure of the code being patched, and where the patching delta is found.

Also args_in_registers() is now called HasArgsInRegisters(). I suggest using
wording like "args in registers" instead of function names in comments when
possible.

http://codereview.chromium.org/6248004/diff/6001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:2963: __ ldr(scratch, MemOperand(inline_site));
This code pattern (the patching of the constant pool entry) is duplicated three
times. Please refactor this into either the macro assembler or a helper
generator function on the InstanceofStub.

http://codereview.chromium.org/6248004/diff/6001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:2991: Register scratch3 = map;
Please add

  map = no_reg;

as well.

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

http://codereview.chromium.org/6248004/diff/6001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1628: // We use Factory::the_hole_value() on
purpose to force relocation to be
Extend comment with "instead of loading from the root array" (I assume that is
what you mean).

http://codereview.chromium.org/6248004/diff/6001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1633: // We use Factory::the_hole_value() on
purpose to force relocation to be
Ditto.

http://codereview.chromium.org/6248004/diff/6001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1684: static const int kAdditionalDelta = 4;  //
3 instructions + 1 for reloc info.
I think the 4 here is 4 instructions Call always generates two instructions, and
the reloc info goes in the constant pool later.

Regarding the constant pool I am quite sure that we need to block the constant
pool for for these 4 instructions. Otherwise we might potentially get a constant
pool inside this sequence and the calculation of the patch site will be wrong.

http://codereview.chromium.org/6248004/diff/6001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1688: __ mov(temp, Operand(delta *
kPointerSize));
On IA32 we have EspIndexForPushAll, and I think we need something like that
here, maybe a macro assembler instruction like StoreToSafePointRegister to work
in correspondance with PushSafepointRegisters/PopSafepointRegisters.
karlklose@chromium.org is also working on the safepoint stuff adding saving of
doubles, see http://codereview.chromium.org/6164005/.

[Alexandre, 2011-01-14 17:53:39]

http://codereview.chromium.org/6248004/diff/6001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6248004/diff/6001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:2893: // Uses registers r0 to r4. Expected input is
On 2011/01/13 15:14:36, Søren Gjesse wrote:
> Please update this comment to include information on the possible patching,
the
> structure of the code being patched, and where the patching delta is found.
> 
> Also args_in_registers() is now called HasArgsInRegisters(). I suggest using
> wording like "args in registers" instead of function names in comments when
> possible.

Done.

http://codereview.chromium.org/6248004/diff/6001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:2963: __ ldr(scratch, MemOperand(inline_site));
Introduced a new GetRelocatedValueLocation MacroAssembler helper.
On 2011/01/13 15:14:36, Søren Gjesse wrote:
> This code pattern (the patching of the constant pool entry) is duplicated
three
> times. Please refactor this into either the macro assembler or a helper
> generator function on the InstanceofStub.

http://codereview.chromium.org/6248004/diff/6001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:2991: Register scratch3 = map;
On 2011/01/13 15:14:36, Søren Gjesse wrote:
> Please add
> 
>   map = no_reg;
> 
> as well.

Done.

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

http://codereview.chromium.org/6248004/diff/6001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1628: // We use Factory::the_hole_value() on
purpose to force relocation to be
It is. Updated.
On 2011/01/13 15:14:36, Søren Gjesse wrote:
> Extend comment with "instead of loading from the root array" (I assume that is
> what you mean).

http://codereview.chromium.org/6248004/diff/6001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1633: // We use Factory::the_hole_value() on
purpose to force relocation to be
On 2011/01/13 15:14:36, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/6248004/diff/6001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1684: static const int kAdditionalDelta = 4;  //
3 instructions + 1 for reloc info.
The +1 was referring to the additional instruction used to load the target in
Call. (which is relocated here.)
You are right about blocking the constant pool here.
Changed the comment and blocked constant pool emission.
On 2011/01/13 15:14:36, Søren Gjesse wrote:
> I think the 4 here is 4 instructions Call always generates two instructions,
and
> the reloc info goes in the constant pool later.
> 
> Regarding the constant pool I am quite sure that we need to block the constant
> pool for for these 4 instructions. Otherwise we might potentially get a
constant
> pool inside this sequence and the calculation of the patch site will be wrong.

http://codereview.chromium.org/6248004/diff/6001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:1688: __ mov(temp, Operand(delta *
kPointerSize));
Added SafepointRegisterSlot  and  StoreToSafepointRegister helpers.
This is not optimal as it only use an offset from sp...
On 2011/01/13 15:14:36, Søren Gjesse wrote:
> On IA32 we have EspIndexForPushAll, and I think we need something like that
> here, maybe a macro assembler instruction like StoreToSafePointRegister to
work
> in correspondance with PushSafepointRegisters/PopSafepointRegisters.
> mailto:karlklose@chromium.org is also working on the safepoint stuff adding
saving of
> doubles, see http://codereview.chromium.org/6164005/.

[Søren Thygesen Gjesse, 2011-01-17 13:18:07]

Some more comments.

http://codereview.chromium.org/6248004/diff/15001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6248004/diff/15001/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:2895: // * object: r0  or  at sp+1*kPointerSize .
Spaces around binary operations, and no space before ..

http://codereview.chromium.org/6248004/diff/15001/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:3062: __ EnterInternalFrame();
LeaveInternalFrame after the call.

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

http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-arm.cc#newc...
src/arm/lithium-arm.cc:1079: return MarkAsCall(DefineFixed(result, r0), instr);
Now that we have fast straight line code and slow code in deferred code this
should not use MarkAsCall any more, but
AssignEnvironment(AssignPointerMap(DefineFixed(result, r0))). The IA32 version
uses MarkAsSaveDoubles(result) as well, but with the newly added safepoint with
doubles (see comments in lithium-codegen-arm.cc) that should be used instead.

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

http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:1995: __ PushSafepointRegisters();
I think we need to use PushSafepointRegistersAndDoubles here. We have no control
over whether the InstanceofStub will change the content of the double registers.

http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2007: __ BlockConstPoolFor(4);
4 -> kAdditionalDelta

http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2013: RecordSafepointWithRegisters(
Use RecordSafepointWithRegistersAndDoubles here.

http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2017: __ str(r0, MemOperand(sp, result.code() *
kPointerSize));
Use StoreToSafepointRegisterSlot here.

http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2019: __ PopSafepointRegisters();
And use PopSafepointRegistersAndDoubles here.

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

http://codereview.chromium.org/6248004/diff/15001/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:488: void
MacroAssembler::StoreToSafepointRegisters(Register reg) {
StoreToSafepointRegisters -> StoreToSafepointRegisterSlot

http://codereview.chromium.org/6248004/diff/15001/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:502: return MemOperand(sp, reg.code() *
kInstrSize);
reg.code() -> SafepointRegisterStackIndex(reg.code())

[Alexandre, 2011-01-17 18:06:41]

Thanks for the review Søren.

Could you have a look at my comment on the PushSafepointRegisters before we
upload the patch?

The InstanceofStub does not touch the floating point registers, so do we
actually need to save them?


Alexandre

http://codereview.chromium.org/6248004/diff/15001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6248004/diff/15001/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:2895: // * object: r0  or  at sp+1*kPointerSize .
On 2011/01/17 13:18:07, Søren Gjesse wrote:
> Spaces around binary operations, and no space before ..

Done.

http://codereview.chromium.org/6248004/diff/15001/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:3062: __ EnterInternalFrame();
On 2011/01/17 13:18:07, Søren Gjesse wrote:
> LeaveInternalFrame after the call.

Done.

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

http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-arm.cc#newc...
src/arm/lithium-arm.cc:1079: return MarkAsCall(DefineFixed(result, r0), instr);
On 2011/01/17 13:18:07, Søren Gjesse wrote:
> Now that we have fast straight line code and slow code in deferred code this
> should not use MarkAsCall any more, but
> AssignEnvironment(AssignPointerMap(DefineFixed(result, r0))). The IA32 version
> uses MarkAsSaveDoubles(result) as well, but with the newly added safepoint
with
> doubles (see comments in lithium-codegen-arm.cc) that should be used instead.

Done.

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

http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:1995: __ PushSafepointRegisters();
Do we really need to? The InstanceofStub doesn't use floating point registers,
so it does not seem necessary.

On 2011/01/17 13:18:07, Søren Gjesse wrote:
> I think we need to use PushSafepointRegistersAndDoubles here. We have no
control
> over whether the InstanceofStub will change the content of the double
registers.

http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2007: __ BlockConstPoolFor(4);
On 2011/01/17 13:18:07, Søren Gjesse wrote:
> 4 -> kAdditionalDelta

Done.

http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2013: RecordSafepointWithRegisters(
See previous comment about PushSafepointRegisters.
On 2011/01/17 13:18:07, Søren Gjesse wrote:
> Use RecordSafepointWithRegistersAndDoubles here.

http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2017: __ str(r0, MemOperand(sp, result.code() *
kPointerSize));
Done.
Added an ASSERT to check that instr->result() is r0 .
On 2011/01/17 13:18:07, Søren Gjesse wrote:
> Use StoreToSafepointRegisterSlot here.

http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2019: __ PopSafepointRegisters();
See previous comment about PushSafepointRegisters.

On 2011/01/17 13:18:07, Søren Gjesse wrote:
> And use PopSafepointRegistersAndDoubles here.

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

http://codereview.chromium.org/6248004/diff/15001/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:488: void
MacroAssembler::StoreToSafepointRegisters(Register reg) {
On 2011/01/17 13:18:07, Søren Gjesse wrote:
> StoreToSafepointRegisters -> StoreToSafepointRegisterSlot

Done.

http://codereview.chromium.org/6248004/diff/15001/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:502: return MemOperand(sp, reg.code() *
kInstrSize);
On 2011/01/17 13:18:07, Søren Gjesse wrote:
> reg.code() -> SafepointRegisterStackIndex(reg.code())

Done.

[Søren Thygesen Gjesse, 2011-01-18 13:23:28]

You are right that the InstanceofStub does not directly touch the floating point
registers, however it can do a tail call to the builtin INSTANCE_OF and from
there we have no idea of what is actually happening.

In the IA32 version I did MarkAsSaveDoubles in
LChunkBuilder::DoInstanceOfKnownGlobal for the instruction. That will break the
live range of all double registers, which does also achieve what we want. Which
approach is the best is difficult to know for sure, but most likely instanceof
will not be used in the middle of floating point calculations.

So we should go for using MarkAsSaveDoubles instead of a sefepoint with doubles.

[Søren Thygesen Gjesse, 2011-01-19 14:56:56]

LGTM

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

Thanks for the patch!