wasmtime: Remove ALL constant phis

When we're trying to delete block-params that can be replaced by a
single dominating value, we weren't handling some cases.

In particular, if we concluded that a block formal parameter (say, `v3`)
had more than one value, then we believed that any uses of that
parameter (say, defining another formal parameter `v4`) also had more
than one value, and therefore could not be deleted.

However, in such cases we can try using `v3` itself as the definition of
`v4`. If there are no other definitions of `v4`, then it can be deleted.

With this change, if a block has only one predecessor, it is now true
that this pass will delete all of its block params. We couldn't rely on
that property before.

Author: jameysharp

cranelift/codegen/src/remove_constant_phis.rs

@@ -80,25 +80,6 @@ enum AbstractValue {
 }
 
 impl AbstractValue {
-    fn join(self, other: AbstractValue) -> AbstractValue {
-        match (self, other) {
-            // Joining with `None` has no effect
-            (AbstractValue::None, p2) => p2,
-            (p1, AbstractValue::None) => p1,
-            // Joining with `Many` produces `Many`
-            (AbstractValue::Many, _p2) => AbstractValue::Many,
-            (_p1, AbstractValue::Many) => AbstractValue::Many,
-            // The only interesting case
-            (AbstractValue::One(v1), AbstractValue::One(v2)) => {
-                if v1 == v2 {
-                    AbstractValue::One(v1)
-                } else {
-                    AbstractValue::Many
-                }
-            }
-        }
-    }
-
     fn is_one(self) -> bool {
         matches!(self, AbstractValue::One(_))
     }
@@ -200,14 +181,12 @@ impl SolverState {
     }
 
     fn get(&self, actual: Value) -> AbstractValue {
-        *self
-            .absvals
-            .get(&actual)
+        self.maybe_get(actual)
             .unwrap_or_else(|| panic!("SolverState::get: formal param {:?} is untracked?!", actual))
     }
 
-    fn maybe_get(&self, actual: Value) -> Option<&AbstractValue> {
-        self.absvals.get(&actual)
+    fn maybe_get(&self, actual: Value) -> Option<AbstractValue> {
+        self.absvals.get(&actual).copied()
     }
 
     fn set(&mut self, actual: Value, lp: AbstractValue) {
@@ -292,30 +271,50 @@ pub fn do_remove_constant_phis(func: &mut Function, domtree: &mut DominatorTree)
             let src_summary = &summaries[src];
             for edge in &src_summary.dests {
                 assert!(edge.block != entry_block);
-                // By contrast, the dst block must have a summary.  Phase 1
-                // will have only included an entry in `src_summary.dests` if
-                // that branch/jump carried at least one parameter.  So the
-                // dst block does take parameters, so it must have a summary.
+                // Phase 1 will have only saved an edge if that branch/jump
+                // carried at least one parameter. Therefore the dst block does
+                // take parameters, and it must have a summary.
                 let dst_summary = &summaries[edge.block];
                 let dst_formals = &dst_summary.formals;
                 assert_eq!(edge.args.len(), dst_formals.len());
-                for (formal, actual) in dst_formals.iter().zip(edge.args) {
-                    // Find the abstract value for `actual`.  If it is a block
-                    // formal parameter then the most recent abstract value is
-                    // to be found in the solver state.  If not, then it's a
-                    // real value defining point (not a phi), in which case
-                    // return it itself.
-                    let actual_absval = match state.maybe_get(*actual) {
-                        Some(pt) => *pt,
-                        None => AbstractValue::One(*actual),
+                for (&formal, &actual) in dst_formals.iter().zip(edge.args) {
+                    // In case `actual` is itself defined by a block formal
+                    // parameter, look up our current abstract value for that
+                    // formal's definition.
+                    let replacement = match state.maybe_get(actual) {
+                        // If `actual` isn't one of the formal parameters we're
+                        // considering, or we've already proven that there is no
+                        // one value we can substitute for it, then use `actual`
+                        // itself.
+                        None | Some(AbstractValue::Many) => actual,
+
+                        // Otherwise, the evidence we've found so far says
+                        // we can replace `actual` with some other value.
+                        // Assume that hypothesis is true and propagate the
+                        // replacement. We may later prove this false, but we'll
+                        // fix it on later fix-point iterations.
+                        Some(AbstractValue::One(replacement)) => replacement,
+
+                        // Since we visit blocks in reverse post-order, we must
+                        // have visited at least one predecessor of the block
+                        // that defines this formal parameter, and gotten at
+                        // least one actual value for it.
+                        Some(AbstractValue::None) => unreachable!(),
                     };
 
-                    // And `join` the new value with the old.
-                    let formal_absval_old = state.get(*formal);
-                    let formal_absval_new = formal_absval_old.join(actual_absval);
+                    // We have one value for this formal; join it with any
+                    // others we've found previously.
+                    let formal_absval_old = state.get(formal);
+                    let formal_absval_new = match formal_absval_old {
+                        AbstractValue::Many => AbstractValue::Many,
+                        // If the value is different, there are many values.
+                        AbstractValue::One(v) if v != replacement => AbstractValue::Many,
+                        // Otherwise no previous value or it was the same value.
+                        _ => AbstractValue::One(replacement),
+                    };
                     if formal_absval_new != formal_absval_old {
                         changed = true;
-                        state.set(*formal, formal_absval_new);
+                        state.set(formal, formal_absval_new);
                     }
                 }
             }

cranelift/filetests/filetests/remove-constant-phis/maximal.clif

@@ -0,0 +1,33 @@
+test optimize precise-output
+target x86_64
+
+function u0:0(i32, i32, i32) -> i32 {
+block0(v0: i32, v1: i32, v2: i32):
+    brif v0, block1(v1), block1(v2)
+
+block1(v3: i32):
+    brif v0, block3(v3), block2(v3)
+
+block2(v4: i32):
+    jump block3(v4)
+
+block3(v5: i32):
+    return v5
+}
+
+; function u0:0(i32, i32, i32) -> i32 fast {
+; block0(v0: i32, v1: i32, v2: i32):
+;     brif v0, block1(v1), block1(v2)
+;
+; block1(v3: i32):
+;     v4 -> v3
+;     v5 -> v3
+;     brif.i32 v0, block3, block2
+;
+; block2:
+;     jump block3
+;
+; block3:
+;     return v5
+; }
+