Don't reorder an implicit trap with a global side effect (#1133)

Author: kripken

src/ast/effects.h

@@ -48,18 +48,18 @@ struct EffectAnalyzer : public PostWalker<EffectAnalyzer> {
   bool readsMemory = false;
   bool writesMemory = false;
   bool implicitTrap = false; // a load or div/rem, which may trap. we ignore trap
-                             // differences, so it is ok to reorder these, and we
-                             // also allow reordering them with other effects
-                             // (so a trap may occur later or earlier, if it is
-                             // going to occur anyhow), but we can't remove them,
-                             // they count as side effects
+                             // differences, so it is ok to reorder these, but we can't
+                             // remove them, as they count as side effects, and we
+                             // can't move them in a way that would cause other noticeable
+                             // (global) side effects
   bool isAtomic = false; // An atomic load/store/RMW/Cmpxchg or an operator that
                          // has a defined ordering wrt atomics (e.g. grow_memory)
 
   bool accessesLocal() { return localsRead.size() + localsWritten.size() > 0; }
   bool accessesGlobal() { return globalsRead.size() + globalsWritten.size() > 0; }
   bool accessesMemory() { return calls || readsMemory || writesMemory; }
-  bool hasSideEffects() { return calls || localsWritten.size() > 0 || writesMemory || branches || globalsWritten.size() > 0 || implicitTrap || isAtomic; }
+  bool hasGlobalSideEffects() { return calls || globalsWritten.size() > 0 || writesMemory || isAtomic; }
+  bool hasSideEffects() { return hasGlobalSideEffects() || localsWritten.size() > 0 || branches || implicitTrap; }
   bool hasAnything() { return branches || calls || accessesLocal() || readsMemory || writesMemory || accessesGlobal() || implicitTrap || isAtomic; }
 
   // checks if these effects would invalidate another set (e.g., if we write, we invalidate someone that reads, they can't be moved past us)
@@ -98,6 +98,10 @@ struct EffectAnalyzer : public PostWalker<EffectAnalyzer> {
     if ((implicitTrap && other.branches) || (other.implicitTrap && branches)) {
       return true;
     }
+    // we can't reorder an implicit trap in a way that alters global state
+    if ((implicitTrap && other.hasGlobalSideEffects()) || (other.implicitTrap && hasGlobalSideEffects())) {
+      return true;
+    }
     return false;
   }
 

test/passes/simplify-locals-nostructure.txt

@@ -66,4 +66,38 @@
   (local $x i32)
   (unreachable)
  )
+ (func $implicit-trap-and-global-effects (type $0)
+  (local $var$0 i32)
+  (set_local $var$0
+   (i32.trunc_u/f64
+    (f64.const -nan:0xfffffffffffc3)
+   )
+  )
+  (f32.store align=1
+   (i32.const 22)
+   (f32.const 154)
+  )
+  (drop
+   (get_local $var$0)
+  )
+ )
+ (func $implicit-trap-and-local-effects (type $0)
+  (local $var$0 i32)
+  (local $other i32)
+  (nop)
+  (set_local $other
+   (i32.const 100)
+  )
+  (drop
+   (i32.trunc_u/f64
+    (f64.const -nan:0xfffffffffffc3)
+   )
+  )
+  (if
+   (i32.const 1)
+   (drop
+    (get_local $other)
+   )
+  )
+ )
 )

test/passes/simplify-locals-nostructure.wast

@@ -36,5 +36,38 @@
       )
     )
   )
+  (func $implicit-trap-and-global-effects
+    (local $var$0 i32)
+    (set_local $var$0
+     (i32.trunc_u/f64
+      (f64.const -nan:0xfffffffffffc3) ;; this implicit trap will actually trap
+     )
+    )
+    (f32.store align=1 ;; and if we move it across this store, the store will execute, having global side effects
+     (i32.const 22)
+     (f32.const 154)
+    )
+    (drop
+     (get_local $var$0)
+    )
+  )
+  (func $implicit-trap-and-local-effects
+    (local $var$0 i32)
+    (local $other i32)
+    (set_local $var$0
+     (i32.trunc_u/f64
+      (f64.const -nan:0xfffffffffffc3) ;; this implicit trap will actually trap
+     )
+    )
+    (set_local $other (i32.const 100)) ;; but it's fine to move it across a local effect, that vanishes anyhow
+    (drop
+     (get_local $var$0)
+    )
+    (if (i32.const 1)
+     (drop
+      (get_local $other)
+     )
+    )
+  )
 )