Vector ICs: Turbofan vector store ic support

Vector ICs: Turbofan vector store ic support

Turbofan needs to pass vector slots around for named and keyed stores.
Also, the CL addresses a missing slot for ClassLiterals.

BUG=

Committed: https://crrev.com/17c8ffeaa3e4e1769f0b4d0e7c8a6fb26b5297bd
Cr-Commit-Position: refs/heads/master@{#29173}

[mvstanton, 2015-06-11 23:13:33]

mvstanton@chromium.org changed reviewers:
+ jkummerow@chromium.org, mstarzinger@chromium.org

[mvstanton, 2015-06-11 23:13:34]

Hi guys,
I'd ask Michael to review the changes under src/compiler, and Jakob to look at
the full-codegen stuff. The full-codegen change was noticed as an oversight to
the earlier work I did there, and it'd be cool if I don't have to split up the
CLs.

All the best, thanks for the look guys,
--Michael

[Michael Starzinger, 2015-06-12 07:59:06]

The TurboFan part is looking good. Mostly nits.

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:1645: ResolveFeedbackSlot(slot),
BailoutId::None(),
nit: Let's put this into a local variable for consistency with the rest of the
code.

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:1914: DCHECK(!FLAG_vector_stores ||
store_slot_index == expr->slot_count());
nit: Please move this to before the call to ProdcueValue(), every expression
visitor has a call to ProduceValue() right at the bottom, otherwise the contract
is broken. I would like to visually keep it that way.

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:1951: // TODO(mvstanton): this should be a
runtime call. We can't use an
Let's discuss this TODO in person, I would like to understand this further.

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:2693: ResolveFeedbackSlot(expr->CountSlot()),
nit: Let's put this into a local variable for consistency with the rest of the
code.

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:2702: ResolveFeedbackSlot(expr->CountSlot()),
nit: Likewise.

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:2714: ResolveFeedbackSlot(expr->CountSlot()),
nit: Likewise.

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:3145: DCHECK(!FLAG_vector_stores ||
arguments->location() != Variable::UNALLOCATED);
nit: This DCHECK is subsumed by the one before, please drop it.

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:3166: DCHECK(!FLAG_vector_stores ||
rest->location() != Variable::UNALLOCATED);
nit: This DCHECK is subsumed by the one before, please drop it.

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:3181: this_function_var->location() !=
Variable::UNALLOCATED);
nit: Let's drop this DCHECK.

[mvstanton, 2015-06-12 17:18:47]

Hi guys,
Addressed Michael's comments. Best,
--Michael

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:1645: ResolveFeedbackSlot(slot),
BailoutId::None(),
On 2015/06/12 07:59:06, Michael Starzinger wrote:
> nit: Let's put this into a local variable for consistency with the rest of the
> code.

Done.

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:1914: DCHECK(!FLAG_vector_stores ||
store_slot_index == expr->slot_count());
On 2015/06/12 07:59:06, Michael Starzinger wrote:
> nit: Please move this to before the call to ProdcueValue(), every expression
> visitor has a call to ProduceValue() right at the bottom, otherwise the
contract
> is broken. I would like to visually keep it that way.

Well said, done.

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:1951: // TODO(mvstanton): this should be a
runtime call. We can't use an
On 2015/06/12 07:59:06, Michael Starzinger wrote:
> Let's discuss this TODO in person, I would like to understand this further.

Yeah, the deal is to use a vector-based IC you need a slot. So if optimized code
wants to go off and use one, the slot needs to be allocated in the AST. And that
would be a major shame if full code doesn't use it. Benedikt knows something
about this line too, he suggested the runtime call.

Alternatively, we beef up full code to use an IC here, and then I'm happy in a
different way. :p What do you think?

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:2693: ResolveFeedbackSlot(expr->CountSlot()),
On 2015/06/12 07:59:06, Michael Starzinger wrote:
> nit: Let's put this into a local variable for consistency with the rest of the
> code.

Done.

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:2702: ResolveFeedbackSlot(expr->CountSlot()),
On 2015/06/12 07:59:06, Michael Starzinger wrote:
> nit: Likewise.

Done.

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:2714: ResolveFeedbackSlot(expr->CountSlot()),
On 2015/06/12 07:59:05, Michael Starzinger wrote:
> nit: Likewise.

Done.

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:3145: DCHECK(!FLAG_vector_stores ||
arguments->location() != Variable::UNALLOCATED);
On 2015/06/12 07:59:06, Michael Starzinger wrote:
> nit: This DCHECK is subsumed by the one before, please drop it.

Will do. The reason for the paranoia is that this is the only place in the code
where I'm explicitly passing an invalid ic slot, because I know that
BuildVariableAssignment won't need it.

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:3166: DCHECK(!FLAG_vector_stores ||
rest->location() != Variable::UNALLOCATED);
On 2015/06/12 07:59:06, Michael Starzinger wrote:
> nit: This DCHECK is subsumed by the one before, please drop it.

Done.

https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:3181: this_function_var->location() !=
Variable::UNALLOCATED);
On 2015/06/12 07:59:06, Michael Starzinger wrote:
> nit: Let's drop this DCHECK.

Done.

[Jakob Kummerow, 2015-06-15 11:04:57]

non-TF changes LGTM.

[Michael Starzinger, 2015-06-15 14:20:11]

LGTM, just nits.

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

https://codereview.chromium.org/1178363002/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1355:
ResolveFeedbackSlot(stmt->EachFeedbackSlot()),
nit: Please pull into local variable for consistency.

https://codereview.chromium.org/1178363002/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1952: // IC unless we have it in full code as
well.
As discussed offline: It's not the responsibility of the AstGraphBuilder to
decide whether to use a runtime call or an IC.

Also, these stores are good candidates for load-store-elimination and escape
analysis, so I would strongly vote for keeping them as normal keyed stores.

Hence I think we should just drop this TODO and address this in the generic
lowering once vectore store ICs are actually called there. Of course I am fine
with keeping a comment here saying that we just don't have feedback slots for
these stores.

https://codereview.chromium.org/1178363002/diff/20001/src/compiler/js-operator.h
File src/compiler/js-operator.h (right):

https://codereview.chromium.org/1178363002/diff/20001/src/compiler/js-operato...
src/compiler/js-operator.h:302: // Defines the property being loaded from an
object. This is
nit: s/loaded from an object/stored to an object/

https://codereview.chromium.org/1178363002/diff/20001/src/compiler/js-operato...
src/compiler/js-operator.h:303: // used as a parameter by JSLoadProperty
operators.
nit: s/JSLoadProperty/JSStoreProperty/

https://codereview.chromium.org/1178363002/diff/20001/src/compiler/js-operato...
src/compiler/js-operator.h:410: const Unique<Name>& name);
nit: IMHO the name should go first or at least before the feedback.

[mvstanton, 2015-06-21 13:31:13]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2015-06-21 13:31:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1178363002/#ps60001
(title: "Fix mips64 compile error.")

[commit-bot: I haz the power, 2015-06-21 13:31:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1178363002/60001

[mvstanton, 2015-06-21 13:37:36]

thanks!

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

https://codereview.chromium.org/1178363002/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1355:
ResolveFeedbackSlot(stmt->EachFeedbackSlot()),
On 2015/06/15 14:20:11, Michael Starzinger wrote:
> nit: Please pull into local variable for consistency.

Done.

https://codereview.chromium.org/1178363002/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1952: // IC unless we have it in full code as
well.
On 2015/06/15 14:20:11, Michael Starzinger wrote:
> As discussed offline: It's not the responsibility of the AstGraphBuilder to
> decide whether to use a runtime call or an IC.
> 
> Also, these stores are good candidates for load-store-elimination and escape
> analysis, so I would strongly vote for keeping them as normal keyed stores.
> 
> Hence I think we should just drop this TODO and address this in the generic
> lowering once vectore store ICs are actually called there. Of course I am fine
> with keeping a comment here saying that we just don't have feedback slots for
> these stores.

Done.

https://codereview.chromium.org/1178363002/diff/20001/src/compiler/js-operator.h
File src/compiler/js-operator.h (right):

https://codereview.chromium.org/1178363002/diff/20001/src/compiler/js-operato...
src/compiler/js-operator.h:302: // Defines the property being loaded from an
object. This is
On 2015/06/15 14:20:11, Michael Starzinger wrote:
> nit: s/loaded from an object/stored to an object/

Done.

https://codereview.chromium.org/1178363002/diff/20001/src/compiler/js-operato...
src/compiler/js-operator.h:303: // used as a parameter by JSLoadProperty
operators.
On 2015/06/15 14:20:11, Michael Starzinger wrote:
> nit: s/JSLoadProperty/JSStoreProperty/

thx, done.

https://codereview.chromium.org/1178363002/diff/20001/src/compiler/js-operato...
src/compiler/js-operator.h:410: const Unique<Name>& name);
On 2015/06/15 14:20:11, Michael Starzinger wrote:
> nit: IMHO the name should go first or at least before the feedback.

Done.

[commit-bot: I haz the power, 2015-06-21 13:55:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-21 13:55:40]

Patchset 4 (id:??) landed as
https://crrev.com/17c8ffeaa3e4e1769f0b4d0e7c8a6fb26b5297bd
Cr-Commit-Position: refs/heads/master@{#29173}