[TNH Fuzzing] Fix some equality checks that ignored effects (#4951)

Fuzzing with TrapsNeverHappen found a bug, and then reading similar code
I found another, where we check structural equality but ignored effects. Some
things look equal but may have different values at runtime:

(foo
  (call $x)
  (call $y)
)

The arms are identical structurally but due to side effects may not be identical
in value.

3 files changed, 67 insertions, 8 deletions

diff --git a/src/passes/OptimizeInstructions.cpp b/src/passes/OptimizeInstructions.cpp
index 394a7c998..8b71da824 100644
--- a/src/passes/OptimizeInstructions.cpp
+++ b/src/passes/OptimizeInstructions.cpp
@@ -1019,7 +1019,7 @@ struct OptimizeInstructions
       if (auto* binary = curr->value->dynCast<Binary>()) {
         if ((binary->op == Abstract::getBinary(binary->type, Abstract::Mul) ||
              binary->op == Abstract::getBinary(binary->type, Abstract::DivS)) &&
-            ExpressionAnalyzer::equal(binary->left, binary->right)) {
+            areConsecutiveInputsEqual(binary->left, binary->right)) {
           return replaceCurrent(binary);
         }
         // abs(0 - x)   ==>   abs(x),
@@ -2026,13 +2026,14 @@ private:
   // simple peephole optimizations - all we care about is a single instruction
   // at a time, and its inputs).
   //
-  // This also checks that the inputs are removable.
+  // This also checks that the inputs are removable (but we do not assume the
+  // caller will always remove them).
   bool areConsecutiveInputsEqualAndRemovable(Expression* left,
                                              Expression* right) {
     // First, check for side effects. If there are any, then we can't even
     // assume things like local.get's of the same index being identical. (It is
-    // also ok to have side effects here, if we can remove them, as we are also
-    // checking if we can remove the two inputs anyhow.)
+    // also ok to have removable side effects here, see the function
+    // description.)
     auto& passOptions = getPassOptions();
     if (EffectAnalyzer(passOptions, *getModule(), left)
           .hasUnremovableSideEffects() ||
@@ -2055,9 +2056,13 @@ private:
     return true;
   }
 
-  // Check if two consecutive inputs to an instruction are equal and can be
-  // folded into the first of the two. This identifies reads from the same local
-  // variable when one of them is a "tee" operation.
+  // Check if two consecutive inputs to an instruction are equal and can also be
+  // folded into the first of the two (but we do not assume the caller will
+  // always fold them). This is similar to areConsecutiveInputsEqualAndRemovable
+  // but also identifies reads from the same local variable when the first of
+  // them is a "tee" operation and the second is a get (in which case, it is
+  // fine to remove the get, but not the tee).
+  //
   // The inputs here must be consecutive, but it is also ok to have code with no
   // side effects at all in the middle. For example, a Const in between is ok.
   bool areConsecutiveInputsEqualAndFoldable(Expression* left,
@@ -2074,6 +2079,13 @@ private:
     return areConsecutiveInputsEqualAndRemovable(left, right);
   }
 
+  // Similar to areConsecutiveInputsEqualAndFoldable, but only checks that they
+  // are equal (and not that they are foldable).
+  bool areConsecutiveInputsEqual(Expression* left, Expression* right) {
+    // TODO: optimize cases that must be equal but are *not* foldable.
+    return areConsecutiveInputsEqualAndFoldable(left, right);
+  }
+
   // Canonicalizing the order of a symmetric binary helps us
   // write more concise pattern matching code elsewhere.
   void canonicalize(Binary* binary) {
@@ -3839,7 +3851,7 @@ private:
     auto& options = getPassOptions();
 
     if (options.ignoreImplicitTraps || options.trapsNeverHappen) {
-      if (ExpressionAnalyzer::equal(memCopy->dest, memCopy->source)) {
+      if (areConsecutiveInputsEqual(memCopy->dest, memCopy->source)) {
         // memory.copy(x, x, sz)  ==>  {drop(x), drop(x), drop(sz)}
         Builder builder(*getModule());
         return builder.makeBlock({builder.makeDrop(memCopy->dest),
diff --git a/test/lit/passes/optimize-instructions-ignore-traps.wast b/test/lit/passes/optimize-instructions-ignore-traps.wast
index cca6061f4..a199f1b46 100644
--- a/test/lit/passes/optimize-instructions-ignore-traps.wast
+++ b/test/lit/passes/optimize-instructions-ignore-traps.wast
@@ -5,8 +5,13 @@
 (module
   ;; CHECK:      (type $0 (func (param i32 i32) (result i32)))
   (type $0 (func (param i32 i32) (result i32)))
+  ;; CHECK:      (import "a" "b" (func $get-i32 (result i32)))
+
   ;; CHECK:      (memory $0 0)
   (memory $0 0)
+
+  (import "a" "b" (func $get-i32 (result i32)))
+
   ;; CHECK:      (func $conditionals (param $0 i32) (param $1 i32) (result i32)
   ;; CHECK-NEXT:  (local $2 i32)
   ;; CHECK-NEXT:  (local $3 i32)
@@ -732,6 +737,19 @@
  ;; CHECK-NEXT:    (local.get $src)
  ;; CHECK-NEXT:   )
  ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT:  (memory.copy
+ ;; CHECK-NEXT:   (call $get-i32)
+ ;; CHECK-NEXT:   (call $get-i32)
+ ;; CHECK-NEXT:   (local.get $sz)
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT:  (block
+ ;; CHECK-NEXT:   (drop
+ ;; CHECK-NEXT:    (call $get-i32)
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:   (drop
+ ;; CHECK-NEXT:    (call $get-i32)
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  )
  ;; CHECK-NEXT: )
  (func $optimize-bulk-memory-copy (param $dst i32) (param $src i32) (param $sz i32)
   (memory.copy  ;; nop
@@ -745,6 +763,16 @@
     (local.get $src)
     (i32.const 0)
   )
+  (memory.copy  ;; not a nop as the runtime dst/src may differ
+    (call $get-i32)
+    (call $get-i32)
+    (local.get $sz)
+  )
+  (memory.copy  ;; as above, but with a size of 0. the calls must remain.
+    (call $get-i32)
+    (call $get-i32)
+    (i32.const 0)
+  )
  )
 
  ;; CHECK:      (func $optimize-bulk-memory-fill (param $dst i32) (param $val i32) (param $sz i32)
diff --git a/test/lit/passes/optimize-instructions.wast b/test/lit/passes/optimize-instructions.wast
index 336bb63ae..73e7fd286 100644
--- a/test/lit/passes/optimize-instructions.wast
+++ b/test/lit/passes/optimize-instructions.wast
@@ -5,6 +5,10 @@
   (memory 0)
   ;; CHECK:      (type $0 (func (param i32 i64)))
   (type $0 (func (param i32 i64)))
+
+  ;; CHECK:      (import "a" "b" (func $get-f64 (result f64)))
+  (import "a" "b" (func $get-f64 (result f64)))
+
   ;; CHECK:      (func $and-and (param $i1 i32) (result i32)
   ;; CHECK-NEXT:  (i32.and
   ;; CHECK-NEXT:   (local.get $i1)
@@ -13238,6 +13242,14 @@
   ;; CHECK-NEXT:   )
   ;; CHECK-NEXT:  )
   ;; CHECK-NEXT:  (drop
+  ;; CHECK-NEXT:   (f64.abs
+  ;; CHECK-NEXT:    (f64.mul
+  ;; CHECK-NEXT:     (call $get-f64)
+  ;; CHECK-NEXT:     (call $get-f64)
+  ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT:  (drop
   ;; CHECK-NEXT:   (f64.div
   ;; CHECK-NEXT:    (local.get $x0)
   ;; CHECK-NEXT:    (local.get $x0)
@@ -13378,6 +13390,13 @@
         (local.get $y0)
       )
     ))
+    ;; this one cannot be optimized as the runtime values may differ
+    (drop (f64.abs
+      (f64.mul
+        (call $get-f64)
+        (call $get-f64)
+      )
+    ))
 
     ;; abs(x / x)   ==>   x / x
     (drop (f64.abs