Create a function call IC

Function calls are handled with a LoadIC and a FunctionCallStub. This works well, but we can create a platform to gather custom feedback by inventing a CallIC. CallIC takes a JSFunction as input, maintains UNINIT/MONO/MEGA state in a feedback slot, and engages in limited patching for the most efficient code.

The change creates a degrade in NBody tests, but a follow-on CL addresses it by using this custom feedback platform to give a 45% improvement.

R=verwaest@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=20474

[mvstanton, 2014-03-04 14:21:08]

Hi Toon,
Here is the "new style" CallIC, which uses at most two precompiled stubs to do
it's work at each site.

The UNINITIALIZED, MEGAMORPHIC, and "most popular" MONOMORPHIC case are in one
stub, with other MONOMORPHIC cases patched as needed. The hybrid is achieved by
using the type vector to store feedback.

Thanks for having a look, no ports done yet.
--Michael

[Toon Verwaest, 2014-03-10 13:50:43]

Added comments. I like the overall approach. It's pretty simple.

In addition to the comments, please use enums instead of bools (as you already
told me you would).

https://codereview.chromium.org/172523002/diff/160001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/172523002/diff/160001/src/code-stubs.h#newcod...
src/code-stubs.h:837: bool strict_native() const { return
state_.strict_native(); }
is_strict_or_native?

https://codereview.chromium.org/172523002/diff/160001/src/debug.h
File src/debug.h (right):

https://codereview.chromium.org/172523002/diff/160001/src/debug.h#newcode454
src/debug.h:454: static void GenerateCallICDebugBreak(MacroAssembler* masm);
Seems like this function is dead. If so, can you remove it?

https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2424: void CallICStub::GenerateCall(MacroAssembler*
masm, bool monomorphic,
one arg per line

https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2528: __ cmp(ecx, edi);
Wouldn't it be faster to flip over the conditions, ie, inline the fast path as
early as possible and jump farther only in the miss / generic cases.

https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2534: // If we got here we went megamorphic. Don't
bother missing, just update.
we went -> If we get here, go from monomorphic to megamorphic. Don't ..

https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2544: // Verify still monomorphic, miss otherwise
Verify whether the input function matches the recorded monomorphic target?

https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2548: __ j(equal, &mono);
Same here as above. This path should be inlined here.

https://codereview.chromium.org/172523002/diff/160001/src/ia32/full-codegen-i...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/172523002/diff/160001/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:2609: CallIC(ic);  // NOTE: no type feedback id.
What about something like:
// Don't assign a type feedback id to the IC, since type feedback is provided by
the vector above.

https://codereview.chromium.org/172523002/diff/160001/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:2652: Handle<Code> ic =
CallIC::initialize_stub(isolate(), arg_count,
Doesn't this fit on one line? Otherwise 
... CallIC::initialize_stub(
    isolate(), arg_count, true);

https://codereview.chromium.org/172523002/diff/160001/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:2671: void
FullCodeGenerator::EmitCallWithStub(Call* expr) {
I guess we can now remove this function and just use EmitCallWithIC?
CallWithStub became a CallWithIC, so at least the name makes no sense anymore.

https://codereview.chromium.org/172523002/diff/160001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode95
src/ic.cc:95: if (new_target->kind() != Code::CALL_IC) {
Isn't this only relevant for KEYED_STORE_IC?

https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1800
src/ic.cc:1800: if (feedback->IsJSFunction()) {
Shouldn't this be feedback->IsJSFunction() && function->IsJSFunction() ?

https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1802
src/ic.cc:1802: // otherwise the slow path will be taken.
Weird sentence

https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1805
src/ic.cc:1805: // Do we need to patch? Only if we currently don't have the
default stub
We only need to patch if ...

https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1815
src/ic.cc:1815: // Ugh. Do nothing if the function isn't a function, the slow
path is taken.
Remove ugh.

https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1836
src/ic.cc:1836: // Ideally we can just patch the cell and return.
Are we sure that we can only get here because of snapshot code?

Why not just: Stay monomorphic if the type feedback in the vector was cleared,
but the IC wasn't (and is still monomorphic).

https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1846
src/ic.cc:1846: Handle<String> name;
Don't you want to initialize this to something like the empty_string?

https://codereview.chromium.org/172523002/diff/160001/src/ic.h
File src/ic.h (right):

https://codereview.chromium.org/172523002/diff/160001/src/ic.h#newcode314
src/ic.h:314: static int ExtractArgcFromMinorKey(int minor_key) {
minor key -> ExtraICState? Only code stubs have major / minor keys. The rest of
the IC mechanism talks about extra IC state (which is encoded in the code flags,
independent of where the minor / major stuff is encoded).

https://codereview.chromium.org/172523002/diff/160001/src/ic.h#newcode330
src/ic.h:330: STATIC_ASSERT(Code::kArgumentsBits == 16);
What about just using kArgumentsBits in the counts below so we don't need to
change it by hand later?

https://codereview.chromium.org/172523002/diff/160001/src/ic.h#newcode336
src/ic.h:336: // We have a few extra bits.
Not sure how many you have left. Aren't these encoded in ExtraICStateField?
That's only 20 bits isn't it?

[mvstanton, 2014-03-20 15:51:53]

Hi Toon,
Thanks for the comments, and sorry for the delay in addressing! PTAL, thanks,
--Michael

https://codereview.chromium.org/172523002/diff/160001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/172523002/diff/160001/src/code-stubs.h#newcod...
src/code-stubs.h:837: bool strict_native() const { return
state_.strict_native(); }
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> is_strict_or_native?

The move to enums addressed this satisfactorily I think.

https://codereview.chromium.org/172523002/diff/160001/src/debug.h
File src/debug.h (right):

https://codereview.chromium.org/172523002/diff/160001/src/debug.h#newcode454
src/debug.h:454: static void GenerateCallICDebugBreak(MacroAssembler* masm);
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> Seems like this function is dead. If so, can you remove it?

Done.

https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2424: void CallICStub::GenerateCall(MacroAssembler*
masm, bool monomorphic,
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> one arg per line

Done.

https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2528: __ cmp(ecx, edi);
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> Wouldn't it be faster to flip over the conditions, ie, inline the fast path as
> early as possible and jump farther only in the miss / generic cases.

Excellent idea, it reads cleaner too.

https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2534: // If we got here we went megamorphic. Don't
bother missing, just update.
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> we went -> If we get here, go from monomorphic to megamorphic. Don't ..

Done.

https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2544: // Verify still monomorphic, miss otherwise
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> Verify whether the input function matches the recorded monomorphic target?

After rearranging the function, I kept your ideal phrase "recorded monomorphic
target."

https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2548: __ j(equal, &mono);
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> Same here as above. This path should be inlined here.

Done.

https://codereview.chromium.org/172523002/diff/160001/src/ia32/full-codegen-i...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/172523002/diff/160001/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:2609: CallIC(ic);  // NOTE: no type feedback id.
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> What about something like:
> // Don't assign a type feedback id to the IC, since type feedback is provided
by
> the vector above.

Done.

https://codereview.chromium.org/172523002/diff/160001/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:2652: Handle<Code> ic =
CallIC::initialize_stub(isolate(), arg_count,
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> Doesn't this fit on one line? Otherwise 
> ... CallIC::initialize_stub(
>     isolate(), arg_count, true);

Done.

https://codereview.chromium.org/172523002/diff/160001/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:2671: void
FullCodeGenerator::EmitCallWithStub(Call* expr) {
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> I guess we can now remove this function and just use EmitCallWithIC?
> CallWithStub became a CallWithIC, so at least the name makes no sense anymore.

Cool, this refactored very nicely.

https://codereview.chromium.org/172523002/diff/160001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode95
src/ic.cc:95: if (new_target->kind() != Code::CALL_IC) {
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> Isn't this only relevant for KEYED_STORE_IC?

Done.

https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1800
src/ic.cc:1800: if (feedback->IsJSFunction()) {
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> Shouldn't this be feedback->IsJSFunction() && function->IsJSFunction() ?

I went ahead and just returned without changing anything now if
!function->IsJSFunction().

https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1802
src/ic.cc:1802: // otherwise the slow path will be taken.
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> Weird sentence

Removed.

https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1805
src/ic.cc:1805: // Do we need to patch? Only if we currently don't have the
default stub
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> We only need to patch if ...

Done.

https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1815
src/ic.cc:1815: // Ugh. Do nothing if the function isn't a function, the slow
path is taken.
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> Remove ugh.

Done.

https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1836
src/ic.cc:1836: // Ideally we can just patch the cell and return.
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> Are we sure that we can only get here because of snapshot code?
> 
> Why not just: Stay monomorphic if the type feedback in the vector was cleared,
> but the IC wasn't (and is still monomorphic).

I went to objects-visiting-inl.h and make sure that monomorphic call ics are
cleared in sync with the associated feedback cells. This is the simplest thing
for now. So if our type cell is uninitialized we won't have a specialized
MONOMORPHIC CallIC in place.

If this turns out to be a performance issue I can address it with your
suggestion, which sounds good.

https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1846
src/ic.cc:1846: Handle<String> name;
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> Don't you want to initialize this to something like the empty_string?

Yes, I could simplify that even more.

https://codereview.chromium.org/172523002/diff/160001/src/ic.h
File src/ic.h (right):

https://codereview.chromium.org/172523002/diff/160001/src/ic.h#newcode330
src/ic.h:330: STATIC_ASSERT(Code::kArgumentsBits == 16);
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> What about just using kArgumentsBits in the counts below so we don't need to
> change it by hand later?

Done.

https://codereview.chromium.org/172523002/diff/160001/src/ic.h#newcode336
src/ic.h:336: // We have a few extra bits.
On 2014/03/10 13:50:44, Toon Verwaest wrote:
> Not sure how many you have left. Aren't these encoded in ExtraICStateField?
> That's only 20 bits isn't it?

Old comment, removed.

[Toon Verwaest, 2014-03-24 10:25:44]

Some more comments. Getting there. I especially like the fullcodegen cleanup.

https://codereview.chromium.org/172523002/diff/280001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/172523002/diff/280001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2456: if (attributes ==
CallIC::NOT_STRICT_OR_NATIVE) {
CallIC::SLOPPY

Seems like in this case you don't need the checks above.

https://codereview.chromium.org/172523002/diff/280001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2469: if (argument_check == CallIC::ARGUMENTS_MATCH)
{
ARGUMENTS_MUST_MATCH, to distinguish from the other case,
ARGUMENTS_COUNT_UNKNOWN

https://codereview.chromium.org/172523002/diff/280001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2527: __ mov(ecx, FieldOperand(ebx, edx,
times_half_pointer_size,
__ cmp(edi, Field...

https://codereview.chromium.org/172523002/diff/280001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2532: if (stub_type() != CallIC::MONOMORPHIC) {
stub_type() == CallIC::DEFAULT_MONOMORPHIC

https://codereview.chromium.org/172523002/diff/280001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2535: CallIC::ARGUMENTS_MATCH,
Can we merge this with the non-default case by just setting up argument_check()
and function_attributes() correctly for the default case?

https://codereview.chromium.org/172523002/diff/280001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2536: CallIC::NOT_STRICT_OR_NATIVE);
CallIC::default_strict_or_native() {
  return argument_check() == ARGUMENTS_MATCH && function_attributes() ==
CallIC::SLOPPY;
}

https://codereview.chromium.org/172523002/diff/280001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/172523002/diff/280001/src/ic.cc#newcode1836
src/ic.cc:1836: return;
Patch in the generic case, which doesn't record anymore, but also doesn't check.

[mvstanton, 2014-03-25 15:34:50]

Thanks Toon, here are the refactorings we discussed,
--Michael

https://codereview.chromium.org/172523002/diff/280001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/172523002/diff/280001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2456: if (attributes ==
CallIC::NOT_STRICT_OR_NATIVE) {
On 2014/03/24 10:25:44, Toon Verwaest wrote:
> CallIC::SLOPPY
> 
> Seems like in this case you don't need the checks above.

Good catch, thanks. Done.

https://codereview.chromium.org/172523002/diff/280001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2469: if (argument_check == CallIC::ARGUMENTS_MATCH)
{
On 2014/03/24 10:25:44, Toon Verwaest wrote:
> ARGUMENTS_MUST_MATCH, to distinguish from the other case,
> ARGUMENTS_COUNT_UNKNOWN

Done.

https://codereview.chromium.org/172523002/diff/280001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2527: __ mov(ecx, FieldOperand(ebx, edx,
times_half_pointer_size,
On 2014/03/24 10:25:44, Toon Verwaest wrote:
> __ cmp(edi, Field...

Done.

https://codereview.chromium.org/172523002/diff/280001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2532: if (stub_type() != CallIC::MONOMORPHIC) {
On 2014/03/24 10:25:44, Toon Verwaest wrote:
> stub_type() == CallIC::DEFAULT_MONOMORPHIC

Done.

https://codereview.chromium.org/172523002/diff/280001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2535: CallIC::ARGUMENTS_MATCH,
On 2014/03/24 10:25:44, Toon Verwaest wrote:
> Can we merge this with the non-default case by just setting up
argument_check()
> and function_attributes() correctly for the default case?

Yes indeed, thanks!

https://codereview.chromium.org/172523002/diff/280001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/172523002/diff/280001/src/ic.cc#newcode1836
src/ic.cc:1836: return;
On 2014/03/24 10:25:44, Toon Verwaest wrote:
> Patch in the generic case, which doesn't record anymore, but also doesn't
check.

I fixed this to patch to megamorphic in this case, but I don't have a real
GENERIC stub, I only have:

DEFAULT_MONOMORPHIC - maintains type cell, embeds a popular monomorphic case,
handles everything.

MONOMORPHIC - maintains type cell, but customized to the function.

I think it's okay to plug in DEFAULT_MONOMORPHIC. It does mean we keep checking
against the MEGAMORPHIC sentinel, but this doesn't need to be a faster path
anyway. The old code did the same thing.

[Toon Verwaest, 2014-03-27 16:02:25]

LGTM, go ahead with the ports.
Just some minor nits.

https://codereview.chromium.org/172523002/diff/300001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/172523002/diff/300001/src/code-stubs.h#newcod...
src/code-stubs.h:842: return state_.IsSloppyOrNonNative();
IsSloppyNonNative()
It's and, not or.
That, or IsStrictOrNative()

https://codereview.chromium.org/172523002/diff/300001/src/code-stubs.h#newcod...
src/code-stubs.h:1671: }
Spurious change

https://codereview.chromium.org/172523002/diff/300001/src/ia32/debug-ia32.cc
File src/ia32/debug-ia32.cc (right):

https://codereview.chromium.org/172523002/diff/300001/src/ia32/debug-ia32.cc#...
src/ia32/debug-ia32.cc:208: // TODO(mvstanton): should eax be saved, non-object
reg?
So?

https://codereview.chromium.org/172523002/diff/300001/src/ic.h
File src/ic.h (right):

https://codereview.chromium.org/172523002/diff/300001/src/ic.h#newcode314
src/ic.h:314: enum FunctionAttributes { SLOPPY_OR_NONNATIVE, STRICT_OR_NATIVE };
SLOPPY_NONNATIVE

https://codereview.chromium.org/172523002/diff/300001/src/ic.h#newcode395
src/ic.h:395: class ArgBits: public BitField<int, 0, Code::kArgumentsBits> {};
Arg or ArgcBits? Just make sure it's consistent everywhere.

[mvstanton, 2014-04-01 13:01:48]

Thanks Toon, nits and ports addressed.
I did find a bug: with incremental marking, we might clear ICs in a code object
without also clearing type cells. I fixed this with a change in
SharedFunctionInfo::ResetForNewContext(). 

Will do some perf jobs, etc., before final submit.
--Michael

https://codereview.chromium.org/172523002/diff/300001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/172523002/diff/300001/src/code-stubs.h#newcod...
src/code-stubs.h:842: return state_.IsSloppyOrNonNative();
On 2014/03/27 16:02:25, Toon Verwaest wrote:
> IsSloppyNonNative()
> It's and, not or.
> That, or IsStrictOrNative()

Done.

https://codereview.chromium.org/172523002/diff/300001/src/code-stubs.h#newcod...
src/code-stubs.h:1671: }
On 2014/03/27 16:02:25, Toon Verwaest wrote:
> Spurious change

Done.

https://codereview.chromium.org/172523002/diff/300001/src/ia32/debug-ia32.cc
File src/ia32/debug-ia32.cc (right):

https://codereview.chromium.org/172523002/diff/300001/src/ia32/debug-ia32.cc#...
src/ia32/debug-ia32.cc:208: // TODO(mvstanton): should eax be saved, non-object
reg?
On 2014/03/27 16:02:25, Toon Verwaest wrote:
> So?

Right, no need for that, I had some confusion with CallConstructStub which does
need eax saved.

https://codereview.chromium.org/172523002/diff/300001/src/ic.h
File src/ic.h (right):

https://codereview.chromium.org/172523002/diff/300001/src/ic.h#newcode314
src/ic.h:314: enum FunctionAttributes { SLOPPY_OR_NONNATIVE, STRICT_OR_NATIVE };
On 2014/03/27 16:02:25, Toon Verwaest wrote:
> SLOPPY_NONNATIVE

Done.

https://codereview.chromium.org/172523002/diff/300001/src/ic.h#newcode395
src/ic.h:395: class ArgBits: public BitField<int, 0, Code::kArgumentsBits> {};
On 2014/03/27 16:02:25, Toon Verwaest wrote:
> Arg or ArgcBits? Just make sure it's consistent everywhere.

Done, I went with ArgcBits.

[mvstanton, 2014-04-03 11:41:28]

Committed patchset #14 manually as r20474 (presubmit successful).