[interpreter] Make the binary op with Smi bytecode handlers collect type feedback.

[interpreter] Make the binary op with Smi bytecode handlers collect type feedback.

Drive-by fix: the order of parameters in the BinaryOpWithFeedback TurboFan code stubs now reflects the convention of having the context at the end.

BUG=v8:5273
Committed: https://crrev.com/b305c7dfcbc11bf9d6f440cba039d733bade92c3
Cr-Commit-Position: refs/heads/master@{#38832}

[epertoso, 2016-08-22 15:25:12]

Description was changed from

==========
[interpreter] Make the binary op with Smi bytecode handlers collect type
feedback.

Drive-by fix: the order of parameters in the BinaryOpWithFeedback TurboFan
codestubs now reflects the convention of having the context as the last
parameter.

BUG=v8:5273
==========

to

==========
[interpreter] Make the binary op with Smi bytecode handlers collect type
feedback.

Drive-by fix: the order of parameters in the BinaryOpWithFeedback TurboFan
codestubs now reflects the convention of having the context as the last
parameter.

BUG=v8:5273
==========

[epertoso, 2016-08-22 15:25:12]

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

[epertoso, 2016-08-22 15:25:13]

epertoso@chromium.org changed required reviewers:
+ rmcilroy@chromium.org

[epertoso, 2016-08-22 15:26:13]

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

[commit-bot: I haz the power, 2016-08-22 15:26:18]

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

[epertoso, 2016-08-22 15:43:40]

Description was changed from

==========
[interpreter] Make the binary op with Smi bytecode handlers collect type
feedback.

Drive-by fix: the order of parameters in the BinaryOpWithFeedback TurboFan
codestubs now reflects the convention of having the context as the last
parameter.

BUG=v8:5273
==========

to

==========
[interpreter] Make the binary op with Smi bytecode handlers collect type
feedback.

Drive-by fix: the order of parameters in the BinaryOpWithFeedback TurboFan code
stubs now reflects the convention of having the context at the end.

BUG=v8:5273
==========

[commit-bot: I haz the power, 2016-08-22 15:45:39]

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

[commit-bot: I haz the power, 2016-08-22 15:45:40]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[epertoso, 2016-08-23 08:48:44]

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

[commit-bot: I haz the power, 2016-08-23 08:48:52]

Dry run: 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-23 09:12:19]

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

[commit-bot: I haz the power, 2016-08-23 09:12:20]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-08-23 10:28:53]

Looks great, thanks! LGTM, but if you can add some test-interpreter tests to
test feedback progression.

https://codereview.chromium.org/2263253002/diff/20001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2263253002/diff/20001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1206: BinaryOperationHint
BytecodeGraphBuilder::GetBinaryOperationHint(int index) {
nit - operand_index

https://codereview.chromium.org/2263253002/diff/20001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/2263253002/diff/20001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.h:140: BinaryOperationHint
GetBinaryOperationHint(int index);
nit - operand_index

https://codereview.chromium.org/2263253002/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/2263253002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:512:
TEST(InterpreterBinaryOpTypeFeedback) {
Could you add a variant of these tests which take immediate RHS operands to test
the feedback progression of Add/SubSmi bytecodes.

https://codereview.chromium.org/2263253002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:735:
TEST(InterpreterBitwiseTypeFeedback) {
Ditto with the bitwise SMI variants.

[epertoso, 2016-08-23 13:24:28]

https://codereview.chromium.org/2263253002/diff/20001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2263253002/diff/20001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1206: BinaryOperationHint
BytecodeGraphBuilder::GetBinaryOperationHint(int index) {
On 2016/08/23 10:28:53, rmcilroy wrote:
> nit - operand_index

Done.

https://codereview.chromium.org/2263253002/diff/20001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/2263253002/diff/20001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.h:140: BinaryOperationHint
GetBinaryOperationHint(int index);
On 2016/08/23 10:28:53, rmcilroy wrote:
> nit - operand_index

Done.

https://codereview.chromium.org/2263253002/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/2263253002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:512:
TEST(InterpreterBinaryOpTypeFeedback) {
On 2016/08/23 10:28:53, rmcilroy wrote:
> Could you add a variant of these tests which take immediate RHS operands to
test
> the feedback progression of Add/SubSmi bytecodes.

Done.

https://codereview.chromium.org/2263253002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:735:
TEST(InterpreterBitwiseTypeFeedback) {
On 2016/08/23 10:28:53, rmcilroy wrote:
> Ditto with the bitwise SMI variants.

Done.

[epertoso, 2016-08-23 13:24:48]

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

[commit-bot: I haz the power, 2016-08-23 13:24:53]

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

[rmcilroy, 2016-08-23 13:31:17]

https://codereview.chromium.org/2263253002/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2263253002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:222: BytecodeArrayBuilder&
BinaryOperationSmi(Token::Value binop,
Please don't add this as a specific operation, it should only ever be emitted by
the peephole optimizer. You should be able to get the same result by emitting
the following code in the test:

builder()->LoadLiteral(test_case.arg1)
       .StoreAccumulatorInRegister(reg)
       .LoadLiteral(test_case.arg2)
       .BinaryOperation(test_case.op, reg, vector->GetIndex(slot0))

[Jarin, 2016-08-23 13:35:35]

lgtm

[commit-bot: I haz the power, 2016-08-23 13:48:29]

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

[commit-bot: I haz the power, 2016-08-23 13:48:30]

Dry run: This issue passed the CQ dry run.

[epertoso, 2016-08-23 14:22:16]

https://codereview.chromium.org/2263253002/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2263253002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:222: BytecodeArrayBuilder&
BinaryOperationSmi(Token::Value binop,
On 2016/08/23 13:31:17, rmcilroy wrote:
> Please don't add this as a specific operation, it should only ever be emitted
by
> the peephole optimizer. You should be able to get the same result by emitting
> the following code in the test:
> 
> builder()->LoadLiteral(test_case.arg1)
>        .StoreAccumulatorInRegister(reg)
>        .LoadLiteral(test_case.arg2)
>        .BinaryOperation(test_case.op, reg, vector->GetIndex(slot0))

Done.

[epertoso, 2016-08-23 14:24:56]

The CQ bit was checked by epertoso@chromium.org

[epertoso, 2016-08-23 14:24:57]

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

[commit-bot: I haz the power, 2016-08-23 14:25:03]

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-23 14:59:17]

Description was changed from

==========
[interpreter] Make the binary op with Smi bytecode handlers collect type
feedback.

Drive-by fix: the order of parameters in the BinaryOpWithFeedback TurboFan code
stubs now reflects the convention of having the context at the end.

BUG=v8:5273
==========

to

==========
[interpreter] Make the binary op with Smi bytecode handlers collect type
feedback.

Drive-by fix: the order of parameters in the BinaryOpWithFeedback TurboFan code
stubs now reflects the convention of having the context at the end.

BUG=v8:5273
==========

[commit-bot: I haz the power, 2016-08-23 14:59:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-23 14:59:39]

Description was changed from

==========
[interpreter] Make the binary op with Smi bytecode handlers collect type
feedback.

Drive-by fix: the order of parameters in the BinaryOpWithFeedback TurboFan code
stubs now reflects the convention of having the context at the end.

BUG=v8:5273
==========

to

==========
[interpreter] Make the binary op with Smi bytecode handlers collect type
feedback.

Drive-by fix: the order of parameters in the BinaryOpWithFeedback TurboFan code
stubs now reflects the convention of having the context at the end.

BUG=v8:5273

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

[commit-bot: I haz the power, 2016-08-23 14:59:40]

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