diff --git a/cranelift/codegen/src/isa/x64/lower.isle b/cranelift/codegen/src/isa/x64/lower.isle index 7940f326ab1e..33e976fa3ffe 100644 --- a/cranelift/codegen/src/isa/x64/lower.isle +++ b/cranelift/codegen/src/isa/x64/lower.isle @@ -1098,8 +1098,8 @@ ;; 8-bit base case, needs a special instruction encoding and additionally ;; move sinkable loads to the right. -(rule -8 (lower (imul $I8 x y)) (x64_mul8 false x y)) -(rule -7 (lower (imul $I8 (sinkable_load x) y)) (x64_mul8 false y x)) +(rule -10 (lower (imul $I8 x y)) (x64_mul8 false x y)) +(rule -9 (lower (imul $I8 (sinkable_load x) y)) (x64_mul8 false y x)) ;; 16-to-64-bit base cases, same as above by moving sinkable loads to the right. (rule -6 (lower (imul (ty_int_ref_16_to_64 ty) x y)) @@ -1113,6 +1113,14 @@ (rule -3 (lower (imul (ty_int_ref_16_to_64 ty) (i32_from_iconst x) y)) (x64_imul_imm ty y x)) +;; i8 multiplication can use a 32-bit immediate multiply: only the low byte of +;; each operand affects the low byte of the result. Force the other operand into +;; a register so that a sinkable i8 load is never widened to a 32-bit load. +(rule -8 (lower (imul $I8 x (i32_from_iconst y))) + (x64_imul_imm $I32 (put_in_gpr x) y)) +(rule -7 (lower (imul $I8 (i32_from_iconst x) y)) + (x64_imul_imm $I32 (put_in_gpr y) x)) + ;; Special case widening multiplication from 8-to-16-bits with a single ;; instruction since the 8-bit-multiply places both the high and low halves in ;; the same register diff --git a/cranelift/filetests/filetests/isa/x64/imul-i8-const.clif b/cranelift/filetests/filetests/isa/x64/imul-i8-const.clif new file mode 100644 index 000000000000..731b365f8b9a --- /dev/null +++ b/cranelift/filetests/filetests/isa/x64/imul-i8-const.clif @@ -0,0 +1,82 @@ +test compile +set unwind_info=false +target x86_64 +set opt_level=speed +target x86_64 + +;; Regression test for #14660: use a 32-bit immediate multiply for an i8 +;; constant on either side, without a byte multiply or constant-pool load. + +function %imul_i8_const_rhs(i8) -> i8 { +block0(v0: i8): + v1 = iconst.i8 97 + v2 = imul v0, v1 + return v2 +} + +; not: mulb +; not: %rip +; check: block0: +; nextln: imull $$0x61, %edi, %eax +; nextln: movq %rbp, %rsp +; nextln: popq %rbp +; nextln: retq +; not: mulb +; not: %rip + +function %imul_i8_const_lhs(i8) -> i8 { +block0(v0: i8): + v1 = iconst.i8 97 + v2 = imul v1, v0 + return v2 +} + +; not: mulb +; not: %rip +; check: block0: +; nextln: imull $$0x61, %edi, %eax +; nextln: movq %rbp, %rsp +; nextln: popq %rbp +; nextln: retq +; not: mulb +; not: %rip + +;; The i8 load must stay a byte load. In particular, do not fold it into +;; a 32-bit memory operand of imull, which would read past the input byte. +function %imul_i8_load_const_rhs(i64) -> i8 { +block0(v0: i64): + v1 = load.i8 v0 + v2 = iconst.i8 97 + v3 = imul v1, v2 + return v3 +} + +; not: mulb +; not: %rip +; check: block0: +; nextln: movzb$(=q|l) (%rdi), %$(=[a-z0-9]+) +; nextln: imull $$0x61, %$(=[a-z0-9]+), %eax +; nextln: movq %rbp, %rsp +; nextln: popq %rbp +; nextln: retq +; not: mulb +; not: %rip + +function %imul_i8_load_const_lhs(i64) -> i8 { +block0(v0: i64): + v1 = load.i8 v0 + v2 = iconst.i8 97 + v3 = imul v2, v1 + return v3 +} + +; not: mulb +; not: %rip +; check: block0: +; nextln: movzb$(=q|l) (%rdi), %$(=[a-z0-9]+) +; nextln: imull $$0x61, %$(=[a-z0-9]+), %eax +; nextln: movq %rbp, %rsp +; nextln: popq %rbp +; nextln: retq +; not: mulb +; not: %rip diff --git a/cranelift/filetests/filetests/isa/x64/mul.clif b/cranelift/filetests/filetests/isa/x64/mul.clif index a5e8aad798e2..51f301287a36 100644 --- a/cranelift/filetests/filetests/isa/x64/mul.clif +++ b/cranelift/filetests/filetests/isa/x64/mul.clif @@ -237,8 +237,7 @@ block0(v0: i8): ; pushq %rbp ; movq %rsp, %rbp ; block0: -; movq %rdi, %rax -; mulb (%rip) ;; implicit: %ax +; imull $0x61, %edi, %eax ; movq %rbp, %rsp ; popq %rbp ; retq @@ -248,17 +247,10 @@ block0(v0: i8): ; pushq %rbp ; movq %rsp, %rbp ; block1: ; offset 0x4 -; movq %rdi, %rax -; mulb 0xa(%rip) +; imull $0x61, %edi, %eax ; movq %rbp, %rsp ; popq %rbp ; retq -; addb %al, (%rax) -; addb %al, (%rax) -; addb %ah, (%rcx) -; addb %al, (%rax) -; addb %al, (%rax) -; addb %al, (%rax) function %imul_i16_const(i16) -> i16{ block0(v0: i16): @@ -699,4 +691,3 @@ block0: ; andb (%rax), %al ; addb %al, (%rax) ; .byte 0x00 - diff --git a/cranelift/filetests/filetests/runtests/imul-i8-const.clif b/cranelift/filetests/filetests/runtests/imul-i8-const.clif new file mode 100644 index 000000000000..ca25530870f8 --- /dev/null +++ b/cranelift/filetests/filetests/runtests/imul-i8-const.clif @@ -0,0 +1,71 @@ +test interpret +test run +target x86_64 +set opt_level=speed +target x86_64 + +;; Regression test for #14660: widening an i8 constant multiply to i32 must +;; preserve wrapping i8 results, including negative and unsigned constants. + +function %imul_i8_const_rhs(i8) -> i8 { +block0(v0: i8): + v1 = iconst.i8 97 + v2 = imul v0, v1 + return v2 +} +; run: %imul_i8_const_rhs(0) == 0 +; run: %imul_i8_const_rhs(2) == 0xc2 +; run: %imul_i8_const_rhs(0x7f) == 0x1f +; run: %imul_i8_const_rhs(0x80) == 0x80 +; run: %imul_i8_const_rhs(0xff) == 0x9f + +function %imul_i8_const_lhs(i8) -> i8 { +block0(v0: i8): + v1 = iconst.i8 193 + v2 = imul v1, v0 + return v2 +} +; run: %imul_i8_const_lhs(1) == 0xc1 +; run: %imul_i8_const_lhs(2) == 0x82 +; run: %imul_i8_const_lhs(0x7f) == 0xbf +; run: %imul_i8_const_lhs(0xff) == 0x3f + +;; ireduce can leave junk in the register's high bits. Only the low byte of +;; the product may survive the uextend, even when that byte has its sign bit set. +function %imul_i8_ireduce_negative(i64) -> i32 { +block0(v0: i64): + v1 = ireduce.i8 v0 + v2 = iconst.i8 -97 + v3 = imul v1, v2 + v4 = uextend.i32 v3 + return v4 +} +; run: %imul_i8_ireduce_negative(0x1234_5678_9abc_de00) == 0 +; run: %imul_i8_ireduce_negative(0x1234_5678_9abc_de01) == 159 +; run: %imul_i8_ireduce_negative(0x1234_5678_9abc_de02) == 62 +; run: %imul_i8_ireduce_negative(0x1234_5678_9abc_de80) == 128 +; run: %imul_i8_ireduce_negative(-1) == 97 + +function %imul_i8_loaded(i64) -> i8 { +block0(v0: i64): + v1 = load.i8 v0 + v2 = iconst.i8 193 + v3 = imul v2, v1 + return v3 +} + +;; Use a separate function so store-to-load forwarding cannot remove the load. +;; The compile test also checks that the memory access is not widened. +function %imul_i8_load(i8) -> i8 { + ss0 = explicit_slot 1 + fn0 = %imul_i8_loaded(i64) -> i8 + +block0(v0: i8): + v1 = stack_addr.i64 ss0 + store v0, v1 + v2 = call fn0(v1) + return v2 +} +; run: %imul_i8_load(2) == 0x82 +; run: %imul_i8_load(0x7f) == 0xbf +; run: %imul_i8_load(0xff) == 0x3f