aarch64: lower ishl + ushr/sshr pairs to a single ubfm/sbfm instruction

[Rafferty97]

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.

[github-actions]

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]

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!

[Rafferty97]

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);
            }
        },

[cfallin]

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

[Rafferty97]

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

[cfallin]

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