From 27c1c1d74c1ab6faddf072a6c79f8990e795a78c Mon Sep 17 00:00:00 2001 From: Jamey Sharp Date: Mon, 6 May 2024 19:11:57 -0700 Subject: [PATCH] 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. --- cranelift/codegen/src/remove_constant_phis.rs | 81 +++++++++---------- .../remove-constant-phis/maximal.clif | 33 ++++++++ 2 files changed, 73 insertions(+), 41 deletions(-) create mode 100644 cranelift/filetests/filetests/remove-constant-phis/maximal.clif diff --git a/cranelift/codegen/src/remove_constant_phis.rs b/cranelift/codegen/src/remove_constant_phis.rs index 483788e278d6..ed8ac03f51f0 100644 --- a/cranelift/codegen/src/remove_constant_phis.rs +++ b/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 { + 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); } } } diff --git a/cranelift/filetests/filetests/remove-constant-phis/maximal.clif b/cranelift/filetests/filetests/remove-constant-phis/maximal.clif new file mode 100644 index 000000000000..a5dcb3c93e11 --- /dev/null +++ b/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 +; } +