Generalize moving of identical code from if/select arms (#3833)

Effects are fine in the moved code, if we are doing so on an if
(which runs just one arm anyhow).

Allow unreachable, which lets us hoist returns for example.

Allow none, which lets us hoist drop and call for example. For
this we also need to be careful with subtyping, as at least drop
is polymorphic, so the child types may not have an LUB (see
example in code).

Adds a small ShallowEffectAnalyzer child of EffectAnalyzer that
calls visit to just do a shallow analysis (instead of walk which
walks the children).

1 files changed, 30 insertions, 19 deletions

diff --git a/src/passes/OptimizeInstructions.cpp b/src/passes/OptimizeInstructions.cpp
index 5df057dcc..5eba1e3b9 100644
--- a/src/passes/OptimizeInstructions.cpp
+++ b/src/passes/OptimizeInstructions.cpp
@@ -2786,10 +2786,6 @@ private:
   // Optimize an if-else or a select, something with a condition and two
   // arms with outputs.
   template<typename T> void optimizeTernary(T* curr) {
-    if (curr->type == Type::unreachable) {
-      return;
-    }
-
     using namespace Match;
     Builder builder(*getModule());
 
@@ -2809,7 +2805,7 @@ private:
     //      (Y)
     //    )
     //  )
-    {
+    if (curr->type != Type::unreachable) {
       Unary* un;
       Expression* x;
       Const* c;
@@ -2860,27 +2856,42 @@ private:
       // as we go, then do a single replaceCurrent() at the end.
       SmallVector<Expression*, 1> chain;
       while (1) {
-        // TODO: handle none as well as concrete types (can pull a drop out, for
-        //       example)
-        // TODO: consider the case with more children than 1
-        if (curr->ifTrue->type.isConcrete() &&
-            !Properties::isControlFlowStructure(curr->ifTrue) &&
+        // Ignore control flow structures (which are handled in MergeBlocks).
+        if (!Properties::isControlFlowStructure(curr->ifTrue) &&
             ExpressionAnalyzer::shallowEqual(curr->ifTrue, curr->ifFalse)) {
+          // TODO: consider the case with more than one child.
           ChildIterator ifTrueChildren(curr->ifTrue);
           if (ifTrueChildren.children.size() == 1) {
+            ChildIterator ifFalseChildren(curr->ifFalse);
+            // ifTrue and ifFalse's children will become the direct children of
+            // curr, and so there must be an LUB for curr to have a proper type.
+            // An example where that does not happen is this:
+            //
+            //  (if
+            //    (condition)
+            //    (drop (i32.const 1))
+            //    (drop (f64.const 2.0))
+            //  )
+            auto* ifTrueChild = *ifTrueChildren.begin();
+            auto* ifFalseChild = *ifFalseChildren.begin();
+            bool validTypes =
+              Type::hasLeastUpperBound(ifTrueChild->type, ifFalseChild->type);
             // If the expression we are about to move outside has side effects,
-            // we cannot replace two instances with one (and there might be
-            // ordering issues as well, for a select's condition).
-            // TODO: handle certain side effects when possible
-            EffectAnalyzer shallowEffects(getPassOptions(),
-                                          getModule()->features);
-            shallowEffects.visit(curr->ifTrue);
-            if (!shallowEffects.hasSideEffects()) {
+            // then we cannot do so in general with a select: we'd be reducing
+            // the amount of the effects as well as moving them. For an if,
+            // the side effects execute once, so there is no problem.
+            // TODO: handle certain side effects when possible in select
+            bool validEffects = std::is_same<T, If>::value ||
+                                !ShallowEffectAnalyzer(getPassOptions(),
+                                                       getModule()->features,
+                                                       curr->ifTrue)
+                                   .hasSideEffects();
+            if (validTypes && validEffects) {
               // Replace ifTrue with its child.
-              curr->ifTrue = *ifTrueChildren.begin();
+              curr->ifTrue = ifTrueChild;
               // Relace ifFalse with its child, and reuse that node outside.
               auto* reuse = curr->ifFalse;
-              curr->ifFalse = *ChildIterator(curr->ifFalse).begin();
+              curr->ifFalse = ifFalseChild;
               // curr's type may have changed, if the instructions we moved out
               // had different input types than output types.
               curr->finalize();