Repository navigation
aarch64: remove MInst::Extend - #14793
Open
Rafferty97 wants to merge 3 commits into
Open
Rafferty97 wants to merge 3 commits into
Rafferty97 wants to merge 3 commits into
Conversation
Rafferty97
force-pushed
the
remove-sextend-aarm64
branch
2 times, most recently
from
October 17, 2026 07:55
6f502f2 to
47df008
Compare
Rafferty97
force-pushed
the
remove-sextend-aarm64
branch
from
October 17, 2026 08:39
47df008 to
fb9d752
Compare
which was removed in the last commit
Rafferty97
force-pushed
the
remove-sextend-aarm64
branch
from
October 17, 2026 10:01
e18d652 to
c2f1d73
Compare
Subscribe to Label Actioncc @avanhatt, @cfallin, @fitzgen, @mmcloughlin, @saulecabrera DetailsThis issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:meta", "isle", "winch"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
This branch has not been deployed
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
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.
Motivation
My last PR (#14187) introduced
MInst::BitfieldMoveto represent all instances of theubfmandsbfminstructions. As a result, zero- and sign-extensions can now be expressed asMInst::BitfieldMoveorMInst::Movdirectly, making theMInst::Extendvariant redundant, so this PR removes it.Changes
I have removed the
MInst::Extendvariant, and replaced sites that constructed it with calls toMInst::gen_extend,which produces a
MInst::BitfieldMoveorMInst::Movas necessary.I've also reimplemented the
extendhelper function defined in ISLE in a similar fashion, and added a helperin the process (
u8_into_uimm6).The code that performed code generation for
MInst::Extendhad special-cased the case wherefrom_bits == 1,producing an
andinstruction rather than aubfminstruction on performance grounds. I haven't carried this over to the newgen_extendfunction as it would be unreachable in practice. (Claude tells me this is a relic from when boolean types likeb1existed and was now effectively dead code, but I haven't confirmed this myself.)I've changed the test cases in
codegen/src/isa/aarch64/inst/emit_tests.rsthat exercisedMInst::Extendto use
MInst::BitfieldMoveinstead, and removed the cases that lowered to different instructions, as thosecases should already be covered elsewhere. I also added a few extra cases with
immr != 0for better coverage.To preserve the behaviour of
print_with_state, I've added a special case to theMInst::BitfieldMovebranchthat prints the appropriate aliases for
ubfm/sbfmsuch that all the file tests still pass without adjustment.Finally, in order to get
isle/veri/isaspec/script/generate.sh -lto work, I had to replace a fewinstances of
std::fmtwithcore::fmt. This isn't related to these changes, so I can separate it into a separate PR if that's preferred.Verification
In removing
MInst::Extend, I necessarily also had to remove the code that generated theextend.islespec file. To avoid a verification regression, I've added limited verification ofMInst::BitfieldMove, restricted to just the cases needed for zero-/sign-extension.This is also the part I'm most unsure about, so I'm more than open to feedback.
Future work
I dug a bit deeper into the optimisation of using
andin place ofubfmwhen zero-extending a 1-bit input,and it seems the rationale is sound and extends to zero-extending any input (other than 32-bit to 64-bit, which is a
mov). It seems LLVM does this for all aarch64 targets, which is evidence that it's a worthwhile optimisation to consider for a future PR.