Extend CallIC to support non-constant names....

Extend CallIC to support non-constant names.
This speeds up constructs like this: 
var zz='replace'; '123'[zz]('3','4');


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

[antonm, 2010-05-27 11:04:52]

First pass.

I overall like the approach, but think that we need Mads' blessing anyway.

Could we refactor argc, in_loop and call_mode into a small structure (or just
thread flags around)?

http://codereview.chromium.org/2280007/diff/1/16
File src/codegen.cc (right):

http://codereview.chromium.org/2280007/diff/1/16#newcode258
src/codegen.cc:258: StubCache::ComputeCallInitialize(argc, in_loop, FIXED_NAME),
Code);
NAMED_CALL?

http://codereview.chromium.org/2280007/diff/1/5
File src/globals.h (right):

http://codereview.chromium.org/2280007/diff/1/5#newcode462
src/globals.h:462: FIXED_NAME,
sorry for repetition: I'd prefer to see NAMED_CALL/MODE, KEYED_CALL/MODE

http://codereview.chromium.org/2280007/diff/1/6
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/1/6#newcode5963
src/ia32/codegen-ia32.cc:5963: // Push the name of the function onto the frame.
Load the name?

http://codereview.chromium.org/2280007/diff/1/8
File src/ia32/codegen-ia32.h (right):

http://codereview.chromium.org/2280007/diff/1/8#newcode597
src/ia32/codegen-ia32.h:597: static Handle<Code> ComputeCallInitialize(
only for ia32?

http://codereview.chromium.org/2280007/diff/1/7
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/1/7#newcode1860
src/ia32/full-codegen-ia32.cc:1860: // instruction after the call it is treated
specially
the call <as> it is ?  I see you just moved it, but maybe you can correct while
you're here.

http://codereview.chromium.org/2280007/diff/1/13
File src/ic.cc (right):

http://codereview.chromium.org/2280007/diff/1/13#newcode160
src/ic.cc:160: (kind == Code::CALL_IC && target->ic_call_mode() ==
VARIABLE_NAME)) {
why you added CallIIC case check here?

http://codereview.chromium.org/2280007/diff/1/13#newcode1374
src/ic.cc:1374: ic.KeyedLoadFunction(state, args.at<Object>(0),
args.at<Object>(1));
nit: indent, I'd add two more spaces

http://codereview.chromium.org/2280007/diff/1/13#newcode1376
src/ic.cc:1376: // The first time the inline cache is updated may be the first
time the
I wonder if the code below should be refactored into a separate function.

[antonm, 2010-05-27 11:19:36]

And I would love to see tons of tests :)

http://codereview.chromium.org/2280007/diff/1/20
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/2280007/diff/1/20#newcode293
src/arm/ic-arm.cc:293: CallModeFlag call_mode) {
why call_mode is not used here and in methods below.

http://codereview.chromium.org/2280007/diff/1/7
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/1/7#newcode1851
src/ia32/full-codegen-ia32.cc:1851: if (prop->is_synthetic()) {
that is weird: you've checked prop->is_synthetic above, don't you?

[Vladislav Kaznacheev, 2010-05-27 12:06:57]

I forgot to add that this change is for ia32 only. ARM and x64 changes are only
made to make them compile.

http://codereview.chromium.org/2280007/diff/1/20
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/2280007/diff/1/20#newcode293
src/arm/ic-arm.cc:293: CallModeFlag call_mode) {
I forgot to tell everyone that this change is for ia32 only. Changes to ARM and
X64 only made to make them compile.

On 2010/05/27 11:19:36, antonm wrote:
> why call_mode is not used here and in methods below.

http://codereview.chromium.org/2280007/diff/1/16
File src/codegen.cc (right):

http://codereview.chromium.org/2280007/diff/1/16#newcode258
src/codegen.cc:258: StubCache::ComputeCallInitialize(argc, in_loop, FIXED_NAME),
Code);
Yes, absolutely.
On 2010/05/27 11:04:52, antonm wrote:
> NAMED_CALL?

http://codereview.chromium.org/2280007/diff/1/5
File src/globals.h (right):

http://codereview.chromium.org/2280007/diff/1/5#newcode462
src/globals.h:462: FIXED_NAME,
I am not happy with the names either, thanks for the suggestion. I actually like
NAMED_CALL/KEYED_CALL. Will change to that if no one comes up with better idea.
On 2010/05/27 11:04:52, antonm wrote:
> sorry for repetition: I'd prefer to see NAMED_CALL/MODE, KEYED_CALL/MODE

http://codereview.chromium.org/2280007/diff/1/6
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/1/6#newcode5963
src/ia32/codegen-ia32.cc:5963: // Push the name of the function onto the frame.
On 2010/05/27 11:04:52, antonm wrote:
> Load the name?

Done.

http://codereview.chromium.org/2280007/diff/1/7
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/1/7#newcode1851
src/ia32/full-codegen-ia32.cc:1851: if (prop->is_synthetic()) {
Yes, I just moved the whole block of code under the condition without really
reading it. Simplified.
On 2010/05/27 11:19:36, antonm wrote:
> that is weird: you've checked prop->is_synthetic above, don't you?

http://codereview.chromium.org/2280007/diff/1/7#newcode1860
src/ia32/full-codegen-ia32.cc:1860: // instruction after the call it is treated
specially
On 2010/05/27 11:04:52, antonm wrote:
> the call <as> it is ?  I see you just moved it, but maybe you can correct
while
> you're here.

Done.

http://codereview.chromium.org/2280007/diff/1/13
File src/ic.cc (right):

http://codereview.chromium.org/2280007/diff/1/13#newcode160
src/ic.cc:160: (kind == Code::CALL_IC && target->ic_call_mode() ==
VARIABLE_NAME)) {
It is redundant. Removed.
On 2010/05/27 11:04:52, antonm wrote:
> why you added CallIIC case check here?

http://codereview.chromium.org/2280007/diff/1/13#newcode1374
src/ic.cc:1374: ic.KeyedLoadFunction(state, args.at<Object>(0),
args.at<Object>(1));
On 2010/05/27 11:04:52, antonm wrote:
> nit: indent, I'd add two more spaces

Done.

http://codereview.chromium.org/2280007/diff/1/13#newcode1376
src/ic.cc:1376: // The first time the inline cache is updated may be the first
time the
Tried that. An extra call slows things down by 0.5-1.0%. Will look into it
again.
On 2010/05/27 11:04:52, antonm wrote:
> I wonder if the code below should be refactored into a separate function.

[Mads Ager (chromium), 2010-05-27 12:07:46]

First high-level comments without really reading much of the code:

1. Doesn't this mean that we will always enter the runtime system for calling
functions in arrays: a[0]()? This used to be a KeyedLoadIC followed by a call
which is pretty fast. We should make sure that this is still fast.

2. Do we really want to have a megamorphic case here or do we want an approach
more like for the KeyedLoadICs where we have a generic stub that handles most
cases (array loads and the like) fairly well and just always go back to the
generic version on miss?

[Vladislav Kaznacheev, 2010-05-31 10:04:53]

I have extended the megamorphic case to handle non-string keys. Now it starts
with looking up monomorphic cache (this still works pretty good in many cases),
but if it misses it calls the KeyedCallIC_Generic stub to get the function in
the fastest possible way. This does not regress usual tests (SunSpider etc).

Also rebased a CL to the current TOT.

On 2010/05/27 12:07:46, Mads Ager wrote:
> First high-level comments without really reading much of the code:
> 
> 1. Doesn't this mean that we will always enter the runtime system for calling
> functions in arrays: a[0]()? This used to be a KeyedLoadIC followed by a call
> which is pretty fast. We should make sure that this is still fast.
> 
> 2. Do we really want to have a megamorphic case here or do we want an approach
> more like for the KeyedLoadICs where we have a generic stub that handles most
> cases (array loads and the like) fairly well and just always go back to the
> generic version on miss?

[antonm, 2010-05-31 13:05:50]

That looks mostly LGTM.  But I really would like to see tons of tests for
various transitions.

http://codereview.chromium.org/2280007/diff/20001/21005
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/20001/21005#newcode5953
src/ia32/codegen-ia32.cc:5953: Load(property->obj());
nit: maybe unify with lines 5917--5936 (add a comment, blank line)

http://codereview.chromium.org/2280007/diff/20001/21011
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/20001/21011#newcode1110
src/ia32/ic-ia32.cc:1110: __ EnterInternalFrame();
why enter/exit internal frame here?

http://codereview.chromium.org/2280007/diff/20001/21011#newcode1131
src/ia32/ic-ia32.cc:1131: //  -- edi                 : function to call (if
lookup=true)
if lookup != true?

http://codereview.chromium.org/2280007/diff/20001/21012
File src/ic.cc (right):

http://codereview.chromium.org/2280007/diff/20001/21012#newcode160
src/ic.cc:160: target->ic_call_mode() == KEYED_CALL) {
shouldn't you check CALL_IC kind before?

[Mads Ager (chromium), 2010-05-31 13:14:08]

This is getting there.

http://codereview.chromium.org/2280007/diff/20001/21011
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/20001/21011#newcode1116
src/ia32/ic-ia32.cc:1116: CallIC::GenerateNormal(masm, argc, call_mode, false);
Normal here means normalized properties, this is not what we want to generate
here. GenerateNormal that does not probe the dictionary does not make sense. If
you want to reuse that code, please extract a static helper function
CheckFunctionAndInvoke or something like that and use it here and in
GenerateNormalHelper. Also, GenerateNormal will check that a lot of stuff about
the receiver that the KeyedLoadIC should have already checked...

http://codereview.chromium.org/2280007/diff/20001/21012
File src/ic.cc (right):

http://codereview.chromium.org/2280007/diff/20001/21012#newcode155
src/ic.cc:155: // For keyed load/store, the most likely cause of cache failure
is
Update comment.

http://codereview.chromium.org/2280007/diff/20001/21012#newcode160
src/ic.cc:160: target->ic_call_mode() == KEYED_CALL) {
KEYED_CALL ICs are a different kind of IC. Why don't you introduce a separate
kind instead of having a separate call_mode flag? The keyed call IC stubs needs
to be treated like a completely different kind, so I don't see a reason to treat
them in a special way when it comes to the kind encoding.

http://codereview.chromium.org/2280007/diff/20001/21012#newcode492
src/ic.cc:492: if (object->IsString() || object->IsNumber() ||
object->IsBoolean()) {
Check for undefined and null to generate error messages that match that of other
calls and to not generate code when we know it will miss?

http://codereview.chromium.org/2280007/diff/20001/21012#newcode1384
src/ic.cc:1384: CallIC ic(KEYED_CALL);
It would be nicer to have a KeyedCallIC instance here. I find it confusing that
we have these separate stubs that are of a completely separate kind but we do
not have the KeyedCallIC class that I would expect.

http://codereview.chromium.org/2280007/diff/20001/21012#newcode1389
src/ic.cc:1389: // The first time the inline cache is updated may be the first
time the
Maybe extract the rest of this function into a static helper called something
like EnsureCompiledIfFunction and use it from CallIC_Miss and KeyedCallIC_Miss?

http://codereview.chromium.org/2280007/diff/20001/21013
File src/ic.h (right):

http://codereview.chromium.org/2280007/diff/20001/21013#newcode188
src/ic.h:188: class CallIC: public IC {
I would prefer to have a KeyedCallIC class instead of putting in a flag in the
CallIC. KeyedCallIC should be treated like a completely different kind of IC.
You should still be able to share almost everything by using static helper
functions?

http://codereview.chromium.org/2280007/diff/20001/21018
File src/objects-inl.h (right):

http://codereview.chromium.org/2280007/diff/20001/21018#newcode2240
src/objects-inl.h:2240: CallModeFlag Code::ic_call_mode() {
I don't like this much. We should just have one more IC kind instead. That would
avoid the rest of the changes to this file as well.

[Vladislav Kaznacheev, 2010-06-01 11:26:05]

I have completely removed CallModeFlag and introduced KeyedCallIC. 
Reusing generating functions in ic-ia32.cc in a more transparent manner.
Will add a test soon.

http://codereview.chromium.org/2280007/diff/20001/21005
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/20001/21005#newcode5953
src/ia32/codegen-ia32.cc:5953: Load(property->obj());
On 2010/05/31 13:05:50, antonm wrote:
> nit: maybe unify with lines 5917--5936 (add a comment, blank line)

Done.

http://codereview.chromium.org/2280007/diff/20001/21011
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/20001/21011#newcode1116
src/ia32/ic-ia32.cc:1116: CallIC::GenerateNormal(masm, argc, call_mode, false);
Stopped reusing GenerateNormal, inlined some code instead. Will refactor for
more inlining in the next CL.
On 2010/05/31 13:14:08, Mads Ager wrote:
> Normal here means normalized properties, this is not what we want to generate
> here. GenerateNormal that does not probe the dictionary does not make sense.
If
> you want to reuse that code, please extract a static helper function
> CheckFunctionAndInvoke or something like that and use it here and in
> GenerateNormalHelper. Also, GenerateNormal will check that a lot of stuff
about
> the receiver that the KeyedLoadIC should have already checked...

http://codereview.chromium.org/2280007/diff/20001/21011#newcode1131
src/ia32/ic-ia32.cc:1131: //  -- edi                 : function to call (if
lookup=true)
Removed.
On 2010/05/31 13:05:50, antonm wrote:
> if lookup != true?

http://codereview.chromium.org/2280007/diff/20001/21012
File src/ic.cc (right):

http://codereview.chromium.org/2280007/diff/20001/21012#newcode155
src/ic.cc:155: // For keyed load/store, the most likely cause of cache failure
is
On 2010/05/31 13:14:08, Mads Ager wrote:
> Update comment.

Done.

http://codereview.chromium.org/2280007/diff/20001/21012#newcode160
src/ic.cc:160: target->ic_call_mode() == KEYED_CALL) {
checking the kind now, call mode went away 
On 2010/05/31 13:05:50, antonm wrote:
> shouldn't you check CALL_IC kind before?

http://codereview.chromium.org/2280007/diff/20001/21012#newcode492
src/ic.cc:492: if (object->IsString() || object->IsNumber() ||
object->IsBoolean()) {
On 2010/05/31 13:14:08, Mads Ager wrote:
> Check for undefined and null to generate error messages that match that of
other
> calls and to not generate code when we know it will miss?

Done.

http://codereview.chromium.org/2280007/diff/20001/21012#newcode492
src/ic.cc:492: if (object->IsString() || object->IsNumber() ||
object->IsBoolean()) {
On 2010/05/31 13:14:08, Mads Ager wrote:
> Check for undefined and null to generate error messages that match that of
other
> calls and to not generate code when we know it will miss?

Done.

http://codereview.chromium.org/2280007/diff/20001/21012#newcode1384
src/ic.cc:1384: CallIC ic(KEYED_CALL);
On 2010/05/31 13:14:08, Mads Ager wrote:
> It would be nicer to have a KeyedCallIC instance here. I find it confusing
that
> we have these separate stubs that are of a completely separate kind but we do
> not have the KeyedCallIC class that I would expect.

Done.

http://codereview.chromium.org/2280007/diff/20001/21012#newcode1389
src/ic.cc:1389: // The first time the inline cache is updated may be the first
time the
On 2010/05/31 13:14:08, Mads Ager wrote:
> Maybe extract the rest of this function into a static helper called something
> like EnsureCompiledIfFunction and use it from CallIC_Miss and
KeyedCallIC_Miss? 

Extracted the actual compilation and left the first check inline (otherwise the
function gets a bit slower on tests).

http://codereview.chromium.org/2280007/diff/20001/21013
File src/ic.h (right):

http://codereview.chromium.org/2280007/diff/20001/21013#newcode188
src/ic.h:188: class CallIC: public IC {
Created a separate class KeyedCallIC. It is not that easy to extract
UpdateCaches into a static (as it calls IC::set_target).
On 2010/05/31 13:14:08, Mads Ager wrote:
> I would prefer to have a KeyedCallIC class instead of putting in a flag in the
> CallIC. KeyedCallIC should be treated like a completely different kind of IC.
> You should still be able to share almost everything by using static helper
> functions?

http://codereview.chromium.org/2280007/diff/20001/21018
File src/objects-inl.h (right):

http://codereview.chromium.org/2280007/diff/20001/21018#newcode2240
src/objects-inl.h:2240: CallModeFlag Code::ic_call_mode() {
On 2010/05/31 13:14:08, Mads Ager wrote:
> I don't like this much. We should just have one more IC kind instead. That
would
> avoid the rest of the changes to this file as well.

Done. But I needed to change ComputeFlag for a different reason (to distinguish
miss stubs for CALL_IC and KEYED_CALL_IC)

[Mads Ager (chromium), 2010-06-01 12:59:00]

This patch is getting very close now. Make sure that there are tests and address
these comments and I think we are ready to go.

http://codereview.chromium.org/2280007/diff/56001/57014
File src/codegen.cc (right):

http://codereview.chromium.org/2280007/diff/56001/57014#newcode264
src/codegen.cc:264: #if V8_TARGET_ARCH_IA32
I guess you can get rid of the #ifdef now?

http://codereview.chromium.org/2280007/diff/56001/57004
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/56001/57004#newcode5971
src/ia32/codegen-ia32.cc:5971: frame_->CallCallIC(RelocInfo::CODE_TARGET,
Either make this CallKeyedCallIC or change the full code generator to share the
EmitCallIC code passing in a code kind?

http://codereview.chromium.org/2280007/diff/56001/57010
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/56001/57010#newcode1299
src/ia32/ic-ia32.cc:1299: __ push(ecx);
You should move the push and pop of ecx inside the Enter/LeaveInternalFrame so
that the GC knows that there is a potential heap-object on the stack here. The
current code is not GC safe.

http://codereview.chromium.org/2280007/diff/56001/57011
File src/ic.cc (right):

http://codereview.chromium.org/2280007/diff/56001/57011#newcode404
src/ic.cc:404: Handle<Object> object,
Indentation.

http://codereview.chromium.org/2280007/diff/56001/57012
File src/ic.h (right):

http://codereview.chromium.org/2280007/diff/56001/57012#newcode188
src/ic.h:188: class CallIC_Base: public IC {
Remove the underscore? CallICBase

http://codereview.chromium.org/2280007/diff/56001/57012#newcode234
src/ic.h:234: { ASSERT(target()->is_keyed_call_stub());}
Strange indentation:

KeyedCallIC() : CallICBase(Code::KEYED_CALL_IC) {
  ASSERT(...);
}

http://codereview.chromium.org/2280007/diff/56001/57022
File src/log.cc (right):

http://codereview.chromium.org/2280007/diff/56001/57022#newcode1295
src/log.cc:1295: case Code::KEYED_CALL_IC:
It would be nice to know in the profiles if it is a keyed or a named call IC.
Please have a separate "A keyed call IC from the snapshot" and have a separate
logger tag for keyed calls.

http://codereview.chromium.org/2280007/diff/56001/57013
File src/stub-cache.cc (right):

http://codereview.chromium.org/2280007/diff/56001/57013#newcode711
src/stub-cache.cc:711: Code::ComputeFlags(kind, NOT_IN_LOOP, MEGAMORPHIC,
NORMAL, argc, 1);
We can avoid the extra argument to compute flags here in another way. This is
really a bit of a hack to make sure that we can use the stub cache to share the
call/keyed call miss stubs for the same arg count. Instead of adding an extra
argument, you can just use the MONOMORPHIC_PROTOTYPE_FAILURE InlineCacheState
since this is  pseudo-type which no actual IC stub has.

We should make sure to add a comment here stating that the use of
MONOMORPHIC_PROTOTYPE_FAILURE is a hack to share the stubs while making sure
that these stubs are never confused with normal IC stubs in the cache.

http://codereview.chromium.org/2280007/diff/56001/57003
File src/stub-cache.h (right):

http://codereview.chromium.org/2280007/diff/56001/57003#newcode206
src/stub-cache.h:206: static Object* ComputeCallDebugBreak(int argc, Code::Kind
= Code::CALL_IC);
Let's avoid default arguments.

http://codereview.chromium.org/2280007/diff/56001/57003#newcode338
src/stub-cache.h:338: Handle<Code> ComputeCallMiss(int argc, Code::Kind =
Code::CALL_IC);
Let's avoid default arguments.

[Vladislav Kaznacheev, 2010-06-03 15:00:03]

I have responded to all comments and added tests.

http://codereview.chromium.org/2280007/diff/56001/57014
File src/codegen.cc (right):

http://codereview.chromium.org/2280007/diff/56001/57014#newcode264
src/codegen.cc:264: #if V8_TARGET_ARCH_IA32
Removed. Had to add this method to codegen-arm.h and codegen-x64.h to make these
platforms compile.

On 2010/06/01 12:59:00, Mads Ager wrote:
> I guess you can get rid of the #ifdef now?

http://codereview.chromium.org/2280007/diff/56001/57004
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/56001/57004#newcode5971
src/ia32/codegen-ia32.cc:5971: frame_->CallCallIC(RelocInfo::CODE_TARGET,
Created CallKeyedCallIC.
On 2010/06/01 12:59:00, Mads Ager wrote:
> Either make this CallKeyedCallIC or change the full code generator to share
the
> EmitCallIC code passing in a code kind?

http://codereview.chromium.org/2280007/diff/56001/57010
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/56001/57010#newcode1299
src/ia32/ic-ia32.cc:1299: __ push(ecx);
On 2010/06/01 12:59:00, Mads Ager wrote:
> You should move the push and pop of ecx inside the Enter/LeaveInternalFrame so
> that the GC knows that there is a potential heap-object on the stack here. The
> current code is not GC safe.

Done.

http://codereview.chromium.org/2280007/diff/56001/57011
File src/ic.cc (right):

http://codereview.chromium.org/2280007/diff/56001/57011#newcode404
src/ic.cc:404: Handle<Object> object,
On 2010/06/01 12:59:00, Mads Ager wrote:
> Indentation.

Done.

http://codereview.chromium.org/2280007/diff/56001/57012
File src/ic.h (right):

http://codereview.chromium.org/2280007/diff/56001/57012#newcode188
src/ic.h:188: class CallIC_Base: public IC {
On 2010/06/01 12:59:00, Mads Ager wrote:
> Remove the underscore? CallICBase

Done.

http://codereview.chromium.org/2280007/diff/56001/57012#newcode234
src/ic.h:234: { ASSERT(target()->is_keyed_call_stub());}
On 2010/06/01 12:59:00, Mads Ager wrote:
> Strange indentation:
> 
> KeyedCallIC() : CallICBase(Code::KEYED_CALL_IC) {
>   ASSERT(...);
> }

Done.

http://codereview.chromium.org/2280007/diff/56001/57022
File src/log.cc (right):

http://codereview.chromium.org/2280007/diff/56001/57022#newcode1295
src/log.cc:1295: case Code::KEYED_CALL_IC:
On 2010/06/01 12:59:00, Mads Ager wrote:
> It would be nice to know in the profiles if it is a keyed or a named call IC.
> Please have a separate "A keyed call IC from the snapshot" and have a separate
> logger tag for keyed calls.

Done.

http://codereview.chromium.org/2280007/diff/56001/57013
File src/stub-cache.cc (right):

http://codereview.chromium.org/2280007/diff/56001/57013#newcode711
src/stub-cache.cc:711: Code::ComputeFlags(kind, NOT_IN_LOOP, MEGAMORPHIC,
NORMAL, argc, 1);
On 2010/06/01 12:59:00, Mads Ager wrote:
> We can avoid the extra argument to compute flags here in another way. This is
> really a bit of a hack to make sure that we can use the stub cache to share
the
> call/keyed call miss stubs for the same arg count. Instead of adding an extra
> argument, you can just use the MONOMORPHIC_PROTOTYPE_FAILURE InlineCacheState
> since this is  pseudo-type which no actual IC stub has.
> 
> We should make sure to add a comment here stating that the use of
> MONOMORPHIC_PROTOTYPE_FAILURE is a hack to share the stubs while making sure
> that these stubs are never confused with normal IC stubs in the cache.

Done.

http://codereview.chromium.org/2280007/diff/56001/57003
File src/stub-cache.h (right):

http://codereview.chromium.org/2280007/diff/56001/57003#newcode206
src/stub-cache.h:206: static Object* ComputeCallDebugBreak(int argc, Code::Kind
= Code::CALL_IC);
On 2010/06/01 12:59:00, Mads Ager wrote:
> Let's avoid default arguments.

Done.

http://codereview.chromium.org/2280007/diff/56001/57003#newcode338
src/stub-cache.h:338: Handle<Code> ComputeCallMiss(int argc, Code::Kind =
Code::CALL_IC);
On 2010/06/01 12:59:00, Mads Ager wrote:
> Let's avoid default arguments.

Done.

[antonm, 2010-06-03 16:18:06]

http://codereview.chromium.org/2280007/diff/72002/86010
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/72002/86010#newcode1080
src/ia32/ic-ia32.cc:1080: __ CmpInstanceType(ebx, FIRST_NONSTRING_TYPE);
I believe it was a bug we discussed with you?

http://codereview.chromium.org/2280007/diff/72002/86010#newcode1100
src/ia32/ic-ia32.cc:1100: __ bind(&miss);
maybe pass miss label as an argument?

I think it would make it easier to use this function and looks more v8ish imho.

http://codereview.chromium.org/2280007/diff/72002/86010#newcode1200
src/ia32/ic-ia32.cc:1200: __ bind(&miss);
ditto

http://codereview.chromium.org/2280007/diff/72002/86010#newcode1302
src/ia32/ic-ia32.cc:1302: GenerateMonomorphicCacheProbe(masm, argc,
Code::KEYED_CALL_IC);
isn't there is a problem if GenerateMonomorphicCacheProbe would jump to miss
label?  In that case you would take smi_key branch, correct?  I think that key
should be something like null or undefined to hit this path.

I believe (but I would appreciate if you add tests for that) that
KeyedLoadIC_Generic could cope with such keys, but maybe it should be made more
explicit in the code (e.g. rename the label)

http://codereview.chromium.org/2280007/diff/72002/86010#newcode1302
src/ia32/ic-ia32.cc:1302: GenerateMonomorphicCacheProbe(masm, argc,
Code::KEYED_CALL_IC);
similar note: it doesn't look like we go in miss case if we failed to lookup a
stub for the name, correct?

http://codereview.chromium.org/2280007/diff/72002/86010#newcode1325
src/ia32/ic-ia32.cc:1325: __ j(below, &miss, not_taken);
cache probing does some of these checks, I wonder if we could unify them, but
that's probably too much of trouble

http://codereview.chromium.org/2280007/diff/72002/86011
File src/ic.cc (right):

http://codereview.chromium.org/2280007/diff/72002/86011#newcode605
src/ic.cc:605: } else {
not insisting, but I'd rather omit the else-clause as you quite from then-clause
quickly anyway

http://codereview.chromium.org/2280007/diff/72002/86011#newcode1390
src/ic.cc:1390: // Used from ic_<arch>.cc.
Lie! used from ic-<arch>.cc :)

http://codereview.chromium.org/2280007/diff/72002/86012
File src/ic.h (right):

http://codereview.chromium.org/2280007/diff/72002/86012#newcode217
src/ic.h:217: class CallIC: public CallICBase {
I know we already inherit from IC and there is no CallIC/KeyedCallIC specific
fields, but still when I see growing hierarchy w/o virtual destructors, I am
somewhat uneasy

http://codereview.chromium.org/2280007/diff/72002/86013
File src/stub-cache.cc (right):

http://codereview.chromium.org/2280007/diff/72002/86013#newcode445
src/stub-cache.cc:445: (kind == Code::CALL_IC ? Logger::type:
Logger::KEYED_##type)
nit: a space before : (coming from ? : construct)

http://codereview.chromium.org/2280007/diff/72002/86029
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/2280007/diff/72002/86029#newcode7126
test/cctest/test-api.cc:7126: "proto1 = new Object();"
please, rename proto1 to o and o to proto1 to have o -> proto1 -> proto2 chain

http://codereview.chromium.org/2280007/diff/72002/86029#newcode7144
test/cctest/test-api.cc:7144: // global object which is between interceptor and
constant function' holders.
am I missing something, I don't see any override here.

Overall, it might be good to add a test case for two paths to miss case: when
prototype chain changes from receiver to interceptor holder and from interceptor
holder to cached lookup holder---we need to be sure correct name is passed.  But
yes, it should be the case.

http://codereview.chromium.org/2280007/diff/72002/86030
File test/mjsunit/keyed-call-ic.js (right):

http://codereview.chromium.org/2280007/diff/72002/86030#newcode158
test/mjsunit/keyed-call-ic.js:158: function testTypeTransitions() {
why it's empty?

http://codereview.chromium.org/2280007/diff/72002/86030#newcode161
test/mjsunit/keyed-call-ic.js:161: testTypeTransitions();
I think you should add a test for receiver changing its type.

[Vladislav Kaznacheev, 2010-06-04 07:56:25]

Covered all Anton's comments except the idea about api tests (need to discuss)

http://codereview.chromium.org/2280007/diff/72002/86010
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/72002/86010#newcode1080
src/ia32/ic-ia32.cc:1080: __ CmpInstanceType(ebx, FIRST_NONSTRING_TYPE);
Exactly. As ebx contains the objects map was always above, so a secondary probe
was always skipped.

On 2010/06/03 16:18:06, antonm wrote:
> I believe it was a bug we discussed with you?

http://codereview.chromium.org/2280007/diff/72002/86010#newcode1100
src/ia32/ic-ia32.cc:1100: __ bind(&miss);
Done. Added a comment that the generated code can fall through if both probes
miss.

On 2010/06/03 16:18:06, antonm wrote:
> maybe pass miss label as an argument?
> 
> I think it would make it easier to use this function and looks more v8ish
imho.

http://codereview.chromium.org/2280007/diff/72002/86010#newcode1200
src/ia32/ic-ia32.cc:1200: __ bind(&miss);
Added a label parameter. Commented that this code never falls through.
On 2010/06/03 16:18:06, antonm wrote:
> ditto

http://codereview.chromium.org/2280007/diff/72002/86010#newcode1302
src/ia32/ic-ia32.cc:1302: GenerateMonomorphicCacheProbe(masm, argc,
Code::KEYED_CALL_IC);
There are many reasons for monomorphic cache to miss even for a perfectly good
key.

I do not see a reason to test KeyedLoadIC_Generic as it is used widely across v8
and I am sure it is tested thoroughly.

Renamed the label to 'skip_probe'.

On 2010/06/03 16:18:06, antonm wrote:
> isn't there is a problem if GenerateMonomorphicCacheProbe would jump to miss
> label?  In that case you would take smi_key branch, correct?  I think that key
> should be something like null or undefined to hit this path.
> 
> I believe (but I would appreciate if you add tests for that) that
> KeyedLoadIC_Generic could cope with such keys, but maybe it should be made
more
> explicit in the code (e.g. rename the label)

http://codereview.chromium.org/2280007/diff/72002/86010#newcode1302
src/ia32/ic-ia32.cc:1302: GenerateMonomorphicCacheProbe(masm, argc,
Code::KEYED_CALL_IC);
If the stub lookup failed we fall through and call KeyedLoadIC_Generic. This is
the most sensible and fast thing to do, and there is absolutely no reason to go
into the miss case.
On 2010/06/03 16:18:06, antonm wrote:
> similar note: it doesn't look like we go in miss case if we failed to lookup a
> stub for the name, correct?

http://codereview.chromium.org/2280007/diff/72002/86010#newcode1325
src/ia32/ic-ia32.cc:1325: __ j(below, &miss, not_taken);
Some of these checks are redundant, but I have a plan to get rid of them in the
next patch. There I will inline KeyedLoadIC_Generic code and redirect its
bailouts to bypass this code (I already have a rough prototype).

On 2010/06/03 16:18:06, antonm wrote:
> cache probing does some of these checks, I wonder if we could unify them, but
> that's probably too much of trouble

http://codereview.chromium.org/2280007/diff/72002/86011
File src/ic.cc (right):

http://codereview.chromium.org/2280007/diff/72002/86011#newcode605
src/ic.cc:605: } else {
On 2010/06/03 16:18:06, antonm wrote:
> not insisting, but I'd rather omit the else-clause as you quite from
then-clause
> quickly anyway

Done.

http://codereview.chromium.org/2280007/diff/72002/86011#newcode1390
src/ic.cc:1390: // Used from ic_<arch>.cc.
Fixed everywhere.

On 2010/06/03 16:18:06, antonm wrote:
> Lie! used from ic-<arch>.cc :)

http://codereview.chromium.org/2280007/diff/72002/86012
File src/ic.h (right):

http://codereview.chromium.org/2280007/diff/72002/86012#newcode217
src/ic.h:217: class CallIC: public CallICBase {
We only ever construct these objects on stack, so the type is known at compile
time, no risk to mess things up. In fact, we hardly ever heap-allocate C++
objects in v8.

On 2010/06/03 16:18:06, antonm wrote:
> I know we already inherit from IC and there is no CallIC/KeyedCallIC specific
> fields, but still when I see growing hierarchy w/o virtual destructors, I am
> somewhat uneasy

http://codereview.chromium.org/2280007/diff/72002/86013
File src/stub-cache.cc (right):

http://codereview.chromium.org/2280007/diff/72002/86013#newcode445
src/stub-cache.cc:445: (kind == Code::CALL_IC ? Logger::type:
Logger::KEYED_##type)
On 2010/06/03 16:18:06, antonm wrote:
> nit: a space before : (coming from ? : construct)

Done.

http://codereview.chromium.org/2280007/diff/72002/86029
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/2280007/diff/72002/86029#newcode7126
test/cctest/test-api.cc:7126: "proto1 = new Object();"
On 2010/06/03 16:18:06, antonm wrote:
> please, rename proto1 to o and o to proto1 to have o -> proto1 -> proto2 chain

Done.

http://codereview.chromium.org/2280007/diff/72002/86029#newcode7144
test/cctest/test-api.cc:7144: // global object which is between interceptor and
constant function' holders.
Forgot to change the comment.

I am not quite getting your idea about adding another test, let's discuss it.

On 2010/06/03 16:18:06, antonm wrote:
> am I missing something, I don't see any override here.
> 
> Overall, it might be good to add a test case for two paths to miss case: when
> prototype chain changes from receiver to interceptor holder and from
interceptor
> holder to cached lookup holder---we need to be sure correct name is passed. 
But
> yes, it should be the case.

http://codereview.chromium.org/2280007/diff/72002/86030
File test/mjsunit/keyed-call-ic.js (right):

http://codereview.chromium.org/2280007/diff/72002/86030#newcode158
test/mjsunit/keyed-call-ic.js:158: function testTypeTransitions() {
Oops. Forgot about that!
On 2010/06/03 16:18:06, antonm wrote:
> why it's empty?

http://codereview.chromium.org/2280007/diff/72002/86030#newcode161
test/mjsunit/keyed-call-ic.js:161: testTypeTransitions();
On 2010/06/03 16:18:06, antonm wrote:
> I think you should add a test for receiver changing its type.

Done.

[Mads Ager (chromium), 2010-06-04 09:22:17]

LGTM when Anton is happy with the tests. :)

Could you upload a new version for Anton to sign off on? The fixes to Anton's
comments are not in the currently uploaded version.

http://codereview.chromium.org/2280007/diff/72002/86010
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/72002/86010#newcode1321
src/ia32/ic-ia32.cc:1321: // Check that the receiver is a valid JS object.
Use CmpObjectType for this?

http://codereview.chromium.org/2280007/diff/72002/86021
File src/log.h (right):

http://codereview.chromium.org/2280007/diff/72002/86021#newcode111
src/log.h:111: "KeyedCallDebugPrepareStepIn",        \
Strange indentation. I understand why you did it this way but I would prefer:

V(KEYED_...,
  "Keyed...",
  "kcdbsi")

http://codereview.chromium.org/2280007/diff/72002/86030
File test/mjsunit/keyed-call-ic.js (right):

http://codereview.chromium.org/2280007/diff/72002/86030#newcode1
test/mjsunit/keyed-call-ic.js:1: // Copyright 2008 the V8 project authors. All
rights reserved.
2010

[Vladislav Kaznacheev, 2010-06-04 10:04:45]

Anton, please take a look at the two new tests in test-api.cc.

http://codereview.chromium.org/2280007/diff/72002/86010
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/2280007/diff/72002/86010#newcode1321
src/ia32/ic-ia32.cc:1321: // Check that the receiver is a valid JS object.
On 2010/06/04 09:22:17, Mads Ager wrote:
> Use CmpObjectType for this?

Done.

http://codereview.chromium.org/2280007/diff/72002/86021
File src/log.h (right):

http://codereview.chromium.org/2280007/diff/72002/86021#newcode111
src/log.h:111: "KeyedCallDebugPrepareStepIn",        \
On 2010/06/04 09:22:17, Mads Ager wrote:
> Strange indentation. I understand why you did it this way but I would prefer:
> 
> V(KEYED_...,
>   "Keyed...",
>   "kcdbsi")

Done.

http://codereview.chromium.org/2280007/diff/72002/86030
File test/mjsunit/keyed-call-ic.js (right):

http://codereview.chromium.org/2280007/diff/72002/86030#newcode1
test/mjsunit/keyed-call-ic.js:1: // Copyright 2008 the V8 project authors. All
rights reserved.
On 2010/06/04 09:22:17, Mads Ager wrote:
> 2010

Done.

[antonm, 2010-06-04 10:29:34]

LGTM if it passes ARM/x64 tests

http://codereview.chromium.org/2280007/diff/134001/119012
File src/ic.cc (right):

http://codereview.chromium.org/2280007/diff/134001/119012#newcode605
src/ic.cc:605: } else {
apparently you forgot to remove else-clause here.  not mandatory

http://codereview.chromium.org/2280007/diff/134001/119030
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/2280007/diff/134001/119030#newcode7207
test/cctest/test-api.cc:7207: "  if (i == 5) { o.prop = 1; };"
I'd rather change method itself (to something like function(x) { return x - 1;
}, otherwise this test doesn't prove we're functioning fine.

http://codereview.chromium.org/2280007/diff/134001/119030#newcode7229
test/cctest/test-api.cc:7229: "  if (i == 5) { proto.prop = 1; };"
ditto

http://codereview.chromium.org/2280007/diff/134001/119031
File test/mjsunit/keyed-call-ic.js (right):

http://codereview.chromium.org/2280007/diff/134001/119031#newcode158
test/mjsunit/keyed-call-ic.js:158: function testTypeTransitions() {
you might want to consider adding test cases which turn receiver into not
JSObjects like numbers or strings.

[Vladislav Kaznacheev, 2010-06-04 11:11:31]

All done. Will check for perf regressions again and submit.

http://codereview.chromium.org/2280007/diff/134001/119012
File src/ic.cc (right):

http://codereview.chromium.org/2280007/diff/134001/119012#newcode605
src/ic.cc:605: } else {
Sorry, removed now.

On 2010/06/04 10:29:34, antonm wrote:
> apparently you forgot to remove else-clause here.  not mandatory

http://codereview.chromium.org/2280007/diff/134001/119030
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/2280007/diff/134001/119030#newcode7207
test/cctest/test-api.cc:7207: "  if (i == 5) { o.prop = 1; };"
On 2010/06/04 10:29:34, antonm wrote:
> I'd rather change method itself (to something like function(x) { return x - 1;
> }, otherwise this test doesn't prove we're functioning fine.

Done.

http://codereview.chromium.org/2280007/diff/134001/119030#newcode7229
test/cctest/test-api.cc:7229: "  if (i == 5) { proto.prop = 1; };"
On 2010/06/04 10:29:34, antonm wrote:
> ditto

Done.

http://codereview.chromium.org/2280007/diff/134001/119031
File test/mjsunit/keyed-call-ic.js (right):

http://codereview.chromium.org/2280007/diff/134001/119031#newcode158
test/mjsunit/keyed-call-ic.js:158: function testTypeTransitions() {
On 2010/06/04 10:29:34, antonm wrote:
> you might want to consider adding test cases which turn receiver into not
> JSObjects like numbers or strings.

Done.

[antonm, 2010-06-04 11:19:02]

Still lgtm

http://codereview.chromium.org/2280007/diff/144001/145029
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/2280007/diff/144001/145029#newcode7210
test/cctest/test-api.cc:7210: CHECK_EQ(42*5+40*5,
context->Global()->Get(v8_str("result"))->Int32Value());
nit: spaces around + (here and below)

[Vladislav Kaznacheev, 2010-06-09 12:56:36]

http://codereview.chromium.org/2280007/diff/144001/145029
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/2280007/diff/144001/145029#newcode7210
test/cctest/test-api.cc:7210: CHECK_EQ(42*5+40*5,
context->Global()->Get(v8_str("result"))->Int32Value());
On 2010/06/04 11:19:02, antonm wrote:
> nit: spaces around + (here and below)

Done.