[Wasm GC] Fix RemoveUnusedModuleEffects struct field reads with call.without.effects (#5477)

If we see

(struct.new $A
  (call $call.without.effects ;; intrinsic
    (ref.func $getter)
  )
)

then we can ignore side effects in that call, as the intrinsic tells us to. However,
that function is still called (it will be called normally, after intrinsics are lowered
away), and we should not remove it.

That is, such an intrinsic call can be removed, but it cannot be left as it is
and also ignored as if it does not exist.

2 files changed, 104 insertions, 5 deletions

diff --git a/src/passes/RemoveUnusedModuleElements.cpp b/src/passes/RemoveUnusedModuleElements.cpp
index 2ae03391b..358add76b 100644
--- a/src/passes/RemoveUnusedModuleElements.cpp
+++ b/src/passes/RemoveUnusedModuleElements.cpp
@@ -38,6 +38,7 @@
 #include <memory>
 
 #include "ir/element-utils.h"
+#include "ir/find_all.h"
 #include "ir/intrinsics.h"
 #include "ir/module-utils.h"
 #include "ir/subtypes.h"
@@ -492,11 +493,36 @@ struct Analyzer {
     for (Index i = 0; i < new_->operands.size(); i++) {
       auto* operand = new_->operands[i];
       auto structField = StructField{type, i};
-      if (readStructFields.count(structField) ||
-          EffectAnalyzer(options, *module, operand).hasSideEffects()) {
-        // This data can be read, so just walk it. Or, this has side effects,
-        // which is tricky to reason about - the side effects must happen even
-        // if we never read the struct field - so give up and consider it used.
+
+      // If this struct field has already been read, then we should use the
+      // contents there now.
+      auto useOperandNow = readStructFields.count(structField);
+
+      // Side effects are tricky to reason about - the side effects must happen
+      // even if we never read the struct field - so give up and consider it
+      // used.
+      if (!useOperandNow) {
+        useOperandNow =
+          EffectAnalyzer(options, *module, operand).hasSideEffects();
+      }
+
+      // We must handle the call.without.effects intrinsic here in a special
+      // manner. That intrinsic is reported as having no side effects in
+      // EffectAnalyzer, but even though for optimization purposes we can ignore
+      // effects, the called code *is* actually reached, and it might have side
+      // effects. In other words, the point of the intrinsic is to temporarily
+      // ignore those effects during one phase of optimization. Or, put another
+      // way, the intrinsic lets us ignore the effects of computing some value,
+      // but we do still need to compute that value if it is received and used
+      // (if it is not received and used, other passes will remove it).
+      if (!useOperandNow) {
+        // To detect this, look for any call. A non-intrinsic call would have
+        // already been detected when we looked for side effects, so this will
+        // only notice intrinsic calls.
+        useOperandNow = !FindAll<Call>(operand).list.empty();
+      }
+
+      if (useOperandNow) {
         use(operand);
       } else {
         // This data does not need to be read now, but might be read later. Note
diff --git a/test/lit/passes/remove-unused-module-elements-refs.wast b/test/lit/passes/remove-unused-module-elements-refs.wast
index 2bbba0cad..61ccb4daf 100644
--- a/test/lit/passes/remove-unused-module-elements-refs.wast
+++ b/test/lit/passes/remove-unused-module-elements-refs.wast
@@ -1841,3 +1841,76 @@
   (func $f (type $void)
   )
 )
+
+;; The call.without.effects intrinsic reports itself as having no side effects.
+;; We do still need to consider the target as being called, however, even if it
+;; is in a struct field.
+(module
+  ;; CHECK:      (type $funcref_=>_i32 (func (param funcref) (result i32)))
+
+  ;; CHECK:      (type $none_=>_none (func))
+
+  ;; CHECK:      (type $A (struct (field i32)))
+  ;; OPEN_WORLD:      (type $funcref_=>_i32 (func (param funcref) (result i32)))
+
+  ;; OPEN_WORLD:      (type $none_=>_none (func))
+
+  ;; OPEN_WORLD:      (type $A (struct (field i32)))
+  (type $A (struct (field i32)))
+
+  ;; CHECK:      (type $none_=>_i32 (func (result i32)))
+
+  ;; CHECK:      (import "binaryen-intrinsics" "call.without.effects" (func $call.without.effects (param funcref) (result i32)))
+  ;; OPEN_WORLD:      (type $none_=>_i32 (func (result i32)))
+
+  ;; OPEN_WORLD:      (import "binaryen-intrinsics" "call.without.effects" (func $call.without.effects (param funcref) (result i32)))
+  (import "binaryen-intrinsics" "call.without.effects" (func $call.without.effects (param funcref) (result i32)))
+
+  ;; CHECK:      (elem declare func $getter)
+
+  ;; CHECK:      (export "main" (func $main))
+
+  ;; CHECK:      (func $main (type $none_=>_none)
+  ;; CHECK-NEXT:  (drop
+  ;; CHECK-NEXT:   (struct.new $A
+  ;; CHECK-NEXT:    (call $call.without.effects
+  ;; CHECK-NEXT:     (ref.func $getter)
+  ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )
+  ;; OPEN_WORLD:      (elem declare func $getter)
+
+  ;; OPEN_WORLD:      (export "main" (func $main))
+
+  ;; OPEN_WORLD:      (func $main (type $none_=>_none)
+  ;; OPEN_WORLD-NEXT:  (drop
+  ;; OPEN_WORLD-NEXT:   (struct.new $A
+  ;; OPEN_WORLD-NEXT:    (call $call.without.effects
+  ;; OPEN_WORLD-NEXT:     (ref.func $getter)
+  ;; OPEN_WORLD-NEXT:    )
+  ;; OPEN_WORLD-NEXT:   )
+  ;; OPEN_WORLD-NEXT:  )
+  ;; OPEN_WORLD-NEXT: )
+  (func $main (export "main")
+    (drop
+      (struct.new $A
+        (call $call.without.effects
+          (ref.func $getter)
+        )
+      )
+    )
+  )
+
+  ;; CHECK:      (func $getter (type $none_=>_i32) (result i32)
+  ;; CHECK-NEXT:  (i32.const 42)
+  ;; CHECK-NEXT: )
+  ;; OPEN_WORLD:      (func $getter (type $none_=>_i32) (result i32)
+  ;; OPEN_WORLD-NEXT:  (i32.const 42)
+  ;; OPEN_WORLD-NEXT: )
+  (func $getter (result i32)
+    ;; This function body should not be turned into an unreachable. It is
+    ;; reached from $main, even though the call is marked as not having effects.
+    (i32.const 42)
+  )
+)