saulecabrera requested cfallin for a review on PR #6951.
saulecabrera opened PR #6951 from saulecabrera:winch-br_table
to bytecodealliance:main
:
This change adds support for the
br_table
instruction, including several modifications to the existing control flow implementation:
Improved handling of jumps to loops: Previously, the compiler erroneously treated the result of loop blocks as the definitive result of the jump. This change fixes this bug.
Streamlined result handling and stack pointer balancing: In the past, these operations were executed in two distinct steps, complicating the process of ensuring the correct invariants when emitting unconditional jumps. To simplify this,
CodeGenContext::unconditional_jump
is introduced . This function guarantees all necessary invariants are met, encapsulating the entire operation within a single function for easier understanding and maintenance.Handling of unreachable state at the end of a function: when reaching the end of a function in an unreachable state, clear the stack and ensure that the machine stack pointer is correctly placed according to the expectations of the outermost block.
In addition to the above refactoring, the main implementation of the
br_table
instruction involves emitting labels for each target. Within each label, an unconditional jump is emitted to the frame's label, ensuring correct stack pointer balancing when the jump is emitted.While it is possible to optimize this process by avoiding intermediate labels when balancing isn't required, we've opted to maintain the current implementation until such optimization becomes necessary.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
saulecabrera requested wasmtime-compiler-reviewers for a review on PR #6951.
saulecabrera requested alexcrichton for a review on PR #6951.
saulecabrera requested wasmtime-core-reviewers for a review on PR #6951.
saulecabrera edited PR #6951:
This change adds support for the
br_table
instruction, including several modifications to the existing control flow implementation:
Improved handling of jumps to loops: Previously, the compiler erroneously treated the result of loop blocks as the definitive result of the jump. This change fixes this bug.
Streamlined result handling and stack pointer balancing: In the past, these operations were executed in two distinct steps, complicating the process of ensuring the correct invariants when emitting unconditional jumps. To simplify this,
CodeGenContext::unconditional_jump
is introduced . This function guarantees all necessary invariants are met, encapsulating the entire operation within a single function for easier understanding and maintenance.Handling of unreachable state at the end of a function: when reaching the end of a function in an unreachable state, clear the stack and ensure that the machine stack pointer is correctly placed according to the expectations of the outermost block.
In addition to the above refactoring, the main implementation of the
br_table
instruction involves emitting labels for each target. Within each label, an unconditional jump is emitted to the frame's label, ensuring correct stack pointer balancing when the jump is emitted.While it is possible to optimize this process by avoiding intermediate labels when balancing isn't required, I've opted to maintain the current implementation until such optimization becomes necessary.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
saulecabrera updated PR #6951.
saulecabrera edited PR #6951:
Part of https://github.com/bytecodealliance/wasmtime/issues/6528
This change adds support for the
br_table
instruction, including several modifications to the existing control flow implementation:
Improved handling of jumps to loops: Previously, the compiler erroneously treated the result of loop blocks as the definitive result of the jump. This change fixes this bug.
Streamlined result handling and stack pointer balancing: In the past, these operations were executed in two distinct steps, complicating the process of ensuring the correct invariants when emitting unconditional jumps. To simplify this,
CodeGenContext::unconditional_jump
is introduced . This function guarantees all necessary invariants are met, encapsulating the entire operation within a single function for easier understanding and maintenance.Handling of unreachable state at the end of a function: when reaching the end of a function in an unreachable state, clear the stack and ensure that the machine stack pointer is correctly placed according to the expectations of the outermost block.
In addition to the above refactoring, the main implementation of the
br_table
instruction involves emitting labels for each target. Within each label, an unconditional jump is emitted to the frame's label, ensuring correct stack pointer balancing when the jump is emitted.While it is possible to optimize this process by avoiding intermediate labels when balancing isn't required, I've opted to maintain the current implementation until such optimization becomes necessary.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
saulecabrera updated PR #6951.
saulecabrera requested wasmtime-fuzz-reviewers for a review on PR #6951.
saulecabrera updated PR #6951.
saulecabrera requested elliottt for a review on PR #6951.
cfallin submitted PR review:
This looks overall good to me, except for a bit of confusion around one stack-alignment case. With a slightly more detailed description in the comment I think it should be good though!
(also as an fyi, I'm on PTO all of next week; happy to re-review today if it happens, otherwise further reviews will be a bit delayed, sorry)
cfallin submitted PR review:
This looks overall good to me, except for a bit of confusion around one stack-alignment case. With a slightly more detailed description in the comment I think it should be good though!
(also as an fyi, I'm on PTO all of next week; happy to re-review today if it happens, otherwise further reviews will be a bit delayed, sorry)
cfallin created PR review comment:
s/invaid/invalid/
cfallin created PR review comment:
I know this is pre-existing text (I might have even reviewed it the first time?) but for some reason I'm having trouble wrapping my head around the
current_sp < target_sp
case here. Is the argument basically that the Wasm stack invariants (proper typing of blocks, etc) will fix it up? Or is this handled at the use-cases of this function? Either way it would be helpful to point out concretely what piece of the codegen handles the popping.
saulecabrera updated PR #6951.
saulecabrera submitted PR review.
saulecabrera created PR review comment:
I've added a bit more of docs on how this could happen. In general the first assert in this function (above) should always be true, which is what I was after with this refactor, which makes it easier to reason about IMHO. But with that comment I'm noting that when the code reaches the the stack pointer balancing part, the SP might be less if during the invocation of the callback the caller preemptively handles return values via
CodeGenContext::pop_abi_results
(e.g. what would happen during the emission ofbr
). Hope this clarifies things.
cfallin submitted PR review:
LGTM!
cfallin merged PR #6951.
Last updated: Dec 23 2024 at 12:05 UTC