From 22439f7b398743a8527a6f32e69c232f788a7f5a Mon Sep 17 00:00:00 2001 From: Chris Fallin Date: Fri, 16 Dec 2022 14:18:22 -0800 Subject: [PATCH] support select_spectre_guard and select on i128 conditions on all platforms. (#5460) Fixes #5199. Fixes #5200. Fixes #5452. Fixes #5453. On riscv64, there is apparently an autoconversion from `ValueRegs` to `Reg` that takes just the low register [0], and removing this conversion causes 48 errors. As a result of this, `select` with an `i128` condition was silently miscompiling, testing only the low 64 bits. We should remove this autoconversion to ensure we aren't missing any other silent truncations, but for now this PR just adds the explicit `I128` logic for `select` / `select_spectre_guard`. [0] https://github.com/bytecodealliance/wasmtime/blob/d9fdbfd50e653f93403405e4c4fd56cb77d034ae/cranelift/codegen/src/isa/riscv64/inst.isle#L1762 --- cranelift/codegen/src/isa/aarch64/lower.isle | 22 +++++++++++++++++-- cranelift/codegen/src/isa/riscv64/inst.isle | 11 ++++++++++ cranelift/codegen/src/isa/riscv64/lower.isle | 4 ++-- cranelift/codegen/src/isa/x64/lower.isle | 10 ++++++++- .../filetests/runtests/i128-select.clif | 11 ++++++++++ .../runtests/selectif-spectre-guard.clif | 12 +++++++++- cranelift/fuzzgen/src/function_generator.rs | 16 -------------- 7 files changed, 64 insertions(+), 22 deletions(-) diff --git a/cranelift/codegen/src/isa/aarch64/lower.isle b/cranelift/codegen/src/isa/aarch64/lower.isle index 0e1e97e820..b435a36eb1 100644 --- a/cranelift/codegen/src/isa/aarch64/lower.isle +++ b/cranelift/codegen/src/isa/aarch64/lower.isle @@ -1739,12 +1739,21 @@ (cmp (OperandSize.Size32) rcond (zero_reg)) (Cond.Ne) ty rn rm))) -(rule -3 (lower (has_type ty (select rcond rn rm))) +(rule -3 (lower (has_type ty (select rcond @ (value_type (fits_in_64 _)) rn rm))) (let ((rcond Reg (put_in_reg_zext64 rcond))) (lower_select (cmp (OperandSize.Size64) rcond (zero_reg)) (Cond.Ne) ty rn rm))) +(rule -4 (lower (has_type ty (select rcond @ (value_type $I128) rn rm))) + (let ((c ValueRegs (put_in_regs rcond)) + (c_lo Reg (value_regs_get c 0)) + (c_hi Reg (value_regs_get c 1)) + (rt Reg (orr $I64 c_lo c_hi))) + (lower_select + (cmp (OperandSize.Size64) rt (zero_reg)) + (Cond.Ne) ty rn rm))) + ;;;; Rules for `select_spectre_guard` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; (rule (lower (has_type ty @@ -1761,12 +1770,21 @@ (_ InstOutput (side_effect (csdb)))) dst)) -(rule -1 (lower (has_type ty (select_spectre_guard rcond rn rm))) +(rule -1 (lower (has_type ty (select_spectre_guard rcond @ (value_type (fits_in_64 _)) rn rm))) (let ((rcond Reg (put_in_reg_zext64 rcond))) (lower_select (cmp (OperandSize.Size64) rcond (zero_reg)) (Cond.Ne) ty rn rm))) +(rule -2 (lower (has_type ty (select_spectre_guard rcond @ (value_type $I128) rn rm))) + (let ((c ValueRegs (put_in_regs rcond)) + (c_lo Reg (value_regs_get c 0)) + (c_hi Reg (value_regs_get c 1)) + (rt Reg (orr $I64 c_lo c_hi))) + (lower_select + (cmp (OperandSize.Size64) rt (zero_reg)) + (Cond.Ne) ty rn rm))) + ;;;; Rules for `vconst` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; (rule (lower (has_type (ty_vec128 _) (vconst (u128_from_constant x)))) diff --git a/cranelift/codegen/src/isa/riscv64/inst.isle b/cranelift/codegen/src/isa/riscv64/inst.isle index 4c5df742a7..acaf90f23f 100644 --- a/cranelift/codegen/src/isa/riscv64/inst.isle +++ b/cranelift/codegen/src/isa/riscv64/inst.isle @@ -1899,6 +1899,17 @@ (rule (normalize_cmp_value $I64 r) r) (rule (normalize_cmp_value $I128 r) r) +;; Convert a truthy value, possibly of more than one register (an +;; I128), to one register. If narrower than 64 bits, must have already +;; been masked (e.g. by `normalize_cmp_value`). +(decl truthy_to_reg (Type ValueRegs) Reg) +(rule 1 (truthy_to_reg (fits_in_64 _) regs) + (value_regs_get regs 0)) +(rule 0 (truthy_to_reg $I128 regs) + (let ((lo Reg (value_regs_get regs 0)) + (hi Reg (value_regs_get regs 1))) + (alu_rrr (AluOPRRR.Or) lo hi))) + ;;;;; (rule (lower_branch (brz v @ (value_type ty) _ _) targets) diff --git a/cranelift/codegen/src/isa/riscv64/lower.isle b/cranelift/codegen/src/isa/riscv64/lower.isle index 219f48e902..9209babc32 100644 --- a/cranelift/codegen/src/isa/riscv64/lower.isle +++ b/cranelift/codegen/src/isa/riscv64/lower.isle @@ -612,7 +612,7 @@ ;;;;; Rules for `select`;;;;;;;;; (rule (lower (has_type ty (select c @ (value_type cty) x y))) - (gen_select ty (normalize_cmp_value cty c) x y)) + (gen_select ty (truthy_to_reg cty (normalize_cmp_value cty c)) x y)) (rule 1 (lower (has_type ty (select (icmp cc a b) x y))) @@ -843,7 +843,7 @@ (rule -1 (lower (has_type ty (select_spectre_guard c @ (value_type cty) x y))) - (gen_select ty (normalize_cmp_value cty c) x y)) + (gen_select ty (truthy_to_reg cty (normalize_cmp_value cty c)) x y)) ;;;;; Rules for `bmask`;;;;;;;;; (rule diff --git a/cranelift/codegen/src/isa/x64/lower.isle b/cranelift/codegen/src/isa/x64/lower.isle index 0d31fc08dc..2e5adea32c 100644 --- a/cranelift/codegen/src/isa/x64/lower.isle +++ b/cranelift/codegen/src/isa/x64/lower.isle @@ -1749,6 +1749,10 @@ (gpr_c Gpr (put_in_gpr c))) (with_flags (x64_test size gpr_c gpr_c) (cmove_from_values ty (CC.NZ) x y)))) +(rule -2 (lower (has_type ty (select c @ (value_type $I128) x y))) + (let ((cond_result IcmpCondResult (cmp_zero_i128 (CC.Z) c))) + (select_icmp cond_result x y))) + ;; Rules for `clz` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; If available, we can use a plain lzcnt instruction here. Note no @@ -2954,7 +2958,11 @@ (rule -1 (lower (has_type ty (select_spectre_guard c @ (value_type (fits_in_64 a_ty)) x y))) (let ((size OperandSize (raw_operand_size_of_type a_ty)) (gpr_c Gpr (put_in_gpr c))) - (with_flags (x64_test size gpr_c gpr_c) (cmove_from_values ty (CC.NZ) x y)))) + (with_flags (x64_test size gpr_c gpr_c) (cmove_from_values ty (CC.NZ) x y)))) + +(rule -2 (lower (has_type ty (select_spectre_guard c @ (value_type $I128) x y))) + (let ((cond_result IcmpCondResult (cmp_zero_i128 (CC.Z) c))) + (select_icmp cond_result x y))) ;; Rules for `fcvt_from_sint` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/cranelift/filetests/filetests/runtests/i128-select.clif b/cranelift/filetests/filetests/runtests/i128-select.clif index e29cb8119f..e61534f086 100644 --- a/cranelift/filetests/filetests/runtests/i128-select.clif +++ b/cranelift/filetests/filetests/runtests/i128-select.clif @@ -24,3 +24,14 @@ block0(v0: f32, v1: i128, v2: i128): } ; run: %i128_fcmp_eq_select(0x42.42, 1, 0) == 1 ; run: %i128_fcmp_eq_select(NaN, 1, 0) == 0 + +function %i128_cond_select(i128, i128, i128) -> i128 { +block0(v0: i128, v1: i128, v2: i128): + v3 = select.i128 v0, v1, v2 + return v3 +} +; run: %i128_cond_select(1, 0, 1) == 0 +; run: %i128_cond_select(0, 0, 1) == 1 +; run: %i128_cond_select(1, 0x00000000_00000000_DECAFFFF_C0FFEEEE, 0xFFFFFFFF_FFFFFFFF_C0FFEEEE_DECAFFFF) == 0x00000000_00000000_DECAFFFF_C0FFEEEE +; run: %i128_cond_select(0, 0x00000000_00000000_DECAFFFF_C0FFEEEE, 0xFFFFFFFF_FFFFFFFF_C0FFEEEE_DECAFFFF) == 0xFFFFFFFF_FFFFFFFF_C0FFEEEE_DECAFFFF +; run: %i128_cond_select(0x1_00000000_00000000, 2, 3) == 2 diff --git a/cranelift/filetests/filetests/runtests/selectif-spectre-guard.clif b/cranelift/filetests/filetests/runtests/selectif-spectre-guard.clif index 8baa70e39e..1ce7fb2e43 100644 --- a/cranelift/filetests/filetests/runtests/selectif-spectre-guard.clif +++ b/cranelift/filetests/filetests/runtests/selectif-spectre-guard.clif @@ -1,4 +1,4 @@ -;; the interpreter does not support `select_spectre_guard`. +test interpret test run set enable_llvm_abi_extensions=true target aarch64 @@ -314,3 +314,13 @@ block0(v0: i8, v1: i128, v2: i128): ; run: %select_spectre_guard_i128_sle(127, 32, -1) == -1 ; run: %select_spectre_guard_i128_sle(127, 32, 19000000000000000000) == 19000000000000000000 ; run: %select_spectre_guard_i128_sle(42, 32, 19000000000000000000) == 32 + +function %select_spectre_guard_i128_cond(i128, i128, i128) -> i128 { +block0(v0: i128, v1: i128, v2: i128): + v3 = select_spectre_guard.i128 v0, v1, v2 + return v3 +} +; run: %select_spectre_guard_i128_cond(1, 2, 3) == 2 +; run: %select_spectre_guard_i128_cond(0, 2, 3) == 3 +; run: %select_spectre_guard_i128_cond(18446744073709551616, 2, 3) == 2 +; run: %select_spectre_guard_i128_cond(18446744073709551616, 18446744073709551616, 3) == 18446744073709551616 diff --git a/cranelift/fuzzgen/src/function_generator.rs b/cranelift/fuzzgen/src/function_generator.rs index 4b228f8c7c..f1c0059ea4 100644 --- a/cranelift/fuzzgen/src/function_generator.rs +++ b/cranelift/fuzzgen/src/function_generator.rs @@ -704,9 +704,6 @@ const OPCODE_SIGNATURES: &'static [( #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))] (Opcode::Bitselect, &[I128, I128, I128], &[I128], insert_opcode), // Select - // TODO: Some ops disabled: - // x64: https://github.com/bytecodealliance/wasmtime/issues/5199 - // AArch64: https://github.com/bytecodealliance/wasmtime/issues/5200 (Opcode::Select, &[I8, I8, I8], &[I8], insert_opcode), (Opcode::Select, &[I8, I16, I16], &[I16], insert_opcode), (Opcode::Select, &[I8, I32, I32], &[I32], insert_opcode), @@ -727,20 +724,12 @@ const OPCODE_SIGNATURES: &'static [( (Opcode::Select, &[I64, I32, I32], &[I32], insert_opcode), (Opcode::Select, &[I64, I64, I64], &[I64], insert_opcode), (Opcode::Select, &[I64, I128, I128], &[I128], insert_opcode), - #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))] (Opcode::Select, &[I128, I8, I8], &[I8], insert_opcode), - #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))] (Opcode::Select, &[I128, I16, I16], &[I16], insert_opcode), - #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))] (Opcode::Select, &[I128, I32, I32], &[I32], insert_opcode), - #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))] (Opcode::Select, &[I128, I64, I64], &[I64], insert_opcode), - #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))] (Opcode::Select, &[I128, I128, I128], &[I128], insert_opcode), // SelectSpectreGuard - // TODO: Some ops disabled: - // x64: https://github.com/bytecodealliance/wasmtime/issues/5452 - // AArch64: https://github.com/bytecodealliance/wasmtime/issues/5453 (Opcode::SelectSpectreGuard, &[I8, I8, I8], &[I8], insert_opcode), (Opcode::SelectSpectreGuard, &[I8, I16, I16], &[I16], insert_opcode), (Opcode::SelectSpectreGuard, &[I8, I32, I32], &[I32], insert_opcode), @@ -761,15 +750,10 @@ const OPCODE_SIGNATURES: &'static [( (Opcode::SelectSpectreGuard, &[I64, I32, I32], &[I32], insert_opcode), (Opcode::SelectSpectreGuard, &[I64, I64, I64], &[I64], insert_opcode), (Opcode::SelectSpectreGuard, &[I64, I128, I128], &[I128], insert_opcode), - #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))] (Opcode::SelectSpectreGuard, &[I128, I8, I8], &[I8], insert_opcode), - #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))] (Opcode::SelectSpectreGuard, &[I128, I16, I16], &[I16], insert_opcode), - #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))] (Opcode::SelectSpectreGuard, &[I128, I32, I32], &[I32], insert_opcode), - #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))] (Opcode::SelectSpectreGuard, &[I128, I64, I64], &[I64], insert_opcode), - #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))] (Opcode::SelectSpectreGuard, &[I128, I128, I128], &[I128], insert_opcode), // Fadd (Opcode::Fadd, &[F32, F32], &[F32], insert_opcode),