From 9b29fb7497a8a03de4a65c8c07d41f45959a84c2 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 2 Feb 2023 10:56:53 -0800 Subject: [PATCH 1/4] show bug --- .../remove-unused-module-elements-refs.wast | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/test/lit/passes/remove-unused-module-elements-refs.wast b/test/lit/passes/remove-unused-module-elements-refs.wast index 2bbba0cad13..00dea21e007 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: (unreachable) + ;; 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) + ) +) From d342837f7293e9af85b9ddbade4aa951ba49a31e Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 2 Feb 2023 11:04:53 -0800 Subject: [PATCH 2/4] fix --- src/passes/RemoveUnusedModuleElements.cpp | 34 ++++++++++++++++--- .../remove-unused-module-elements-refs.wast | 2 +- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/src/passes/RemoveUnusedModuleElements.cpp b/src/passes/RemoveUnusedModuleElements.cpp index 2ae03391b71..6abcd7d829a 100644 --- a/src/passes/RemoveUnusedModuleElements.cpp +++ b/src/passes/RemoveUnusedModuleElements.cpp @@ -492,11 +492,35 @@ 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. We cannot ignore + // them here as if we did we might consider the called code unreachable + // when it won't be after the intrinsic is lowered away. + 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(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 00dea21e007..61ccb4daf62 100644 --- a/test/lit/passes/remove-unused-module-elements-refs.wast +++ b/test/lit/passes/remove-unused-module-elements-refs.wast @@ -1903,7 +1903,7 @@ ) ;; CHECK: (func $getter (type $none_=>_i32) (result i32) - ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: (i32.const 42) ;; CHECK-NEXT: ) ;; OPEN_WORLD: (func $getter (type $none_=>_i32) (result i32) ;; OPEN_WORLD-NEXT: (i32.const 42) From e6a1d26b7e6aaa3dd08015a6f93a8f0a8e54e39c Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 2 Feb 2023 11:06:09 -0800 Subject: [PATCH 3/4] fix --- src/passes/RemoveUnusedModuleElements.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/passes/RemoveUnusedModuleElements.cpp b/src/passes/RemoveUnusedModuleElements.cpp index 6abcd7d829a..e81822d0bcf 100644 --- a/src/passes/RemoveUnusedModuleElements.cpp +++ b/src/passes/RemoveUnusedModuleElements.cpp @@ -38,6 +38,7 @@ #include #include "ir/element-utils.h" +#include "ir/find_all.h" #include "ir/intrinsics.h" #include "ir/module-utils.h" #include "ir/subtypes.h" From 3e99938c4659454d9c8ffd919dbcf374948ebbda Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 2 Feb 2023 11:09:11 -0800 Subject: [PATCH 4/4] clarify --- src/passes/RemoveUnusedModuleElements.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/passes/RemoveUnusedModuleElements.cpp b/src/passes/RemoveUnusedModuleElements.cpp index e81822d0bcf..358add76b13 100644 --- a/src/passes/RemoveUnusedModuleElements.cpp +++ b/src/passes/RemoveUnusedModuleElements.cpp @@ -511,9 +511,10 @@ struct Analyzer { // 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. We cannot ignore - // them here as if we did we might consider the called code unreachable - // when it won't be after the intrinsic is lowered away. + // 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