Stream: git-wasmtime

Topic: wasmtime / PR #6550 winch(x64) Add support for if/else

Wasmtime GitHub notifications bot (Jun 09 2023 at 17:41):

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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Jun 09 2023 at 17:41):

saulecabrera requested itsrainy for a review on PR #6550.

Wasmtime GitHub notifications bot (Jun 09 2023 at 17:41):

saulecabrera requested wasmtime-fuzz-reviewers for a review on PR #6550.

Wasmtime GitHub notifications bot (Jun 09 2023 at 17:41):

saulecabrera requested wasmtime-compiler-reviewers for a review on PR #6550.

Wasmtime GitHub notifications bot (Jun 09 2023 at 17:41):

saulecabrera requested jameysharp for a review on PR #6550.

Wasmtime GitHub notifications bot (Jun 09 2023 at 17:41):

saulecabrera requested cfallin for a review on PR #6550.

Wasmtime GitHub notifications bot (Jun 09 2023 at 17:56):

saulecabrera updated PR #6550.

Wasmtime GitHub notifications bot (Jun 12 2023 at 15:57):

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.

Wasmtime GitHub notifications bot (Jun 12 2023 at 15:57):

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.

Wasmtime GitHub notifications bot (Jun 12 2023 at 15:57):

cfallin created PR review comment:

s/doesnt/doesn't/

Wasmtime GitHub notifications bot (Jun 12 2023 at 15:57):

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 and self.frame?

Maybe spill_impl instead?

Wasmtime GitHub notifications bot (Jun 12 2023 at 15:57):

cfallin created PR review comment:

I kind of like what cranelift-wasm does: it has another frame type Else, and the else opcode pops the If and pushes an Else. 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, the emit_else method could mutate self (*self = ControlStackFrame::Else { ... }). This also removes the need for the conditional in the end case below and the somewhat subtle invariant around the option-of-label. What do you think?

Wasmtime GitHub notifications bot (Jun 12 2023 at 15:57):

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 like 64 or thereabouts?

Wasmtime GitHub notifications bot (Jun 13 2023 at 16:47):

saulecabrera updated PR #6550.

Wasmtime GitHub notifications bot (Jun 13 2023 at 16:47):

saulecabrera created PR review comment:

Yeah spill_impl makes sense to me.

Wasmtime GitHub notifications bot (Jun 13 2023 at 16:48):

saulecabrera created PR review comment:

The resulting code is definitely cleaner with this approach, thanks!

Wasmtime GitHub notifications bot (Jun 13 2023 at 16:49):

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.

Wasmtime GitHub notifications bot (Jun 13 2023 at 16:54):

saulecabrera updated PR #6550.

Wasmtime GitHub notifications bot (Jun 13 2023 at 18:13):

saulecabrera merged PR #6550.

Last updated: Oct 23 2024 at 20:03 UTC