More precise implicitTrap detection for binary extressions (#3312)

Division and remainder do not have an implicit trap if the
right-hand side is a constant and not one of the dangerous
values there.

Also refactor ignoreImplicitTrap handling for clarity.

4 files changed, 77 insertions, 92 deletions

diff --git a/src/ir/effects.h b/src/ir/effects.h
index 1a713f944..9426b8df4 100644
--- a/src/ir/effects.h
+++ b/src/ir/effects.h
@@ -45,6 +45,10 @@ struct EffectAnalyzer
     breakTargets.clear();
     walk(ast);
     assert(tryDepth == 0);
+
+    if (ignoreImplicitTraps) {
+      implicitTrap = false;
+    }
   }
 
   // Core effect tracking
@@ -139,8 +143,8 @@ struct EffectAnalyzer
            throws;
   }
   bool hasSideEffects() const {
-    return hasGlobalSideEffects() || localsWritten.size() > 0 ||
-           transfersControlFlow() || implicitTrap || danglingPop;
+    return implicitTrap || localsWritten.size() > 0 || danglingPop ||
+           hasGlobalSideEffects() || transfersControlFlow();
   }
   bool hasAnything() const {
     return hasSideEffects() || accessesLocal() || readsMemory ||
@@ -160,7 +164,7 @@ struct EffectAnalyzer
     if ((transfersControlFlow() && other.hasSideEffects()) ||
         (other.transfersControlFlow() && hasSideEffects()) ||
         ((writesMemory || calls) && other.accessesMemory()) ||
-        (accessesMemory() && (other.writesMemory || other.calls)) ||
+        ((other.writesMemory || other.calls) && accessesMemory()) ||
         (danglingPop || other.danglingPop)) {
       return true;
     }
@@ -171,7 +175,7 @@ struct EffectAnalyzer
       return true;
     }
     for (auto local : localsWritten) {
-      if (other.localsWritten.count(local) || other.localsRead.count(local)) {
+      if (other.localsRead.count(local) || other.localsWritten.count(local)) {
         return true;
       }
     }
@@ -180,13 +184,13 @@ struct EffectAnalyzer
         return true;
       }
     }
-    if ((accessesGlobal() && other.calls) ||
-        (other.accessesGlobal() && calls)) {
+    if ((other.calls && accessesGlobal()) ||
+        (calls && other.accessesGlobal())) {
       return true;
     }
     for (auto global : globalsWritten) {
-      if (other.globalsWritten.count(global) ||
-          other.globalsRead.count(global)) {
+      if (other.globalsRead.count(global) ||
+          other.globalsWritten.count(global)) {
         return true;
       }
     }
@@ -318,32 +322,24 @@ struct EffectAnalyzer
   void visitLoad(Load* curr) {
     readsMemory = true;
     isAtomic |= curr->isAtomic;
-    if (!ignoreImplicitTraps) {
-      implicitTrap = true;
-    }
+    implicitTrap = true;
   }
   void visitStore(Store* curr) {
     writesMemory = true;
     isAtomic |= curr->isAtomic;
-    if (!ignoreImplicitTraps) {
-      implicitTrap = true;
-    }
+    implicitTrap = true;
   }
   void visitAtomicRMW(AtomicRMW* curr) {
     readsMemory = true;
     writesMemory = true;
     isAtomic = true;
-    if (!ignoreImplicitTraps) {
-      implicitTrap = true;
-    }
+    implicitTrap = true;
   }
   void visitAtomicCmpxchg(AtomicCmpxchg* curr) {
     readsMemory = true;
     writesMemory = true;
     isAtomic = true;
-    if (!ignoreImplicitTraps) {
-      implicitTrap = true;
-    }
+    implicitTrap = true;
   }
   void visitAtomicWait(AtomicWait* curr) {
     readsMemory = true;
@@ -352,9 +348,7 @@ struct EffectAnalyzer
     // write.
     writesMemory = true;
     isAtomic = true;
-    if (!ignoreImplicitTraps) {
-      implicitTrap = true;
-    }
+    implicitTrap = true;
   }
   void visitAtomicNotify(AtomicNotify* curr) {
     // AtomicNotify doesn't strictly write memory, but it does modify the
@@ -363,9 +357,7 @@ struct EffectAnalyzer
     readsMemory = true;
     writesMemory = true;
     isAtomic = true;
-    if (!ignoreImplicitTraps) {
-      implicitTrap = true;
-    }
+    implicitTrap = true;
   }
   void visitAtomicFence(AtomicFence* curr) {
     // AtomicFence should not be reordered with any memory operations, so we set
@@ -381,9 +373,7 @@ struct EffectAnalyzer
   void visitSIMDShift(SIMDShift* curr) {}
   void visitSIMDLoad(SIMDLoad* curr) {
     readsMemory = true;
-    if (!ignoreImplicitTraps) {
-      implicitTrap = true;
-    }
+    implicitTrap = true;
   }
   void visitSIMDLoadStoreLane(SIMDLoadStoreLane* curr) {
     if (curr->isLoad()) {
@@ -391,74 +381,73 @@ struct EffectAnalyzer
     } else {
       writesMemory = true;
     }
-    if (!ignoreImplicitTraps) {
-      implicitTrap = true;
-    }
+    implicitTrap = true;
   }
   void visitMemoryInit(MemoryInit* curr) {
     writesMemory = true;
-    if (!ignoreImplicitTraps) {
-      implicitTrap = true;
-    }
+    implicitTrap = true;
   }
   void visitDataDrop(DataDrop* curr) {
     // data.drop does not actually write memory, but it does alter the size of
     // a segment, which can be noticeable later by memory.init, so we need to
     // mark it as having a global side effect of some kind.
     writesMemory = true;
-    if (!ignoreImplicitTraps) {
-      implicitTrap = true;
-    }
+    implicitTrap = true;
   }
   void visitMemoryCopy(MemoryCopy* curr) {
     readsMemory = true;
     writesMemory = true;
-    if (!ignoreImplicitTraps) {
-      implicitTrap = true;
-    }
+    implicitTrap = true;
   }
   void visitMemoryFill(MemoryFill* curr) {
     writesMemory = true;
-    if (!ignoreImplicitTraps) {
-      implicitTrap = true;
-    }
+    implicitTrap = true;
   }
   void visitConst(Const* curr) {}
   void visitUnary(Unary* curr) {
-    if (!ignoreImplicitTraps) {
-      switch (curr->op) {
-        case TruncSFloat32ToInt32:
-        case TruncSFloat32ToInt64:
-        case TruncUFloat32ToInt32:
-        case TruncUFloat32ToInt64:
-        case TruncSFloat64ToInt32:
-        case TruncSFloat64ToInt64:
-        case TruncUFloat64ToInt32:
-        case TruncUFloat64ToInt64: {
-          implicitTrap = true;
-          break;
-        }
-        default: {
-        }
+    switch (curr->op) {
+      case TruncSFloat32ToInt32:
+      case TruncSFloat32ToInt64:
+      case TruncUFloat32ToInt32:
+      case TruncUFloat32ToInt64:
+      case TruncSFloat64ToInt32:
+      case TruncSFloat64ToInt64:
+      case TruncUFloat64ToInt32:
+      case TruncUFloat64ToInt64: {
+        implicitTrap = true;
+        break;
+      }
+      default: {
       }
     }
   }
   void visitBinary(Binary* curr) {
-    if (!ignoreImplicitTraps) {
-      switch (curr->op) {
-        case DivSInt32:
-        case DivUInt32:
-        case RemSInt32:
-        case RemUInt32:
-        case DivSInt64:
-        case DivUInt64:
-        case RemSInt64:
-        case RemUInt64: {
+    switch (curr->op) {
+      case DivSInt32:
+      case DivUInt32:
+      case RemSInt32:
+      case RemUInt32:
+      case DivSInt64:
+      case DivUInt64:
+      case RemSInt64:
+      case RemUInt64: {
+        // div and rem may contain implicit trap only if RHS is
+        // non-constant or constant which equal zero or -1 for
+        // signed divisions. Reminder traps only with zero
+        // divider.
+        if (auto* c = curr->right->dynCast<Const>()) {
+          if (c->value.isZero()) {
+            implicitTrap = true;
+          } else if ((curr->op == DivSInt32 || curr->op == DivSInt64) &&
+                     c->value.getInteger() == -1LL) {
+            implicitTrap = true;
+          }
+        } else {
           implicitTrap = true;
-          break;
-        }
-        default: {
         }
+        break;
+      }
+      default: {
       }
     }
   }
@@ -496,15 +485,13 @@ struct EffectAnalyzer
     if (tryDepth == 0) {
       throws = true;
     }
-    if (!ignoreImplicitTraps) { // rethrow traps when the arg is null
-      implicitTrap = true;
-    }
+    // rethrow traps when the arg is null
+    implicitTrap = true;
   }
   void visitBrOnExn(BrOnExn* curr) {
     breakTargets.insert(curr->name);
-    if (!ignoreImplicitTraps) { // br_on_exn traps when the arg is null
-      implicitTrap = true;
-    }
+    // br_on_exn traps when the arg is null
+    implicitTrap = true;
   }
   void visitNop(Nop* curr) {}
   void visitUnreachable(Unreachable* curr) { branchesOut = true; }
diff --git a/test/passes/inlining-optimizing_optimize-level=3.txt b/test/passes/inlining-optimizing_optimize-level=3.txt
index c49fead59..42420c3c9 100644
--- a/test/passes/inlining-optimizing_optimize-level=3.txt
+++ b/test/passes/inlining-optimizing_optimize-level=3.txt
@@ -7414,19 +7414,23 @@
       (i32.const 48)
      )
     )
-    (local.set $0
+    (local.set $1
      (i32.div_u
-      (local.tee $1
-       (local.get $0)
-      )
+      (local.get $0)
       (i32.const 10)
      )
     )
-    (br_if $while-in1
+    (if
      (i32.ge_u
-      (local.get $1)
+      (local.get $0)
       (i32.const 10)
      )
+     (block
+      (local.set $0
+       (local.get $1)
+      )
+      (br $while-in1)
+     )
     )
    )
   )
diff --git a/test/passes/remove-unused-brs_shrink-level=1.txt b/test/passes/remove-unused-brs_shrink-level=1.txt
index d6d8b4704..62491431f 100644
--- a/test/passes/remove-unused-brs_shrink-level=1.txt
+++ b/test/passes/remove-unused-brs_shrink-level=1.txt
@@ -24,13 +24,13 @@
    )
   )
   (drop
-   (if (result i32)
-    (i32.const 1)
+   (select
     (i32.rem_s
      (i32.const 11)
      (i32.const 12)
     )
     (i32.const 27)
+    (i32.const 1)
    )
   )
   (drop
diff --git a/test/passes/vacuum_all-features.txt b/test/passes/vacuum_all-features.txt
index 2581f7654..0b9d70ca4 100644
--- a/test/passes/vacuum_all-features.txt
+++ b/test/passes/vacuum_all-features.txt
@@ -261,12 +261,6 @@
  )
  (func $unary-binary-may-trap
   (drop
-   (i64.div_s
-    (i64.const -1)
-    (i64.const 729618461987467893)
-   )
-  )
-  (drop
    (i64.trunc_f32_u
     (f32.const 70847791997969805621592064)
    )