Wasmtime GitHub notifications bot (Nov 20 2024 at 15:55): Wasmtime GitHub notifications bot (Nov 20 2024 at 15:55):alexcrichton requested abrown for a review on PR #9629.
Wasmtime GitHub notifications bot (Nov 20 2024 at 15:55):alexcrichton requested wasmtime-default-reviewers for a review on PR #9629.
Wasmtime GitHub notifications bot (Nov 20 2024 at 15:55):alexcrichton opened PR #9629 from alexcrichton:refactor-pulley-loop to bytecodealliance:main:
- Define loop-over-match and loop-with-tail-calls in separate files to make it more clear which is in which (and less
#[cfg])- Move per-opcode handlers to
interp.rsoutside of a macro invocation to get better native editor support (e.g. formatting, hints, etc).This is roughly intended to be perf-neutral but we don't have many automated benchmarks yet for Pulley so it's intended to profile later as well.
<!--
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 (Nov 20 2024 at 16:11):alexcrichton commented on PR #9629:
cc @fitzgen @Kmeakin
Wasmtime GitHub notifications bot (Nov 20 2024 at 16:54):abrown requested fitzgen for a review on PR #9629.
Wasmtime GitHub notifications bot (Nov 20 2024 at 16:55):@fitzgen, I'll leave this for you for when you get back as I assume you have stronger opinions on this level of changes.
Wasmtime GitHub notifications bot (Nov 20 2024 at 18:47):github-actions[bot] commented on PR #9629:
Subscribe to Label Action
cc @fitzgen
<details>
This issue or pull request has been labeled: "pulley"Thus the following users have been cc'd because of the following labels:
- fitzgen: pulley
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Nov 20 2024 at 21:22):Kmeakin commented on PR #9629:
cc @fitzgen @Kmeakin
No objections from me
Wasmtime GitHub notifications bot (Nov 21 2024 at 02:00):alexcrichton updated PR #9629.
Wasmtime GitHub notifications bot (Nov 21 2024 at 02:00):alexcrichton commented on PR #9629:
I've done a bit more refactoring here with the goal of modeling the interpreter as an implementation of
OpVisitorandExtendedOpVisitor. This should help improve error messages and type-checking for missing operations when new ones are added.
Wasmtime GitHub notifications bot (Nov 21 2024 at 16:11):alexcrichton commented on PR #9629:
I think that @fitzgen may be out for another week or so and I'm starting to accumulate other changes built on this, so @abrown would you be ok approving this from a "lgtm" point of view? Pulley is still under heavy development so it should be ok to break things/regress things temporarily and I'm happy to take responsibility for following-up on this with Nick when he's back.
Wasmtime GitHub notifications bot (Nov 22 2024 at 18:20):abrown submitted PR review:
Makes sense to me; take my comment below with a grain of salt since I wasn't involved in initially designing this (but, yeah, it isn't too clear _when_ interpretation is occurring in the two modes).
Wasmtime GitHub notifications bot (Nov 22 2024 at 18:20):abrown created PR review comment:
One thing that is confusing here (and that I realize is present in the previous version as well) is that that the decoder is actually calling the visitor that does the interpretation. To avoid the reader looking around for "where is the handler that gets called here?" perhaps in the future we think through a different way to surface what is happening here; alternately, a comment here could be helpful.
Wasmtime GitHub notifications bot (Nov 22 2024 at 18:20):abrown created PR review comment:
TIL: break with expressions is a thing...
Wasmtime GitHub notifications bot (Nov 22 2024 at 18:21):abrown submitted PR review.
Wasmtime GitHub notifications bot (Nov 22 2024 at 18:21):abrown created PR review comment:
Alternately, rename the
decode_onefunction?
Wasmtime GitHub notifications bot (Nov 22 2024 at 19:03):alexcrichton updated PR #9629.
Wasmtime GitHub notifications bot (Nov 22 2024 at 19:04):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 22 2024 at 19:04):alexcrichton created PR review comment:
Good points! I've tried to add more words here to explain what's going on
Wasmtime GitHub notifications bot (Nov 22 2024 at 19:04):alexcrichton commented on PR #9629:
Ok I'm going to flag this for merging but @fitzgen you'll definitely be interested in this for when you get back, and I'm happy to address any feedback you have in follow-ups.
Wasmtime GitHub notifications bot (Nov 22 2024 at 19:04):alexcrichton has enabled auto merge for PR #9629.
Wasmtime GitHub notifications bot (Nov 22 2024 at 19:40):alexcrichton merged PR #9629.
Wasmtime GitHub notifications bot (Dec 04 2024 at 20:16):fitzgen submitted PR review:
LGTM!
Last updated: Dec 23 2024 at 13:07 UTC