Stream: git-wasmtime

Topic: wasmtime / PR #9099 pulley: superinstructions for pushing...

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2024 at 21:34):

Kmeakin requested abrown for a review on PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2024 at 21:34):

Kmeakin requested wasmtime-compiler-reviewers for a review on PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2024 at 21:34):

Kmeakin requested wasmtime-default-reviewers for a review on PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2024 at 21:34):

Kmeakin opened PR #9099 from Kmeakin:km/pulley-push-pop-many to bytecodealliance:main:

Adds superinstructions for pushing/popping any subset of XRegs in one go

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2024 at 21:37):

Kmeakin updated PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2024 at 22:44):

github-actions[bot] commented on PR #9099:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "pulley"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Aug 12 2024 at 14:54):

alexcrichton requested fitzgen for a review on PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 22:13):

fitzgen commented on PR #9099:

And then once https://github.com/bytecodealliance/wasmtime/pull/9088 is rebased and lands, we can do the same with this one. Thanks again for making these improvements @Kmeakin!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2024 at 14:52):

Kmeakin updated PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2024 at 15:05):

Kmeakin updated PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2024 at 15:40):

Kmeakin updated PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2024 at 20:27):

fitzgen commented on PR #9099:

This needs a rebase after #9088

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 00:20):

Kmeakin updated PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 13:54):

Kmeakin updated PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 16:50):

fitzgen submitted PR review:

Looks great! Just a couple things to address below before we merge this.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 16:50):

fitzgen created PR review comment:

These are only safe if we have enough stack space to push to or pop from, right?

So either we should make these not safe for fuzzing, or we should keep track of how much stack we've consumed and still have available at the point where this op is executed and gate xpush/xpop/push_frame/pop_frame's fuzzing safety on whether there is enough stack to safely execute them.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 16:50):

fitzgen created PR review comment:

I feel like first/last is less clear than min/max -- is there a particular reason you changed this convention?

Happy to change pop to pop_max so that we can have pop_min without introducing ambiguity, but this can all be done without changing to first/last.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 16:51):

fitzgen edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 17:11):

Kmeakin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 17:11):

Kmeakin created PR review comment:

ScalarBitset now implements Ord (because pulley Instr derives Ord). So calls to bitset.min() will resolve to the min method from Ord, rather than the inherent method attached to ScalarBitset

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 17:24):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 17:24):

alexcrichton created PR review comment:

Are there perhaps some typos here? Otherwise sp is set twice. I also wouldn't expect sp to be set here at all other than through the side-effects of pop()?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 17:41):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 17:41):

fitzgen created PR review comment:

Can we avoid Ord for ScalarBitSet? And instead implement it for RegSet?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 22:42):

Kmeakin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 22:42):

Kmeakin created PR review comment:

I'll return false for now, we can add a static analysis later

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 22:46):

Kmeakin updated PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 22:48):

Kmeakin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 22:48):

Kmeakin created PR review comment:

We can remove the #[derive(PartialOrd, Ord)] on Op, since it's not used anywhere.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 22:58):

Kmeakin updated PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 23:04):

Kmeakin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 23:04):

Kmeakin created PR review comment:

I didn't reverse the logic from push_frame completely. IIUC, sp should be restored from fp before popping fp and lr.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 23:06):

Kmeakin updated PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2024 at 23:34):

Kmeakin updated PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 21 2024 at 20:58):

fitzgen submitted PR review:

Looks great, thanks!

I hope to get the runtime integration soon, and then we can start enabling Wasm spec tests and such one by one until we have everything running under Pulley!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 21 2024 at 21:09):

alexcrichton commented on PR #9099:

I believe the error in CI is that in the scripts/publish.rs file the cranelift-bitset crate needs to be reordered before the pulley family of crates since they now depend on cranelift-bitset

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2024 at 01:04):

Kmeakin updated PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2024 at 14:38):

Kmeakin updated PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2024 at 15:54):

Kmeakin updated PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2024 at 15:56):

fitzgen has enabled auto merge for PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2024 at 16:08):

fitzgen commented on PR #9099:

Side note, it will be a little faster to iterate here to add an empty commit with the message prtest:full so that the full CI checks happen and we don't have to wait for the merge queue.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2024 at 16:11):

fitzgen commented on PR #9099:

I'm not totally sure what is going on here, new-ish dependencies for new-ish are always a little annoying to get set up correctly so that when we publish we don't run into any issues. I guess we are effectively front-loading the issues, rather than dealing with them at publish time, which is probably the right choice. Anyways, I think you should be able to repro locally with

$ rustc scripts/publish.rs
$ ./publish verify

Note that if you have a .cargo directory in your wasmtime checkout already, you'll want to move it somewhere else temporarily while running the above.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2024 at 16:11):

fitzgen edited a comment on PR #9099:

I'm not totally sure what is going on here, new-ish dependencies for new-ish crates are always a little annoying to get set up correctly so that when we publish we don't run into any issues. I guess we are effectively front-loading the issues, rather than dealing with them at publish time, which is probably the right choice. Anyways, I think you should be able to repro locally with

$ rustc scripts/publish.rs
$ ./publish verify

Note that if you have a .cargo directory in your wasmtime checkout already, you'll want to move it somewhere else temporarily while running the above.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2024 at 17:10):

fitzgen commented on PR #9099:

I believe the error in CI is that in the scripts/publish.rs file the cranelift-bitset crate needs to be reordered before the pulley family of crates since they now depend on cranelift-bitset

^ Did the fix for this perhaps get lost in a rebase?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2024 at 17:13):

Kmeakin commented on PR #9099:

I believe the error in CI is that in the scripts/publish.rs file the cranelift-bitset crate needs to be reordered before the pulley family of crates since they now depend on cranelift-bitset

^ Did the fix for this perhaps get lost in a rebase?

Yes, thanks

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2024 at 17:14):

Kmeakin updated PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2024 at 17:19):

fitzgen has enabled auto merge for PR #9099.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2024 at 17:36):

fitzgen merged PR #9099.

Last updated: Nov 22 2024 at 17:03 UTC