Simpify PassRunner.add() and automatically parallelize parallel functions (#2242)

Main change here is in pass.h, everything else is changes to work with the new API.

The add("name") remains as before, while the weird variadic add(..) which constructed the pass now just gets a std::unique_ptr of a pass. This also makes the memory management internally fully automatic. And it makes it trivial to parallelize WalkerPass::run on parallel passes.

As a benefit, this allows removing a lot of code since in many cases there is no need to create a new pass runner, and running a pass can be just a single line.

21 files changed, 63 insertions, 116 deletions

diff --git a/src/asm2wasm.h b/src/asm2wasm.h
index d4b4f8674..3f7206f4d 100644
--- a/src/asm2wasm.h
+++ b/src/asm2wasm.h
@@ -1068,10 +1068,10 @@ void Asm2WasmBuilder::processAsm(Ref ast) {
           passRunner.setValidateGlobally(false);
         }
         // run autodrop first, before optimizations
-        passRunner.add<AutoDrop>();
+        passRunner.add(make_unique<AutoDrop>());
         if (preprocessor.debugInfo) {
           // fix up debug info to better survive optimization
-          passRunner.add<AdjustDebugInfo>();
+          passRunner.add(make_unique<AdjustDebugInfo>());
         }
         // optimize relooper label variable usage at the wasm level, where it is
         // easy
@@ -1680,7 +1680,7 @@ void Asm2WasmBuilder::processAsm(Ref ast) {
   }
   // finalizeCalls also does autoDrop, which is crucial for the non-optimizing
   // case, so that the output of the first pass is valid
-  passRunner.add<FinalizeCalls>(this);
+  passRunner.add(make_unique<FinalizeCalls>(this));
   passRunner.add(ABI::getLegalizationPass(legalizeJavaScriptFFI
                                             ? ABI::LegalizationLevel::Full
                                             : ABI::LegalizationLevel::Minimal));
@@ -1697,11 +1697,11 @@ void Asm2WasmBuilder::processAsm(Ref ast) {
     if (preprocessor.debugInfo) {
       // we would have run this before if optimizing, do it now otherwise. must
       // precede ApplyDebugInfo
-      passRunner.add<AdjustDebugInfo>();
+      passRunner.add(make_unique<AdjustDebugInfo>());
     }
   }
   if (preprocessor.debugInfo) {
-    passRunner.add<ApplyDebugInfo>();
+    passRunner.add(make_unique<ApplyDebugInfo>());
     // FIXME maybe just remove the nops that were debuginfo nodes, if not
     // optimizing?
     passRunner.add("vacuum");
diff --git a/src/binaryen-c.cpp b/src/binaryen-c.cpp
index fd0ac889a..1b4defd09 100644
--- a/src/binaryen-c.cpp
+++ b/src/binaryen-c.cpp
@@ -3390,10 +3390,8 @@ void BinaryenModuleAutoDrop(BinaryenModuleRef module) {
   }
 
   Module* wasm = (Module*)module;
-  PassRunner passRunner(wasm);
-  passRunner.options = globalPassOptions;
-  passRunner.add<AutoDrop>();
-  passRunner.run();
+  PassRunner runner(wasm, globalPassOptions);
+  AutoDrop().run(&runner, wasm);
 }
 
 static BinaryenBufferSizes writeModule(BinaryenModuleRef module,
diff --git a/src/ir/flat.h b/src/ir/flat.h
index b9d272bf5..749d4532d 100644
--- a/src/ir/flat.h
+++ b/src/ir/flat.h
@@ -114,9 +114,7 @@ inline void verifyFlatness(Module* module) {
   };
 
   PassRunner runner(module);
-  runner.setIsNested(true);
-  runner.add<VerifyFlatness>();
-  runner.run();
+  VerifyFlatness().run(&runner, module);
 }
 
 } // namespace Flat
diff --git a/src/ir/module-utils.h b/src/ir/module-utils.h
index 88298fd43..16ecd54b0 100644
--- a/src/ir/module-utils.h
+++ b/src/ir/module-utils.h
@@ -319,7 +319,6 @@ template<typename T> struct ParallelFunctionMap {
 
     // Run on the implemented functions.
     struct Mapper : public WalkerPass<PostWalker<Mapper>> {
-
       bool isFunctionParallel() override { return true; }
 
       Mapper(Info* info) : info(info) {}
@@ -338,9 +337,7 @@ template<typename T> struct ParallelFunctionMap {
     Info info = {&map, work};
 
     PassRunner runner(&wasm);
-    runner.setIsNested(true);
-    runner.add<Mapper>(&info);
-    runner.run();
+    Mapper(&info).run(&runner, &wasm);
   }
 };
 
diff --git a/src/pass.h b/src/pass.h
index 5c1315844..a9b14907b 100644
--- a/src/pass.h
+++ b/src/pass.h
@@ -39,7 +39,7 @@ struct PassRegistry {
   typedef std::function<Pass*()> Creator;
 
   void registerPass(const char* name, const char* description, Creator create);
-  Pass* createPass(std::string name);
+  std::unique_ptr<Pass> createPass(std::string name);
   std::vector<std::string> getRegisteredNames();
   std::string getPassDescription(std::string name);
 
@@ -142,7 +142,7 @@ struct PassOptions {
 struct PassRunner {
   Module* wasm;
   MixedArena* allocator;
-  std::vector<Pass*> passes;
+  std::vector<std::unique_ptr<Pass>> passes;
   PassOptions options;
 
   PassRunner(Module* wasm) : wasm(wasm), allocator(&wasm->allocator) {}
@@ -163,16 +163,18 @@ struct PassRunner {
     options.validateGlobally = validate;
   }
 
+  // Add a pass using its name.
   void add(std::string passName) {
     auto pass = PassRegistry::get()->createPass(passName);
     if (!pass) {
       Fatal() << "Could not find pass: " << passName << "\n";
     }
-    doAdd(pass);
+    doAdd(std::move(pass));
   }
 
-  template<class P, class... Args> void add(Args&&... args) {
-    doAdd(new P(std::forward<Args>(args)...));
+  // Add a pass given an instance.
+  template<class P> void add(std::unique_ptr<P> pass) {
+    doAdd(std::move(pass));
   }
 
   // Adds the default set of optimization passes; this is
@@ -205,8 +207,6 @@ struct PassRunner {
   // Get the last pass that was already executed of a certain type.
   template<class P> P* getLast();
 
-  ~PassRunner();
-
   // When running a pass runner within another pass runner, this
   // flag should be set. This influences how pass debugging works,
   // and may influence other things in the future too.
@@ -227,7 +227,7 @@ protected:
   bool isNested = false;
 
 private:
-  void doAdd(Pass* pass);
+  void doAdd(std::unique_ptr<Pass> pass);
 
   void runPass(Pass* pass);
   void runPassOnFunction(Pass* pass, Function* func);
@@ -315,6 +315,17 @@ protected:
 
 public:
   void run(PassRunner* runner, Module* module) override {
+    // Parallel pass running is implemented in the PassRunner.
+    if (isFunctionParallel()) {
+      PassRunner runner(module);
+      runner.setIsNested(true);
+      std::unique_ptr<Pass> copy;
+      copy.reset(create());
+      runner.add(std::move(copy));
+      runner.run();
+      return;
+    }
+    // Single-thread running just calls the walkModule traversal.
     setPassRunner(runner);
     WalkerType::setModule(module);
     WalkerType::walkModule(module);
diff --git a/src/passes/Asyncify.cpp b/src/passes/Asyncify.cpp
index a283ac359..cb80b90c7 100644
--- a/src/passes/Asyncify.cpp
+++ b/src/passes/Asyncify.cpp
@@ -1025,7 +1025,7 @@ struct Asyncify : public Pass {
         runner.add("reorder-locals");
         runner.add("merge-blocks");
       }
-      runner.add<AsyncifyFlow>(&analyzer);
+      runner.add(make_unique<AsyncifyFlow>(&analyzer));
       runner.setIsNested(true);
       runner.setValidateGlobally(false);
       runner.run();
@@ -1040,7 +1040,7 @@ struct Asyncify : public Pass {
       if (optimize) {
         runner.addDefaultFunctionOptimizationPasses();
       }
-      runner.add<AsyncifyLocals>(&analyzer);
+      runner.add(make_unique<AsyncifyLocals>(&analyzer));
       if (optimize) {
         runner.addDefaultFunctionOptimizationPasses();
       }
diff --git a/src/passes/DeadArgumentElimination.cpp b/src/passes/DeadArgumentElimination.cpp
index 9d60efd42..789332b93 100644
--- a/src/passes/DeadArgumentElimination.cpp
+++ b/src/passes/DeadArgumentElimination.cpp
@@ -236,12 +236,7 @@ struct DAE : public Pass {
       }
     }
     // Scan all the functions.
-    {
-      PassRunner runner(module);
-      runner.setIsNested(true);
-      runner.add<DAEScanner>(&infoMap);
-      runner.run();
-    }
+    DAEScanner(&infoMap).run(runner, module);
     // Combine all the info.
     std::unordered_map<Name, std::vector<Call*>> allCalls;
     for (auto& pair : infoMap) {
diff --git a/src/passes/Directize.cpp b/src/passes/Directize.cpp
index 54315dc77..f6969cce5 100644
--- a/src/passes/Directize.cpp
+++ b/src/passes/Directize.cpp
@@ -109,12 +109,7 @@ struct Directize : public Pass {
       return;
     }
     // The table exists and is constant, so this is possible.
-    {
-      PassRunner runner(module);
-      runner.setIsNested(true);
-      runner.add<FunctionDirectizer>(&flatTable);
-      runner.run();
-    }
+    FunctionDirectizer(&flatTable).run(runner, module);
   }
 };
 
diff --git a/src/passes/DuplicateFunctionElimination.cpp b/src/passes/DuplicateFunctionElimination.cpp
index 3f048d79a..445a024e4 100644
--- a/src/passes/DuplicateFunctionElimination.cpp
+++ b/src/passes/DuplicateFunctionElimination.cpp
@@ -70,10 +70,7 @@ struct DuplicateFunctionElimination : public Pass {
       limit--;
       // Hash all the functions
       auto hashes = FunctionHasher::createMap(module);
-      PassRunner hasherRunner(module);
-      hasherRunner.setIsNested(true);
-      hasherRunner.add<FunctionHasher>(&hashes);
-      hasherRunner.run();
+      FunctionHasher(&hashes).run(runner, module);
       // Find hash-equal groups
       std::map<uint32_t, std::vector<Function*>> hashGroups;
       ModuleUtils::iterDefinedFunctions(*module, [&](Function* func) {
@@ -121,10 +118,7 @@ struct DuplicateFunctionElimination : public Pass {
                 v.end());
         module->updateMaps();
         // replace direct calls
-        PassRunner replacerRunner(module);
-        replacerRunner.setIsNested(true);
-        replacerRunner.add<FunctionReplacer>(&replacements);
-        replacerRunner.run();
+        FunctionReplacer(&replacements).run(runner, module);
         // replace in table
         for (auto& segment : module->table.segments) {
           for (auto& name : segment.data) {
diff --git a/src/passes/FuncCastEmulation.cpp b/src/passes/FuncCastEmulation.cpp
index b4e600b31..9d6923703 100644
--- a/src/passes/FuncCastEmulation.cpp
+++ b/src/passes/FuncCastEmulation.cpp
@@ -185,10 +185,7 @@ struct FuncCastEmulation : public Pass {
       }
     }
     // update call_indirects
-    PassRunner subRunner(module, runner->options);
-    subRunner.setIsNested(true);
-    subRunner.add<ParallelFuncCastEmulation>(ABIType);
-    subRunner.run();
+    ParallelFuncCastEmulation(ABIType).run(runner, module);
   }
 
 private:
diff --git a/src/passes/Inlining.cpp b/src/passes/Inlining.cpp
index 1e5089b97..a625464cd 100644
--- a/src/passes/Inlining.cpp
+++ b/src/passes/Inlining.cpp
@@ -266,9 +266,7 @@ struct Inlining : public Pass {
       infos[func->name];
     }
     PassRunner runner(module);
-    runner.setIsNested(true);
-    runner.add<FunctionInfoScanner>(&infos);
-    runner.run();
+    FunctionInfoScanner(&infos).run(&runner, module);
     // fill in global uses
     // anything exported or used in a table should not be inlined
     for (auto& ex : module->exports) {
@@ -300,12 +298,7 @@ struct Inlining : public Pass {
       state.actionsForFunction[func->name];
     }
     // find and plan inlinings
-    {
-      PassRunner runner(module);
-      runner.setIsNested(true);
-      runner.add<Planner>(&state);
-      runner.run();
-    }
+    Planner(&state).run(runner, module);
     // perform inlinings TODO: parallelize
     std::unordered_map<Name, Index> inlinedUses; // how many uses we inlined
     // which functions were inlined into
diff --git a/src/passes/LegalizeJSInterface.cpp b/src/passes/LegalizeJSInterface.cpp
index 23cceb4ef..1404f6894 100644
--- a/src/passes/LegalizeJSInterface.cpp
+++ b/src/passes/LegalizeJSInterface.cpp
@@ -119,10 +119,7 @@ struct LegalizeJSInterface : public Pass {
         }
       };
 
-      PassRunner passRunner(module);
-      passRunner.setIsNested(true);
-      passRunner.add<FixImports>(&illegalImportsToLegal);
-      passRunner.run();
+      FixImports(&illegalImportsToLegal).run(runner, module);
     }
   }
 
diff --git a/src/passes/Print.cpp b/src/passes/Print.cpp
index bae5b25d4..1f14819cf 100644
--- a/src/passes/Print.cpp
+++ b/src/passes/Print.cpp
@@ -2103,10 +2103,8 @@ Pass* createPrintStackIRPass() { return new PrintStackIR(); }
 // Print individual expressions
 
 std::ostream& WasmPrinter::printModule(Module* module, std::ostream& o) {
-  PassRunner passRunner(module);
-  passRunner.setIsNested(true);
-  passRunner.add<Printer>(&o);
-  passRunner.run();
+  PassRunner runner(module);
+  Printer(&o).run(&runner, module);
   return o;
 }
 
diff --git a/src/passes/ReorderFunctions.cpp b/src/passes/ReorderFunctions.cpp
index 5cd70c20f..05df8cbe7 100644
--- a/src/passes/ReorderFunctions.cpp
+++ b/src/passes/ReorderFunctions.cpp
@@ -62,12 +62,7 @@ struct ReorderFunctions : public Pass {
       counts[func->name];
     }
     // find counts on function calls
-    {
-      PassRunner runner(module);
-      runner.setIsNested(true);
-      runner.add<CallCountScanner>(&counts);
-      runner.run();
-    }
+    CallCountScanner(&counts).run(runner, module);
     // find counts on global usages
     if (module->start.is()) {
       counts[module->start]++;
diff --git a/src/passes/SafeHeap.cpp b/src/passes/SafeHeap.cpp
index 52b4580b5..afcfcdb5a 100644
--- a/src/passes/SafeHeap.cpp
+++ b/src/passes/SafeHeap.cpp
@@ -106,10 +106,7 @@ struct SafeHeap : public Pass {
     // add imports
     addImports(module);
     // instrument loads and stores
-    PassRunner instrumenter(module);
-    instrumenter.setIsNested(true);
-    instrumenter.add<AccessInstrumenter>();
-    instrumenter.run();
+    AccessInstrumenter().run(runner, module);
     // add helper checking funcs and imports
     addGlobals(module, module->features);
   }
diff --git a/src/passes/SimplifyGlobals.cpp b/src/passes/SimplifyGlobals.cpp
index 992126cfe..a111f31b5 100644
--- a/src/passes/SimplifyGlobals.cpp
+++ b/src/passes/SimplifyGlobals.cpp
@@ -119,11 +119,7 @@ struct SimplifyGlobals : public Pass {
         map[ex->value].exported = true;
       }
     }
-    {
-      PassRunner subRunner(module, runner->options);
-      subRunner.add<GlobalUseScanner>(&map);
-      subRunner.run();
-    }
+    GlobalUseScanner(&map).run(runner, module);
     // We now know which are immutable in practice.
     for (auto& global : module->globals) {
       auto& info = map[global->name];
@@ -157,9 +153,7 @@ struct SimplifyGlobals : public Pass {
         }
       }
       // Apply to the gets.
-      PassRunner subRunner(module, runner->options);
-      subRunner.add<GlobalUseModifier>(&copiedParentMap);
-      subRunner.run();
+      GlobalUseModifier(&copiedParentMap).run(runner, module);
     }
     // If any immutable globals have constant values, we can just apply them
     // (the global itself will be removed by another pass, as it/ won't have
@@ -172,9 +166,7 @@ struct SimplifyGlobals : public Pass {
       }
     }
     if (!constantGlobals.empty()) {
-      PassRunner subRunner(module, runner->options);
-      subRunner.add<ConstantGlobalApplier>(&constantGlobals);
-      subRunner.run();
+      ConstantGlobalApplier(&constantGlobals).run(runner, module);
     }
     // TODO a mutable global's initial value can be applied to another global
     // after it, as the mutable one can't mutate during instance startup
diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp
index 0b5c53296..5d8fb6d61 100644
--- a/src/passes/pass.cpp
+++ b/src/passes/pass.cpp
@@ -46,11 +46,12 @@ void PassRegistry::registerPass(const char* name,
   passInfos[name] = PassInfo(description, create);
 }
 
-Pass* PassRegistry::createPass(std::string name) {
+std::unique_ptr<Pass> PassRegistry::createPass(std::string name) {
   if (passInfos.find(name) == passInfos.end()) {
     return nullptr;
   }
-  auto ret = passInfos[name].create();
+  std::unique_ptr<Pass> ret;
+  ret.reset(passInfos[name].create());
   ret->name = name;
   return ret;
 }
@@ -455,13 +456,13 @@ void PassRunner::run() {
       validationFlags = validationFlags | WasmValidator::Globally;
     }
     std::cerr << "[PassRunner] running passes..." << std::endl;
-    for (auto pass : passes) {
+    for (auto& pass : passes) {
       padding = std::max(padding, pass->name.size());
     }
     if (passDebug >= 3) {
       dumpWast("before", wasm);
     }
-    for (auto* pass : passes) {
+    for (auto& pass : passes) {
       // ignoring the time, save a printout of the module before, in case this
       // pass breaks it, so we can print the before and after
       std::stringstream moduleBefore;
@@ -477,9 +478,9 @@ void PassRunner::run() {
       if (pass->isFunctionParallel()) {
         // function-parallel passes should get a new instance per function
         ModuleUtils::iterDefinedFunctions(
-          *wasm, [&](Function* func) { runPassOnFunction(pass, func); });
+          *wasm, [&](Function* func) { runPassOnFunction(pass.get(), func); });
       } else {
-        runPass(pass);
+        runPass(pass.get());
       }
       auto after = std::chrono::steady_clock::now();
       std::chrono::duration<double> diff = after - before;
@@ -554,12 +555,12 @@ void PassRunner::run() {
       }
       stack.clear();
     };
-    for (auto* pass : passes) {
+    for (auto& pass : passes) {
       if (pass->isFunctionParallel()) {
-        stack.push_back(pass);
+        stack.push_back(pass.get());
       } else {
         flush();
-        runPass(pass);
+        runPass(pass.get());
       }
     }
     flush();
@@ -571,20 +572,14 @@ void PassRunner::runOnFunction(Function* func) {
     std::cerr << "[PassRunner] running passes on function " << func->name
               << std::endl;
   }
-  for (auto* pass : passes) {
-    runPassOnFunction(pass, func);
+  for (auto& pass : passes) {
+    runPassOnFunction(pass.get(), func);
   }
 }
 
-PassRunner::~PassRunner() {
-  for (auto pass : passes) {
-    delete pass;
-  }
-}
-
-void PassRunner::doAdd(Pass* pass) {
-  passes.push_back(pass);
+void PassRunner::doAdd(std::unique_ptr<Pass> pass) {
   pass->prepareToRun(this, wasm);
+  passes.emplace_back(std::move(pass));
 }
 
 // Checks that the state is valid before and after a
diff --git a/src/tools/wasm-metadce.cpp b/src/tools/wasm-metadce.cpp
index 96b368bc4..91dbe2d21 100644
--- a/src/tools/wasm-metadce.cpp
+++ b/src/tools/wasm-metadce.cpp
@@ -274,9 +274,7 @@ struct MetaDCEGraph {
     };
 
     PassRunner runner(&wasm);
-    runner.setIsNested(true);
-    runner.add<Scanner>(this);
-    runner.run();
+    Scanner(this).run(&runner, &wasm);
   }
 
 private:
diff --git a/src/tools/wasm2js.cpp b/src/tools/wasm2js.cpp
index cf2532c6c..2007a65d8 100644
--- a/src/tools/wasm2js.cpp
+++ b/src/tools/wasm2js.cpp
@@ -57,8 +57,7 @@ static void optimizeWasm(Module& wasm, PassOptions options) {
   };
 
   PassRunner runner(&wasm, options);
-  runner.add<OptimizeForJS>();
-  runner.run();
+  OptimizeForJS().run(&runner, &wasm);
 }
 
 template<typename T> static void printJS(Ref ast, T& output) {
diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp
index 8ada2af70..24f301110 100644
--- a/src/wasm/wasm-validator.cpp
+++ b/src/wasm/wasm-validator.cpp
@@ -1919,9 +1919,7 @@ bool WasmValidator::validate(Module& module, Flags flags) {
   info.quiet = (flags & Quiet) != 0;
   // parallel wasm logic validation
   PassRunner runner(&module);
-  runner.add<FunctionValidator>(&info);
-  runner.setIsNested(true);
-  runner.run();
+  FunctionValidator(&info).run(&runner, &module);
   // validate globally
   if (info.validateGlobally) {
     validateImports(module, info);
diff --git a/src/wasm2js.h b/src/wasm2js.h
index a1a0f13a7..c5fa54028 100644
--- a/src/wasm2js.h
+++ b/src/wasm2js.h
@@ -283,7 +283,7 @@ Ref Wasm2JSBuilder::processWasm(Module* wasm, Name funcName) {
   // First, do the lowering to a JS-friendly subset.
   {
     PassRunner runner(wasm, options);
-    runner.add<AutoDrop>();
+    runner.add(make_unique<AutoDrop>());
     runner.add("legalize-js-interface");
     // First up remove as many non-JS operations we can, including things like
     // 64-bit integer multiplication/division, `f32.nearest` instructions, etc.