Kmeakin requested abrown for a review on PR #9099.
Kmeakin requested wasmtime-compiler-reviewers for a review on PR #9099.
Kmeakin requested wasmtime-default-reviewers for a review on PR #9099.
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
Kmeakin updated PR #9099.
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:
- fitzgen: pulley
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
alexcrichton requested fitzgen for a review on PR #9099.
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!
Kmeakin updated PR #9099.
Kmeakin updated PR #9099.
Kmeakin updated PR #9099.
fitzgen commented on PR #9099:
This needs a rebase after #9088
Kmeakin updated PR #9099.
Kmeakin updated PR #9099.
fitzgen submitted PR review:
Looks great! Just a couple things to address below before we merge this.
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 gatexpush
/xpop
/push_frame
/pop_frame
's fuzzing safety on whether there is enough stack to safely execute them.
fitzgen created PR review comment:
I feel like
first
/last
is less clear thanmin
/max
-- is there a particular reason you changed this convention?Happy to change
pop
topop_max
so that we can havepop_min
without introducing ambiguity, but this can all be done without changing tofirst
/last
.
fitzgen edited PR review comment.
Kmeakin submitted PR review.
Kmeakin created PR review comment:
ScalarBitset
now implementsOrd
(because pulleyInstr
derivesOrd
). So calls tobitset.min()
will resolve to themin
method fromOrd
, rather than the inherent method attached toScalarBitset
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Are there perhaps some typos here? Otherwise
sp
is set twice. I also wouldn't expectsp
to be set here at all other than through the side-effects ofpop()
?
fitzgen submitted PR review.
fitzgen created PR review comment:
Can we avoid
Ord
forScalarBitSet
? And instead implement it forRegSet
?
Kmeakin submitted PR review.
Kmeakin created PR review comment:
I'll return
false
for now, we can add a static analysis later
Kmeakin updated PR #9099.
Kmeakin submitted PR review.
Kmeakin created PR review comment:
We can remove the
#[derive(PartialOrd, Ord)]
onOp
, since it's not used anywhere.
Kmeakin updated PR #9099.
Kmeakin submitted PR review.
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.
Kmeakin updated PR #9099.
Kmeakin updated PR #9099.
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!
alexcrichton commented on PR #9099:
I believe the error in CI is that in the
scripts/publish.rs
file thecranelift-bitset
crate needs to be reordered before thepulley
family of crates since they now depend oncranelift-bitset
Kmeakin updated PR #9099.
Kmeakin updated PR #9099.
Kmeakin updated PR #9099.
fitzgen has enabled auto merge for PR #9099.
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.
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.
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.
fitzgen commented on PR #9099:
I believe the error in CI is that in the
scripts/publish.rs
file thecranelift-bitset
crate needs to be reordered before thepulley
family of crates since they now depend oncranelift-bitset
^ Did the fix for this perhaps get lost in a rebase?
Kmeakin commented on PR #9099:
I believe the error in CI is that in the
scripts/publish.rs
file thecranelift-bitset
crate needs to be reordered before thepulley
family of crates since they now depend oncranelift-bitset
^ Did the fix for this perhaps get lost in a rebase?
Yes, thanks
Kmeakin updated PR #9099.
fitzgen has enabled auto merge for PR #9099.
fitzgen merged PR #9099.
Last updated: Dec 23 2024 at 12:05 UTC