Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2407103003:
[Interpreter] Collect feedback about Oddballs in Bitwise, Inc, Dec operations. (Closed)
Description[Interpreter] Collect feedback about Oddballs in Bitwise, Inc, Dec operations.
Add support to collect feedback about oddballs for Bitwise binary operations and
Increment and decrement operations. For the case of Oddballs the code to convert
them to numbers is inlined into the handlers instead of calling the
NonNumberToNumber Stub.
BUG=v8:4280, v8:5400
Committed: https://crrev.com/8a80c3b70fbfd5a7bec3ad3946bfd726d2262637
Cr-Commit-Position: refs/heads/master@{#40468}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added a comment. #
Total comments: 3
Patch Set 3 : Added an assert. #
Total comments: 2
Patch Set 4 : Moved the assert to higher level. #Messages
Total messages: 35 (22 generated)
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Interpreter] Collect feedback about Oddballs in Bitwise, Increment and Decrement. Add support to collect feedback about oddballs for Bitwise binary operations and Increment and decrement operations. For the case of Oddballs the code to convert them to numbers is inlined into the handlers instead of calling the NonNumberToNumber Stub. BUG=v8:4280, v8:5400 ========== to ========== [Interpreter] Collect feedback about Oddballs in Bitwise, Inc, Dec operations. Add support to collect feedback about oddballs for Bitwise binary operations and Increment and decrement operations. For the case of Oddballs the code to convert them to numbers is inlined into the handlers instead of calling the NonNumberToNumber Stub. BUG=v8:4280, v8:5400 ==========
mythria@chromium.org changed reviewers: + bmeurer@chromium.org, leszeks@chromium.org
This is the follow up cl to collect feebdack about Oddballs in bitwise, increment and decrements operations. Here I inline the code to convert oddball to a number, to avoid some checks. PTAL. Thanks, Mythri
This is the last patch for collecting information about Oddballs in ignition. This patch adds support for Inc, Dec and other bitwise operators. Bitwise operators use TruncateTaggedToWord32WithFeedback to collect feedback about lhs and rhs. So changes are in Inc, Dec and TruncateTaggedToWord32WithFeedback stubs. PTAL. Thanks, Mythri
lgtm
https://codereview.chromium.org/2407103003/diff/1/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2407103003/diff/1/src/code-stubs.cc#newcode1505 src/code-stubs.cc:1505: var_type_feedback.Bind(assembler->Int32Constant( Because of the loop, this should possibly be an "or" with the current value (similar to the smi check above), in case e.g. the value comes from calling ToNumber on an Any type
Leszek, I added a comment to explain why we don't need an Or. PTAL. Thanks, Mythri https://codereview.chromium.org/2407103003/diff/1/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2407103003/diff/1/src/code-stubs.cc#newcode1505 src/code-stubs.cc:1505: var_type_feedback.Bind(assembler->Int32Constant( On 2016/10/19 08:42:48, Leszek Swirski wrote: > Because of the loop, this should possibly be an "or" with the current value > (similar to the smi check above), in case e.g. the value comes from calling > ToNumber on an Any type As I explained offline, we don't need it here because ToNumber always converts it to a Number and we don't reach this path twice. I added a comment on why we don't need this.
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2407103003/diff/20001/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2407103003/diff/20001/src/code-stubs.cc#newco... src/code-stubs.cc:1639: // only reach this path on the first pass. Can we add an assert that var_type_feedback is not bound?
https://codereview.chromium.org/2407103003/diff/20001/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2407103003/diff/20001/src/code-stubs.cc#newco... src/code-stubs.cc:1639: // only reach this path on the first pass. On 2016/10/19 11:34:01, Leszek Swirski wrote: > Can we add an assert that var_type_feedback is not bound? Um, by not bound I mean "value is 0" or something along those lines.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Leszek. Done. PTAL. Thanks, Mythri https://codereview.chromium.org/2407103003/diff/20001/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2407103003/diff/20001/src/code-stubs.cc#newco... src/code-stubs.cc:1639: // only reach this path on the first pass. On 2016/10/19 11:35:30, Leszek Swirski wrote: > On 2016/10/19 11:34:01, Leszek Swirski wrote: > > Can we add an assert that var_type_feedback is not bound? > > Um, by not bound I mean "value is 0" or something along those lines. Great suggestion. I wanted to some kind of check but was only thinking about DCHECK. Done.
LGTM after one last nit. https://codereview.chromium.org/2407103003/diff/40001/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2407103003/diff/40001/src/code-stubs.cc#newco... src/code-stubs.cc:1505: // We do not require an Or with earlier feedback here because once we One last thing: can we move this assert outside of this if, to if_valuenotnumber, so that it covers both oddballs and non-oddballs?
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Leszek. Moved it to cover both paths. Thanks, Mythri. https://codereview.chromium.org/2407103003/diff/40001/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2407103003/diff/40001/src/code-stubs.cc#newco... src/code-stubs.cc:1505: // We do not require an Or with earlier feedback here because once we On 2016/10/20 09:40:15, Leszek Swirski wrote: > One last thing: can we move this assert outside of this if, to > if_valuenotnumber, so that it covers both oddballs and non-oddballs? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mythria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org, leszeks@chromium.org Link to the patchset: https://codereview.chromium.org/2407103003/#ps60001 (title: "Moved the assert to higher level.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Collect feedback about Oddballs in Bitwise, Inc, Dec operations. Add support to collect feedback about oddballs for Bitwise binary operations and Increment and decrement operations. For the case of Oddballs the code to convert them to numbers is inlined into the handlers instead of calling the NonNumberToNumber Stub. BUG=v8:4280, v8:5400 ========== to ========== [Interpreter] Collect feedback about Oddballs in Bitwise, Inc, Dec operations. Add support to collect feedback about oddballs for Bitwise binary operations and Increment and decrement operations. For the case of Oddballs the code to convert them to numbers is inlined into the handlers instead of calling the NonNumberToNumber Stub. BUG=v8:4280, v8:5400 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Collect feedback about Oddballs in Bitwise, Inc, Dec operations. Add support to collect feedback about oddballs for Bitwise binary operations and Increment and decrement operations. For the case of Oddballs the code to convert them to numbers is inlined into the handlers instead of calling the NonNumberToNumber Stub. BUG=v8:4280, v8:5400 ========== to ========== [Interpreter] Collect feedback about Oddballs in Bitwise, Inc, Dec operations. Add support to collect feedback about oddballs for Bitwise binary operations and Increment and decrement operations. For the case of Oddballs the code to convert them to numbers is inlined into the handlers instead of calling the NonNumberToNumber Stub. BUG=v8:4280, v8:5400 Committed: https://crrev.com/8a80c3b70fbfd5a7bec3ad3946bfd726d2262637 Cr-Commit-Position: refs/heads/master@{#40468} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8a80c3b70fbfd5a7bec3ad3946bfd726d2262637 Cr-Commit-Position: refs/heads/master@{#40468} |
