[Interpreter] Assign feedback slots for binary operations and use them in ignition.

[Interpreter] Assign feedback slots for binary operations and use them in ignition.

Assign feedback slots in the type feedback vector for binary operations.
Update bytecode-generator to use these slots and add them as an operand
to binary operations.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/9e3e2ee2dd033965606213ad6f721425cb1f02a6
Cr-Commit-Position: refs/heads/master@{#38408}

[mythria, 2016-08-03 10:44:30]

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

[commit-bot: I haz the power, 2016-08-03 10:44:39]

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

[mythria, 2016-08-03 11:00:55]

mythria@chromium.org changed reviewers:
+ mvstanton@chromium.org, rmcilroy@chromium.org

[mythria, 2016-08-03 11:00:55]

This cl adds a slot for binary operations in type feedback vector. These will be
used by ignition to collect type feedback. I assign these slots even when
ignition is not turned on. If I assign them only on ignition, we have some
problems with snapshotted functions, because the type feedback vector changes
with and without ignition. So we thought it is better to have the slots always. 

PTAL.

Thanks,
Mythri

[commit-bot: I haz the power, 2016-08-03 11:05:53]

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

[commit-bot: I haz the power, 2016-08-03 11:05:54]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-08-03 16:04:30]

Interpreter changes LGTM, thanks.

https://codereview.chromium.org/2209633002/diff/1/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/2209633002/diff/1/src/ast/ast.cc#newcode292
src/ast/ast.cc:292: // Assign a slot to collect feedback about binary
operations. used only in
/s/used/Used

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

https://codereview.chromium.org/2209633002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:2214:
feedback_index(expr->binary_operation()->BinaryOperationFeedbackSlot());
nit - keep variable as "FeedbackVectorSlot slot" and just call
feedback_index(slot) when passing to BinaryOperation function

https://codereview.chromium.org/2209633002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:2858: int feedback_slot_index =
feedback_index(expr->CountBinaryOpFeedbackSlot());
ditto

https://codereview.chromium.org/2209633002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:2930: int feedback_slot_index =
feedback_index(expr->BinaryOperationFeedbackSlot());
ditto

https://codereview.chromium.org/2209633002/diff/1/test/unittests/interpreter/...
File test/unittests/interpreter/bytecode-array-builder-unittest.cc (right):

https://codereview.chromium.org/2209633002/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:743:
CHECK_EQ(iterator.GetImmediateOperand(0), -i * 2 - 5 - 1);
Why is the last one special? Could you update the comment above (or if possible
remove the special casing of the last loop).

[rmcilroy, 2016-08-04 19:11:55]

rmcilroy@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[rmcilroy, 2016-08-04 19:11:55]

Michi: Michael Stanton is OOO, could you review the type feedback vector
changes?

[mythria, 2016-08-05 07:05:37]

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

[commit-bot: I haz the power, 2016-08-05 07:05:47]

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-05 07:07:16]

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

[commit-bot: I haz the power, 2016-08-05 07:07:17]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/10382)
  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_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10368)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/10326)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/21068)

[mythria, 2016-08-05 07:08:15]

Thanks Ross, I fixed your comments. Also thanks for adding mstarzinger for
review.

https://codereview.chromium.org/2209633002/diff/1/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/2209633002/diff/1/src/ast/ast.cc#newcode292
src/ast/ast.cc:292: // Assign a slot to collect feedback about binary
operations. used only in
On 2016/08/03 16:04:29, rmcilroy wrote:
> /s/used/Used

Done.

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

https://codereview.chromium.org/2209633002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:2214:
feedback_index(expr->binary_operation()->BinaryOperationFeedbackSlot());
On 2016/08/03 16:04:30, rmcilroy wrote:
> nit - keep variable as "FeedbackVectorSlot slot" and just call
> feedback_index(slot) when passing to BinaryOperation function

Done.

https://codereview.chromium.org/2209633002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:2858: int feedback_slot_index =
feedback_index(expr->CountBinaryOpFeedbackSlot());
On 2016/08/03 16:04:29, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/2209633002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:2930: int feedback_slot_index =
feedback_index(expr->BinaryOperationFeedbackSlot());
On 2016/08/03 16:04:30, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/2209633002/diff/1/test/unittests/interpreter/...
File test/unittests/interpreter/bytecode-array-builder-unittest.cc (right):

https://codereview.chromium.org/2209633002/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:743:
CHECK_EQ(iterator.GetImmediateOperand(0), -i * 2 - 5 - 1);
On 2016/08/03 16:04:30, rmcilroy wrote:
> Why is the last one special? Could you update the comment above (or if
possible
> remove the special casing of the last loop).

Done. I removed the special case.

[mythria, 2016-08-05 07:16:15]

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

[commit-bot: I haz the power, 2016-08-05 07:16:25]

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-05 07:35:39]

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

[commit-bot: I haz the power, 2016-08-05 07:35:40]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-08-05 09:44:27]

LGTM.

https://codereview.chromium.org/2209633002/diff/40001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2209633002/diff/40001/src/ast/ast.h#newcode2444
src/ast/ast.h:2444: FeedbackVectorSlot binary_operation_slot_;
nit: This field seems to be unused, the BytecodeGenerator is already using
binary_operation()->BinaryOperationFeedbackSlot() instead. Can we drop this
field?

https://codereview.chromium.org/2209633002/diff/40001/test/cctest/test-feedba...
File test/cctest/test-feedback-vector.cc (right):

https://codereview.chromium.org/2209633002/diff/40001/test/cctest/test-feedba...
test/cctest/test-feedback-vector.cc:547: // we also assign slot for binary
operations.
nit: Capitalize comment. Or drop this one completely, it's covered by the below
comment.

[mythria, 2016-08-05 09:55:18]

Thanks Michi, I fixed both of them.

Thanks,
Mythri

https://codereview.chromium.org/2209633002/diff/40001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2209633002/diff/40001/src/ast/ast.h#newcode2444
src/ast/ast.h:2444: FeedbackVectorSlot binary_operation_slot_;
On 2016/08/05 09:44:27, Michael Starzinger wrote:
> nit: This field seems to be unused, the BytecodeGenerator is already using
> binary_operation()->BinaryOperationFeedbackSlot() instead. Can we drop this
> field?

Thanks, I missed removing this one. Initially I had a slot for Assignment and
then merged with BinaryOperation. Done.

https://codereview.chromium.org/2209633002/diff/40001/test/cctest/test-feedba...
File test/cctest/test-feedback-vector.cc (right):

https://codereview.chromium.org/2209633002/diff/40001/test/cctest/test-feedba...
test/cctest/test-feedback-vector.cc:547: // we also assign slot for binary
operations.
On 2016/08/05 09:44:27, Michael Starzinger wrote:
> nit: Capitalize comment. Or drop this one completely, it's covered by the
below
> comment.

Done.

[mythria, 2016-08-05 09:55:34]

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

[commit-bot: I haz the power, 2016-08-05 09:55:39]

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-05 09:58:29]

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

[commit-bot: I haz the power, 2016-08-05 09:58:30]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/10401)
  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_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10387)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/10346)

[mythria, 2016-08-08 00:31:01]

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

[commit-bot: I haz the power, 2016-08-08 00:31:03]

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-08 00:32:34]

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

[commit-bot: I haz the power, 2016-08-08 00:32:36]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/10484)
  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_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10470)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/10427)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/6480)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/21189)

[mythria, 2016-08-08 00:42:55]

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

[commit-bot: I haz the power, 2016-08-08 00:43:09]

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-08 01:05:53]

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

[commit-bot: I haz the power, 2016-08-08 01:05:54]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-08-08 01:12:55]

The CQ bit was checked by mythria@chromium.org

[mythria, 2016-08-08 01:12:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2209633002/#ps100001
(title: "Rebased the patch.")

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

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-08 01:15:32]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-08 01:17:05]

Description was changed from

==========
[Interpreter] Assign feedback slots for binary operations and use them in
ignition.

Assign feedback slots in the type feedback vector for binary operations.
Update bytecode-generator to use these slots and add them as an operand
to binary operations.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Assign feedback slots for binary operations and use them in
ignition.

Assign feedback slots in the type feedback vector for binary operations.
Update bytecode-generator to use these slots and add them as an operand
to binary operations.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/9e3e2ee2dd033965606213ad6f721425cb1f02a6
Cr-Commit-Position: refs/heads/master@{#38408}
==========

[commit-bot: I haz the power, 2016-08-08 01:17:06]

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