Stream: git-wasmtime

Topic: wasmtime / PR #6603 winch(x64): Add support for `loop`, `...

Wasmtime GitHub notifications bot (Jun 19 2023 at 10:49):

saulecabrera opened PR #6603 from saulecabrera:winch-control-flow-loop to bytecodealliance:main:

Part of https://github.com/bytecodealliance/wasmtime/issues/6528

This change adds support for the loop, br and br_if instructions as well as unreachable code handling. Whenever an instruction that affects reachability is emitted (br in the case of this PR), the compiler will enter into an unreachable code state, essentially ignoring most of the subsequent instructions. When handling the unreachable code state some instructions are still observed, in order to determine if reachability should be restored.

This change, particulary the handling of unreachable code, adds all the necessary building blocks to the compiler to emit other instructions that affect reachability (e.g unreachable, return).

I decided to bundle these instructions together to be able to test more complex scenarios with loop, but I'm happy to split this change into two (or more).

<!--
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 19 2023 at 10:49):

saulecabrera requested alexcrichton for a review on PR #6603.

Wasmtime GitHub notifications bot (Jun 19 2023 at 10:49):

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

Wasmtime GitHub notifications bot (Jun 19 2023 at 10:49):

saulecabrera requested jameysharp for a review on PR #6603.

Wasmtime GitHub notifications bot (Jun 19 2023 at 10:49):

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

Wasmtime GitHub notifications bot (Jun 19 2023 at 10:49):

saulecabrera requested cfallin for a review on PR #6603.

Wasmtime GitHub notifications bot (Jun 20 2023 at 14:53):

alexcrichton created PR review comment:

Bit of a drive-by comment, but altering the core loop here felt a bit unfortunate mainly along the lines of the translation of If for example was spread across handle_unreachable_state as well as the typical visitor.

One option though might be to use macro-magic to get ValidateThenVisit to skip all instructions except the allow-listed control flow instructions when the state is unreachable, sort of like how the visitor today fails on all instructions except for an allow-listed set that's implemented?

Wasmtime GitHub notifications bot (Jun 20 2023 at 16:30):

saulecabrera created PR review comment:

Actually this is something that I meant to revisit after getting everything working as I didn't want to get into macro-magic at beginning, while getting all the pieces working together, but it fell through the cracks! I'll give the macro approach a try!

Wasmtime GitHub notifications bot (Jun 20 2023 at 16:32):

cfallin submitted PR review:

A few thoughts but overall I like how this is turning out -- the abstractions around the ControlStackFrame are very clean!

Wasmtime GitHub notifications bot (Jun 20 2023 at 16:32):

cfallin submitted PR review:

A few thoughts but overall I like how this is turning out -- the abstractions around the ControlStackFrame are very clean!

Wasmtime GitHub notifications bot (Jun 20 2023 at 16:32):

cfallin created PR review comment:

Can we call this handle_unreachable_operator instead? The current name handle_unreachable_state implies to me maybe that it's invoked when we enter an unreachable region, or something like that; it's still a per-operator handler, just for unreachable operators.

Wasmtime GitHub notifications bot (Jun 20 2023 at 16:32):

cfallin created PR review comment:

Can we call this handle_unreachable_operator instead? The current name handle_unreachable_state implies to me maybe that it's invoked when we enter an unreachable region, or something like that; it's still a per-operator handler, just for unreachable operators.

Wasmtime GitHub notifications bot (Jun 20 2023 at 16:32):

cfallin created PR review comment:

Can we call this field something more like exit_reachable? branch_target makes me think this is a label; really this is a reachability flag for some other program point than the current one.

Wasmtime GitHub notifications bot (Jun 20 2023 at 16:32):

cfallin created PR review comment:

Does this need to handle implicit frames? Or is there an invariant here that only the outer body frame is implicit? If so, maybe we could remove the implicit flag (since it allows representing state that violates invariants) and instead handle the body-block case another way (e.g. at end/pop, recognize that it's the outermost block)?

Wasmtime GitHub notifications bot (Jun 20 2023 at 16:32):

cfallin created PR review comment:

It's unfortunately not quite as simple as skipping ops, as the logic itself differs too... cranelift-wasm also separates out the "unreachable case" to a wholly separate handler.

I do agree that the asymmetry between ordinary visitors (which are quite nice) and an explicit match-on-op is not great. We could put the logic back into visit_* for cases where both reachable and unreachable have handlers (e.g. control-flow and end) and, at the top of every "irrelevant in unreachable code" op, do a if self.context.unreachable { return Ok(()); } or wrap that in a skip_if_unreachable!(); macro or something. @saulecabrera what do you think?

Wasmtime GitHub notifications bot (Jun 20 2023 at 16:32):

cfallin created PR review comment:

Actually, after reading further: I was struggling to understand how this got set when the block "falls through"; it turns out it it isn't, and rather that case is implicitly handled because if we reach an End for a block while in reachable code, we'll remain reachable. So maybe something like is_branch_target makes it clear it's a predicate.

Wasmtime GitHub notifications bot (Jun 20 2023 at 16:44):

saulecabrera created PR review comment:

If the macro approach doesn't work and if there's a strong preference to avoid mixing the visitors with match-on-op, @cfallin's suggestion is what I had in mind as a fallback. One thing that I like of the current approach, is that the handling of unreachable code is isolated and should only be updated if there's a need to deal with such state while handling the unreachable code state in each visitor function is a bit more manual and error prone, but in the spirit of more explicit code I think that's fine.

Wasmtime GitHub notifications bot (Jun 20 2023 at 17:10):

saulecabrera created PR review comment:

Yeah, really the only implicit frame that should exist should be the outermost. So this flag can be removed I believe and instead use the "outermost" frame as the invariant here.

Wasmtime GitHub notifications bot (Jun 20 2023 at 20:02):

saulecabrera updated PR #6603.

Wasmtime GitHub notifications bot (Jun 20 2023 at 20:17):

saulecabrera updated PR #6603.

Wasmtime GitHub notifications bot (Jun 20 2023 at 20:19):

saulecabrera created PR review comment:

Updated!

Wasmtime GitHub notifications bot (Jun 20 2023 at 20:19):

saulecabrera created PR review comment:

Updated!

Wasmtime GitHub notifications bot (Jun 20 2023 at 20:22):

alexcrichton created PR review comment:

Also to be clear I don't feel strongly about this at all, so whatever feels like it works best I think is fine

Wasmtime GitHub notifications bot (Jun 20 2023 at 20:24):

saulecabrera created PR review comment:

Updated to a new name according to the new approach for handling unreachable operators.

Wasmtime GitHub notifications bot (Jun 20 2023 at 20:24):

saulecabrera requested cfallin for a review on PR #6603.

Wasmtime GitHub notifications bot (Jun 20 2023 at 22:23):

alexcrichton submitted PR review:

Nice! Seems good to me but I'd defer to @cfallin still for other technical review bits

Wasmtime GitHub notifications bot (Jun 20 2023 at 22:23):

alexcrichton submitted PR review:

Nice! Seems good to me but I'd defer to @cfallin still for other technical review bits

Wasmtime GitHub notifications bot (Jun 20 2023 at 22:23):

alexcrichton created PR review comment:

If this is only ever used for End and Else and never anything else, then it may be worth splitting this into two functions?

Wasmtime GitHub notifications bot (Jun 20 2023 at 22:32):

cfallin submitted PR review:

Generally LGTM, thanks! A few comments below but feel free to merge when addressed.

Wasmtime GitHub notifications bot (Jun 20 2023 at 22:32):

cfallin submitted PR review:

Generally LGTM, thanks! A few comments below but feel free to merge when addressed.

Wasmtime GitHub notifications bot (Jun 20 2023 at 22:32):

cfallin created PR review comment:

s/unrechable/unreachable/

Wasmtime GitHub notifications bot (Jun 20 2023 at 22:32):

cfallin created PR review comment:

Agreed, let's split these cases out, like the other helpers.

Wasmtime GitHub notifications bot (Jun 20 2023 at 22:32):

cfallin created PR review comment:

Will this do a string-compare at runtime? I think it would be better to have a predicate on the opcode (actual enum value) that returns a bool -- call it something like visit_op_when_unreachable -- and use it here.

Wasmtime GitHub notifications bot (Jun 21 2023 at 00:53):

saulecabrera updated PR #6603.

Wasmtime GitHub notifications bot (Jun 21 2023 at 00:53):

saulecabrera created PR review comment:

Good call, updated!

Wasmtime GitHub notifications bot (Jun 21 2023 at 01:08):

saulecabrera updated PR #6603.

Wasmtime GitHub notifications bot (Jun 21 2023 at 11:02):

saulecabrera merged PR #6603.

Last updated: Oct 23 2024 at 20:03 UTC