Wasmtime GitHub notifications bot (Sep 01 2023 at 15:16): Wasmtime GitHub notifications bot (Sep 01 2023 at 15:16):saulecabrera requested cfallin for a review on PR #6951.
Wasmtime GitHub notifications bot (Sep 01 2023 at 15:16):saulecabrera opened PR #6951 from saulecabrera:winch-br_table to bytecodealliance:main:
This change adds support for the
br_tableinstruction, 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_jumpis 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_tableinstruction 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
-->
Wasmtime GitHub notifications bot (Sep 01 2023 at 15:16):saulecabrera requested wasmtime-compiler-reviewers for a review on PR #6951.
Wasmtime GitHub notifications bot (Sep 01 2023 at 15:16):saulecabrera requested alexcrichton for a review on PR #6951.
Wasmtime GitHub notifications bot (Sep 01 2023 at 15:16):saulecabrera requested wasmtime-core-reviewers for a review on PR #6951.
Wasmtime GitHub notifications bot (Sep 01 2023 at 15:16):saulecabrera edited PR #6951:
This change adds support for the
br_tableinstruction, 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_jumpis 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_tableinstruction 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
-->
Wasmtime GitHub notifications bot (Sep 01 2023 at 15:17):saulecabrera updated PR #6951.
Wasmtime GitHub notifications bot (Sep 01 2023 at 15:17):saulecabrera edited PR #6951:
Part of https://github.com/bytecodealliance/wasmtime/issues/6528
This change adds support for the
br_tableinstruction, 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_jumpis 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_tableinstruction 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
-->
Wasmtime GitHub notifications bot (Sep 01 2023 at 15:30):saulecabrera updated PR #6951.
Wasmtime GitHub notifications bot (Sep 01 2023 at 15:53):saulecabrera requested wasmtime-fuzz-reviewers for a review on PR #6951.
Wasmtime GitHub notifications bot (Sep 01 2023 at 15:53):saulecabrera updated PR #6951.
Wasmtime GitHub notifications bot (Sep 01 2023 at 15:53):saulecabrera requested elliottt for a review on PR #6951.
Wasmtime GitHub notifications bot (Sep 01 2023 at 18:58):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)
Wasmtime GitHub notifications bot (Sep 01 2023 at 18:58):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)
Wasmtime GitHub notifications bot (Sep 01 2023 at 18:58):cfallin created PR review comment:
s/invaid/invalid/
Wasmtime GitHub notifications bot (Sep 01 2023 at 18:58):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_spcase 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.
Wasmtime GitHub notifications bot (Sep 01 2023 at 19:10):saulecabrera updated PR #6951.
Wasmtime GitHub notifications bot (Sep 01 2023 at 19:15):saulecabrera submitted PR review.
Wasmtime GitHub notifications bot (Sep 01 2023 at 19:15):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.
Wasmtime GitHub notifications bot (Sep 01 2023 at 20:45):cfallin submitted PR review:
LGTM!
Wasmtime GitHub notifications bot (Sep 02 2023 at 00:00):cfallin merged PR #6951.
Last updated: Nov 22 2024 at 16:03 UTC