Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2537123002:
[ignition] Optimize jump checks to range checks (Closed)
|
Created:
4 years ago by Leszek Swirski Modified:
4 years ago Reviewers:
rmcilroy CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[ignition] Optimize jump checks to range checks
Reorders the jump bytecodes so that the majority of jump checks can be
implemented as range checks (rather than a list of comparisons that get
compiled to a bunch of jumps).
Committed: https://crrev.com/a32a67c7d907daf811d3c2479993100d905bc1e9
Cr-Commit-Position: refs/heads/master@{#41498}
Patch Set 1 #Patch Set 2 : Add tests #
Total comments: 4
Patch Set 3 : Simplify jump bytecode macros #
Messages
Total messages: 21 (13 generated)
The CQ bit was checked by leszeks@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...
leszeks@chromium.org changed reviewers: + rmcilroy@chromium.org
Hi Ross, This is a little hacky, but I do see a non-zero improvement from it on MandreelLatency (2%ish).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm fine with the range checks, but could we add a unittest similar to the one I added in bytecode-operands-unittest.cc in https://codereview.chromium.org/2542903003/. You would probably need to split the Jump bytecodes into a separate macro (similar to the DEBUG bytecodes) - this would be fine to me.
The CQ bit was checked by leszeks@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...
On 2016/12/02 10:50:58, rmcilroy wrote: > I'm fine with the range checks, but could we add a unittest similar to the one I > added in bytecode-operands-unittest.cc in > https://codereview.chromium.org/2542903003/. You would probably need to split > the Jump bytecodes into a separate macro (similar to the DEBUG bytecodes) - this > would be fine to me. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a nit, thanks. https://codereview.chromium.org/2537123002/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecodes.h (right): https://codereview.chromium.org/2537123002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecodes.h:377: V(JumpIfNotHole) nit - could you build these up so there aren't repeated bytecodes, e.g., JUMP_TOBOOLEAN_CONDITIONAL_IMMEDIATE, JUMP_CONDITITIONAL_IMMEDIAIATE JUMP_TOBOOLEAN_CONDITIONAL_CONSTANT, etc. And then build the other macros out of these (e.g. CONDITIONAL adding the TOBOOLEAN variants and JUMP_FORWARD adding everything except JumpLoop. https://codereview.chromium.org/2537123002/diff/20001/test/unittests/interpre... File test/unittests/interpreter/bytecodes-unittest.cc (right): https://codereview.chromium.org/2537123002/diff/20001/test/unittests/interpre... test/unittests/interpreter/bytecodes-unittest.cc:208: ([](Bytecode bytecode) { return false LIST(OR_IS_BYTECODE); }(BYTECODE)) I had this hack earlier in bytecode-operands and didn't like it, but fine in tests :).
https://codereview.chromium.org/2537123002/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecodes.h (right): https://codereview.chromium.org/2537123002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecodes.h:377: V(JumpIfNotHole) On 2016/12/05 14:33:40, rmcilroy wrote: > nit - could you build these up so there aren't repeated bytecodes, e.g., > JUMP_TOBOOLEAN_CONDITIONAL_IMMEDIATE, JUMP_CONDITITIONAL_IMMEDIAIATE > JUMP_TOBOOLEAN_CONDITIONAL_CONSTANT, etc. And then build the other macros out of > these (e.g. CONDITIONAL adding the TOBOOLEAN variants and JUMP_FORWARD adding > everything except JumpLoop. Done, except for some duplication in JumpForward caused by JumpLoop being unconditional. https://codereview.chromium.org/2537123002/diff/20001/test/unittests/interpre... File test/unittests/interpreter/bytecodes-unittest.cc (right): https://codereview.chromium.org/2537123002/diff/20001/test/unittests/interpre... test/unittests/interpreter/bytecodes-unittest.cc:208: ([](Bytecode bytecode) { return false LIST(OR_IS_BYTECODE); }(BYTECODE)) On 2016/12/05 14:33:40, rmcilroy wrote: > I had this hack earlier in bytecode-operands and didn't like it, but fine in > tests :). Yeah, I hated it too but I couldn't think of a better way to exhaustively check all bytecodes.
The CQ bit was checked by leszeks@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/2537123002/#ps40001 (title: "Simplify jump bytecode macros")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480960401572780,
"parent_rev": "9c4f89b7e26215a2c01311da3614819748f6a611", "commit_rev":
"b3354e895acecb72b7b9437a0e48c117f91ecd12"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [ignition] Optimize jump checks to range checks Reorders the jump bytecodes so that the majority of jump checks can be implemented as range checks (rather than a list of comparisons that get compiled to a bunch of jumps). ========== to ========== [ignition] Optimize jump checks to range checks Reorders the jump bytecodes so that the majority of jump checks can be implemented as range checks (rather than a list of comparisons that get compiled to a bunch of jumps). Committed: https://crrev.com/a32a67c7d907daf811d3c2479993100d905bc1e9 Cr-Commit-Position: refs/heads/master@{#41498} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a32a67c7d907daf811d3c2479993100d905bc1e9 Cr-Commit-Position: refs/heads/master@{#41498} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
