From ee738ac1f838a090cac74ba8981e2104b6c02d44 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 9 May 2023 11:17:18 -0700 Subject: Remove TypeUpdater in Vacuum and always ReFinalize (#5707) TypeUpdater::remove must be called after removing a thing from the tree. If not, then we can get confused by something like this: (block $b (br $b) ) If we first call TypeUpdater::remove then we see that the block's only br is going away, so it becomes unreachable. But when we then remove the br then the block should have type none. Removing the br first from the IR, and then calling TypeUpdater::remove, is the safe way to do it. However, changing that order in Vacuum is not trivial. After looking into this, I realized that it is just simpler to remove TypeUpdater entirely. Instead, we can ReFinalize at the end unconditionally. This has the downside that we do not propagate type updates "as we go", but that should be very rare. Another downside is that TypeUpdater tracks the number of brs, which can help remove code like in the one test that regresses here (see comment there). But I'm not sure that removal was valid - Vacuum should not really be doing it, and it looks like its related to this bug actually. Instead, we have a dedicated pass for removing unused brs - RemoveUnusedBrs - so leave things for it. This PR's benefit, aside from now handling the fuzz testcase, is that it makes the code simpler and faster. I see a 10-25% speedup on the Vacuum pass on various binaries I tested on. (Vacuum is one of our faster passes anyhow, though, so the runtime of -O1 is not much improved.) Another minor benefit might be that running ReFinalize more often can propagate type info more quickly, thanks to #5704 etc. But that is probably very minor. --- src/passes/Vacuum.cpp | 49 +------------------------------------------------ 1 file changed, 1 insertion(+), 48 deletions(-) (limited to 'src') diff --git a/src/passes/Vacuum.cpp b/src/passes/Vacuum.cpp index 7c394900d..8d89553b2 100644 --- a/src/passes/Vacuum.cpp +++ b/src/passes/Vacuum.cpp @@ -23,7 +23,6 @@ #include #include #include -#include #include #include #include @@ -36,32 +35,9 @@ struct Vacuum : public WalkerPass> { std::unique_ptr create() override { return std::make_unique(); } - TypeUpdater typeUpdater; - - // The TypeUpdater class handles efficient updating of unreachability as we - // go, but we may also refine types, which requires refinalization. - bool refinalize = false; - - Expression* replaceCurrent(Expression* expression) { - auto* old = getCurrent(); - if (expression->type != old->type && - expression->type != Type::unreachable) { - // We are changing this to a new type that is not unreachable, so it is a - // refinement that we need to use refinalize to propagate up. - refinalize = true; - } - super::replaceCurrent(expression); - // also update the type updater - typeUpdater.noteReplacement(old, expression); - return expression; - } - void doWalkFunction(Function* func) { - typeUpdater.walk(func->body); walk(func->body); - if (refinalize) { - ReFinalize().walkFunctionInModule(func, getModule()); - } + ReFinalize().walkFunctionInModule(func, getModule()); } // Returns nullptr if curr is dead, curr if it must stay as is, or one of its @@ -185,11 +161,9 @@ struct Vacuum : public WalkerPass> { } } if (!optimized) { - typeUpdater.noteRecursiveRemoval(child); skip++; } else { if (optimized != child) { - typeUpdater.noteReplacement(child, optimized); list[z] = optimized; } if (skip > 0) { @@ -198,14 +172,7 @@ struct Vacuum : public WalkerPass> { } // if this is unreachable, the rest is dead code if (list[z - skip]->type == Type::unreachable && z < size - 1) { - for (Index i = z - skip + 1; i < list.size(); i++) { - auto* remove = list[i]; - if (remove) { - typeUpdater.noteRecursiveRemoval(remove); - } - } list.resize(z - skip + 1); - typeUpdater.maybeUpdateTypeToUnreachable(curr); skip = 0; // nothing more to do on the list break; } @@ -213,7 +180,6 @@ struct Vacuum : public WalkerPass> { } if (skip > 0) { list.resize(size - skip); - typeUpdater.maybeUpdateTypeToUnreachable(curr); } // the block may now be a trivial one that we can get rid of and just leave // its contents @@ -227,15 +193,10 @@ struct Vacuum : public WalkerPass> { Expression* child; if (value->value.getInteger()) { child = curr->ifTrue; - if (curr->ifFalse) { - typeUpdater.noteRecursiveRemoval(curr->ifFalse); - } } else { if (curr->ifFalse) { child = curr->ifFalse; - typeUpdater.noteRecursiveRemoval(curr->ifTrue); } else { - typeUpdater.noteRecursiveRemoval(curr); ExpressionManipulator::nop(curr); return; } @@ -245,10 +206,6 @@ struct Vacuum : public WalkerPass> { } // if the condition is unreachable, just return it if (curr->condition->type == Type::unreachable) { - typeUpdater.noteRecursiveRemoval(curr->ifTrue); - if (curr->ifFalse) { - typeUpdater.noteRecursiveRemoval(curr->ifFalse); - } replaceCurrent(curr->condition); return; } @@ -384,9 +341,6 @@ struct Vacuum : public WalkerPass> { // the try's body. if (!EffectAnalyzer(getPassOptions(), *getModule(), curr->body).throws()) { replaceCurrent(curr->body); - for (auto* catchBody : curr->catchBodies) { - typeUpdater.noteRecursiveRemoval(catchBody); - } return; } @@ -398,7 +352,6 @@ struct Vacuum : public WalkerPass> { if (curr->type == Type::none && curr->hasCatchAll() && !EffectAnalyzer(getPassOptions(), *getModule(), curr) .hasUnremovableSideEffects()) { - typeUpdater.noteRecursiveRemoval(curr); ExpressionManipulator::nop(curr); } } -- cgit v1.2.3