aarch64: lower `ishl` + `ushr`/`sshr` pairs to a single `ubfm`/`sbfm` instruction by Rafferty97 · Pull Request #14187 · bytecodealliance/wasmtime

Rafferty97 · 2026-08-21T09:13:17Z

Motivation

AArch64 can express a left-shift followed by a right-shift as a single bitfield-move instruction, either ubfm or sbfm, which are commonly aliased to sbfx/sbfiz/ubfx/ubfiz. The aarch64 backend currently emits two instructions, since no rule inspects a sshr/ushr's operand for a producing ishl. The appropriate encoder already exists in emit.rs as the function enc_bfm, but only serves MInst::Extend, which only covers fixed-width sign extension.

There has been previous discussion around adding support for this kind of lowering here: #1067

Changes

I've added MInst::BitfieldMove to express the bitfield move family of AArch64 instructions (bfm, ubfm, sbfm) more generally than the pre-existing MInst::Extend. I then added lowering rules to recognise a sequence of ishl + ushr or ishl + sshr operations that could be lowered to ubfm or sbfm respectively. This necessitated two helper functions (sbfm_immr and sbfm_imms) to calculate the appropriate values for the immr and imms immediates.

I have taken care to support both 32-bit and 64-bit instructions, and to mask off the shift amounts as required by CLIF's semantics. I've added tests to shift-rotate.clif that cover all these cases.

I've also lightly modified the enc_bfm function signature to take a BfmOp rather than raw bits, for better separation of concerns.

Future work

Now that MInst can represent the full suite of bitfield-move instructions precisely, there's an argument for removing the Extend variant and instead lowering zero- and sign-extension operations to BitfieldMove directly. To bound the scope of this PR, though, I've left it in place.

eligible `ishl`/`sshr` pairs into one `sbfm` instruction.

methods `sbfm_immr`/`sbfm_imms`; added additional test cases

github-actions · 2026-08-21T13:03:04Z

Subscribe to Label Action

cc @cfallin, @fitzgen

Details

This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "isle"

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

cfallin: isle
fitzgen: isle

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

Learn more.

cfallin

Thanks! This looks generally good. Just a few comments below.

Also: would you be interested in seeing if you can use the verification framework (cranelift/isle/veri) that is in-tree, but not yet in enforcing mode, to verify these new lowerings? (I will soon turn it on but this would be a good test-case for usability in the meantime.) It might just work if the instruction specs for ubfm/sbfm are already generated; I'm not sure. Let us know if any issues with this!

cfallin · 2026-08-24T19:44:48Z

+        let w = ty.lane_bits() as u8;
+        let a = (a as u8) & (w - 1);
+        let b = (b as u8) & (w - 1);
+        UImm6::maybe_from_u8(if a <= b { b - a } else { w - (a - b) }).unwrap()


Comment here that the unwrap should always succeed because w is at most 64? Probably also debug_assert!(w <= 64) above.

Sure, good suggestions. I usually prefer expect over unwrap with a comment as it surfaces the reasoning in panic messages too. Will update.

cfallin · 2026-08-24T19:45:25Z

        (to_bits u8))

+        ;; A bitfield move instruction, which encompasses
+        ;; the BFM, UBFM and SBFM instructions.


Add "Overwrites whole rd (the bits outside the specified bitfield are zeroed)." here for clarity, per above.

As mentioned above, the situation's a bit more nuanced. How does this read?

;; A bitfield move instruction, which encompasses ;; the BFM, UBFM and SBFM instructions. ;; ;; The net effect of these instructions is to move a ;; contiguous subset of bits in `rn` into `rd`, possibly ;; at an offset, then: ;; * For BFM: the other bits in `rd` are preserved ;; * For UBFM/SBFM: the other bits are cleared/sign-extended

Rafferty97 · 2026-08-25T22:11:33Z

Thanks! This looks generally good. Just a few comments below.

Also: would you be interested in seeing if you can use the verification framework (cranelift/isle/veri) that is in-tree, but not yet in enforcing mode, to verify these new lowerings? (I will soon turn it on but this would be a good test-case for usability in the meantime.) It might just work if the instruction specs for ubfm/sbfm are already generated; I'm not sure. Let us know if any issues with this!

@cfallin Ah, I forgot to reply to this one. Sure, I'd be interested in adding some tests in the new verification framework. Is it alright if this comes via a seperate PR?

cfallin · 2026-08-25T22:25:38Z

@cfallin Ah, I forgot to reply to this one. Sure, I'd be interested in adding some tests in the new verification framework. Is it alright if this comes via a seperate PR?

Yes, that's fine -- it was an opportunistic request, we don't have it on in fully gating mode yet, so it's not yet an expectation :-)

cfallin

Thanks!

Rafferty97 · 2026-08-25T23:01:44Z

@cfallin Thanks again for reviewing this so quickly. It looks like the verifier check has failed, due to these errors:

=== EXPANSION ERRORS (2) ===
ERROR #3702	sbfm	expanding constructor 'sbfm_immr': no spec for term sbfm_immr
ERROR #3703	ubfm	expanding constructor 'sbfm_immr': no spec for term sbfm_immr

These are the two helper functions that calculate immr and imms. I assume these errors mean I need to add a(spec ...) declaration for each one? I'll have a look to see if I can work out where to put these and what to put in them, but if you had any guidance that'd be appreciated :)

cfallin · 2026-08-25T23:18:10Z

Ah, yes, we don't have full verification on but we do check that specs are present. All helpers that bottom out in Rust (and that are reachable) need specs -- hopefully you'll be able to follow the examples on the existing helpers, but feel free to ping if not and we can help.

UBFM; add spec clauses to both helpers

Rafferty97 · 2026-08-26T07:00:06Z

Ah, yes, we don't have full verification on but we do check that specs are present. All helpers that bottom out in Rust (and that are reachable) need specs -- hopefully you'll be able to follow the examples on the existing helpers, but feel free to ping if not and we can help.

Thanks. I've added the spec clauses, and just assert that both inputs and the output are <64 - not sure if there are any stronger pre- or post-conditions applicable to these helpers.

I'm curious why this PR passed CI before approval, but failed in the merge queue?

avanhatt · 2026-08-26T14:07:44Z

The spec stubs do still need to type-check under the current merge queue CI gate, so I'd suggest just making them vacuous "return true".

(spec (bfm_immr ty a b) (provide true))
(spec (bfm_imms ty a b) (provide true))

I checked locally that this should pass the current CI.

Sorry for the churn as we work through the kinks of the new verifier integration/instructions!

avanhatt · 2026-08-26T14:11:38Z

Also re: the merge queue, it runs a superset of tests; you can run all for a commit by including prtest:full in the commit string.

Rafferty97 · 2026-08-26T16:08:25Z

@avanhatt Thanks for the helpful answers - I'll update my PR now as per your suggestion

Rafferty97 · 2026-08-26T16:19:24Z

I was also wondering - would there be interest in a follow up PR that removes MInst::Extend since this is now representable as an MInst:BitfieldMove, which is more general?

Rafferty97 · 2026-08-26T23:07:21Z

Hi @cfallin, @avanhatt - I've hit another wall. I'm now seeing this output from the "ISLE verifier basic check":

=== EXPANSION ERRORS (2) ===
ERROR #3683	sbfm	expanding constructor 'MInst.BitfieldMove': no spec for term MInst.BitfieldMove
ERROR #3684	ubfm	expanding constructor 'MInst.BitfieldMove': no spec for term MInst.BitfieldMove

I've looked at MInst.Expand as a template, so I think I need to add a define_bitfield_move function in cranelift/isle/veri/isaspec/src/instructions.rs, but I'm not sure.

Rafferty97 · 2026-08-27T01:37:16Z

Hi @cfallin, @avanhatt - I've hit another wall. I'm now seeing this output from the "ISLE verifier basic check":
=== EXPANSION ERRORS (2) ===
ERROR #3683	sbfm	expanding constructor 'MInst.BitfieldMove': no spec for term MInst.BitfieldMove
ERROR #3684	ubfm	expanding constructor 'MInst.BitfieldMove': no spec for term MInst.BitfieldMove
I've looked at MInst.Expand as a template, so I think I need to add a define_bitfield_move function in cranelift/isle/veri/isaspec/src/instructions.rs, but I'm not sure.

I've attempted to do this - with a lot of help from Claude - and the solution it came up with seems to make the CI test pass now. However, I'm not confident that it's the best solution, or even needed at all right now, and the fact that it generates a 50k ISLE file seems a bit off.

The code is in this commit, if you wouldn't mind having a quick glance to see if it's going in the right direction: Rafferty97@dc043ad

avanhatt · 2026-08-27T02:08:59Z

@Rafferty97 sorry, I should have caught this! For this to not block this PR, you can just mark this as a TODO via:

(attr MInst.BitfieldMove (tag TODO))

I'm happy to help with adding a real spec forMInst.BitfieldMove in a followup PR, the ISA spec generation logic can result in that large of an auto-generated spec, but I'll have more time to look it over in a later PR.

avanhatt · 2026-08-27T02:09:56Z

I can also take over doing that PR in its entirely if you're not interested in it, since this may be more than you bargained for! Thanks again for your patience with this new process.

Rafferty97 · 2026-08-27T02:14:29Z

I can also take over doing that PR in its entirely if you're not interested in it, since this may be more than you bargained for! Thanks again for your patience with this new process.

I'd appreciate that, actually :) I'm not sure if the LLM-assisted code in the commit I mentioned is helpful at all, but the reason it generates so much ISLE is that it has to enumerate all 64x64 combination of immr and imms immediate values - I think.

I've just pushed a commit with the added (attr MInst.BitfieldMove (tag TODO)). Hopefully that gets this across the line :)

Rafferty97 · 2026-08-27T02:18:21Z

One other minor thing: While working through that latest CI issue, I tried running the CI command locally, i.e.:

cargo run -p cranelift-isle-veri --bin veri -- --config cranelift/isle/veri/configs/aarch64-fast.args --skip-solver

But in order to get it working, I had to make a small fix to cranelift/isle/veri/isaspec/Cargo.toml, adding "std" to the feature list:

cranelift-codegen = { workspace = true, features = ["all-arch", "std"] }

I'm not sure if this is just an issue with my setup, though, and I'd guess it is.

Rafferty97 added 3 commits August 21, 2026 17:43

Add BitfieldMove to AArch64 MInst enum, and a lowering that combines

b1e2c4a

eligible `ishl`/`sshr` pairs into one `sbfm` instruction.

Generalised the sbfm lowering rule to cover more cases; added helper

377b157

methods `sbfm_immr`/`sbfm_imms`; added additional test cases

Add ubfm rule, the unsigned analogue of sbfm, with test cases

7211dff

Rafferty97 requested a review from a team as a code owner August 21, 2026 09:13

Rafferty97 requested review from cfallin and removed request for a team August 21, 2026 09:13

Rafferty97 mentioned this pull request Aug 21, 2026

Bitfield Extract/Create instructions #1067

Closed

github-actions Bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. isle Related to the ISLE domain-specific language labels Aug 21, 2026

cfallin reviewed Aug 24, 2026

View reviewed changes

Rafferty97 force-pushed the aarch64-ubfm-sbfm branch from 76d7197 to 572335f Compare August 25, 2026 01:07

PR feedback; remove BFM from BfmOp, as it has modify semantics on rd

882b994

Rafferty97 force-pushed the aarch64-ubfm-sbfm branch from 572335f to 882b994 Compare August 25, 2026 01:17

aarch64: add representation for BFM instruction

f8b1ef1

Rafferty97 requested a review from cfallin August 25, 2026 22:08

cfallin approved these changes Aug 25, 2026

View reviewed changes

cfallin added this pull request to the merge queue Aug 25, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 25, 2026

rename sbfm_imm(r|s) to bfm_imm(r|s), as it's used for both SBFM and

c21d5d1

UBFM; add spec clauses to both helpers

Rafferty97 requested a review from cfallin August 26, 2026 07:07

fix bfm_immr/imms spec stubs; prtest:full

84028f7

mark BitfieldMove with TODO tag; prtest:full

0abea88

Uh oh!

Conversation

Rafferty97 commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Future work

Uh oh!

github-actions Bot commented Aug 21, 2026

Subscribe to Label Action

Uh oh!

cfallin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cfallin Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

Rafferty97 Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

cfallin Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

Rafferty97 Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

Rafferty97 commented Aug 25, 2026

Uh oh!

cfallin commented Aug 25, 2026

Uh oh!

cfallin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Rafferty97 commented Aug 25, 2026

Uh oh!

cfallin commented Aug 25, 2026

Uh oh!

Rafferty97 commented Aug 26, 2026

Uh oh!

avanhatt commented Aug 26, 2026

Uh oh!

avanhatt commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rafferty97 commented Aug 26, 2026

Uh oh!

Rafferty97 commented Aug 26, 2026

Uh oh!

Rafferty97 commented Aug 26, 2026

Uh oh!

Rafferty97 commented Aug 27, 2026

Uh oh!

avanhatt commented Aug 27, 2026

Uh oh!

avanhatt commented Aug 27, 2026

Uh oh!

Rafferty97 commented Aug 27, 2026

Uh oh!

Rafferty97 commented Aug 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Rafferty97 commented Aug 21, 2026 •

edited

Loading

avanhatt commented Aug 26, 2026 •

edited

Loading