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] d9fdbfd50e/cranelift/codegen/src/isa/riscv64/inst.isle (L1762)
2022-12-16 14:18:22 -08:00
parent d9fdbfd50e
commit 22439f7b39
7 changed files with 64 additions and 22 deletions
--- 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))))
--- 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)
--- 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
--- 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
@@ -2956,6 +2960,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_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` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

 (rule 2 (lower (has_type $F32 (fcvt_from_sint a @ (value_type $I8))))
--- 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
--- 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
--- 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),