RemoveUnusedBrs: Properly check for effects in selectify() (#3310)

Selectify turns an if-else into a select where possible. Previously we abandoned
hope if any part of the if had a side effect. But it's fine for the condition to have a
side effect, so long as moving it to the end doesn't invalidate the arms.

1 files changed, 25 insertions, 11 deletions

diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp
index 0a3374c4d..6e9c9c5df 100644
--- a/src/passes/RemoveUnusedBrs.cpp
+++ b/src/passes/RemoveUnusedBrs.cpp
@@ -930,6 +930,17 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
             !iff->ifFalse->type.isSingle()) {
           return nullptr;
         }
+        if (iff->condition->type == Type::unreachable) {
+          // An if with an unreachable condition may nonetheless have a type
+          // that is not unreachable,
+          //
+          // (if (result i32) (unreachable) ..)
+          //
+          // Turning such an if into a select would change the type of the
+          // expression, which would require updating types further up. Avoid
+          // that, leaving dead code elimination to that dedicated pass.
+          return nullptr;
+        }
         // This is always helpful for code size, but can be a tradeoff with
         // performance as we run both code paths. So when shrinking we always
         // try to do this, but otherwise must consider more carefully.
@@ -937,20 +948,23 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
               passOptions, iff->ifTrue, iff->ifFalse)) {
           return nullptr;
         }
-        // Check if side effects allow this.
+        // Check if side effects allow this: we need to execute the two arms
+        // unconditionally, and also to make the condition run last.
         FeatureSet features = getModule()->features;
+        EffectAnalyzer ifTrue(passOptions, features, iff->ifTrue);
+        if (ifTrue.hasSideEffects()) {
+          return nullptr;
+        }
+        EffectAnalyzer ifFalse(passOptions, features, iff->ifFalse);
+        if (ifFalse.hasSideEffects()) {
+          return nullptr;
+        }
         EffectAnalyzer condition(passOptions, features, iff->condition);
-        if (!condition.hasSideEffects()) {
-          EffectAnalyzer ifTrue(passOptions, features, iff->ifTrue);
-          if (!ifTrue.hasSideEffects()) {
-            EffectAnalyzer ifFalse(passOptions, features, iff->ifFalse);
-            if (!ifFalse.hasSideEffects()) {
-              return Builder(*getModule())
-                .makeSelect(iff->condition, iff->ifTrue, iff->ifFalse);
-            }
-          }
+        if (condition.invalidates(ifTrue) || condition.invalidates(ifFalse)) {
+          return nullptr;
         }
-        return nullptr;
+        return Builder(*getModule())
+          .makeSelect(iff->condition, iff->ifTrue, iff->ifFalse);
       }
 
       void visitLocalSet(LocalSet* curr) {