Probe keyed load cache in generic keyed load stub....

Probe keyed load cache in generic keyed load stub.

Only implemented on ia32 and x64 for now.  The generic keyed load stub
on arm is falling behind and it is time to fix that, but that will be
a separate change.



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

[Erik Corry, 2009-12-09 10:43:40]

LGTM.

The comments for the 32 bit version mostly apply to the 64 bit version too.

I don't see a performance bot run of the 64 bit version, which would have
provided a nice verification that the optimization was working there.

http://codereview.chromium.org/460142/diff/1/8
File src/heap.h (right):

http://codereview.chromium.org/460142/diff/1/8#newcode1297
src/heap.h:1297: static const int kLength = 64;
On a related note we should run a few performance bot runs with different values
for this to see if it makes any difference.

http://codereview.chromium.org/460142/diff/1/3
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/460142/diff/1/3#newcode53
src/ia32/ic-ia32.cc:53: Register name, bool check_dictionary) {
This should be an enum with CHECK_DICTIONARY and DICTIONARY_CHECK_DONE

http://codereview.chromium.org/460142/diff/1/3#newcode83
src/ia32/ic-ia32.cc:83: // Possible work-around for http://crbug.com/16276.
Hmmm.  Did this work?

http://codereview.chromium.org/460142/diff/1/3#newcode307
src/ia32/ic-ia32.cc:307: __ test(ebx, Immediate(String::kIsArrayIndexMask));
Can't we do this with a single test instruction instead of loading into a
register first?

http://codereview.chromium.org/460142/diff/1/3#newcode312
src/ia32/ic-ia32.cc:312: __ test(ebx, Immediate(kIsSymbolMask));
And here?

http://codereview.chromium.org/460142/diff/1/3#newcode322
src/ia32/ic-ia32.cc:322: // Load the map of the receiver, compute the keyed
lookup cache hash
Don't we need to check the interceptor bit here or check for the global proxy
object?

http://codereview.chromium.org/460142/diff/1/3#newcode337
src/ia32/ic-ia32.cc:337: __ shl(edi, kPointerSizeLog2 + 1);
You can combine this mov-shl-mov-cmp combination into the operand of a single
cmp I think.

http://codereview.chromium.org/460142/diff/1/3#newcode344
src/ia32/ic-ia32.cc:344: __ cmp(eax, Operand(edi));
And this cmp could just take the operand instead of using eax as a temp.

http://codereview.chromium.org/460142/diff/1/3#newcode347
src/ia32/ic-ia32.cc:347: // Get field offset and check that it is an in-object
property.
In a later change we could handle out-of-object fast case properties here,
right?

http://codereview.chromium.org/460142/diff/1/6
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/460142/diff/1/6#newcode53
src/x64/ic-x64.cc:53: Register name, bool check_dictionary) {
Use enum.

[Mads Ager (chromium), 2009-12-10 09:18:18]

http://codereview.chromium.org/460142/diff/1/8
File src/heap.h (right):

http://codereview.chromium.org/460142/diff/1/8#newcode1297
src/heap.h:1297: static const int kLength = 64;
On 2009/12/09 10:43:40, Erik Corry wrote:
> On a related note we should run a few performance bot runs with different
values
> for this to see if it makes any difference.

Makes sense, yes.

http://codereview.chromium.org/460142/diff/1/3
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/460142/diff/1/3#newcode53
src/ia32/ic-ia32.cc:53: Register name, bool check_dictionary) {
On 2009/12/09 10:43:40, Erik Corry wrote:
> This should be an enum with CHECK_DICTIONARY and DICTIONARY_CHECK_DONE

Done.

http://codereview.chromium.org/460142/diff/1/3#newcode83
src/ia32/ic-ia32.cc:83: // Possible work-around for http://crbug.com/16276.
On 2009/12/09 10:43:40, Erik Corry wrote:
> Hmmm.  Did this work?

I don't know, but you are right that we should check.

http://codereview.chromium.org/460142/diff/1/3#newcode307
src/ia32/ic-ia32.cc:307: __ test(ebx, Immediate(String::kIsArrayIndexMask));
On 2009/12/09 10:43:40, Erik Corry wrote:
> Can't we do this with a single test instruction instead of loading into a
> register first?

No. If we jump, we use the fact that the hash field is in ebx.

http://codereview.chromium.org/460142/diff/1/3#newcode312
src/ia32/ic-ia32.cc:312: __ test(ebx, Immediate(kIsSymbolMask));
On 2009/12/09 10:43:40, Erik Corry wrote:
> And here?

No, because we are not looking at a word, but only at a byte.

http://codereview.chromium.org/460142/diff/1/3#newcode322
src/ia32/ic-ia32.cc:322: // Load the map of the receiver, compute the keyed
lookup cache hash
On 2009/12/09 10:43:40, Erik Corry wrote:
> Don't we need to check the interceptor bit here or check for the global proxy
> object?

No. I those cases we will never put the map-name pair in the cache.

http://codereview.chromium.org/460142/diff/1/3#newcode337
src/ia32/ic-ia32.cc:337: __ shl(edi, kPointerSizeLog2 + 1);
On 2009/12/09 10:43:40, Erik Corry wrote:
> You can combine this mov-shl-mov-cmp combination into the operand of a single
> cmp I think.

Done in the ia32 version.  Can't do a times_16 scale factor so I can't do it in
the x64 version.

http://codereview.chromium.org/460142/diff/1/3#newcode344
src/ia32/ic-ia32.cc:344: __ cmp(eax, Operand(edi));
On 2009/12/09 10:43:40, Erik Corry wrote:
> And this cmp could just take the operand instead of using eax as a temp.

Done.

http://codereview.chromium.org/460142/diff/1/3#newcode347
src/ia32/ic-ia32.cc:347: // Get field offset and check that it is an in-object
property.
On 2009/12/09 10:43:40, Erik Corry wrote:
> In a later change we could handle out-of-object fast case properties here,
> right?

Yes, we can and we probably should.