Move method implementations to drop.cpp (#4823)

Now that we have `getDroppedUnconditionalChildrenAndAppend`, all passes
need to use it rather than the old `getDroppedChildrenAndAppend`, which
is used within the new method
`getDroppedUnconditionalChildrenAndAppend`.

This creates `drop.cpp` and move method implementations in drop.h there,
and merge two methods given that the old method is not supposed to be
used from outside anyway, and drops `Unconditional` from the new method
name because this is the only method and doesn't have to be specific
about that.

4 files changed, 109 insertions, 77 deletions

diff --git a/src/ir/CMakeLists.txt b/src/ir/CMakeLists.txt
index ed95c5d7d..e3315565b 100644
--- a/src/ir/CMakeLists.txt
+++ b/src/ir/CMakeLists.txt
@@ -2,6 +2,7 @@ FILE(GLOB ir_HEADERS *.h)
 set(ir_SOURCES
   ExpressionAnalyzer.cpp
   ExpressionManipulator.cpp
+  drop.cpp
   eh-utils.cpp
   intrinsics.cpp
   lubs.cpp
diff --git a/src/ir/drop.cpp b/src/ir/drop.cpp
new file mode 100644
index 000000000..14ab43794
--- /dev/null
+++ b/src/ir/drop.cpp
@@ -0,0 +1,87 @@
+/*
+ * Copyright 2022 WebAssembly Community Group participants
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef wasm_ir_drop_h
+#define wasm_ir_drop_h
+
+#include "ir/branch-utils.h"
+#include "ir/effects.h"
+#include "ir/iteration.h"
+#include "wasm-builder.h"
+#include "wasm.h"
+
+namespace wasm {
+
+// Given an expression, returns a new expression that drops the given
+// expression's children that cannot be removed outright due to their side
+// effects. Note that this only operates on children that execute
+// unconditionally. That is the case in almost all expressions, except for those
+// with conditional execution, like if, which unconditionally executes the
+// condition but then conditionally executes one of the two arms.
+Expression* getDroppedChildrenAndAppend(Expression* curr,
+                                        Module& wasm,
+                                        const PassOptions& options,
+                                        Expression* last) {
+  // We check for shallow effects here, since we may be able to remove |curr|
+  // itself but keep its children around - we don't want effects in the children
+  // to stop us from improving the code. Note that there are cases where the
+  // combined curr+children has fewer effects than curr itself, such as if curr
+  // is a block and the child branches to it, but in such cases we cannot remove
+  // curr anyhow (those cases are ruled out below), so looking at non-shallow
+  // effects would never help us (and would be slower to run).
+  ShallowEffectAnalyzer effects(options, wasm, curr);
+  // Ignore a trap, as the unreachable replacement would trap too.
+  if (last->type == Type::unreachable) {
+    effects.trap = false;
+  }
+
+  // We cannot remove
+  // 1. Expressions with unremovable side effects
+  // 2. if: 'if's contains conditional expressions
+  // 3. try: Removing a try could leave a pop without a proper parent
+  // 4. pop: Pops are struturally necessary in catch bodies
+  // 5. Branch targets: We will need the target for the branches to it to
+  //                    validate.
+  Builder builder(wasm);
+  if (effects.hasUnremovableSideEffects() || curr->is<If>() ||
+      curr->is<Try>() || curr->is<Pop>() ||
+      BranchUtils::getDefinedName(curr).is()) {
+    return builder.makeSequence(builder.makeDrop(curr), last);
+  }
+
+  std::vector<Expression*> contents;
+  for (auto* child : ChildIterator(curr)) {
+    if (!EffectAnalyzer(options, wasm, child).hasUnremovableSideEffects()) {
+      continue;
+    }
+    if (child->type.isConcrete()) {
+      contents.push_back(builder.makeDrop(child));
+    } else {
+      // The child is unreachable, or none (none is possible as a child of a
+      // block or loop, etc.); in both cases we do not need a drop.
+      contents.push_back(child);
+    }
+  }
+  contents.push_back(last);
+  if (contents.size() == 1) {
+    return contents[0];
+  }
+  return builder.makeBlock(contents);
+}
+
+} // namespace wasm
+
+#endif // wasm_ir_drop_h
diff --git a/src/ir/drop.h b/src/ir/drop.h
index a13b83332..78ff98c66 100644
--- a/src/ir/drop.h
+++ b/src/ir/drop.h
@@ -17,52 +17,26 @@
 #ifndef wasm_ir_drop_h
 #define wasm_ir_drop_h
 
-#include "ir/branch-utils.h"
-#include "ir/effects.h"
-#include "ir/iteration.h"
-#include "wasm-builder.h"
 #include "wasm.h"
 
 namespace wasm {
 
+struct PassOptions;
+
 // Given an expression, returns a new expression that drops the given
-// expression's children that cannot be removed outright due to their side
-// effects. This is useful if we know the node is not needed but may need to
-// keep the children around; this utility will automatically remove any children
-// we do not actually need to keep, based on their effects.
+// expression's unconditional children that cannot be removed outright due to
+// their side effects. This is useful if we know the node is not needed but may
+// need to keep the children around; this utility will automatically remove any
+// children we do not actually need to keep, based on their effects.
 //
 // The caller must also pass in a last item to append to the output (which is
 // typically what the original expression is replaced with).
-inline Expression* getDroppedChildrenAndAppend(Expression* curr,
-                                               Module& wasm,
-                                               const PassOptions& options,
-                                               Expression* last) {
-  Builder builder(wasm);
-  std::vector<Expression*> contents;
-  for (auto* child : ChildIterator(curr)) {
-    if (!EffectAnalyzer(options, wasm, child).hasUnremovableSideEffects()) {
-      continue;
-    }
-    if (child->type.isConcrete()) {
-      contents.push_back(builder.makeDrop(child));
-    } else {
-      // The child is unreachable, or none (none is possible as a child of a
-      // block or loop, etc.); in both cases we do not need a drop.
-      contents.push_back(child);
-    }
-  }
-  contents.push_back(last);
-  if (contents.size() == 1) {
-    return contents[0];
-  }
-  return builder.makeBlock(contents);
-}
-
-// As the above, but only operates on children that execute unconditionally.
-// That is the case in almost all expressions, except for those with
-// conditional execution, like if, which unconditionally executes the condition
-// but then conditionally executes one of the two arms. The above function
-// simply returns all children in order, so it does this to if:
+//
+// This function only operates on children that executes unconditionally. That
+// is the case in almost all expressions, except for those with conditional
+// execution, like if, which unconditionally executes the condition but then
+// conditionally executes one of the two arms. The above function simply returns
+// all children in order, so it does this to if:
 //
 //  (if
 //    (condition)
@@ -96,41 +70,12 @@ inline Expression* getDroppedChildrenAndAppend(Expression* curr,
 //  )
 //  (appended last item)
 //
-//  Also this function preserves other unremovable expressions like trys and
-//  pops.
-inline Expression*
-getDroppedUnconditionalChildrenAndAppend(Expression* curr,
-                                         Module& wasm,
-                                         const PassOptions& options,
-                                         Expression* last) {
-  // We check for shallow effects here, since we may be able to remove |curr|
-  // itself but keep its children around - we don't want effects in the children
-  // to stop us from improving the code. Note that there are cases where the
-  // combined curr+children has fewer effects than curr itself, such as if curr
-  // is a block and the child branches to it, but in such cases we cannot remove
-  // curr anyhow (those cases are ruled out below), so looking at non-shallow
-  // effects would never help us (and would be slower to run).
-  ShallowEffectAnalyzer effects(options, wasm, curr);
-  // Ignore a trap, as the unreachable replacement would trap too.
-  if (last->type == Type::unreachable) {
-    effects.trap = false;
-  }
-
-  // We cannot remove
-  // 1. Expressions with unremovable side effects
-  // 2. if: 'if's contains conditional expressions
-  // 3. try: Removing a try could leave a pop without a proper parent
-  // 4. pop: Pops are struturally necessary in catch bodies
-  // 5. Branch targets: We will need the target for the branches to it to
-  //                    validate.
-  if (effects.hasUnremovableSideEffects() || curr->is<If>() ||
-      curr->is<Try>() || curr->is<Pop>() ||
-      BranchUtils::getDefinedName(curr).is()) {
-    Builder builder(wasm);
-    return builder.makeSequence(builder.makeDrop(curr), last);
-  }
-  return getDroppedChildrenAndAppend(curr, wasm, options, last);
-}
+//  Also this function preserves other unremovable expressions like trys, pops,
+//  and named blocks.
+Expression* getDroppedChildrenAndAppend(Expression* curr,
+                                        Module& wasm,
+                                        const PassOptions& options,
+                                        Expression* last);
 
 } // namespace wasm
 
diff --git a/src/passes/GUFA.cpp b/src/passes/GUFA.cpp
index 1ac8ed820..1467b1312 100644
--- a/src/passes/GUFA.cpp
+++ b/src/passes/GUFA.cpp
@@ -100,7 +100,7 @@ struct GUFAOptimizer
     if (contents.getType() == Type::unreachable) {
       // This cannot contain any possible value at all. It must be unreachable
       // code.
-      replaceCurrent(getDroppedUnconditionalChildrenAndAppend(
+      replaceCurrent(getDroppedChildrenAndAppend(
         curr, wasm, options, builder.makeUnreachable()));
       optimized = true;
       return;
@@ -139,8 +139,7 @@ struct GUFAOptimizer
     //       ref.as etc. Once it does those we could assert on the type being
     //       valid here.
     if (Type::isSubType(c->type, curr->type)) {
-      replaceCurrent(
-        getDroppedUnconditionalChildrenAndAppend(curr, wasm, options, c));
+      replaceCurrent(getDroppedChildrenAndAppend(curr, wasm, options, c));
       optimized = true;
     } else {
       // The type is not compatible: we cannot place |c| in this location, even
@@ -149,7 +148,7 @@ struct GUFAOptimizer
         // The type is not compatible and this is a simple constant expression
         // like a ref.func. That means this code must be unreachable. (See below
         // for the case of a non-constant.)
-        replaceCurrent(getDroppedUnconditionalChildrenAndAppend(
+        replaceCurrent(getDroppedChildrenAndAppend(
           curr, wasm, options, builder.makeUnreachable()));
         optimized = true;
       } else {