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 -