Repository: bytecodealliance/wasmtime
Pull request #5729 (2023-02-06 23:51:44)

[6.0.0] Backport "Only build iconst for ints <= 64 bits" (#5723)
This backports #5723 which fixes incorrect optimizations found while auditing `algebraic.isle` for misuse of the `iconst` constructor. No bugs have been reported implicating these optimizations, but this will be the first Cranelift release with egraph optimization enabled by default, so most people wouldn't have run into these bugs sooner.

Since Wasmtime doesn't use the I128 type, it is not affected by the `icmp` fix. I'm not sure whether the `bxor` issue was reachable from WebAssembly.


Patch:
diff --git a/cranelift/codegen/src/egraph/elaborate.rs b/cranelift/codegen/src/egraph/elaborate.rs
index 68f4238cf..d52927ffe 100644
--- a/cranelift/codegen/src/egraph/elaborate.rs
+++ b/cranelift/codegen/src/egraph/elaborate.rs
@@ -278,6 +278,14 @@ impl<'a> Elaborator<'a> {
                         before
                     );
 
+                    // Get the best option; we use `value` (latest
+                    // value) here so we have a full view of the
+                    // eclass.
+                    trace!("looking up best value for {}", value);
+                    let (_, best_value) = self.value_to_best_value[value];
+                    debug_assert_ne!(best_value, Value::reserved_value());
+                    trace!("elaborate: value {} -> best {}", value, best_value);
+
                     let remat = if let Some(elab_val) =
                         self.value_to_elaborated_value.get(&canonical_value)
                     {
@@ -287,7 +295,7 @@ impl<'a> Elaborator<'a> {
                         // from another block. If so, ignore the hit
                         // and recompute below.
                         let remat = elab_val.in_block != self.cur_block
-                            && self.remat_values.contains(&canonical_value);
+                            && self.remat_values.contains(&best_value);
                         if !remat {
                             trace!("elaborate: value {} -> {:?}", value, elab_val);
                             self.stats.elaborate_memoize_hit += 1;
@@ -304,20 +312,12 @@ impl<'a> Elaborator<'a> {
                         // Value not available; but still look up
                         // whether it's been flagged for remat because
                         // this affects placement.
-                        let remat = self.remat_values.contains(&canonical_value);
+                        let remat = self.remat_values.contains(&best_value);
                         trace!(" -> not present in map; remat = {}", remat);
                         remat
                     };
                     self.stats.elaborate_memoize_miss += 1;
 
-                    // Get the best option; we use `value` (latest
-                    // value) here so we have a full view of the
-                    // eclass.
-                    trace!("looking up best value for {}", value);
-                    let (_, best_value) = self.value_to_best_value[value];
-                    debug_assert_ne!(best_value, Value::reserved_value());
-                    trace!("elaborate: value {} -> best {}", value, best_value,);
-
                     // Now resolve the value to its definition to see
                     // how we can compute it.
                     let (inst, result_idx) = match self.func.dfg.value_def(best_value) {
diff --git a/cranelift/codegen/src/opts/algebraic.isle b/cranelift/codegen/src/opts/algebraic.isle
index d889fd8fa..fb856e4cb 100644
--- a/cranelift/codegen/src/opts/algebraic.isle
+++ b/cranelift/codegen/src/opts/algebraic.isle
@@ -101,7 +101,7 @@
       (subsume x))
 
 ;; x ^ x == 0.
-(rule (simplify (bxor (fits_in_64 ty) x x))
+(rule (simplify (bxor (fits_in_64 (ty_int ty)) x x))
       (subsume (iconst ty (imm64 0))))
 
 ;; x ^ not(x) == not(x) ^ x == -1.
@@ -167,6 +167,9 @@
       (iadd ty x x))
 
 ;; x*c == x<<log2(c) when c is a power of two.
+;; Note that the type of `iconst` must be the same as the type of `imul`,
+;; so these rules can only fire in situations where it's safe to construct an
+;; `iconst` of that type.
 (rule (simplify (imul ty x (iconst _ (imm64_power_of_two c))))
       (ishl ty x (iconst ty (imm64 c))))
 (rule (simplify (imul ty (iconst _ (imm64_power_of_two c)) x))
@@ -252,32 +255,32 @@
 ;; `x == x` is always true for integers; `x != x` is false. Strict
 ;; inequalities are false, and loose inequalities are true.
 (rule (simplify
-       (icmp (ty_int ty) (IntCC.Equal) x x))
+       (icmp (fits_in_64 (ty_int ty)) (IntCC.Equal) x x))
       (iconst ty (imm64 1)))
 (rule (simplify
-       (icmp (ty_int ty) (IntCC.NotEqual) x x))
+       (icmp (fits_in_64 (ty_int ty)) (IntCC.NotEqual) x x))
       (iconst ty (imm64 0)))
 (rule (simplify
-       (icmp (ty_int ty) (IntCC.UnsignedGreaterThan) x x))
+       (icmp (fits_in_64 (ty_int ty)) (IntCC.UnsignedGreaterThan) x x))
       (iconst ty (imm64 0)))
 (rule (simplify
-       (icmp (ty_int ty) (IntCC.UnsignedGreaterThanOrEqual) x x))
+       (icmp (fits_in_64 (ty_int ty)) (IntCC.UnsignedGreaterThanOrEqual) x x))
       (iconst ty (imm64 1)))
 (rule (simplify
-       (icmp (ty_int ty) (IntCC.SignedGreaterThan) x x))
+       (icmp (fits_in_64 (ty_int ty)) (IntCC.SignedGreaterThan) x x))
       (iconst ty (imm64 0)))
 (rule (simplify
-       (icmp (ty_int ty) (IntCC.SignedGreaterThanOrEqual) x x))
+       (icmp (fits_in_64 (ty_int ty)) (IntCC.SignedGreaterThanOrEqual) x x))
       (iconst ty (imm64 1)))
 (rule (simplify
-       (icmp (ty_int ty) (IntCC.UnsignedLessThan) x x))
+       (icmp (fits_in_64 (ty_int ty)) (IntCC.UnsignedLessThan) x x))
       (iconst ty (imm64 0)))
 (rule (simplify
-       (icmp (ty_int ty) (IntCC.UnsignedLessThanOrEqual) x x))
+       (icmp (fits_in_64 (ty_int ty)) (IntCC.UnsignedLessThanOrEqual) x x))
       (iconst ty (imm64 1)))
 (rule (simplify
-       (icmp (ty_int ty) (IntCC.SignedLessThan) x x))
+       (icmp (fits_in_64 (ty_int ty)) (IntCC.SignedLessThan) x x))
       (iconst ty (imm64 0)))
 (rule (simplify
-       (icmp (ty_int ty) (IntCC.SignedLessThanOrEqual) x x))
+       (icmp (fits_in_64 (ty_int ty)) (IntCC.SignedLessThanOrEqual) x x))
       (iconst ty (imm64 1)))


Test patch:
diff --git a/cranelift/filetests/filetests/egraph/issue-5716.clif b/cranelift/filetests/filetests/egraph/issue-5716.clif
new file mode 100644
index 000000000..7763d3eac
--- /dev/null
+++ b/cranelift/filetests/filetests/egraph/issue-5716.clif
@@ -0,0 +1,41 @@
+;; This tests that a call does not get rematerialized, even if a remat flag is
+;; set on a different node in its eclass.
+;;
+;; Below, `v97` is an add of `v238` (the call's first return value) and a
+;; constant 0; a mid-end rule rewrites this to just `v238` (i.e., `v97` is unioned
+;; in). Separately, a rule states that an add of a value and a constant always
+;; gets rematerialized at use. When `v97` is used in a later block, it would have
+;; rematerialized the add; except, if we instead use the result of the call
+;; directly, we should *not* remat the call. If we do, a compile error results
+;; later.
+
+test compile
+set opt_level=speed_and_size
+target aarch64
+
+function u0:33() system_v {
+ss0 = explicit_slot 32
+sig0 = (i64, i64, i64, i64, i64) -> i64, i64 system_v
+fn0 = colocated u0:0 sig0
+jt0 = jump_table [block36, block38]
+block0:
+  v80 = iconst.i32 0
+  v91 = iconst.i64 0
+  v92 = iconst.i64 0
+  v96 = iconst.i64 0
+  v235 = iconst.i64 0
+  v236 = iconst.i64 0
+  v237 = iconst.i64 0
+  v238, v239 = call fn0(v236, v237, v91, v92, v235) ; v236 = 0, v237 = 0, v91 = 0, v92 = 0, v235 = 0
+  v97 = iadd v238, v96 ; v96 = 0
+  br_table v80, block37, jt0 ; v80 = 0
+block36:
+  trap user0
+block37:
+  trap unreachable
+block38:
+  v98 = load.i8 notrap v97
+  v99 = fcvt_from_uint.f64 v98
+  stack_store v99, ss0
+  trap user0
+}