[interpreter] Collect type feedback from bitwise binary ops handlers.

[interpreter] Collect type feedback from bitwise binary ops handlers.

Also, re-enables the use of the type feedback in BytecodeGraphBuilder.

BUG=v8:5273
LOG=N

Committed: https://crrev.com/b167ae33614721c6bcf7bd6bf33e1abe24638fd4
Cr-Commit-Position: refs/heads/master@{#38618}

[epertoso, 2016-08-11 09:04:08]

Description was changed from

==========
[interpreter] Collect type feedback from bitwise binary ops.

Also, re-enables the use of the type feedback in BytecodeGraphBuilder.

BUG=v8:5273
LOG=N
==========

to

==========
[interpreter] Collect type feedback from bitwise binary ops handlers.

Also, re-enables the use of the type feedback in BytecodeGraphBuilder.

BUG=v8:5273
LOG=N
==========

[epertoso, 2016-08-11 09:04:08]

epertoso@chromium.org changed reviewers:
+ bmeurer@chromium.org, rmcilroy@chromium.org

[Benedikt Meurer, 2016-08-11 09:32:22]

LGTM with comments.

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

https://codereview.chromium.org/2235133003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:808: __
Int32Constant(BinaryOperationFeedback::kNumber),
We should probably recognize kSigned32 here as well. This could be especially
relevant on 32-bit architectures.
At least we should have a TODO here that we need to revisit this.

https://codereview.chromium.org/2235133003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:830: __
Int32Constant(BinaryOperationFeedback::kNumber),
See comment above.

https://codereview.chromium.org/2235133003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:843: __
Int32Constant(BinaryOperationFeedback::kNumber));
s/kNumber/kSigned32

[rmcilroy, 2016-08-11 09:50:49]

Overarching question - this introduces a number of number type comparison
operations for the type feedback, and then it seems like we repeat those
comparisons in TruncateTaggedToWord32. Could we have a variant of
TruncateTaggedToWord32 which updated type feedback as it went?

[epertoso, 2016-08-11 12:18:49]

On 2016/08/11 09:50:49, rmcilroy wrote:
> Overarching question - this introduces a number of number type comparison
> operations for the type feedback, and then it seems like we repeat those
> comparisons in TruncateTaggedToWord32. Could we have a variant of
> TruncateTaggedToWord32 which updated type feedback as it went?

Added that version in InterpreterAssembler.

PTAL.

[epertoso, 2016-08-11 12:18:57]

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

https://codereview.chromium.org/2235133003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:808: __
Int32Constant(BinaryOperationFeedback::kNumber),
On 2016/08/11 09:32:21, Benedikt Meurer wrote:
> We should probably recognize kSigned32 here as well. This could be especially
> relevant on 32-bit architectures.
> At least we should have a TODO here that we need to revisit this.

We don't have kSigned32 in BinaryOperationFeedback.

https://codereview.chromium.org/2235133003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:830: __
Int32Constant(BinaryOperationFeedback::kNumber),
On 2016/08/11 09:32:21, Benedikt Meurer wrote:
> See comment above.

See answer above.

https://codereview.chromium.org/2235133003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:843: __
Int32Constant(BinaryOperationFeedback::kNumber));
On 2016/08/11 09:32:21, Benedikt Meurer wrote:
> s/kNumber/kSigned32

There's no kSigned32 constant.

[Benedikt Meurer, 2016-08-11 12:42:19]

Still LGTM.

[rmcilroy, 2016-08-11 13:31:07]

LGTM with a suggestion.

https://codereview.chromium.org/2235133003/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2235133003/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:819: typedef InterpreterAssembler::Variable
Variable;
Typedefs are unecessary here

https://codereview.chromium.org/2235133003/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:856: __ Dispatch();
Could we share all the code in a single helper which is passed
Token::Value::BIT_OR/BIT_AND/SHL/ etc. and switches on the token to ouput the
correct operation?

[epertoso, 2016-08-11 14:03:11]

https://codereview.chromium.org/2235133003/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2235133003/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:819: typedef InterpreterAssembler::Variable
Variable;
On 2016/08/11 13:31:07, rmcilroy wrote:
> Typedefs are unecessary here

Done.

https://codereview.chromium.org/2235133003/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:856: __ Dispatch();
On 2016/08/11 13:31:07, rmcilroy wrote:
> Could we share all the code in a single helper which is passed
> Token::Value::BIT_OR/BIT_AND/SHL/ etc. and switches on the token to ouput the
> correct operation?

Done.

[rmcilroy, 2016-08-11 14:09:24]

Couple of last nits, but LGTM. Thanks for the pulling it out to a helper!

https://codereview.chromium.org/2235133003/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2235133003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:823: InterpreterAssembler::Variable
var_lhs_type_feedback(
nit - this can just be Variable (without InterpreterAssembler::).

https://codereview.chromium.org/2235133003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:830: Node* value = nullptr;
Unused outside the switch - just declare in the switch case.

https://codereview.chromium.org/2235133003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:871: InterpreterAssembler::Label ok(assembler);
nit - just Label

[epertoso, 2016-08-11 14:18:06]

https://codereview.chromium.org/2235133003/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2235133003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:823: InterpreterAssembler::Variable
var_lhs_type_feedback(
On 2016/08/11 14:09:23, rmcilroy wrote:
> nit - this can just be Variable (without InterpreterAssembler::).

Done.

https://codereview.chromium.org/2235133003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:830: Node* value = nullptr;
On 2016/08/11 14:09:23, rmcilroy wrote:
> Unused outside the switch - just declare in the switch case.

Done.

https://codereview.chromium.org/2235133003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:871: InterpreterAssembler::Label ok(assembler);
On 2016/08/11 14:09:23, rmcilroy wrote:
> nit - just Label

Done.

[epertoso, 2016-08-11 14:18:11]

The CQ bit was checked by epertoso@chromium.org

[epertoso, 2016-08-11 14:18:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2235133003/#ps60001
(title: "Update.")

[commit-bot: I haz the power, 2016-08-11 14:18:22]

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, 2016-08-11 14:44:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 14:44:10]

Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10818)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[epertoso, 2016-08-11 14:58:09]

The CQ bit was checked by epertoso@chromium.org

[epertoso, 2016-08-11 14:58:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2235133003/#ps80001
(title: "Fixes the assert caused by Bind() after AbortIf().")

[commit-bot: I haz the power, 2016-08-11 14:58:20]

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, 2016-08-11 15:35:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 15:35:52]

Try jobs failed on following builders:
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng_trigg...)

[epertoso, 2016-08-12 12:37:44]

The CQ bit was checked by epertoso@chromium.org

[epertoso, 2016-08-12 12:37:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2235133003/#ps100001
(title: "Fixed mapping kSignedSmall -> kSigned32 to kSignedSmall ->
kSignedSmall.")

[commit-bot: I haz the power, 2016-08-12 12:37:55]

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, 2016-08-12 13:04:19]

Description was changed from

==========
[interpreter] Collect type feedback from bitwise binary ops handlers.

Also, re-enables the use of the type feedback in BytecodeGraphBuilder.

BUG=v8:5273
LOG=N
==========

to

==========
[interpreter] Collect type feedback from bitwise binary ops handlers.

Also, re-enables the use of the type feedback in BytecodeGraphBuilder.

BUG=v8:5273
LOG=N
==========

[commit-bot: I haz the power, 2016-08-12 13:04:20]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-12 13:04:37]

Description was changed from

==========
[interpreter] Collect type feedback from bitwise binary ops handlers.

Also, re-enables the use of the type feedback in BytecodeGraphBuilder.

BUG=v8:5273
LOG=N
==========

to

==========
[interpreter] Collect type feedback from bitwise binary ops handlers.

Also, re-enables the use of the type feedback in BytecodeGraphBuilder.

BUG=v8:5273
LOG=N

Committed: https://crrev.com/b167ae33614721c6bcf7bd6bf33e1abe24638fd4
Cr-Commit-Position: refs/heads/master@{#38618}
==========

[commit-bot: I haz the power, 2016-08-12 13:04:38]

Patchset 6 (id:??) landed as
https://crrev.com/b167ae33614721c6bcf7bd6bf33e1abe24638fd4
Cr-Commit-Position: refs/heads/master@{#38618}