[effects.h] Refactor hasGlobalSideEffects and throw handling. (#3370)

The new writesGlobalState has a name that more clearly indicates what it
actually does: affect global state (that is, memory, globals, the table, etc.).
This removes throw from there, and handles it directly in the single caller
of the method, the licm pass. For simplicity, disallow exceptions in that
pass, leaving it for future work.

2 files changed, 29 insertions, 19 deletions

diff --git a/src/ir/effects.h b/src/ir/effects.h
index 9426b8df4..d788d176d 100644
--- a/src/ir/effects.h
+++ b/src/ir/effects.h
@@ -138,21 +138,21 @@ struct EffectAnalyzer
     return branchesOut || throws || hasExternalBreakTargets();
   }
 
-  bool hasGlobalSideEffects() const {
-    return calls || globalsWritten.size() > 0 || writesMemory || isAtomic ||
-           throws;
+  // Changes something in globally-stored state.
+  bool writesGlobalState() const {
+    return globalsWritten.size() || writesMemory || isAtomic || calls;
   }
+  bool readsGlobalState() const {
+    return globalsRead.size() || readsMemory || isAtomic || calls;
+  }
+
   bool hasSideEffects() const {
-    return implicitTrap || localsWritten.size() > 0 || danglingPop ||
-           hasGlobalSideEffects() || transfersControlFlow();
+    return localsWritten.size() > 0 || danglingPop || writesGlobalState() ||
+           implicitTrap || throws || transfersControlFlow();
   }
   bool hasAnything() const {
     return hasSideEffects() || accessesLocal() || readsMemory ||
-           accessesGlobal() || isAtomic;
-  }
-
-  bool noticesGlobalSideEffects() const {
-    return calls || readsMemory || isAtomic || globalsRead.size();
+           accessesGlobal();
   }
 
   // check if we break to anything external from ourselves
@@ -199,14 +199,21 @@ struct EffectAnalyzer
         return true;
       }
     }
-    // we are ok to reorder implicit traps, but not conditionalize them
+    // We are ok to reorder implicit traps, but not conditionalize them.
     if ((implicitTrap && other.transfersControlFlow()) ||
         (other.implicitTrap && transfersControlFlow())) {
       return true;
     }
-    // we can't reorder an implicit trap in a way that alters global state
-    if ((implicitTrap && other.hasGlobalSideEffects()) ||
-        (other.implicitTrap && hasGlobalSideEffects())) {
+    // Note that the above includes disallowing the reordering of a trap with an
+    // exception (as an exception can transfer control flow inside the current
+    // function, so transfersControlFlow would be true) - while we allow the
+    // reordering of traps with each other, we do not reorder exceptions with
+    // anything.
+    assert(!(implicitTrap && other.throws) && !(throws && other.implicitTrap));
+    // We can't reorder an implicit trap in a way that could alter what global
+    // state is modified.
+    if ((implicitTrap && other.writesGlobalState()) ||
+        (other.implicitTrap && writesGlobalState())) {
       return true;
     }
     return false;
diff --git a/src/passes/LoopInvariantCodeMotion.cpp b/src/passes/LoopInvariantCodeMotion.cpp
index 26e2a0844..a80e3a55d 100644
--- a/src/passes/LoopInvariantCodeMotion.cpp
+++ b/src/passes/LoopInvariantCodeMotion.cpp
@@ -114,17 +114,20 @@ struct LoopInvariantCodeMotion
       }
       if (interestingToMove(curr)) {
         // Let's see if we can move this out.
-        // Global side effects would prevent this - we might end up
+        // Global state changes would prevent this - we might end up
         // executing them just once.
         // And we must also move across anything not moved out already,
         // so check for issues there too.
         // The rest of the loop's effects matter too, we must also
         // take into account global state like interacting loads and
         // stores.
-        bool unsafeToMove = effects.hasGlobalSideEffects() ||
-                            effectsSoFar.invalidates(effects) ||
-                            (effects.noticesGlobalSideEffects() &&
-                             loopEffects.hasGlobalSideEffects());
+        bool unsafeToMove =
+          effects.writesGlobalState() || effectsSoFar.invalidates(effects) ||
+          (effects.readsGlobalState() && loopEffects.writesGlobalState());
+        // TODO: look into optimizing this with exceptions. for now, disallow
+        if (effects.throws || loopEffects.throws) {
+          unsafeToMove = true;
+        }
         if (!unsafeToMove) {
           // So far so good. Check if our local dependencies are all
           // outside of the loop, in which case everything is good -