Move removable code in CodePushing (#5187)

This is safe since we "partially remove" it: we don't move it to a place it might
execute more, but make it possibly execute less. See the new comment for
more details.

Motivated by wasm GC but this can help wasm MVP as well. In both cases
loads from memory can trap, which limits what the VM can do to optimize
them past conditions, but in trapsNeverHappens we can do that at the
toolchain level:

x = read();
if () { .. }
use(x);

=>

if () { .. }
x = read(); // moved to here, and might not execute if the if did a break/return
use(x);

1 files changed, 18 insertions, 3 deletions

diff --git a/src/passes/CodePushing.cpp b/src/passes/CodePushing.cpp
index 1b253c6ea..4adbb3423 100644
--- a/src/passes/CodePushing.cpp
+++ b/src/passes/CodePushing.cpp
@@ -124,11 +124,26 @@ private:
       return nullptr;
     }
     auto index = set->index;
-    // to be pushable, this must be SFA and the right # of gets,
-    // but also have no side effects, as it may not execute if pushed.
+    // To be pushable, this must be SFA and the right # of gets.
+    //
+    // It must also not have side effects, as it may no longer execute after it
+    // is pushed, since it may be behind a condition that ends up false some of
+    // the time. However, removable side effects are ok here. The general
+    // problem with removable effects is that we can only remove them, but not
+    // move them, because of stuff like this:
+    //
+    //   if (x != 0) foo(1 / x);
+    //
+    // If we move 1 / x to execute unconditionally then it may trap, but it
+    // would be fine to remove it. This pass does not move code to places where
+    // it might execute more, but *less*: we keep the code behind any conditions
+    // it was already behind, and potentially put it behind further ones. In
+    // effect, we "partially remove" the code, making it not execute some of the
+    // time, which is fine.
     if (analyzer.isSFA(index) &&
         numGetsSoFar[index] == analyzer.getNumGets(index) &&
-        !EffectAnalyzer(passOptions, module, set->value).hasSideEffects()) {
+        !EffectAnalyzer(passOptions, module, set->value)
+           .hasUnremovableSideEffects()) {
       return set;
     }
     return nullptr;