x64: remove Inst::AluRmiR
#10687
Draft
x64: remove Inst::AluRmiR
#10687
+5,931
−6,432
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change completely removes the `Inst::AluRmiR` variant along with associated code. The only remaining place this was used was via the `Inst::alu_rmi_r` helper, which is replaced by `Inst::add|sub|and|or...` helpers. The lowering, implemented here in Rust, is meant to mimic the behavior also-encoded in ISLE. In discussions about this in the Cranelift meeting, this duplication was of low concern. Nevertheless, this could be improved in the future. In any case, this commit clearly identifies the locations that build an ALU instruction in Rust: ABI logic, Winch, and long-ish lowering sequences.
Up until now, Winch had been using the `Inst::alu_rmi_r` for generating the instructions it needed to emit. With this helper now gone, we have a chose: (a) continue using `Inst` helpers that use the new assembler or (b) use the new assembler directly. This change adopts option (b). Because of how `cranelift-codegen`'s register allocation works, we have been passing a `PairedGpr` into the read-write slots of the new assembler (this enables us to separate out the read-side and write-side as separate virtual registers). For Winch to use this, we have to expose `PairedGpr` via the now-public `isa::x64::inst::external` module.
abrown
commented
Apr 28, 2025
| @@ -31,6 +32,7 @@ use cranelift_codegen::{ | |||
| }; | |||
|
|
|||
| use crate::reg::WritableReg; | |||
| use cranelift_assembler_x64 as asm; | |||
There was a problem hiding this comment.
@saulecabrera, this line is causing issues like the following:
error[E0432]: unresolved import `cranelift_assembler_x64`
--> winch/codegen/src/isa/x64/asm.rs:35:5
|
35 | use cranelift_assembler_x64 as asm;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no external crate `cranelift_assembler_x64`
For more information about this error, try `rustc --explain E0432`.
I believe this is because the cranelift-assembler-x64 dependency is optional and setting a feature in build.rs is too late for Cargo. What do you want to do? One option is to depend on cranelift-assembler-x64 unconditionally; another is just to set all-arch as a default feature. What do you think?
(I'm also interested in your comments on the rest of these Winch changes...)
github-actions
bot
added
cranelift
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change completely removes the
Inst::AluRmiRvariant along with associated code. It updates ABI code to use newInsthelpers and switches Winch to use the new assembler directly. More details in each commit; note that this significantly changes the disassembly tests.