Customized support for feedback on calls to Array.

Customized support for feedback on calls to Array.

Gather transition feedback on array calls, and inline the Array
function call when it makes sense.

R=danno@chromium.org

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

[mvstanton, 2014-05-13 15:15:40]

Hi Danno,
Here is the use of CallICStub to gather feedback for Array() calls. Toon went
over much of this a while back, and his comments resulted in:

1) Helper zone object used in Call::set_extra_info() to wrap an AllocationSite.
The idea being that we might gather myriad different types of feedback from a
call, so just returning an AllocationSite might not be general enough.
2) Some refactoring that introduced Call::global_call() and
Call::known_global_function().

I didn't create ports yet.
Thanks for the look!
--Michael

[danno, 2014-05-16 16:34:11]

Crankshaft stub?

https://codereview.chromium.org/279423005/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/279423005/diff/1/src/ast.h#newcode1733
src/ast.h:1733: class AllocationSiteInfo: public ZoneObject {
CallInfo? Don't wrap this puppy until we actually have another usage where you
need this abstraction.

https://codereview.chromium.org/279423005/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/279423005/diff/1/src/hydrogen.cc#newcode8502
src/hydrogen.cc:8502: if (argument_count == 0) {
ASSERT(argument_count == 0); and remove the if.

https://codereview.chromium.org/279423005/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/279423005/diff/1/src/hydrogen.h#newcode1296
src/hydrogen.h:1296: static const int kLoopUnfoldLimit = 8;
I've always been suspicious of this name. You should call this
kElementLoopUnrollThreshold.

https://codereview.chromium.org/279423005/diff/1/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/279423005/diff/1/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:2550: __ int3();
I think there might be a macro for this.

https://codereview.chromium.org/279423005/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/279423005/diff/1/src/ic.cc#newcode513
src/ic.cc:513: // Install default stub with the immutable parts of existing
state.
Code below is unreachable. Just make above 
ASSERT(existing_state.stub_type() == MONOMORPHIC_ARRAY);
return;

https://codereview.chromium.org/279423005/diff/1/src/ic.h
File src/ic.h (right):

https://codereview.chromium.org/279423005/diff/1/src/ic.h#newcode389
src/ic.h:389: CallType call_type,
fits on a single line?

https://codereview.chromium.org/279423005/diff/1/src/objects-visiting-inl.h
File src/objects-visiting-inl.h (right):

https://codereview.chromium.org/279423005/diff/1/src/objects-visiting-inl.h#n...
src/objects-visiting-inl.h:298: target->kind() == Code::CALL_IC) ||
This code can be removed.

https://codereview.chromium.org/279423005/diff/1/src/type-info.cc
File src/type-info.cc (right):

https://codereview.chromium.org/279423005/diff/1/src/type-info.cc#newcode137
src/type-info.cc:137: return
Handle<JSFunction>(isolate()->native_context()->array_function());
From our conversation: Don't clear the function the TypeVector for call_new if
it is the array function, since that won't leak.

[mvstanton, 2014-05-19 13:45:24]

Thanks Danno, comments addressed. I couldn't address the crankshaft stub idea
though, since we need a number of things like tail calls, and looking at the
receiver on the stack without building a frame in some cases. I will think more
about that though.
--Michael

ps - working on ports..

https://codereview.chromium.org/279423005/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/279423005/diff/1/src/ast.h#newcode1733
src/ast.h:1733: class AllocationSiteInfo: public ZoneObject {
On 2014/05/16 16:34:11, danno wrote:
> CallInfo? Don't wrap this puppy until we actually have another usage where you
> need this abstraction.

Sounds good.

https://codereview.chromium.org/279423005/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/279423005/diff/1/src/hydrogen.cc#newcode8502
src/hydrogen.cc:8502: if (argument_count == 0) {
On 2014/05/16 16:34:11, danno wrote:
> ASSERT(argument_count == 0); and remove the if.

I do need to support the 1 argument case too though. It's been vetted beforehand
to be a single argument within a range that prevents building a loop construct.
But I added the assert (>=0 && <=1) and compactified the expression that
initializes new_object.

https://codereview.chromium.org/279423005/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/279423005/diff/1/src/hydrogen.h#newcode1296
src/hydrogen.h:1296: static const int kLoopUnfoldLimit = 8;
On 2014/05/16 16:34:11, danno wrote:
> I've always been suspicious of this name. You should call this
> kElementLoopUnrollThreshold.

Done.

https://codereview.chromium.org/279423005/diff/1/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/279423005/diff/1/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:2550: __ int3();
On 2014/05/16 16:34:11, danno wrote:
> I think there might be a macro for this.

There isn't, code even in the macro assembler uses int3() to indicate "should
not come here." I could make one, something like __ Trap(), which gets used the
same on all platforms, worthwhile?

https://codereview.chromium.org/279423005/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/279423005/diff/1/src/ic.cc#newcode513
src/ic.cc:513: // Install default stub with the immutable parts of existing
state.
On 2014/05/16 16:34:11, danno wrote:
> Code below is unreachable. Just make above 
> ASSERT(existing_state.stub_type() == MONOMORPHIC_ARRAY);
> return;

Done.

https://codereview.chromium.org/279423005/diff/1/src/ic.h
File src/ic.h (right):

https://codereview.chromium.org/279423005/diff/1/src/ic.h#newcode389
src/ic.h:389: CallType call_type,
On 2014/05/16 16:34:11, danno wrote:
> fits on a single line?

Done.

https://codereview.chromium.org/279423005/diff/1/src/objects-visiting-inl.h
File src/objects-visiting-inl.h (right):

https://codereview.chromium.org/279423005/diff/1/src/objects-visiting-inl.h#n...
src/objects-visiting-inl.h:298: target->kind() == Code::CALL_IC) ||
On 2014/05/16 16:34:11, danno wrote:
> This code can be removed.

Done.

https://codereview.chromium.org/279423005/diff/1/src/type-info.cc
File src/type-info.cc (right):

https://codereview.chromium.org/279423005/diff/1/src/type-info.cc#newcode137
src/type-info.cc:137: return
Handle<JSFunction>(isolate()->native_context()->array_function());
On 2014/05/16 16:34:11, danno wrote:
> From our conversation: Don't clear the function the TypeVector for call_new if
> it is the array function, since that won't leak.

Okay, I've put that over in ClearFunctionTypeFeedback

[mvstanton, 2014-05-20 07:55:31]

Uploaded ports...

[danno, 2014-05-22 08:20:43]

LGTM wit nits.

https://codereview.chromium.org/279423005/diff/40001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/279423005/diff/40001/src/arm/code-stubs-arm.c...
src/arm/code-stubs-arm.cc:3125: // the MONOMORPHIC_ARRAY case is handled with a
helper function.
nit: this comment is a bit misleading, all non-defaults are handled with a
helper function, MONOMORPHIC_ARRAY is just an example.

https://codereview.chromium.org/279423005/diff/40001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/279423005/diff/40001/src/ic.cc#newcode512
src/ic.cc:512: return;
nit: you don't need the return at the end.

https://codereview.chromium.org/279423005/diff/40001/src/ic.cc#newcode1850
src/ic.cc:1850: isolate()->context()->native_context()->array_function(),
nit: strange indentations

https://codereview.chromium.org/279423005/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/279423005/diff/40001/src/objects.cc#newcode11241
src/objects.cc:11241: case ALLOCATION_SITE_TYPE: break;
Perhaps comments here explaining why each of these cases shouldn't clear
feedback?

[mvstanton, 2014-05-22 09:29:46]

Thanks Danno, added some comments, rebased, landing.

https://codereview.chromium.org/279423005/diff/40001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/279423005/diff/40001/src/arm/code-stubs-arm.c...
src/arm/code-stubs-arm.cc:3125: // the MONOMORPHIC_ARRAY case is handled with a
helper function.
On 2014/05/22 08:20:43, danno wrote:
> nit: this comment is a bit misleading, all non-defaults are handled with a
> helper function, MONOMORPHIC_ARRAY is just an example.

Cool, I removed the comment, it's self-explanatory.

https://codereview.chromium.org/279423005/diff/40001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/279423005/diff/40001/src/ic.cc#newcode512
src/ic.cc:512: return;
On 2014/05/22 08:20:43, danno wrote:
> nit: you don't need the return at the end.

Done.

https://codereview.chromium.org/279423005/diff/40001/src/ic.cc#newcode1850
src/ic.cc:1850: isolate()->context()->native_context()->array_function(),
On 2014/05/22 08:20:43, danno wrote:
> nit: strange indentations

Done.

https://codereview.chromium.org/279423005/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/279423005/diff/40001/src/objects.cc#newcode11241
src/objects.cc:11241: case ALLOCATION_SITE_TYPE: break;
On 2014/05/22 08:20:43, danno wrote:
> Perhaps comments here explaining why each of these cases shouldn't clear
> feedback?

Done.

[mvstanton, 2014-05-22 09:30:15]

Committed patchset #4 manually as r21429 (presubmit successful).