cranelift: Optimize select_spectre_guard, carefully (#8139)

* cranelift: Optimize select_spectre_guard, carefully

This commit makes two changes to our treatment of
`select_spectre_guard`.

First, stop annotating this instruction as having any side effects. We
only care that if its value result is used, then it's computed without
branching on the condition input. We don't otherwise care when the value
is computed, or if it's computed at all.

Second, introduce some carefully selected ISLE egraph rewrites for this
instruction. These particular rewrites are those where we can statically
determine which SSA value will be the result of the instruction. Since
there is no actual choice involved, there's no way to accidentally
introduce speculation on the condition input.

* Add filetests

cranelift/codegen/build.rs

@@ -231,8 +231,9 @@ fn get_isle_compilations(
                     src_opts.join("icmp.isle"),
                     src_opts.join("remat.isle"),
                     src_opts.join("selects.isle"),
-                    src_opts.join("spaceship.isle"),
                     src_opts.join("shifts.isle"),
+                    src_opts.join("spaceship.isle"),
+                    src_opts.join("spectre.isle"),
                     src_opts.join("vector.isle"),
                 ],
                 untracked_inputs: vec![clif_opt_isle],

cranelift/codegen/meta/src/shared/instructions.rs

@@ -1449,19 +1449,36 @@ pub(crate) fn define(
             Conditional select intended for Spectre guards.
 
             This operation is semantically equivalent to a select instruction.
-            However, it is guaranteed to not be removed or otherwise altered by any
-            optimization pass, and is guaranteed to result in a conditional-move
-            instruction, not a branch-based lowering.  As such, it is suitable
-            for use when producing Spectre guards. For example, a bounds-check
-            may guard against unsafe speculation past a bounds-check conditional
-            branch by passing the address or index to be accessed through a
-            conditional move, also gated on the same condition. Because no
-            Spectre-vulnerable processors are known to perform speculation on
-            conditional move instructions, this is guaranteed to pick the
-            correct input. If the selected input in case of overflow is a "safe"
-            value, for example a null pointer that causes an exception in the
-            speculative path, this ensures that no Spectre vulnerability will
-            exist.
+            However, this instruction prohibits all speculation on the
+            controlling value when determining which input to use as the result.
+            As such, it is suitable for use in Spectre guards.
+
+            For example, on a target which may speculatively execute branches,
+            the lowering of this instruction is guaranteed to not conditionally
+            branch. Instead it will typically lower to a conditional move
+            instruction. (No Spectre-vulnerable processors are known to perform
+            value speculation on conditional move instructions.)
+
+            Ensure that the instruction you're trying to protect from Spectre
+            attacks has a data dependency on the result of this instruction.
+            That prevents an out-of-order CPU from evaluating that instruction
+            until the result of this one is known, which in turn will be blocked
+            until the controlling value is known.
+
+            Typical usage is to use a bounds-check as the controlling value,
+            and select between either a null pointer if the bounds-check
+            fails, or an in-bounds address otherwise, so that dereferencing
+            the resulting address with a load or store instruction will trap if
+            the bounds-check failed. When this instruction is used in this way,
+            any microarchitectural side effects of the memory access will only
+            occur after the bounds-check finishes, which ensures that no Spectre
+            vulnerability will exist.
+
+            Optimization opportunities for this instruction are limited compared
+            to a normal select instruction, but it is allowed to be replaced
+            by other values which are functionally equivalent as long as doing
+            so does not introduce any new opportunities to speculate on the
+            controlling value.
             "#,
             &formats.ternary,
         )
@@ -1470,11 +1487,7 @@ pub(crate) fn define(
             Operand::new("x", Any).with_doc("Value to use when `c` is true"),
             Operand::new("y", Any).with_doc("Value to use when `c` is false"),
         ])
-        .operands_out(vec![Operand::new("a", Any)])
-        .other_side_effects()
-        // We can de-duplicate spectre selects since the side effect is
-        // idempotent.
-        .side_effects_idempotent(),
+        .operands_out(vec![Operand::new("a", Any)]),
     );
 
     ig.push(

cranelift/codegen/src/opts/spectre.isle

@@ -0,0 +1,14 @@
+;; Rewrites for `select_spectre_guard`. Check these rules carefully! This
+;; instruction prohibits all speculation on the controlling value when
+;; determining which input to use as the result. Rewrites must respect that
+;; requirement.
+
+;; If we statically know which value will be the result, it's safe to just use
+;; that value. No speculation on the controlling value is possible if we simply
+;; don't depend on that value at all.
+(rule (simplify (select_spectre_guard _ _ x x))
+      (subsume x))
+(rule (simplify (select_spectre_guard _ (iconst_u _ (u64_nonzero _)) x _))
+      (subsume x))
+(rule (simplify (select_spectre_guard _ (iconst_u _ 0) _ y))
+      (subsume y))

cranelift/filetests/filetests/egraph/spectre.clif

@@ -0,0 +1,26 @@
+test optimize
+set opt_level=speed
+target x86_64
+
+function %same_value(i8, i64) -> i64 {
+block0(v0: i8, v1: i64):
+    v2 = select_spectre_guard v0, v1, v1
+    return v2
+}
+; check: return v1
+
+function %const_true(i64, i64) -> i64 {
+block0(v0: i64, v1: i64):
+    v2 = iconst.i8 42
+    v3 = select_spectre_guard v2, v0, v1
+    return v3
+}
+; check: return v0
+
+function %const_false(i64, i64) -> i64 {
+block0(v0: i64, v1: i64):
+    v2 = iconst.i8 0
+    v3 = select_spectre_guard v2, v0, v1
+    return v3
+}
+; check: return v1

tests/disas/typed-funcrefs.wat

@@ -151,12 +151,9 @@
 ;;                                     v32 -> v4
 ;;                                     v33 -> v5
 ;; @0048                               v12 = load.i64 notrap aligned v0+72
-;;                                     v62 = iconst.i8 0
-;; @0048                               v15 = iconst.i64 0
 ;;                                     v70 = iconst.i64 8
 ;; @0048                               v14 = iadd v12, v70  ; v70 = 8
-;; @0048                               v16 = select_spectre_guard v62, v15, v14  ; v62 = 0, v15 = 0
-;; @0048                               v17 = load.i64 table_oob aligned table v16
+;; @0048                               v17 = load.i64 table_oob aligned table v14
 ;;                                     v58 = iconst.i64 -2
 ;; @0048                               v18 = band v17, v58  ; v58 = -2
 ;; @0048                               brif v17, block3(v18), block2
@@ -175,22 +172,19 @@
 ;; @004a                               v26 = load.i64 notrap aligned readonly v19+32
 ;; @004a                               v27 = call_indirect sig1, v25(v26, v0, v2, v3, v4, v5)
 ;; @005b                               v38 = load.i64 notrap aligned v0+72
-;;                                     v79 = iconst.i8 0
-;;                                     v80 = iconst.i64 0
 ;;                                     v78 = iconst.i64 16
 ;; @005b                               v40 = iadd v38, v78  ; v78 = 16
-;; @005b                               v42 = select_spectre_guard v79, v80, v40  ; v79 = 0, v80 = 0
-;; @005b                               v43 = load.i64 table_oob aligned table v42
-;;                                     v81 = iconst.i64 -2
-;;                                     v82 = band v43, v81  ; v81 = -2
-;; @005b                               brif v43, block5(v82), block4
+;; @005b                               v43 = load.i64 table_oob aligned table v40
+;;                                     v79 = iconst.i64 -2
+;;                                     v80 = band v43, v79  ; v79 = -2
+;; @005b                               brif v43, block5(v80), block4
 ;;
 ;;                                 block4 cold:
-;;                                     v83 = load.i64 notrap aligned readonly v0+56
-;;                                     v84 = load.i64 notrap aligned readonly v83+72
-;;                                     v85 = iconst.i32 0
+;;                                     v81 = load.i64 notrap aligned readonly v0+56
+;;                                     v82 = load.i64 notrap aligned readonly v81+72
+;;                                     v83 = iconst.i32 0
 ;; @0059                               v34 = iconst.i32 2
-;; @005b                               v50 = call_indirect sig0, v84(v0, v85, v34)  ; v85 = 0, v34 = 2
+;; @005b                               v50 = call_indirect sig0, v82(v0, v83, v34)  ; v83 = 0, v34 = 2
 ;; @005b                               jump block5(v50)
 ;;
 ;;                                 block5(v45: i64):
@@ -227,12 +221,9 @@
 ;;                                     v32 -> v4
 ;;                                     v33 -> v5
 ;; @0075                               v12 = load.i64 notrap aligned v0+72
-;;                                     v62 = iconst.i8 0
-;; @0075                               v15 = iconst.i64 0
 ;;                                     v70 = iconst.i64 8
 ;; @0075                               v14 = iadd v12, v70  ; v70 = 8
-;; @0075                               v16 = select_spectre_guard v62, v15, v14  ; v62 = 0, v15 = 0
-;; @0075                               v17 = load.i64 table_oob aligned table v16
+;; @0075                               v17 = load.i64 table_oob aligned table v14
 ;;                                     v58 = iconst.i64 -2
 ;; @0075                               v18 = band v17, v58  ; v58 = -2
 ;; @0075                               brif v17, block3(v18), block2
@@ -251,22 +242,19 @@
 ;; @0075                               v26 = load.i64 notrap aligned readonly v19+32
 ;; @0075                               v27 = call_indirect sig0, v25(v26, v0, v2, v3, v4, v5)
 ;; @0087                               v38 = load.i64 notrap aligned v0+72
-;;                                     v79 = iconst.i8 0
-;;                                     v80 = iconst.i64 0
 ;;                                     v78 = iconst.i64 16
 ;; @0087                               v40 = iadd v38, v78  ; v78 = 16
-;; @0087                               v42 = select_spectre_guard v79, v80, v40  ; v79 = 0, v80 = 0
-;; @0087                               v43 = load.i64 table_oob aligned table v42
-;;                                     v81 = iconst.i64 -2
-;;                                     v82 = band v43, v81  ; v81 = -2
-;; @0087                               brif v43, block5(v82), block4
+;; @0087                               v43 = load.i64 table_oob aligned table v40
+;;                                     v79 = iconst.i64 -2
+;;                                     v80 = band v43, v79  ; v79 = -2
+;; @0087                               brif v43, block5(v80), block4
 ;;
 ;;                                 block4 cold:
-;;                                     v83 = load.i64 notrap aligned readonly v0+56
-;;                                     v84 = load.i64 notrap aligned readonly v83+72
-;;                                     v85 = iconst.i32 0
+;;                                     v81 = load.i64 notrap aligned readonly v0+56
+;;                                     v82 = load.i64 notrap aligned readonly v81+72
+;;                                     v83 = iconst.i32 0
 ;; @0085                               v34 = iconst.i32 2
-;; @0087                               v50 = call_indirect sig1, v84(v0, v85, v34)  ; v85 = 0, v34 = 2
+;; @0087                               v50 = call_indirect sig1, v82(v0, v83, v34)  ; v83 = 0, v34 = 2
 ;; @0087                               jump block5(v50)
 ;;
 ;;                                 block5(v45: i64):