[GC] Refactor Vacuum pass. NFC. (#3421)

This is shorter by using ChildIterator, so it's more general. The greater generality
should not be noticeable atm, but it will allow this pass to just work on GC
instructions once we have them (without this, in my testing the optimizer does not
do very well on GC).

1 files changed, 65 insertions, 134 deletions

diff --git a/src/passes/Vacuum.cpp b/src/passes/Vacuum.cpp
index e0c00e0da..789c53ad7 100644
--- a/src/passes/Vacuum.cpp
+++ b/src/passes/Vacuum.cpp
@@ -20,6 +20,7 @@
 
 #include <ir/block-utils.h>
 #include <ir/effects.h>
+#include <ir/iteration.h>
 #include <ir/literal-utils.h>
 #include <ir/type-updating.h>
 #include <ir/utils.h>
@@ -49,155 +50,85 @@ struct Vacuum : public WalkerPass<ExpressionStackWalker<Vacuum>> {
     walk(func->body);
   }
 
-  // Returns nullptr if curr is dead, curr if it must stay as is, or another
-  // node if it can be replaced. Takes into account:
+  // Returns nullptr if curr is dead, curr if it must stay as is, or one of its
+  // children if it can be replaced. Takes into account:
+  //
   //  * The result may be used or unused.
-  //  * The type may or may not matter (a drop can drop anything, for example).
+  //  * The type may or may not matter.
+  //
+  // For example,
+  //
+  //  (drop
+  //   (i32.eqz
+  //    (call ..)))
+  //
+  // The drop means that the value is not used later. And while the call has
+  // side effects, the i32.eqz does not. So when we are called on the i32.eqz,
+  // and told the result does not matter, we can return the call. Note that in
+  // this case the type does not matter either, as drop doesn't care, and anyhow
+  // i32.eqz returns the same type as it receives. But for an expression that
+  // returns a different type, if the type matters then we cannot replace it.
   Expression* optimize(Expression* curr, bool resultUsed, bool typeMatters) {
     FeatureSet features = getModule()->features;
     auto type = curr->type;
-    // An unreachable node must not be changed.
+    // If the type is none, then we can never replace it with another type.
+    if (type == Type::none) {
+      typeMatters = true;
+    }
+    // An unreachable node must not be changed. DCE will remove those.
     if (type == Type::unreachable) {
       return curr;
     }
-    // We iterate on possible replacements. If a replacement changes the type,
-    // stop and go back.
+    // resultUsed only makes sense when the type is concrete
+    assert(!resultUsed || curr->type != Type::none);
+    // If we actually need the result, then we must not change anything.
+    // TODO: maybe there is something clever though?
+    if (resultUsed) {
+      return curr;
+    }
+    // We iterate on possible replacements.
     auto* prev = curr;
     while (1) {
+      // If a replacement changes the type, and the type matters, return the
+      // previous one and stop.
       if (typeMatters && curr->type != type) {
         return prev;
       }
       prev = curr;
-      switch (curr->_id) {
-        case Expression::Id::NopId:
-          return nullptr; // never needed
-
-        case Expression::Id::BlockId:
-          return curr; // not always needed, but handled in visitBlock()
-        case Expression::Id::IfId:
-          return curr; // not always needed, but handled in visitIf()
-        case Expression::Id::LoopId:
-          return curr; // not always needed, but handled in visitLoop()
-        case Expression::Id::DropId:
-          return curr; // not always needed, but handled in visitDrop()
-        case Expression::Id::TryId:
-          return curr; // not always needed, but handled in visitTry()
-
-        case Expression::Id::LoadId: {
-          // it is ok to remove a load if the result is not used, and it has no
-          // side effects (the load itself may trap, if we are not ignoring such
-          // things)
-          auto* load = curr->cast<Load>();
-          if (!resultUsed && !EffectAnalyzer(getPassOptions(), features, curr)
-                                .hasSideEffects()) {
-            if (!typeMatters || load->ptr->type == type) {
-              return load->ptr;
-            }
-          }
-          return curr;
-        }
-        case Expression::Id::ConstId:
-        case Expression::Id::LocalGetId:
-        case Expression::Id::GlobalGetId: {
-          if (!resultUsed) {
-            return nullptr;
-          }
-          return curr;
-        }
-
-        case Expression::Id::UnaryId:
-        case Expression::Id::BinaryId:
-        case Expression::Id::SelectId: {
-          if (resultUsed) {
-            return curr; // used, keep it
-          }
-          // for unary, binary, and select, we need to check their arguments for
-          // side effects, as well as the node itself, as some unaries and
-          // binaries have implicit traps
-          if (auto* unary = curr->dynCast<Unary>()) {
-            EffectAnalyzer tester(getPassOptions(), features);
-            tester.visit(unary);
-            if (tester.hasSideEffects()) {
-              return curr;
-            }
-            if (EffectAnalyzer(getPassOptions(), features, unary->value)
-                  .hasSideEffects()) {
-              curr = unary->value;
-              continue;
-            } else {
-              return nullptr;
-            }
-          } else if (auto* binary = curr->dynCast<Binary>()) {
-            EffectAnalyzer tester(getPassOptions(), features);
-            tester.visit(binary);
-            if (tester.hasSideEffects()) {
-              return curr;
-            }
-            if (EffectAnalyzer(getPassOptions(), features, binary->left)
-                  .hasSideEffects()) {
-              if (EffectAnalyzer(getPassOptions(), features, binary->right)
-                    .hasSideEffects()) {
-                return curr; // leave them
-              } else {
-                curr = binary->left;
-                continue;
-              }
-            } else {
-              if (EffectAnalyzer(getPassOptions(), features, binary->right)
-                    .hasSideEffects()) {
-                curr = binary->right;
-                continue;
-              } else {
-                return nullptr;
-              }
-            }
-          } else {
-            // TODO: if two have side effects, we could replace the select with
-            // say an add?
-            auto* select = curr->cast<Select>();
-            if (EffectAnalyzer(getPassOptions(), features, select->ifTrue)
-                  .hasSideEffects()) {
-              if (EffectAnalyzer(getPassOptions(), features, select->ifFalse)
-                    .hasSideEffects()) {
-                return curr; // leave them
-              } else {
-                if (EffectAnalyzer(
-                      getPassOptions(), features, select->condition)
-                      .hasSideEffects()) {
-                  return curr; // leave them
-                } else {
-                  curr = select->ifTrue;
-                  continue;
-                }
-              }
-            } else {
-              if (EffectAnalyzer(getPassOptions(), features, select->ifFalse)
-                    .hasSideEffects()) {
-                if (EffectAnalyzer(
-                      getPassOptions(), features, select->condition)
-                      .hasSideEffects()) {
-                  return curr; // leave them
-                } else {
-                  curr = select->ifFalse;
-                  continue;
-                }
-              } else {
-                if (EffectAnalyzer(
-                      getPassOptions(), features, select->condition)
-                      .hasSideEffects()) {
-                  curr = select->condition;
-                  continue;
-                } else {
-                  return nullptr;
-                }
-              }
-            }
-          }
+      // Some instructions have special handling in visit*, and we should do
+      // nothing for them here.
+      if (curr->is<Drop>() || curr->is<Block>() || curr->is<If>() ||
+          curr->is<Loop>() || curr->is<Try>()) {
+        return curr;
+      }
+      // Check if this expression itself has side effects, ignoring children.
+      EffectAnalyzer self(getPassOptions(), features);
+      self.visit(curr);
+      if (self.hasSideEffects()) {
+        return curr;
+      }
+      // The result isn't used, and this has no side effects itself, so we can
+      // get rid of it. However, the children may have side effects.
+      SmallVector<Expression*, 1> childrenWithEffects;
+      for (auto* child : ChildIterator(curr)) {
+        if (EffectAnalyzer(getPassOptions(), features, child)
+              .hasSideEffects()) {
+          childrenWithEffects.push_back(child);
         }
-
-        default:
-          return curr; // assume needed
       }
+      if (childrenWithEffects.empty()) {
+        return nullptr;
+      }
+      if (childrenWithEffects.size() == 1) {
+        // We know the result isn't used, and curr has no side effects, so we
+        // can skip curr and keep looking into the child.
+        curr = childrenWithEffects[0];
+        continue;
+      }
+      // TODO: with multiple children with side effects, we can perhaps figure
+      // out something clever, like a block with drops, or an i32.add for just
+      // two, etc.
+      return curr;
     }
   }