Skip to content

Conversation

khagankhan
Copy link
Contributor

This adds the missed rotate optimizations in cranelift mid-end opened in #11722

(module
  (func $main (export "main") (result i32)
    ;; Step 1: (i32.rotl (i32.const 1) (i32.const 1))
    ;; 1 in binary (32-bit) = 0b...0001
    ;; Rotate left by 1 bit = 0b...0010 = 2
    (i32.rotl (i32.const 1) (i32.const 1))

    ;; Step 2: (i32.rotr (i32.const 10) (i32.const 1))
    ;; 10 in binary = 0b...1010
    ;; Rotate right by 1 bit = 0b...0101 = 5
    (i32.rotr (i32.const 10) (i32.const 1))

    ;; Step 3: Add the results
    ;; 2 + 5 = 7
    (i32.add)
  )
)

Before PR:

;; Intermediate Representation of function <wasm[0]::function[0]::main>:
function u0:0(i64 vmctx, i64) -> i32 tail {
    gv0 = vmctx
    gv1 = load.i64 notrap aligned readonly gv0+8
    gv2 = load.i64 notrap aligned gv1+16
    stack_limit = gv2

                                block0(v0: i64, v1: i64):
@002d                               jump block1

                                block1:
@0022                               v3 = iconst.i32 1
@0026                               v5 = rotl v3, v3  ; v3 = 1, v3 = 1
@0027                               v6 = iconst.i32 10
@002b                               v8 = rotr v6, v3  ; v6 = 10, v3 = 1
@002c                               v9 = iadd v5, v8
@002d                               return v9
}

After PR:

;; Intermediate Representation of function <wasm[0]::function[0]::main>:
function u0:0(i64 vmctx, i64) -> i32 tail {
    gv0 = vmctx
    gv1 = load.i64 notrap aligned readonly gv0+8
    gv2 = load.i64 notrap aligned gv1+16
    stack_limit = gv2

                                block0(v0: i64, v1: i64):
@002d                               jump block1

                                block1:
                                    v12 = iconst.i32 7
@002d                               return v12  ; v12 = 7
}

@khagankhan khagankhan requested a review from a team as a code owner September 19, 2025 23:56
@khagankhan khagankhan requested review from alexcrichton and removed request for a team September 19, 2025 23:56
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language labels Sep 20, 2025
Copy link

Subscribe to Label Action

cc @cfallin, @fitzgen

This issue or pull request has been labeled: "cranelift", "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.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Mind adding some tests for this too? They'd go in cranelift/filetests/filetests/egraph for testing the IR is updated appropriately and cranelift/filetests/filetests/runtests for ensuring that the behavior matches the interpreter.


#[inline]
fn imm64_rotl(&mut self, ty: Type, x: Imm64, k: Imm64) -> Imm64 {
let bw: u32 = ty.bits().min(64);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The min here should be an assert of some form because if ty.bits() is >=64 then that's an invalid state for this function to be in and it should panic.

#[inline]
fn imm64_rotl(&mut self, ty: Type, x: Imm64, k: Imm64) -> Imm64 {
let bw: u32 = ty.bits().min(64);
let amt: u32 = if bw == 0 { 0 } else { (k.bits() as u32) % bw };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok to not test for 0 here because a 0-size integral type should cause a panic (which % 0 would do I believe). Handling it here would accidentally try to recover from what should otherwise be a panicking situation.

let bw: u32 = ty.bits().min(64);
let amt: u32 = if bw == 0 { 0 } else { (k.bits() as u32) % bw };

let xv = x.bits() as u64;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend using cast_unsigned here as it avoid using as which is something we try to avoid since it's by default a lossy cast.

Comment on lines +37 to +39
8 => (xv as u8).rotate_left(amt) as u64,
16 => (xv as u16).rotate_left(amt) as u64,
32 => (xv as u32).rotate_left(amt) as u64,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For promotion back up to u64 I'd recommend using u64::from((xv as u8).rotate_left(amt)) to avoid as u64. The as u8 is still required, however, as this is intentionally truncating data.

@khagankhan
Copy link
Contributor Author

Sure thing! @alexcrichton Just made PR to get initial thoughts. I will address the comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants