saulecabrera opened PR #6550 from saulecabrera:winch-control-flow
to bytecodealliance:main
:
This change adds the necessary building blocks to support control flow; this change also adds support for the
If
/Else
operators.This change does not include multi-value support. The idea is to add support for multi-value across the compiler (functions and blocks) as a separate future change.
The general gist of the change is to track the presence of control flow frames as part of the code generation context and emit the corresponding labels as and instructions as control flow blocks are found.
<!--
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 itsrainy for a review on PR #6550.
saulecabrera requested wasmtime-fuzz-reviewers for a review on PR #6550.
saulecabrera requested wasmtime-compiler-reviewers for a review on PR #6550.
saulecabrera requested jameysharp for a review on PR #6550.
saulecabrera requested cfallin for a review on PR #6550.
saulecabrera updated PR #6550.
cfallin submitted PR review:
LGTM overall -- excited to have the beginning of control-flow handling, thanks!
A few thoughts for refactors below; curious to know what you think.
cfallin submitted PR review:
LGTM overall -- excited to have the beginning of control-flow handling, thanks!
A few thoughts for refactors below; curious to know what you think.
cfallin created PR review comment:
s/doesnt/doesn't/
cfallin created PR review comment:
The naming here is a little confusing -- I would have expected this method to take a callback (closure) and call it for something, but I guess the idea is that this is actually meant to be called from within a callback, where we have an explicit borrow of
self.regalloc
andself.frame
?Maybe
spill_impl
instead?
cfallin created PR review comment:
I kind of like what
cranelift-wasm
does: it has another frame typeElse
, and theelse
opcode pops theIf
and pushes anElse
. This keeps a 1-to-1 correspondence between the control-flow stack and the position in the code (i.e., the full context is captured) which is a little less error-prone I think. In this design with the methods on the frame struct itself, theemit_else
method could mutateself
(*self = ControlStackFrame::Else { ... }
). This also removes the need for the conditional in theend
case below and the somewhat subtle invariant around the option-of-label. What do you think?
cfallin created PR review comment:
I don't want to bikeshed SmallVec sizes too much (we should just profile) but I'm curious, was this derived from some samples? I've seen block nesting go quite a bit deeper in real wasms; if
ControlStackFrame
isn't too large, and given that this is just one context struct for the length of the compilation, I don't see the harm in something like64
or thereabouts?
saulecabrera updated PR #6550.
saulecabrera created PR review comment:
Yeah
spill_impl
makes sense to me.
saulecabrera created PR review comment:
The resulting code is definitely cleaner with this approach, thanks!
saulecabrera created PR review comment:
I didn't take any samples, so 6 was chosen arbitrarily, I'm not opposed to 64 though. I changed it to 64 and we can modify it later if needed.
saulecabrera updated PR #6550.
saulecabrera merged PR #6550.
Last updated: Dec 23 2024 at 12:05 UTC