[turbofan] Collect and use type feedback on ToNumber.

[turbofan] Collect and use type feedback on ToNumber.

Make Ignition collect BinaryOperationFeedback on ToNumber, using the
shared type feedback slot with the following Inc/Dec bytecode, and use
this feedback in TurboFan to turn the ToNumber(x) operation into a
SpeculativeNumberMultiply(x,1) with the feedback hint.

R=jarin@chromium.org, mstarzinger@chromium.org, rmcilroy@chromium.org
BUG=v8:6214, v8:5267
Review-Url: https://codereview.chromium.org/2804813003
Cr-Commit-Position: refs/heads/master@{#44440}
Committed: https://chromium.googlesource.com/v8/v8/+/dd7ce93ce0262fff292edeb6fb58d9aed2c29554

[Benedikt Meurer, 2017-04-06 11:10:05]

The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 11:10:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rmcilroy, 2017-04-06 11:21:32]

LGTM for now, but I'd like to avoid collecting the same feedback twice in the
same feedback slot going forward (see below).

https://codereview.chromium.org/2804813003/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2804813003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:2979: FeedbackSlot count_slot =
expr->CountBinaryOpFeedbackSlot();
Hmm, we are now recording this type feedback twice. I guess this is fine for
now, but it suggests we should rework this since the only place we use either
the ToNumber or Count bytecodes is for count operations. Maybe we can think
about having a single bytecode for CountPostfix (need to think about how we
deopt though) ? WDYT?

https://codereview.chromium.org/2804813003/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2804813003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:1154: Node*
InterpreterAssembler::ToNumberWithFeedback(Node* context, Node* object,
Please just do this inline InterpreterGenerator::DoToNumber if it's not used by
any other bytecode handler.

[Michael Starzinger, 2017-04-06 11:28:31]

LGTM.

https://codereview.chromium.org/2804813003/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2804813003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:1154: Node*
InterpreterAssembler::ToNumberWithFeedback(Node* context, Node* object,
On 2017/04/06 11:21:31, rmcilroy wrote:
> Please just do this inline InterpreterGenerator::DoToNumber if it's not used
by
> any other bytecode handler.

+1

https://codereview.chromium.org/2804813003/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-generator.cc (right):

https://codereview.chromium.org/2804813003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-generator.cc:1697: // ToNumber
nit: I know it was already missing before, but a better comment would be: "//
ToNumber <dst> <slot>" here.

[Benedikt Meurer, 2017-04-06 11:28:33]

The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 11:28:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Benedikt Meurer, 2017-04-06 11:30:00]

Thanks, feedback addressed.

https://codereview.chromium.org/2804813003/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2804813003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:2979: FeedbackSlot count_slot =
expr->CountBinaryOpFeedbackSlot();
On 2017/04/06 11:21:31, rmcilroy wrote:
> Hmm, we are now recording this type feedback twice. I guess this is fine for
> now, but it suggests we should rework this since the only place we use either
> the ToNumber or Count bytecodes is for count operations. Maybe we can think
> about having a single bytecode for CountPostfix (need to think about how we
> deopt though) ? WDYT?

I'd be in favor of that long-/middle-term. I'll put a TODO here.

https://codereview.chromium.org/2804813003/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2804813003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:1154: Node*
InterpreterAssembler::ToNumberWithFeedback(Node* context, Node* object,
On 2017/04/06 11:21:31, rmcilroy wrote:
> Please just do this inline InterpreterGenerator::DoToNumber if it's not used
by
> any other bytecode handler.

Done.

https://codereview.chromium.org/2804813003/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-generator.cc (right):

https://codereview.chromium.org/2804813003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-generator.cc:1697: // ToNumber
On 2017/04/06 11:28:31, Michael Starzinger wrote:
> nit: I know it was already missing before, but a better comment would be: "//
> ToNumber <dst> <slot>" here.

Done.

[Benedikt Meurer, 2017-04-06 11:30:17]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2017-04-06 11:30:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2804813003/#ps40001
(title: "Address Michis feedback.")

[commit-bot: I haz the power, 2017-04-06 11:30:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 11:54:45]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491478217092920,
"parent_rev": "98d1d4ec9ba55bc11fc94bf87baee080a33a7d57", "commit_rev":
"dd7ce93ce0262fff292edeb6fb58d9aed2c29554"}

[commit-bot: I haz the power, 2017-04-06 11:54:51]

Description was changed from

==========
[turbofan] Collect and use type feedback on ToNumber.

Make Ignition collect BinaryOperationFeedback on ToNumber, using the
shared type feedback slot with the following Inc/Dec bytecode, and use
this feedback in TurboFan to turn the ToNumber(x) operation into a
SpeculativeNumberMultiply(x,1) with the feedback hint.

R=jarin@chromium.org, mstarzinger@chromium.org, rmcilroy@chromium.org
BUG=v8:6214,v8:5267
==========

to

==========
[turbofan] Collect and use type feedback on ToNumber.

Make Ignition collect BinaryOperationFeedback on ToNumber, using the
shared type feedback slot with the following Inc/Dec bytecode, and use
this feedback in TurboFan to turn the ToNumber(x) operation into a
SpeculativeNumberMultiply(x,1) with the feedback hint.

R=jarin@chromium.org, mstarzinger@chromium.org, rmcilroy@chromium.org
BUG=v8:6214,v8:5267

Review-Url: https://codereview.chromium.org/2804813003
Cr-Commit-Position: refs/heads/master@{#44440}
Committed:
https://chromium.googlesource.com/v8/v8/+/dd7ce93ce0262fff292edeb6fb58d9aed2c...
==========

[commit-bot: I haz the power, 2017-04-06 11:54:52]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/dd7ce93ce0262fff292edeb6fb58d9aed2c...