Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2406263002:
[Interpreter] Collect feedback about Oddballs in Add, Mul, Div, Modulus stubs. (Closed)
Description[Interpreter] Collect feedback about Oddballs in Add, Mul, Div, Modulus stubs.
Add support to collect feedback about oddballs in Add, Mul, Div and Modulus stubs.
Turbofan uses NumberOrOddball feedback to reduce the number of deoptimizations.
BUG=v8:4280, v8:5400
LOG=N
Committed: https://crrev.com/d875e2cf800c2110d5588595835e4e524fc3616e
Cr-Commit-Position: refs/heads/master@{#40407}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Addressed comments from Leszek #
Total comments: 8
Patch Set 3 : rebased the patch. #Patch Set 4 : Addressed comments from Leszek #Messages
Total messages: 25 (16 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.
mythria@chromium.org changed reviewers: + bmeurer@chromium.org, leszeks@chromium.org
This is to collect feedback about oddballs similar to the Subtract Stub. I slightly modified the implementation because that seems to save few (~30) assembly instructions. PTAL. Thanks, Mythri
https://codereview.chromium.org/2406263002/diff/1/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2406263002/diff/1/src/code-stubs.cc#newcode743 src/code-stubs.cc:743: Label if_lhsisnumber(assembler); Can we remove this label too? https://codereview.chromium.org/2406263002/diff/1/src/code-stubs.cc#newcode810 src/code-stubs.cc:810: assembler->Bind(&if_lhsisnotnumber); why not merge this and check_string? Then you have the lhs instance type available already. You could make 'check_string' be 'lhsisnotnumber' and have a branch inside of it for 'lhs_is_oddball/lhs_is_not_oddball' https://codereview.chromium.org/2406263002/diff/1/src/code-stubs.cc#newcode1163 src/code-stubs.cc:1163: Label call_with_any_feedback(assembler), nit: move these labels to be up with the rest (or move call_multiply_stub down) https://codereview.chromium.org/2406263002/diff/1/src/code-stubs.cc#newcode1391 src/code-stubs.cc:1391: Label call_with_any_feedback(assembler), nit: as above https://codereview.chromium.org/2406263002/diff/1/src/code-stubs.cc#newcode1396 src/code-stubs.cc:1396: // Check if lhs is an oddball. nit: s/lhs/dividend https://codereview.chromium.org/2406263002/diff/1/src/code-stubs.cc#newcode1420 src/code-stubs.cc:1420: // Check if rhs is an oddball. At this point we know lhs is either a nit: s/rhs/divisor, s/lhs/dividend/ https://codereview.chromium.org/2406263002/diff/1/src/code-stubs.cc#newcode1560 src/code-stubs.cc:1560: Label call_with_any_feedback(assembler), nit: as above
Thanks Leszek. Addressed your comments. https://codereview.chromium.org/2406263002/diff/1/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2406263002/diff/1/src/code-stubs.cc#newcode743 src/code-stubs.cc:743: Label if_lhsisnumber(assembler); On 2016/10/11 14:21:32, Leszek Swirski wrote: > Can we remove this label too? Done. https://codereview.chromium.org/2406263002/diff/1/src/code-stubs.cc#newcode810 src/code-stubs.cc:810: assembler->Bind(&if_lhsisnotnumber); On 2016/10/11 14:21:32, Leszek Swirski wrote: > why not merge this and check_string? Then you have the lhs instance type > available already. You could make 'check_string' be 'lhsisnotnumber' and have a > branch inside of it for 'lhs_is_oddball/lhs_is_not_oddball' Nice suggestion. Done. https://codereview.chromium.org/2406263002/diff/1/src/code-stubs.cc#newcode1163 src/code-stubs.cc:1163: Label call_with_any_feedback(assembler), On 2016/10/11 14:21:33, Leszek Swirski wrote: > nit: move these labels to be up with the rest (or move call_multiply_stub down) Done. https://codereview.chromium.org/2406263002/diff/1/src/code-stubs.cc#newcode1391 src/code-stubs.cc:1391: Label call_with_any_feedback(assembler), On 2016/10/11 14:21:33, Leszek Swirski wrote: > nit: as above Done. https://codereview.chromium.org/2406263002/diff/1/src/code-stubs.cc#newcode1396 src/code-stubs.cc:1396: // Check if lhs is an oddball. On 2016/10/11 14:21:32, Leszek Swirski wrote: > nit: s/lhs/dividend Done. https://codereview.chromium.org/2406263002/diff/1/src/code-stubs.cc#newcode1420 src/code-stubs.cc:1420: // Check if rhs is an oddball. At this point we know lhs is either a On 2016/10/11 14:21:33, Leszek Swirski wrote: > nit: s/rhs/divisor, s/lhs/dividend/ Done. https://codereview.chromium.org/2406263002/diff/1/src/code-stubs.cc#newcode1560 src/code-stubs.cc:1560: Label call_with_any_feedback(assembler), On 2016/10/11 14:21:32, Leszek Swirski wrote: > nit: as above Done.
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: Try jobs failed on following builders: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/10836)
LGTM after nits. There's a lot of repetition in these methods, I wonder if we could somehow merge the implementations of subtract, multiply, divide and modulus. Not in this CL though. https://codereview.chromium.org/2406263002/diff/20001/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2406263002/diff/20001/src/code-stubs.cc#newco... src/code-stubs.cc:677: Label call_with_any_feedback(assembler), nit: merge these two declaration lists, and put the labels in the same order as we bind them (here and subsequent functions) https://codereview.chromium.org/2406263002/diff/20001/src/code-stubs.cc#newco... src/code-stubs.cc:789: lhs_instance_type, assembler->Int32Constant(ODDBALL_TYPE)); can we add an assembler->IsOddballInstanceType, to match IsStringInstanceType? https://codereview.chromium.org/2406263002/diff/20001/src/code-stubs.cc#newco... src/code-stubs.cc:817: assembler->Bind(&if_lhsisoddball); nit: move this block above the the lhsisnotoddball check, so that the order is (branch instruct, true branch, false branch) https://codereview.chromium.org/2406263002/diff/20001/src/code-stubs.cc#newco... src/code-stubs.cc:1171: assembler->Branch(assembler->WordIsSmi(rhs), &call_with_oddball_feedback, nit: for consistency with how we do this elsewhere, this should probably be GotoUnless (here and subsequent functions)
Thanks Leszek. Addressed your comments. I will add Oddball check as an assembly macro in another cl. https://codereview.chromium.org/2406263002/diff/20001/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2406263002/diff/20001/src/code-stubs.cc#newco... src/code-stubs.cc:677: Label call_with_any_feedback(assembler), On 2016/10/14 10:09:50, Leszek Swirski wrote: > nit: merge these two declaration lists, and put the labels in the same order as > we bind them (here and subsequent functions) Done. https://codereview.chromium.org/2406263002/diff/20001/src/code-stubs.cc#newco... src/code-stubs.cc:789: lhs_instance_type, assembler->Int32Constant(ODDBALL_TYPE)); On 2016/10/14 10:09:50, Leszek Swirski wrote: > can we add an assembler->IsOddballInstanceType, to match IsStringInstanceType? I dint do this in this CL. As you said there is a lot of repetition across these binary op stubs. I thought we can handle this along with that or as a different cl. SInce this also affects other places where there is an oddball check. So in short, I will do it as another cl. https://codereview.chromium.org/2406263002/diff/20001/src/code-stubs.cc#newco... src/code-stubs.cc:817: assembler->Bind(&if_lhsisoddball); On 2016/10/14 10:09:50, Leszek Swirski wrote: > nit: move this block above the the lhsisnotoddball check, so that the order is > (branch instruct, true branch, false branch) Done. https://codereview.chromium.org/2406263002/diff/20001/src/code-stubs.cc#newco... src/code-stubs.cc:1171: assembler->Branch(assembler->WordIsSmi(rhs), &call_with_oddball_feedback, On 2016/10/14 10:09:50, Leszek Swirski wrote: > nit: for consistency with how we do this elsewhere, this should probably be > GotoUnless (here and subsequent functions) Done.
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...
LGTM.
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 leszeks@chromium.org Link to the patchset: https://codereview.chromium.org/2406263002/#ps60001 (title: "Addressed comments from Leszek")
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Collect feedback about Oddballs in Add, Mul, Div, Modulus stubs. Add support to collect feedback about oddballs in Add, Mul, Div and Modulus stubs. Turbofan uses NumberOrOddball feedback to reduce the number of deoptimizations. BUG=v8:4280, v8:5400 LOG=N ========== to ========== [Interpreter] Collect feedback about Oddballs in Add, Mul, Div, Modulus stubs. Add support to collect feedback about oddballs in Add, Mul, Div and Modulus stubs. Turbofan uses NumberOrOddball feedback to reduce the number of deoptimizations. BUG=v8:4280, v8:5400 LOG=N Committed: https://crrev.com/d875e2cf800c2110d5588595835e4e524fc3616e Cr-Commit-Position: refs/heads/master@{#40407} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d875e2cf800c2110d5588595835e4e524fc3616e Cr-Commit-Position: refs/heads/master@{#40407} |
