Port caching of lookup followups for interceptors to ARM

Port caching of lookup followups for interceptors to ARM

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

[antonm, 2010-01-28 16:40:24]

First pass

[antonm, 2010-01-28 17:05:25]

Erik,

may you have a look?

I'm planning an additional pass to refactor common infrastructure later.

[Erik Corry, 2010-01-28 20:43:12]

LGTM

http://codereview.chromium.org/551191/diff/2001/3001
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/551191/diff/2001/3001#newcode495
src/arm/stub-cache-arm.cc:495: masm->tst(r1, Operand(kSmiTagMask));
Please use __ instead of masm->
The Macro assembler has the BranchOnSmi call for this pattern.  It's not used
consistently, but I think it should be used for new code.

http://codereview.chromium.org/551191/diff/2001/3001#newcode661
src/arm/stub-cache-arm.cc:661: masm->cmp(r0,
Operand(Factory::no_interceptor_result_sentinel()));
This will load the sentinel from a PC-relative address, then emit reloc info so
the GC understands.  Then it will do a register-register cmp.  Instead it is
more compact to use LoadRoot to load into a scratch register, then do the cmp
explicitly.  It's OK to use ip as the scratch register (that is what the cmp
will do).

http://codereview.chromium.org/551191/diff/2001/3001#newcode791
src/arm/stub-cache-arm.cc:791: masm->cmp(r0,
Operand(Factory::no_interceptor_result_sentinel()));
LoadRoot

http://codereview.chromium.org/551191/diff/2001/3001#newcode803
src/arm/stub-cache-arm.cc:803: // TODO(antonm): get rid of argc_
No commented out code.  What will it take to get rid of argc?  Instead of a TODO
it is preferable to file a bug unless you are going to fix this tomorrow.

http://codereview.chromium.org/551191/diff/2001/3002
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/551191/diff/2001/3002#newcode550
src/ia32/stub-cache-ia32.cc:550: Handle<Code> code(function->code());
Why don't we want to assert this any more?

[antonm, 2010-01-29 15:07:34]

Thanks for comments, Erik.

That was good idea to check I take all the branches, apparently I didn't go into
CallIC and some LoadIC---hopefully that could improve performance.

http://codereview.chromium.org/551191/diff/2001/3001
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/551191/diff/2001/3001#newcode495
src/arm/stub-cache-arm.cc:495: masm->tst(r1, Operand(kSmiTagMask));
On 2010/01/28 20:43:13, Erik Corry wrote:
> Please use __ instead of masm->

Done.  Just for my education, why it's preferred other explicit construction?

And how would you feel if I move (in a later CL) this code into macro assembler
itself (for all platforms)?  Would it be to much?

> The Macro assembler has the BranchOnSmi call for this pattern.  It's not used
> consistently, but I think it should be used for new code.

Cool.  I was looking for JumpOnSmi, too intel'ish I was.

http://codereview.chromium.org/551191/diff/2001/3001#newcode661
src/arm/stub-cache-arm.cc:661: masm->cmp(r0,
Operand(Factory::no_interceptor_result_sentinel()));
On 2010/01/28 20:43:13, Erik Corry wrote:
> This will load the sentinel from a PC-relative address, then emit reloc info
so
> the GC understands.  Then it will do a register-register cmp.  Instead it is
> more compact to use LoadRoot to load into a scratch register, then do the cmp
> explicitly.  It's OK to use ip as the scratch register (that is what the cmp
> will do).

Thanks.  Done.

http://codereview.chromium.org/551191/diff/2001/3001#newcode791
src/arm/stub-cache-arm.cc:791: masm->cmp(r0,
Operand(Factory::no_interceptor_result_sentinel()));
On 2010/01/28 20:43:13, Erik Corry wrote:
> LoadRoot

Done.

http://codereview.chromium.org/551191/diff/2001/3001#newcode803
src/arm/stub-cache-arm.cc:803: // TODO(antonm): get rid of argc_
On 2010/01/28 20:43:13, Erik Corry wrote:
> No commented out code.

Sorry, remnant of moving.  Removed.

>  What will it take to get rid of argc?  Instead of a TODO
> it is preferable to file a bug unless you are going to fix this tomorrow.

Yes, I am planning to do it tomorrow/today :)

http://codereview.chromium.org/551191/diff/2001/3002
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/551191/diff/2001/3002#newcode550
src/ia32/stub-cache-ia32.cc:550: Handle<Code> code(function->code());
On 2010/01/28 20:43:13, Erik Corry wrote:
> Why don't we want to assert this any more?

It's checked 6 lines above.