Teach TurboFan to call vector-based ICs.

Teach TurboFan to call vector-based ICs.

Additional static information needs to be passed to Load and KeyedLoad calls if
--vector-ics is turned on.

R=mstarzinger@chromium.org

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

[mvstanton, 2014-10-08 13:56:45]

Hi Michael,
Thanks for the help in TurboFanLand. Note there is one platform-specific change,
slightly unrelated to the rest of the CL. I'll upload (boilerplate) ports after
you have a look.
All the best,
--Michael

[mvstanton, 2014-10-08 14:24:14]

Patchset #1 (id:1) has been deleted

[Michael Starzinger, 2014-10-08 15:23:09]

https://codereview.chromium.org/633423002/diff/20001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/633423002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.h:55: LoadICFeedbackNode LoadNamedFeedback(int
slot) const {
As discussed offline: Haveing to deal with IC-related data structures here is a
smell. Can we instead just define a vecotr-slot-pair in js-operators.h that is a
pure data container without logic?

nit: This needs a comment. Also we should make it private (by placing it further
down).

https://codereview.chromium.org/633423002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.h:93: Node* BuildVariableLoad(VariableProxy*
proxy, BailoutId bailout_id,
Either all the BuildVariableFoo helpers should work on Variable or on
VariableProxy, this arbitrary dichotomy smells.

As discussed offline: It might be better to instead pass the vector-slot-pair as
an argument.

https://codereview.chromium.org/633423002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.h:129: Handle<TypeFeedbackVector> vector_;
nit: Let's not start caching handles yet, this seems like premature
optimization.

https://codereview.chromium.org/633423002/diff/20001/test/unittests/compiler/...
File test/unittests/compiler/js-typed-lowering-unittest.cc (right):

https://codereview.chromium.org/633423002/diff/20001/test/unittests/compiler/...
test/unittests/compiler/js-typed-lowering-unittest.cc:90:
KeyedLoadICFeedbackNode feedback(Handle<TypeFeedbackVector>::null(), 0);
nit: Please move this out of the for-each loop (to around line 80) for
readability.

[mvstanton, 2014-10-10 08:21:59]

Patchset #2 (id:40001) has been deleted

[mvstanton, 2014-10-10 08:26:16]

Hi Michael, thanks for the look. Here is a new patch!
--Michael

https://codereview.chromium.org/633423002/diff/20001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/633423002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.h:55: LoadICFeedbackNode LoadNamedFeedback(int
slot) const {
On 2014/10/08 15:23:09, Michael Starzinger wrote:
> As discussed offline: Haveing to deal with IC-related data structures here is
a
> smell. Can we instead just define a vecotr-slot-pair in js-operators.h that is
a
> pure data container without logic?
> 
> nit: This needs a comment. Also we should make it private (by placing it
further
> down).

Done.

https://codereview.chromium.org/633423002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.h:93: Node* BuildVariableLoad(VariableProxy*
proxy, BailoutId bailout_id,
On 2014/10/08 15:23:09, Michael Starzinger wrote:
> Either all the BuildVariableFoo helpers should work on Variable or on
> VariableProxy, this arbitrary dichotomy smells.
> 
> As discussed offline: It might be better to instead pass the vector-slot-pair
as
> an argument.

Done.

https://codereview.chromium.org/633423002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.h:129: Handle<TypeFeedbackVector> vector_;
On 2014/10/08 15:23:09, Michael Starzinger wrote:
> nit: Let's not start caching handles yet, this seems like premature
> optimization.

Done.

https://codereview.chromium.org/633423002/diff/20001/test/unittests/compiler/...
File test/unittests/compiler/js-typed-lowering-unittest.cc (right):

https://codereview.chromium.org/633423002/diff/20001/test/unittests/compiler/...
test/unittests/compiler/js-typed-lowering-unittest.cc:90:
KeyedLoadICFeedbackNode feedback(Handle<TypeFeedbackVector>::null(), 0);
On 2014/10/08 15:23:09, Michael Starzinger wrote:
> nit: Please move this out of the for-each loop (to around line 80) for
> readability.

Done.

[Michael Starzinger, 2014-10-10 09:09:59]

LGTM (just one more nit).

https://codereview.chromium.org/633423002/diff/130001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/633423002/diff/130001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1105: int slot =
property->PropertyFeedbackSlot();
nit: Instead of randomly pulling either "slot" or "pair" out into a local
variable, we could uniformly use "pair" everywhere ... then you upcoming change
to the type of "slot" gets easier too. This applies throughout the file.

[mvstanton, 2014-10-10 09:31:16]

Cool, that is much better, thanks again,
--Michael

https://codereview.chromium.org/633423002/diff/130001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/633423002/diff/130001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1105: int slot =
property->PropertyFeedbackSlot();
On 2014/10/10 09:09:58, Michael Starzinger wrote:
> nit: Instead of randomly pulling either "slot" or "pair" out into a local
> variable, we could uniformly use "pair" everywhere ... then you upcoming
change
> to the type of "slot" gets easier too. This applies throughout the file.

Good idea, done.

[mvstanton, 2014-10-10 09:50:02]

Committed patchset #4 (id:220001) manually as 24519 (presubmit successful).