Stream: git-wasmtime

Topic: wasmtime / PR #14187 aarch64: lower `ishl` + `ushr`/`sshr...

Wasmtime GitHub notifications bot (Aug 21 2026 at 09:13):

Rafferty97 opened PR #14187 from Rafferty97:aarch64-ubfm-sbfm to bytecodealliance:main:

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: https://github.com/bytecodealliance/wasmtime/issues/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_bmf 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 bitshift-move instructions precisely, there's an argument for removing the Extract 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.

Wasmtime GitHub notifications bot (Aug 21 2026 at 09:13):

Rafferty97 requested wasmtime-compiler-reviewers for a review on PR #14187.

Wasmtime GitHub notifications bot (Aug 21 2026 at 09:13):

Rafferty97 requested cfallin for a review on PR #14187.

Wasmtime GitHub notifications bot (Aug 21 2026 at 09:24):

Rafferty97 edited PR #14187:

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: https://github.com/bytecodealliance/wasmtime/issues/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.

Wasmtime GitHub notifications bot (Aug 21 2026 at 13:02):

github-actions[bot] added the label cranelift on PR #14187.

Wasmtime GitHub notifications bot (Aug 21 2026 at 13:02):

github-actions[bot] added the label cranelift:area:aarch64 on PR #14187.

Wasmtime GitHub notifications bot (Aug 21 2026 at 13:02):

github-actions[bot] added the label isle on PR #14187.

Wasmtime GitHub notifications bot (Aug 21 2026 at 13:03):

github-actions[bot] commented on PR #14187:

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

Learn more.
</details>

Wasmtime GitHub notifications bot (Aug 22 2026 at 03:45):

Rafferty97 updated PR #14187.

Wasmtime GitHub notifications bot (Aug 24 2026 at 19:47):

:memo: cfallin submitted PR review:

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!

Wasmtime GitHub notifications bot (Aug 24 2026 at 19:47):

:speech_balloon: cfallin created PR review comment:

It might be worth a comment here to address a concern a reader (e.g., me!) might have, that a "bitfield move" does some sort of field insertion and keeps the other original bits in rd (which would require a "modify" effect built of a use and a reuse-def instead). I looked it up and AArch64 is carefully spec'd here to avoid that dependency-creating issue by zeroing the other bits in rd; so this is a true def only (the code is correct). Just wanted to ensure we document that!

Wasmtime GitHub notifications bot (Aug 24 2026 at 19:47):

:speech_balloon: cfallin created PR review comment:

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

Wasmtime GitHub notifications bot (Aug 24 2026 at 19:47):

:speech_balloon: cfallin created PR review comment:

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

Wasmtime GitHub notifications bot (Aug 24 2026 at 23:10):

:memo: Rafferty97 submitted PR review.

Wasmtime GitHub notifications bot (Aug 24 2026 at 23:10):

:speech_balloon: Rafferty97 created PR review comment:

That's a good catch! The bitfield move instruction format splits into three cases based on bfm_op - "BFM", "UBFM" and "SBFM". While UBFM and SBFM zero/sign-extend the other bits in rd, avoiding a dependency, BFM does actually preserve the other original bits in rd. As it happens, my code never emits a BFM instruction, so this isn't an issue right now, but it's probably worth fixing this now anyway in case anyone ever does emit one.

Would the correct fix be something like this? I can't say I fully get what reg_reuse_def does.

        Inst::BitfieldMove { bfm_op, rd, rn, .. } => match bfm_op {
            BfmOp::Bfm => {
                collector.reg_reuse_def(rd, 1);
                collector.reg_use(rn);
            }
            BfmOp::UBfm | BfmOp::SBfm => {
                collector.reg_def(rd);
                collector.reg_use(rn);
            }
        },

Wasmtime GitHub notifications bot (Aug 24 2026 at 23:17):

:memo: Rafferty97 submitted PR review.

Wasmtime GitHub notifications bot (Aug 24 2026 at 23:17):

:speech_balloon: Rafferty97 created PR review comment:

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

Wasmtime GitHub notifications bot (Aug 24 2026 at 23:44):

:memo: Rafferty97 submitted PR review.

Wasmtime GitHub notifications bot (Aug 24 2026 at 23:44):

:speech_balloon: Rafferty97 created PR review comment:

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

Wasmtime GitHub notifications bot (Aug 25 2026 at 00:02):

:speech_balloon: Rafferty97 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 25 2026 at 00:25):

:speech_balloon: Rafferty97 created PR review comment:

@cfallin I discussed the above with an LLM, and it told me my attempted fix above is incorrect because it violates the SSA requirement of virtual registers. It then pointed me at the existing "Op"/"OpMod" convention elsewhere in the codebase.

I could just remove the Bfm variant given its currently unused, but I think it's worth just adding a BitfieldMoveMod now so someone else doesn't have to rediscover it in the future. Happy to split that into its own PR if you'd prefer.

Wasmtime GitHub notifications bot (Aug 25 2026 at 00:25):

:memo: Rafferty97 submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2026 at 01:07):

Rafferty97 updated PR #14187.

Wasmtime GitHub notifications bot (Aug 25 2026 at 01:17):

Rafferty97 updated PR #14187.

Wasmtime GitHub notifications bot (Aug 25 2026 at 16:26):

:memo: cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2026 at 16:26):

:speech_balloon: cfallin created PR review comment:

Ah, I'm glad that I double-checked and raised this then!

Yes, our approach here is to have separate Inst arms for each shape of metadata -- so if one variant of the instruction uses rd as input too, and one doesn't, then they should be separate, as you note.

And the inst itself needs separate VRegs for the "source" and "dest" halves of rd -- e.g. on x86-64 we handle the two-reg destructive-source insts (add rN, rM --> rN += rM;) the same way, and then the inst.isle wrapper allocates a new vreg for the dest and emits with it and returns it, as you've done, but also takes the two input VRegs. The trick is that in emit, we assert that rs1 == rd (the reuse-constraint will guarantee this), then emit with rs1/rd and rs2 as the two args.

Happy to review and land that change if you'd like now, so it's available for later (especially now that we've discussed the semantics). It might be nice to ensure safety via types and have two Op enums -- one for the Move form and one for the Mod form. I guess the latter only has one arm (?) but that's fine...

Wasmtime GitHub notifications bot (Aug 25 2026 at 22:06):

Rafferty97 updated PR #14187.

Wasmtime GitHub notifications bot (Aug 25 2026 at 22:08):

:memo: Rafferty97 submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2026 at 22:08):

:speech_balloon: Rafferty97 created PR review comment:

@cfallin Perfect, I've now split BitfieldMove into BitfieldMove and BitfieldMoveMod, with the former representing UBFM and SBFM, and the latter just BFM`.

Wasmtime GitHub notifications bot (Aug 25 2026 at 22:09):

Rafferty97 requested cfallin for a review on PR #14187.

Wasmtime GitHub notifications bot (Aug 25 2026 at 22:11):

Rafferty97 commented on PR #14187:

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?

Wasmtime GitHub notifications bot (Aug 25 2026 at 22:25):

cfallin commented on PR #14187:

@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 :-)

Wasmtime GitHub notifications bot (Aug 25 2026 at 22:27):

:thumbs_up: cfallin submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (Aug 25 2026 at 22:27):

cfallin added PR #14187 aarch64: lower ishl + ushr/sshr pairs to a single ubfm/sbfm instruction to the merge queue.

Wasmtime GitHub notifications bot (Aug 25 2026 at 22:55):

github-merge-queue[bot] removed PR #14187 aarch64: lower ishl + ushr/sshr pairs to a single ubfm/sbfm instruction from the merge queue.

Wasmtime GitHub notifications bot (Aug 25 2026 at 23:01):

Rafferty97 commented on PR #14187:

@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 :)

Wasmtime GitHub notifications bot (Aug 25 2026 at 23:18):

cfallin commented on PR #14187:

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.

Wasmtime GitHub notifications bot (Aug 26 2026 at 06:58):

Rafferty97 updated PR #14187.

Wasmtime GitHub notifications bot (Aug 26 2026 at 07:00):

Rafferty97 commented on PR #14187:

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?

Wasmtime GitHub notifications bot (Aug 26 2026 at 07:07):

Rafferty97 requested cfallin for a review on PR #14187.

Wasmtime GitHub notifications bot (Aug 26 2026 at 14:07):

avanhatt commented on PR #14187:

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!

Wasmtime GitHub notifications bot (Aug 26 2026 at 14:11):

avanhatt commented on PR #14187:

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.

Wasmtime GitHub notifications bot (Aug 26 2026 at 14:11):

avanhatt edited a comment on PR #14187:

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.

Wasmtime GitHub notifications bot (Aug 26 2026 at 16:08):

Rafferty97 commented on PR #14187:

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

Wasmtime GitHub notifications bot (Aug 26 2026 at 16:12):

Rafferty97 updated PR #14187.

Wasmtime GitHub notifications bot (Aug 26 2026 at 16:19):

Rafferty97 commented on PR #14187:

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?

Wasmtime GitHub notifications bot (Aug 26 2026 at 23:07):

Rafferty97 commented on PR #14187:

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.

Wasmtime GitHub notifications bot (Aug 27 2026 at 01:37):

Rafferty97 commented on PR #14187:

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: https://github.com/Rafferty97/wasmtime/commit/dc043ad03233f513fd7fbf977e87e68cf2a74d8e

Wasmtime GitHub notifications bot (Aug 27 2026 at 02:09):

avanhatt commented on PR #14187:

@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.

Wasmtime GitHub notifications bot (Aug 27 2026 at 02:09):

avanhatt commented on PR #14187:

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.

Wasmtime GitHub notifications bot (Aug 27 2026 at 02:12):

Rafferty97 updated PR #14187.

Wasmtime GitHub notifications bot (Aug 27 2026 at 02:14):

Rafferty97 commented on PR #14187:

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 :)

Wasmtime GitHub notifications bot (Aug 27 2026 at 02:18):

Rafferty97 commented on PR #14187:

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.

Last updated: Aug 30 2026 at 10:08 UTC