Skip to content

x64: Fix panic compiling 16-bit multiply-with-immediate #10817

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 20, 2025
Merged

x64: Fix panic compiling 16-bit multiply-with-immediate #10817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b226c23
x64: Fix panic compiling 16-bit multiply-with-immediate
alexcrichton May 20, 2025
34c77c6
Add some more tests
alexcrichton May 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cranelift/codegen/src/isa/x64/inst.isle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2884,9 +2884,9 @@
(rule 2 (x64_imul_imm $I16 src1 (i8_try_from_i32 src2)) (x64_imulw_rmi_sxb src1 src2))
(rule 2 (x64_imul_imm $I32 src1 (i8_try_from_i32 src2)) (x64_imull_rmi_sxb src1 src2))
(rule 2 (x64_imul_imm $I64 src1 (i8_try_from_i32 src2)) (x64_imulq_rmi_sxb src1 src2))
(rule 1 (x64_imul_imm $I16 src1 (u16_try_from_i32 src2)) (x64_imulw_rmi src1 src2))
(rule 1 (x64_imul_imm $I32 src1 (i32_as_u32 src2)) (x64_imull_rmi src1 src2))
(rule 1 (x64_imul_imm $I64 src1 src2) (x64_imulq_rmi_sxl src1 src2))
(rule 1 (x64_imul_imm $I16 src1 (i16_try_from_i32 src2)) (x64_imulw_rmi src1 (i16_as_u16 src2)))
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll admit this is testing the limits of my CLIF knowledge. This will fail when the i32 value fits in a u16, but not an i16, meaning that we'll still panic during instruction selection from that. I don't know whether that's possible in CLIF though, or if iconst values are always sign-extended from their type.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, missed that, sorry -- unfortunately we've standardized on zero extension instead (it keeps the majority of cases simpler). Fortunately if this extractor returns None then the rule just doesn't match -- no runtime panic unless there's not another fallback? Let's (i) add a test case with a value in that range (say 40000) and (ii) verify there is...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I've added a few CLIF tests for this and it looks like things are actually working as intended. Looks like when accessing the constant in ISLE it gets sign-extended based on the type of the constant itself, which is why this ended up working out. Yay!

(rule 1 (x64_imul_imm $I32 src1 src2) (x64_imull_rmi src1 (i32_as_u32 src2)))
(rule 1 (x64_imul_imm $I64 src1 src2) (x64_imulq_rmi_sxl src1 src2))

;; Helper for creating `mul` instructions or `imul` instructions (depending
;; on `signed`) for 8-bit operands.
Expand Down
12 changes: 10 additions & 2 deletions cranelift/codegen/src/isle_prelude.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ macro_rules! isle_common_prelude_methods {
}

#[inline]
fn i32_as_u32(&mut self, x: i32) -> Option<u32> {
Some(x as u32)
fn i32_as_u32(&mut self, x: i32) -> u32 {
x as u32
}

#[inline]
Expand Down Expand Up @@ -958,6 +958,14 @@ macro_rules! isle_common_prelude_methods {
i8::try_from(val).ok()
}

fn i16_as_u16(&mut self, val: i16) -> u16 {
val as u16
}

fn i16_try_from_i32(&mut self, val: i32) -> Option<i16> {
i16::try_from(val).ok()
}

fn i16_try_from_u64(&mut self, val: u64) -> Option<i16> {
i16::try_from(val).ok()
}
Expand Down
10 changes: 8 additions & 2 deletions cranelift/codegen/src/prelude.isle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,18 +142,24 @@
(decl pure partial i8_try_from_u64 (u64) i8)
(extern constructor i8_try_from_u64 i8_try_from_u64)

(decl pure i16_as_u16 (i16) u16)
(extern constructor i16_as_u16 i16_as_u16)

(decl pure partial i16_try_from_u64 (u64) i16)
(extern constructor i16_try_from_u64 i16_try_from_u64)

(decl pure partial i16_try_from_i32 (i16) i32)
(extern extractor i16_try_from_i32 i16_try_from_i32)

(decl pure partial i32_try_from_u64 (u64) i32)
(extern constructor i32_try_from_u64 i32_try_from_u64)

(decl pure u32_as_u64 (u32) u64)
(extern constructor u32_as_u64 u32_as_u64)
(convert u32 u64 u32_as_u64)

(decl i32_as_u32 (u32) i32)
(extern extractor i32_as_u32 i32_as_u32)
(decl i32_as_u32 (i32) u32)
(extern constructor i32_as_u32 i32_as_u32)

(decl pure i32_as_i64 (i32) i64)
(extern constructor i32_as_i64 i32_as_i64)
Expand Down
31 changes: 31 additions & 0 deletions cranelift/filetests/filetests/isa/x64/mul-with-optimizations.clif
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
test compile precise-output
set unwind_info=false
set opt_level=speed
target x86_64

function %imul_i16_const_unsigned_but_big(i32) -> i16 {
block0(v0: i32):
v3 = imul_imm v0, 0x81111
v4 = ireduce.i16 v3
return v4
}

; VCode:
; pushq %rbp
; movq %rsp, %rbp
; block0:
; imulw $0x1111, %di, %ax
; movq %rbp, %rsp
; popq %rbp
; ret
;
; Disassembled:
; block0: ; offset 0x0
; pushq %rbp
; movq %rsp, %rbp
; block1: ; offset 0x4
; imulw $0x1111, %di, %ax
; movq %rbp, %rsp
; popq %rbp
; retq

75 changes: 75 additions & 0 deletions cranelift/filetests/filetests/isa/x64/mul.clif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,81 @@ block0(v0: i16):
; popq %rbp
; retq

function %imul_i16_const_negative(i16) -> i16 {
block0(v0: i16):
v3 = imul_imm v0, -97
return v3
}

; VCode:
; pushq %rbp
; movq %rsp, %rbp
; block0:
; imulw $0xff9f, %di, %ax
; movq %rbp, %rsp
; popq %rbp
; ret
;
; Disassembled:
; block0: ; offset 0x0
; pushq %rbp
; movq %rsp, %rbp
; block1: ; offset 0x4
; imulw $-0x61, %di, %ax
; movq %rbp, %rsp
; popq %rbp
; retq

function %imul_i16_const_unsigned_but_big(i16) -> i16 {
block0(v0: i16):
v3 = imul_imm v0, 0x8000
return v3
}

; VCode:
; pushq %rbp
; movq %rsp, %rbp
; block0:
; imulw $0x8000, %di, %ax
; movq %rbp, %rsp
; popq %rbp
; ret
;
; Disassembled:
; block0: ; offset 0x0
; pushq %rbp
; movq %rsp, %rbp
; block1: ; offset 0x4
; imulw $0x8000, %di, %ax
; movq %rbp, %rsp
; popq %rbp
; retq

function %imul_i16_const_out_of_bounds(i16) -> i16 {
block0(v0: i16):
v3 = imul_imm v0, 0x80000
return v3
}

; VCode:
; pushq %rbp
; movq %rsp, %rbp
; block0:
; imulw $0x0, %di, %ax
; movq %rbp, %rsp
; popq %rbp
; ret
;
; Disassembled:
; block0: ; offset 0x0
; pushq %rbp
; movq %rsp, %rbp
; block1: ; offset 0x4
; imulw $0, %di, %ax
; movq %rbp, %rsp
; popq %rbp
; retq

function %imul_i32_const(i32) -> i32{
block0(v0: i32):
v3 = imul_imm v0, 97
Expand Down
19 changes: 19 additions & 0 deletions tests/disas/x64-mul16-negative.wat
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
;;! target = 'x86_64'
;;! test = 'compile'

(module
(func (export "mul16") (param i32) (result i32)
local.get 0
i32.const -7937
i32.mul
i32.extend16_s
)
)
;; wasm[0]::function[0]:
;; pushq %rbp
;; movq %rsp, %rbp
;; imulw $0xe0ff, %dx, %dx
;; movswl %dx, %eax
;; movq %rbp, %rsp
;; popq %rbp
;; retq
10 changes: 10 additions & 0 deletions tests/misc_testsuite/mul16-negative.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
(module
(func (export "mul16") (param i32) (result i32)
local.get 0
i32.const -7937
i32.mul
i32.extend16_s
)
)

(assert_return (invoke "mul16" (i32.const 100)) (i32.const -7268))
Loading