[StackIR] Run StackIR during binary writing and not as a pass (#6568)

Previously we had passes --generate-stack-ir, --optimize-stack-ir, --print-stack-ir that could be run like any other passes. After generating StackIR it was stashed on the function and invalidated if we modified BinaryenIR. If it wasn't invalidated then it was used during binary writing. This PR switches things so that we optionally generate, optimize, and print StackIR only during binary writing. It also removes all traces of StackIR from wasm.h - after this, StackIR is a feature of binary writing (and printing) logic only. This is almost NFC, but there are some minor noticeable differences: 1. We no longer print has StackIR in the text format when we see it is there. It will not be there during normal printing, as it is only present during binary writing. (but --print-stack-ir still works as before; as mentioned above it runs during writing). 2. --generate/optimize/print-stack-ir change from being passes to being flags that control that behavior instead. As passes, their order on the commandline mattered, while now it does not, and they only "globally" affect things during writing. 3. The C API changes slightly, as there is no need to pass it an option "optimize" to the StackIR APIs. Whether we optimize is handled by --optimize-stack-ir which is set like other optimization flags on the PassOptions object, so we don't need the old option to those C APIs. The main benefit here is simplifying the code, so we don't need to think about StackIR in more places than just binary writing. That may also allow future improvements to our usage of StackIR.
author: Alon Zakai <azakai@google.com> 2024-05-09 15:00:13 -0700
committer: GitHub <noreply@github.com> 2024-05-09 15:00:13 -0700
commit: 7b2e0190213487b5d2505fe86aa9bbbd30e80fcc (patch)
tree: 2ae614b27102d83452b0f075612c7558c4493aa6 /src/passes/pass.cpp
parent: 006181bb98118c70d36e84e6f1f72b5d60264817 (diff)
download: binaryen-7b2e0190213487b5d2505fe86aa9bbbd30e80fcc.tar.gz
binaryen-7b2e0190213487b5d2505fe86aa9bbbd30e80fcc.tar.bz2
binaryen-7b2e0190213487b5d2505fe86aa9bbbd30e80fcc.zip
1 files changed, 4 insertions, 132 deletions
diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp
index 4a4a9c558..c5a2bcc55 100644
--- a/src/passes/pass.cpp
+++ b/src/passes/pass.cpp
@@ -179,8 +179,6 @@ void PassRegistry::registerPasses() {
                "generate global effect info (helps later passes)",
                createGenerateGlobalEffectsPass);
   registerPass(
-    "generate-stack-ir", "generate Stack IR", createGenerateStackIRPass);
-  registerPass(
     "global-refining", "refine the types of globals", createGlobalRefiningPass);
   registerPass(
     "gsi", "globally optimize struct values", createGlobalStructInferencePass);
@@ -321,8 +319,6 @@ void PassRegistry::registerPasses() {
   registerPass("optimize-instructions",
                "optimizes instruction combinations",
                createOptimizeInstructionsPass);
-  registerPass(
-    "optimize-stack-ir", "optimize Stack IR", createOptimizeStackIRPass);
 // Outlining currently relies on LLVM's SuffixTree, which we can't rely upon
 // when building Binaryen for Emscripten.
 #ifndef SKIP_OUTLINING
@@ -369,9 +365,6 @@ void PassRegistry::registerPasses() {
   registerPass(
     "symbolmap", "(alias for print-function-map)", createPrintFunctionMapPass);
 
-  registerPass("print-stack-ir",
-               "print out Stack IR (useful for internal debugging)",
-               createPrintStackIRPass);
   registerPass("propagate-globals-globally",
                "propagate global values to other globals (useful for tests)",
                createPropagateGlobalsGloballyPass);
@@ -736,15 +729,9 @@ void PassRunner::addDefaultGlobalOptimizationPostPasses() {
   }
   // may allow more inlining/dae/etc., need --converge for that
   addIfNoDWARFIssues("directize");
-  // perform Stack IR optimizations here, at the very end of the
-  // optimization pipeline
-  if (options.optimizeLevel >= 2 || options.shrinkLevel >= 1) {
-    addIfNoDWARFIssues("generate-stack-ir");
-    addIfNoDWARFIssues("optimize-stack-ir");
-  }
 }
 
-static void dumpWasm(Name name, Module* wasm) {
+static void dumpWasm(Name name, Module* wasm, const PassOptions& options) {
   static int counter = 0;
   std::string numstr = std::to_string(counter++);
   while (numstr.size() < 3) {
@@ -757,7 +744,7 @@ static void dumpWasm(Name name, Module* wasm) {
 #endif
   fullName += numstr + "-" + name.toString();
   Colors::setEnabled(false);
-  ModuleWriter writer;
+  ModuleWriter writer(options);
   writer.setDebugInfo(true);
   writer.writeBinary(*wasm, fullName + ".wasm");
 }
@@ -780,7 +767,7 @@ void PassRunner::run() {
       padding = std::max(padding, pass->name.size());
     }
     if (passDebug >= 3 && !isNested) {
-      dumpWasm("before", wasm);
+      dumpWasm("before", wasm, options);
     }
     for (auto& pass : passes) {
       // ignoring the time, save a printout of the module before, in case this
@@ -824,7 +811,7 @@ void PassRunner::run() {
         }
       }
       if (passDebug >= 3) {
-        dumpWasm(pass->name, wasm);
+        dumpWasm(pass->name, wasm, options);
       }
     }
     std::cerr << "[PassRunner] " << what << " took " << totalTime.count()
@@ -908,100 +895,6 @@ void PassRunner::doAdd(std::unique_ptr<Pass> pass) {
 
 void PassRunner::clear() { passes.clear(); }
 
-// Checks that the state is valid before and after a
-// pass runs on a function. We run these extra checks when
-// pass-debug mode is enabled.
-struct AfterEffectFunctionChecker {
-  Function* func;
-  Name name;
-
-  // Check Stack IR state: if the main IR changes, there should be no
-  // stack IR, as the stack IR would be wrong.
-  bool beganWithStackIR;
-  size_t originalFunctionHash;
-
-  // In the creator we can scan the state of the module and function before the
-  // pass runs.
-  AfterEffectFunctionChecker(Function* func) : func(func), name(func->name) {
-    beganWithStackIR = func->stackIR != nullptr;
-    if (beganWithStackIR) {
-      originalFunctionHash = FunctionHasher::hashFunction(func);
-    }
-  }
-
-  // This is called after the pass is run, at which time we can check things.
-  void check() {
-    assert(func->name == name); // no global module changes should have occurred
-    if (beganWithStackIR && func->stackIR) {
-      auto after = FunctionHasher::hashFunction(func);
-      if (after != originalFunctionHash) {
-        Fatal() << "[PassRunner] PASS_DEBUG check failed: had Stack IR before "
-                   "and after the pass ran, and the pass modified the main IR, "
-                   "which invalidates Stack IR - pass should have been marked "
-                   "'modifiesBinaryenIR'";
-      }
-    }
-  }
-};
-
-// Runs checks on the entire module, in a non-function-parallel pass.
-// In particular, in such a pass functions may be removed or renamed, track
-// that.
-struct AfterEffectModuleChecker {
-  Module* module;
-
-  std::vector<AfterEffectFunctionChecker> checkers;
-
-  bool beganWithAnyStackIR;
-
-  AfterEffectModuleChecker(Module* module) : module(module) {
-    for (auto& func : module->functions) {
-      checkers.emplace_back(func.get());
-    }
-    beganWithAnyStackIR = hasAnyStackIR();
-  }
-
-  void check() {
-    if (beganWithAnyStackIR && hasAnyStackIR()) {
-      // If anything changed to the functions, that's not good.
-      if (checkers.size() != module->functions.size()) {
-        error();
-      }
-      for (Index i = 0; i < checkers.size(); i++) {
-        // Did a pointer change? (a deallocated function could cause that)
-        if (module->functions[i].get() != checkers[i].func ||
-            module->functions[i]->body != checkers[i].func->body) {
-          error();
-        }
-        // Did a name change?
-        if (module->functions[i]->name != checkers[i].name) {
-          error();
-        }
-      }
-      // Global function state appears to not have been changed: the same
-      // functions are there. Look into their contents.
-      for (auto& checker : checkers) {
-        checker.check();
-      }
-    }
-  }
-
-  void error() {
-    Fatal() << "[PassRunner] PASS_DEBUG check failed: had Stack IR before and "
-               "after the pass ran, and the pass modified global function "
-               "state - pass should have been marked 'modifiesBinaryenIR'";
-  }
-
-  bool hasAnyStackIR() {
-    for (auto& func : module->functions) {
-      if (func->stackIR) {
-        return true;
-      }
-    }
-    return false;
-  }
-};
-
 void PassRunner::runPass(Pass* pass) {
   assert(!pass->isFunctionParallel());
 
@@ -1009,20 +902,12 @@ void PassRunner::runPass(Pass* pass) {
     return;
   }
 
-  std::unique_ptr<AfterEffectModuleChecker> checker;
-  if (getPassDebug()) {
-    checker = std::unique_ptr<AfterEffectModuleChecker>(
-      new AfterEffectModuleChecker(wasm));
-  }
   // Passes can only be run once and we deliberately do not clear the pass
   // runner after running the pass, so there must not already be a runner here.
   assert(!pass->getPassRunner());
   pass->setPassRunner(this);
   pass->run(wasm);
   handleAfterEffects(pass);
-  if (getPassDebug()) {
-    checker->check();
-  }
 }
 
 void PassRunner::runPassOnFunction(Pass* pass, Function* func) {
@@ -1051,21 +936,12 @@ void PassRunner::runPassOnFunction(Pass* pass, Function* func) {
     bodyBefore << *func->body << '\n';
   }
 
-  std::unique_ptr<AfterEffectFunctionChecker> checker;
-  if (passDebug) {
-    checker = std::make_unique<AfterEffectFunctionChecker>(func);
-  }
-
   // Function-parallel passes get a new instance per function
   auto instance = pass->create();
   instance->setPassRunner(this);
   instance->runOnFunction(wasm, func);
   handleAfterEffects(pass, func);
 
-  if (passDebug) {
-    checker->check();
-  }
-
   if (extraFunctionValidation) {
     if (!WasmValidator().validate(func, *wasm, WasmValidator::Minimal)) {
       Fatal() << "Last nested function-parallel pass (" << pass->name
@@ -1095,10 +971,6 @@ void PassRunner::handleAfterEffects(Pass* pass, Function* func) {
     return;
   }
 
-  // If Binaryen IR is modified, Stack IR must be cleared - it would
-  // be out of sync in a potentially dangerous way.
-  func->stackIR.reset(nullptr);
-
   if (pass->requiresNonNullableLocalFixups()) {
     TypeUpdating::handleNonDefaultableLocals(func, *wasm);
   }
author	Alon Zakai <azakai@google.com>	2024-05-09 15:00:13 -0700
committer	GitHub <noreply@github.com>	2024-05-09 15:00:13 -0700
commit	7b2e0190213487b5d2505fe86aa9bbbd30e80fcc (patch)
tree	2ae614b27102d83452b0f075612c7558c4493aa6 /src/passes/pass.cpp
parent	006181bb98118c70d36e84e6f1f72b5d60264817 (diff)
download	binaryen-7b2e0190213487b5d2505fe86aa9bbbd30e80fcc.tar.gz binaryen-7b2e0190213487b5d2505fe86aa9bbbd30e80fcc.tar.bz2 binaryen-7b2e0190213487b5d2505fe86aa9bbbd30e80fcc.zip