Automatically discard global effects in the rare passes that add effects (#5999)

All logging/instrumentation passes need to do this, to avoid us using stale
global effects that are too low (too high is not optimal either, but at least it
cannot cause bugs).

15 files changed, 146 insertions, 0 deletions

diff --git a/src/pass.h b/src/pass.h
index 090aa8d33..da9a12b95 100644
--- a/src/pass.h
+++ b/src/pass.h
@@ -466,6 +466,13 @@ public:
   // For more details see the LocalStructuralDominance class.
   virtual bool requiresNonNullableLocalFixups() { return true; }
 
+  // Many passes can remove effects, for example, by finding some path is not
+  // reached and removing a throw or a call there. The few passes that *add*
+  // effects must mark themselves as such, so that we know to discard global
+  // effects after running them. For example, a logging pass that adds new calls
+  // to imports must override this to return true.
+  virtual bool addsEffects() { return false; }
+
   std::string name;
 
   PassRunner* getPassRunner() { return runner; }
diff --git a/src/passes/Asyncify.cpp b/src/passes/Asyncify.cpp
index d0674d97c..2412a754a 100644
--- a/src/passes/Asyncify.cpp
+++ b/src/passes/Asyncify.cpp
@@ -870,6 +870,8 @@ struct InstrumentedProxy : public Pass {
 
   bool invalidatesDWARF() override { return pass->invalidatesDWARF(); }
 
+  bool addsEffects() override { return pass->addsEffects(); }
+
   bool requiresNonNullableLocalFixups() override {
     return pass->requiresNonNullableLocalFixups();
   }
@@ -1610,6 +1612,9 @@ static std::string getFullImportName(Name module, Name base) {
 }
 
 struct Asyncify : public Pass {
+  // Adds calls.
+  bool addsEffects() override { return true; }
+
   void run(Module* module) override {
     auto& options = getPassOptions();
     bool optimize = options.optimizeLevel > 0;
diff --git a/src/passes/DeNaN.cpp b/src/passes/DeNaN.cpp
index 44dd0bf15..97f71c39f 100644
--- a/src/passes/DeNaN.cpp
+++ b/src/passes/DeNaN.cpp
@@ -32,6 +32,8 @@ namespace wasm {
 
 struct DeNaN : public WalkerPass<
                  ControlFlowWalker<DeNaN, UnifiedExpressionVisitor<DeNaN>>> {
+  // Adds calls.
+  bool addsEffects() override { return true; }
 
   Name deNan32, deNan64;
 
diff --git a/src/passes/ExtractFunction.cpp b/src/passes/ExtractFunction.cpp
index e3f2a5538..440468ede 100644
--- a/src/passes/ExtractFunction.cpp
+++ b/src/passes/ExtractFunction.cpp
@@ -58,6 +58,9 @@ static void extract(PassRunner* runner, Module* module, Name name) {
 }
 
 struct ExtractFunction : public Pass {
+  // Turns functions into imported functions.
+  bool addsEffects() override { return true; }
+
   void run(Module* module) override {
     Name name = getPassOptions().getArgument(
       "extract-function",
@@ -67,6 +70,9 @@ struct ExtractFunction : public Pass {
 };
 
 struct ExtractFunctionIndex : public Pass {
+  // See above.
+  bool addsEffects() override { return true; }
+
   void run(Module* module) override {
     std::string index =
       getPassOptions().getArgument("extract-function-index",
diff --git a/src/passes/FuncCastEmulation.cpp b/src/passes/FuncCastEmulation.cpp
index 3255208fe..23eb98a8c 100644
--- a/src/passes/FuncCastEmulation.cpp
+++ b/src/passes/FuncCastEmulation.cpp
@@ -151,6 +151,11 @@ private:
 };
 
 struct FuncCastEmulation : public Pass {
+  // Changes the ABI, which alters which indirect calls will trap (normally,
+  // this should prevent traps, but it depends on code this is linked to at
+  // runtime in the case of dynamic linking etc.).
+  bool addsEffects() override { return true; }
+
   void run(Module* module) override {
     Index numParams = std::stoul(
       getPassOptions().getArgumentOrDefault("max-func-params", "16"));
diff --git a/src/passes/I64ToI32Lowering.cpp b/src/passes/I64ToI32Lowering.cpp
index ec66b1121..0f591871b 100644
--- a/src/passes/I64ToI32Lowering.cpp
+++ b/src/passes/I64ToI32Lowering.cpp
@@ -39,6 +39,9 @@ namespace wasm {
 static Name makeHighName(Name n) { return n.toString() + "$hi"; }
 
 struct I64ToI32Lowering : public WalkerPass<PostWalker<I64ToI32Lowering>> {
+  // Adds calls to helper functions.
+  bool addsEffects() override { return true; }
+
   struct TempVar {
     TempVar(Index idx, Type ty, I64ToI32Lowering& pass)
       : idx(idx), pass(pass), moved(false), ty(ty) {}
diff --git a/src/passes/InstrumentLocals.cpp b/src/passes/InstrumentLocals.cpp
index 311d4a27b..bf5de644c 100644
--- a/src/passes/InstrumentLocals.cpp
+++ b/src/passes/InstrumentLocals.cpp
@@ -68,6 +68,9 @@ Name set_funcref("set_funcref");
 Name set_externref("set_externref");
 
 struct InstrumentLocals : public WalkerPass<PostWalker<InstrumentLocals>> {
+  // Adds calls to new imports.
+  bool addsEffects() override { return true; }
+
   void visitLocalGet(LocalGet* curr) {
     Builder builder(*getModule());
     Name import;
diff --git a/src/passes/InstrumentMemory.cpp b/src/passes/InstrumentMemory.cpp
index 486bc7c1f..b3c9aebbd 100644
--- a/src/passes/InstrumentMemory.cpp
+++ b/src/passes/InstrumentMemory.cpp
@@ -97,6 +97,9 @@ static Name array_set_index("array_set_index");
 // TODO: Add support for atomicRMW/cmpxchg
 
 struct InstrumentMemory : public WalkerPass<PostWalker<InstrumentMemory>> {
+  // Adds calls to new imports.
+  bool addsEffects() override { return true; }
+
   void visitLoad(Load* curr) {
     id++;
     Builder builder(*getModule());
diff --git a/src/passes/LegalizeJSInterface.cpp b/src/passes/LegalizeJSInterface.cpp
index 6667862f4..3ca6b7095 100644
--- a/src/passes/LegalizeJSInterface.cpp
+++ b/src/passes/LegalizeJSInterface.cpp
@@ -54,6 +54,9 @@ static Name GET_TEMP_RET_IMPORT("getTempRet0");
 static Name SET_TEMP_RET_IMPORT("setTempRet0");
 
 struct LegalizeJSInterface : public Pass {
+  // Adds calls to new imports.
+  bool addsEffects() override { return true; }
+
   bool full;
 
   LegalizeJSInterface(bool full) : full(full) {}
diff --git a/src/passes/LogExecution.cpp b/src/passes/LogExecution.cpp
index 63ada5819..1b90842bc 100644
--- a/src/passes/LogExecution.cpp
+++ b/src/passes/LogExecution.cpp
@@ -39,6 +39,9 @@ namespace wasm {
 Name LOGGER("log_execution");
 
 struct LogExecution : public WalkerPass<PostWalker<LogExecution>> {
+  // Adds calls to new imports.
+  bool addsEffects() override { return true; }
+
   void visitLoop(Loop* curr) { curr->body = makeLogCall(curr->body); }
 
   void visitReturn(Return* curr) { replaceCurrent(makeLogCall(curr)); }
diff --git a/src/passes/SafeHeap.cpp b/src/passes/SafeHeap.cpp
index 0e9e8aeef..b2f7db567 100644
--- a/src/passes/SafeHeap.cpp
+++ b/src/passes/SafeHeap.cpp
@@ -131,6 +131,8 @@ static std::set<Name> findCalledFunctions(Module* module, Name startFunc) {
 }
 
 struct SafeHeap : public Pass {
+  // Adds calls to new imports.
+  bool addsEffects() override { return true; }
 
   void run(Module* module) override {
     assert(!module->memories.empty());
diff --git a/src/passes/SpillPointers.cpp b/src/passes/SpillPointers.cpp
index f496b8c79..3af7039cd 100644
--- a/src/passes/SpillPointers.cpp
+++ b/src/passes/SpillPointers.cpp
@@ -37,6 +37,9 @@ struct SpillPointers
   : public WalkerPass<LivenessWalker<SpillPointers, Visitor<SpillPointers>>> {
   bool isFunctionParallel() override { return true; }
 
+  // Adds writes to memory.
+  bool addsEffects() override { return true; }
+
   std::unique_ptr<Pass> create() override {
     return std::make_unique<SpillPointers>();
   }
diff --git a/src/passes/StackCheck.cpp b/src/passes/StackCheck.cpp
index 9fba683b8..229a97f26 100644
--- a/src/passes/StackCheck.cpp
+++ b/src/passes/StackCheck.cpp
@@ -126,6 +126,9 @@ private:
 };
 
 struct StackCheck : public Pass {
+  // Adds calls to new imports.
+  bool addsEffects() override { return true; }
+
   void run(Module* module) override {
     Global* stackPointer = getStackPointerGlobal(*module);
     if (!stackPointer) {
diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp
index 3880519da..9b03310b5 100644
--- a/src/passes/pass.cpp
+++ b/src/passes/pass.cpp
@@ -1056,6 +1056,11 @@ void PassRunner::handleAfterEffects(Pass* pass, Function* func) {
   if (pass->requiresNonNullableLocalFixups()) {
     TypeUpdating::handleNonDefaultableLocals(func, *wasm);
   }
+
+  if (options.funcEffectsMap && pass->addsEffects()) {
+    // Effects were added, so discard any computed effects for this function.
+    options.funcEffectsMap->erase(func->name);
+  }
 }
 
 int PassRunner::getPassDebug() {
diff --git a/test/lit/passes/instrument-locals_effects.wast b/test/lit/passes/instrument-locals_effects.wast
new file mode 100644
index 000000000..5c0c61d06
--- /dev/null
+++ b/test/lit/passes/instrument-locals_effects.wast
@@ -0,0 +1,93 @@
+;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
+;; NOTE: This test was ported using port_passes_tests_to_lit.py and could be cleaned up.
+
+;; Test that this instrumentation pass discards global effects, which it must do
+;; as it adds calls to imports. We can optimize below when we do not instrument,
+;; and cannot when we do.
+;;
+;; In NO_INSTRMT below we run another pass, just to prove that not every pass
+;; causes global effects to be discarded.
+
+;; RUN: foreach %s %t wasm-opt --generate-global-effects --instrument-locals --vacuum -S -o - | filecheck %s --check-prefix INSTRUMENT
+;; RUN: foreach %s %t wasm-opt --generate-global-effects --coalesce-locals   --vacuum -S -o - | filecheck %s --check-prefix NO_INSTRMT
+
+(module
+  ;; INSTRUMENT:      (type $0 (func))
+
+  ;; INSTRUMENT:      (type $1 (func (param i32 i32 i32) (result i32)))
+
+  ;; INSTRUMENT:      (type $2 (func (param i32 i32 i64) (result i64)))
+
+  ;; INSTRUMENT:      (type $3 (func (param i32 i32 f32) (result f32)))
+
+  ;; INSTRUMENT:      (type $4 (func (param i32 i32 f64) (result f64)))
+
+  ;; INSTRUMENT:      (import "env" "get_i32" (func $get_i32 (param i32 i32 i32) (result i32)))
+
+  ;; INSTRUMENT:      (import "env" "get_i64" (func $get_i64 (param i32 i32 i64) (result i64)))
+
+  ;; INSTRUMENT:      (import "env" "get_f32" (func $get_f32 (param i32 i32 f32) (result f32)))
+
+  ;; INSTRUMENT:      (import "env" "get_f64" (func $get_f64 (param i32 i32 f64) (result f64)))
+
+  ;; INSTRUMENT:      (import "env" "set_i32" (func $set_i32 (param i32 i32 i32) (result i32)))
+
+  ;; INSTRUMENT:      (import "env" "set_i64" (func $set_i64 (param i32 i32 i64) (result i64)))
+
+  ;; INSTRUMENT:      (import "env" "set_f32" (func $set_f32 (param i32 i32 f32) (result f32)))
+
+  ;; INSTRUMENT:      (import "env" "set_f64" (func $set_f64 (param i32 i32 f64) (result f64)))
+
+  ;; INSTRUMENT:      (func $past-get
+  ;; INSTRUMENT-NEXT:  (call $use-local)
+  ;; INSTRUMENT-NEXT:  (call $nop)
+  ;; INSTRUMENT-NEXT: )
+  ;; NO_INSTRMT:      (type $0 (func))
+
+  ;; NO_INSTRMT:      (func $past-get
+  ;; NO_INSTRMT-NEXT:  (nop)
+  ;; NO_INSTRMT-NEXT: )
+  (func $past-get
+    ;; The called function only sets a local, so we can vacuum it away using
+    ;; global effects. But, if we instrumented it, then it gets an import call,
+    ;; which we should not remove.
+    (call $use-local)
+    ;; If we instrument, then we discard all global effects, even of a non-
+    ;; instrumented function (we don't track individual functions), so we'll
+    ;; lose the ability to vacuum this function away as well in that case.
+    (call $nop)
+  )
+
+  ;; INSTRUMENT:      (func $use-local
+  ;; INSTRUMENT-NEXT:  (local $x i32)
+  ;; INSTRUMENT-NEXT:  (local.set $x
+  ;; INSTRUMENT-NEXT:   (call $set_i32
+  ;; INSTRUMENT-NEXT:    (i32.const 0)
+  ;; INSTRUMENT-NEXT:    (i32.const 0)
+  ;; INSTRUMENT-NEXT:    (i32.const 10)
+  ;; INSTRUMENT-NEXT:   )
+  ;; INSTRUMENT-NEXT:  )
+  ;; INSTRUMENT-NEXT: )
+  ;; NO_INSTRMT:      (func $use-local
+  ;; NO_INSTRMT-NEXT:  (local $0 i32)
+  ;; NO_INSTRMT-NEXT:  (nop)
+  ;; NO_INSTRMT-NEXT: )
+  (func $use-local
+    (local $x i32)
+    ;; This function uses a local, and will get instrumented with a call to log
+    ;; the value out.
+    (local.set $x
+      (i32.const 10)
+    )
+  )
+
+  ;; INSTRUMENT:      (func $nop
+  ;; INSTRUMENT-NEXT:  (nop)
+  ;; INSTRUMENT-NEXT: )
+  ;; NO_INSTRMT:      (func $nop
+  ;; NO_INSTRMT-NEXT:  (nop)
+  ;; NO_INSTRMT-NEXT: )
+  (func $nop
+    ;; This function does nothing.
+  )
+)