Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pulley: superinstructions for pushing/popping list of registers #9099

Merged
merged 3 commits into from
Aug 22, 2024
Merged

pulley: superinstructions for pushing/popping list of registers #9099

merged 3 commits into from
Aug 22, 2024

Conversation

Kmeakin
Copy link
Contributor

@Kmeakin Kmeakin commented Aug 9, 2024

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

@Kmeakin Kmeakin requested review from a team as code owners August 9, 2024 21:34
@Kmeakin Kmeakin requested review from abrown and removed request for a team August 9, 2024 21:34
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator pulley Issues related to the Pulley interpreter labels Aug 9, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 9, 2024

Subscribe to Label Action

cc @fitzgen

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 .github/subscribe-to-label.json configuration file.

Learn more.

@alexcrichton alexcrichton requested review from fitzgen and removed request for a team and abrown August 12, 2024 14:54
@fitzgen
Copy link
Member

fitzgen commented Aug 16, 2024

And then once #9088 is rebased and lands, we can do the same with this one. Thanks again for making these improvements @Kmeakin!

@Kmeakin Kmeakin force-pushed the km/pulley-push-pop-many branch 3 times, most recently from e9c380f to 783bf08 Compare August 19, 2024 15:40
@fitzgen
Copy link
Member

fitzgen commented Aug 19, 2024

This needs a rebase after #9088

@Kmeakin Kmeakin force-pushed the km/pulley-push-pop-many branch 2 times, most recently from e510bbc to 4b27717 Compare August 20, 2024 13:54
fitzgen
fitzgen reviewed Aug 20, 2024
View reviewed changes
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cranelift/bitset/src/compound.rs Outdated Show resolved Hide resolved
pulley/fuzz/src/interp.rs Outdated Show resolved Hide resolved
alexcrichton
alexcrichton reviewed Aug 20, 2024
View reviewed changes
pulley/src/interp.rs Outdated Show resolved Hide resolved
@Kmeakin Kmeakin force-pushed the km/pulley-push-pop-many branch 4 times, most recently from 8e2c733 to 6f5c482 Compare August 20, 2024 23:34
fitzgen
fitzgen approved these changes Aug 21, 2024
View reviewed changes
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@fitzgen fitzgen added this pull request to the merge queue Aug 21, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 21, 2024
@Kmeakin
pulley: superinstructions for pushing/popping list of registers (#9099)
f10aaee 
* pulley: add `push` and `pop` instructions

Add `xpush{32, 64}` and `xpop{32, 64}` for pushing/popping XRegs from the stack,
and `push_frame`/`pop_frame` for saving/restoring LR and FP in function
prologue/epilogue.

Copyright (c) 2024, Arm Limited.

Signed-off-by: Karl Meakin <[email protected]>

* cranelift-bitset: more impls for `ScalarBitset`

Implement `Arbitrary` for `ScalarBitset`.

Also implement `DoubleEndedIterator` and `ExactSizeIterator` for `Iter`.
The `pop_min` method was added to help implement `DoubleEndedIterator`,
and `pop` was renamed to `pop_max` for consistency.

Copyright (c) 2024, Arm Limited.

Signed-off-by: Karl Meakin <[email protected]>

* pulley: add instructions for pushing/popping list of registers

Add `xpush{32,64}_many` and `xpop{32,64}_many` for pushing/popping any
combination of all 32 XRegs.

Copyright (c) 2024, Arm Limited.

Signed-off-by: Karl Meakin <[email protected]>

---------

Signed-off-by: Karl Meakin <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 21, 2024
@alexcrichton
Copy link
Member

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

@Kmeakin
pulley: add push and pop instructions
38b5552 
Add `xpush{32, 64}` and `xpop{32, 64}` for pushing/popping XRegs from the stack,
and `push_frame`/`pop_frame` for saving/restoring LR and FP in function
prologue/epilogue.

Copyright (c) 2024, Arm Limited.

Signed-off-by: Karl Meakin <[email protected]>
@Kmeakin
cranelift-bitset: more impls for ScalarBitset
b5e78ec 
Implement `Arbitrary` for `ScalarBitset`.

Also implement `DoubleEndedIterator` and `ExactSizeIterator` for `Iter`.
The `pop_min` method was added to help implement `DoubleEndedIterator`,
and `pop` was renamed to `pop_max` for consistency.

Copyright (c) 2024, Arm Limited.

Signed-off-by: Karl Meakin <[email protected]>
@alexcrichton alexcrichton added this pull request to the merge queue Aug 22, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 22, 2024
@fitzgen fitzgen enabled auto-merge August 22, 2024 15:56
@fitzgen fitzgen added this pull request to the merge queue Aug 22, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 22, 2024
@Kmeakin
pulley: superinstructions for pushing/popping list of registers (#9099)
c9b3843 
* pulley: add `push` and `pop` instructions

Add `xpush{32, 64}` and `xpop{32, 64}` for pushing/popping XRegs from the stack,
and `push_frame`/`pop_frame` for saving/restoring LR and FP in function
prologue/epilogue.

Copyright (c) 2024, Arm Limited.

Signed-off-by: Karl Meakin <[email protected]>

* cranelift-bitset: more impls for `ScalarBitset`

Implement `Arbitrary` for `ScalarBitset`.

Also implement `DoubleEndedIterator` and `ExactSizeIterator` for `Iter`.
The `pop_min` method was added to help implement `DoubleEndedIterator`,
and `pop` was renamed to `pop_max` for consistency.

Copyright (c) 2024, Arm Limited.

Signed-off-by: Karl Meakin <[email protected]>

* pulley: add instructions for pushing/popping list of registers

Add `xpush{32,64}_many` and `xpop{32,64}_many` for pushing/popping any
combination of all 32 XRegs.

Copyright (c) 2024, Arm Limited.

Signed-off-by: Karl Meakin <[email protected]>

---------

Signed-off-by: Karl Meakin <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 22, 2024
@fitzgen
Copy link
Member

fitzgen commented Aug 22, 2024

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
Copy link
Member

fitzgen commented Aug 22, 2024 •
edited
Loading

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.

@Kmeakin
pulley: add instructions for pushing/popping list of registers
d492f07 
Add `xpush{32,64}_many` and `xpop{32,64}_many` for pushing/popping any
combination of all 32 XRegs.

Copyright (c) 2024, Arm Limited.

Signed-off-by: Karl Meakin <[email protected]>
@fitzgen
Copy link
Member

fitzgen commented Aug 22, 2024

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?

@Kmeakin
Copy link
Contributor Author

Kmeakin commented Aug 22, 2024

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

@fitzgen fitzgen enabled auto-merge August 22, 2024 17:19
@fitzgen fitzgen added this pull request to the merge queue Aug 22, 2024
Merged via the queue into bytecodealliance:main with commit ff92e7a Aug 22, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcrichton alexcrichton alexcrichton left review comments

@fitzgen fitzgen fitzgen approved these changes

Assignees
No one assigned
Labels
cranelift Issues related to the Cranelift code generator pulley Issues related to the Pulley interpreter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Kmeakin @fitzgen @alexcrichton