Allow multiple invocations with arguments for all passes, allow --pass-arg only for additional arguments.

DirtyHairy · DirtyHairy · commit a12f18b1d6bf · 2024-06-25T23:38:53.000+02:00
diff --git a/src/binaryen-c.cpp b/src/binaryen-c.cpp
@@ -5451,7 +5451,9 @@ void BinaryenModuleRunPasses(BinaryenModuleRef module,
   PassRunner passRunner((Module*)module);
   passRunner.options = globalPassOptions;
   for (BinaryenIndex i = 0; i < numPasses; i++) {
-    passRunner.add(passes[i]);
+    passRunner.add(passes[i]), globalPassOptions.arguments.count(passes[i]) > 0
+                                 ? globalPassOptions.arguments[passes[i]]
+                                 : std::optional<std::string>();
   }
   passRunner.run();
 }
@@ -5696,7 +5698,10 @@ void BinaryenFunctionRunPasses(BinaryenFunctionRef func,
   PassRunner passRunner((Module*)module);
   passRunner.options = globalPassOptions;
   for (BinaryenIndex i = 0; i < numPasses; i++) {
-    passRunner.add(passes[i]);
+    passRunner.add(passes[i],
+                   globalPassOptions.arguments.count(passes[i]) > 0
+                     ? globalPassOptions.arguments[passes[i]]
+                     : std::optional<std::string>());
   }
   passRunner.runOnFunction((Function*)func);
 }
diff --git a/src/pass.h b/src/pass.h
@@ -38,10 +38,7 @@ struct PassRegistry {
 
   using Creator = std::function<Pass*()>;
 
-  void registerPass(const char* name,
-                    const char* description,
-                    Creator create,
-                    bool allowMultipleInstancesWithArgs = false);
+  void registerPass(const char* name, const char* description, Creator create);
   ;
   // Register a pass that's used for internal testing. These passes do not show
   // up in --help.
@@ -51,7 +48,6 @@ struct PassRegistry {
   std::vector<std::string> getRegisteredNames();
   std::string getPassDescription(std::string name);
   bool isPassHidden(std::string name);
-  bool doesPassAllowMultipleInstancesWithArgs(std::string name);
 
 private:
   void registerPasses();
@@ -60,14 +56,9 @@ struct PassRegistry {
     std::string description;
     Creator create;
     bool hidden;
-    bool allowMultipleInstancesWithArgs;
     PassInfo() = default;
-    PassInfo(std::string description,
-             Creator create,
-             bool hidden = false,
-             bool allowMultipleInstancesWithArgs = false)
-      : description(description), create(create), hidden(hidden),
-        allowMultipleInstancesWithArgs(allowMultipleInstancesWithArgs) {}
+    PassInfo(std::string description, Creator create, bool hidden = false)
+      : description(description), create(create), hidden(hidden) {}
   };
   std::map<std::string, PassInfo> passInfos;
 };
@@ -495,7 +486,7 @@ class Pass {
   // to imports must override this to return true.
   virtual bool addsEffects() { return false; }
 
-  void setPassArg(std::string value) { passArg = value; }
+  void setPassArg(const std::string& value) { passArg = value; }
 
   std::string name;
 
@@ -508,6 +499,12 @@ class Pass {
   PassOptions& getPassOptions() { return runner->options; }
 
 protected:
+  bool hasArgument(const std::string& key);
+  std::string getArgument(const std::string& key,
+                          const std::string& errorTextIfMissing);
+  std::string getArgumentOrDefault(const std::string& key,
+                                   const std::string& defaultValue);
+
   std::optional<std::string> passArg;
 
   Pass() = default;
diff --git a/src/passes/Asyncify.cpp b/src/passes/Asyncify.cpp
@@ -1608,54 +1608,49 @@ struct Asyncify : public Pass {
   bool addsEffects() override { return true; }
 
   void run(Module* module) override {
-    auto& options = getPassOptions();
-    bool optimize = options.optimizeLevel > 0;
+    bool optimize = getPassOptions().optimizeLevel > 0;
 
     // Find which things can change the state.
     auto stateChangingImports = String::trim(read_possible_response_file(
-      options.getArgumentOrDefault("asyncify-imports", "")));
-    auto ignoreImports =
-      options.getArgumentOrDefault("asyncify-ignore-imports", "");
+      getArgumentOrDefault("asyncify-imports", "")));
+    auto ignoreImports = getArgumentOrDefault("asyncify-ignore-imports", "");
     bool allImportsCanChangeState =
       stateChangingImports == "" && ignoreImports == "";
     String::Split listedImports(stateChangingImports,
                                 String::Split::NewLineOr(","));
     // canIndirectChangeState is the default.  asyncify-ignore-indirect sets it
     // to false.
-    auto canIndirectChangeState =
-      !options.hasArgument("asyncify-ignore-indirect");
+    auto canIndirectChangeState = !hasArgument("asyncify-ignore-indirect");
     std::string removeListInput =
-      options.getArgumentOrDefault("asyncify-removelist", "");
+      getArgumentOrDefault("asyncify-removelist", "");
     if (removeListInput.empty()) {
       // Support old name for now to avoid immediate breakage TODO remove
-      removeListInput = options.getArgumentOrDefault("asyncify-blacklist", "");
+      removeListInput = getArgumentOrDefault("asyncify-blacklist", "");
     }
     String::Split removeList(
       String::trim(read_possible_response_file(removeListInput)),
       String::Split::NewLineOr(","));
-    String::Split addList(
-      String::trim(read_possible_response_file(
-        options.getArgumentOrDefault("asyncify-addlist", ""))),
-      String::Split::NewLineOr(","));
-    std::string onlyListInput =
-      options.getArgumentOrDefault("asyncify-onlylist", "");
+    String::Split addList(String::trim(read_possible_response_file(
+                            getArgumentOrDefault("asyncify-addlist", ""))),
+                          String::Split::NewLineOr(","));
+    std::string onlyListInput = getArgumentOrDefault("asyncify-onlylist", "");
     if (onlyListInput.empty()) {
       // Support old name for now to avoid immediate breakage TODO remove
-      onlyListInput = options.getArgumentOrDefault("asyncify-whitelist", "");
+      onlyListInput = getArgumentOrDefault("asyncify-whitelist", "");
     }
     String::Split onlyList(
       String::trim(read_possible_response_file(onlyListInput)),
       String::Split::NewLineOr(","));
-    auto asserts = options.hasArgument("asyncify-asserts");
-    auto verbose = options.hasArgument("asyncify-verbose");
-    auto relocatable = options.hasArgument("asyncify-relocatable");
-    auto secondaryMemory = options.hasArgument("asyncify-in-secondary-memory");
-    auto propagateAddList = options.hasArgument("asyncify-propagate-addlist");
+    auto asserts = hasArgument("asyncify-asserts");
+    auto verbose = hasArgument("asyncify-verbose");
+    auto relocatable = hasArgument("asyncify-relocatable");
+    auto secondaryMemory = hasArgument("asyncify-in-secondary-memory");
+    auto propagateAddList = hasArgument("asyncify-propagate-addlist");
 
     // Ensure there is a memory, as we need it.
     if (secondaryMemory) {
       auto secondaryMemorySizeString =
-        options.getArgumentOrDefault("asyncify-secondary-memory-size", "1");
+        getArgumentOrDefault("asyncify-secondary-memory-size", "1");
       Address secondaryMemorySize = std::stoi(secondaryMemorySizeString);
       asyncifyMemory = createSecondaryMemory(module, secondaryMemorySize);
     } else {
diff --git a/src/passes/Directize.cpp b/src/passes/Directize.cpp
@@ -203,7 +203,7 @@ struct Directize : public Pass {
 
     // TODO: consider a per-table option here
     auto initialContentsImmutable =
-      getPassOptions().hasArgument("directize-initial-contents-immutable");
+      hasArgument("directize-initial-contents-immutable");
 
     // Set up the initial info.
     TableInfoMap tables;
diff --git a/src/passes/ExtractFunction.cpp b/src/passes/ExtractFunction.cpp
@@ -62,7 +62,7 @@ struct ExtractFunction : public Pass {
   bool addsEffects() override { return true; }
 
   void run(Module* module) override {
-    Name name = getPassOptions().getArgument(
+    Name name = getArgument(
       "extract-function",
       "ExtractFunction usage:  wasm-opt --extract-function=FUNCTION_NAME");
     extract(getPassRunner(), module, name);
@@ -74,10 +74,9 @@ struct ExtractFunctionIndex : public Pass {
   bool addsEffects() override { return true; }
 
   void run(Module* module) override {
-    std::string index =
-      getPassOptions().getArgument("extract-function-index",
-                                   "ExtractFunctionIndex usage: wasm-opt "
-                                   "--extract-function-index=FUNCTION_INDEX");
+    std::string index = getArgument("extract-function-index",
+                                    "ExtractFunctionIndex usage: wasm-opt "
+                                    "--extract-function-index=FUNCTION_INDEX");
     for (char c : index) {
       if (!std::isdigit(c)) {
         Fatal() << "Expected numeric function index";
diff --git a/src/passes/FuncCastEmulation.cpp b/src/passes/FuncCastEmulation.cpp
@@ -157,8 +157,7 @@ struct FuncCastEmulation : public Pass {
   bool addsEffects() override { return true; }
 
   void run(Module* module) override {
-    Index numParams = std::stoul(
-      getPassOptions().getArgumentOrDefault("max-func-params", "16"));
+    Index numParams = std::stoul(getArgumentOrDefault("max-func-params", "16"));
     // we just need the one ABI function type for all indirect calls
     HeapType ABIType(
       Signature(Type(std::vector<Type>(numParams, Type::i64)), Type::i64));
diff --git a/src/passes/JSPI.cpp b/src/passes/JSPI.cpp
@@ -83,18 +83,17 @@ struct JSPI : public Pass {
   void run(Module* module) override {
     Builder builder(*module);
 
-    auto& options = getPassOptions();
     // Find which imports can suspend.
-    auto stateChangingImports = String::trim(read_possible_response_file(
-      options.getArgumentOrDefault("jspi-imports", "")));
+    auto stateChangingImports = String::trim(
+      read_possible_response_file(getArgumentOrDefault("jspi-imports", "")));
     String::Split listedImports(stateChangingImports, ",");
 
     // Find which exports should create a promise.
-    auto stateChangingExports = String::trim(read_possible_response_file(
-      options.getArgumentOrDefault("jspi-exports", "")));
+    auto stateChangingExports = String::trim(
+      read_possible_response_file(getArgumentOrDefault("jspi-exports", "")));
     String::Split listedExports(stateChangingExports, ",");
 
-    bool wasmSplit = options.hasArgument("jspi-split-module");
+    bool wasmSplit = hasArgument("jspi-split-module");
     if (wasmSplit) {
       // Make an import for the load secondary module function so a JSPI wrapper
       // version will be created.
diff --git a/src/passes/LegalizeJSInterface.cpp b/src/passes/LegalizeJSInterface.cpp
@@ -64,9 +64,8 @@ struct LegalizeJSInterface : public Pass {
     setTempRet0 = nullptr;
     getTempRet0 = nullptr;
     auto exportOriginals =
-      getPassOptions().hasArgument("legalize-js-interface-export-originals");
-    exportedHelpers =
-      getPassOptions().hasArgument("legalize-js-interface-exported-helpers");
+      hasArgument("legalize-js-interface-export-originals");
+    exportedHelpers = hasArgument("legalize-js-interface-exported-helpers");
     // for each illegal export, we must export a legalized stub instead
     std::vector<std::unique_ptr<Export>> newExports;
     for (auto& ex : module->exports) {
diff --git a/src/passes/LogExecution.cpp b/src/passes/LogExecution.cpp
@@ -46,8 +46,7 @@ struct LogExecution : public WalkerPass<PostWalker<LogExecution>> {
   bool addsEffects() override { return true; }
 
   void run(Module* module) override {
-    auto& options = getPassOptions();
-    loggerModule = options.getArgumentOrDefault("log-execution", "");
+    loggerModule = getArgumentOrDefault("log-execution", "");
     super::run(module);
   }
 
diff --git a/src/passes/NoInline.cpp b/src/passes/NoInline.cpp
@@ -49,9 +49,7 @@ struct NoInline : public Pass {
 
   void run(Module* module) override {
     std::string pattern =
-      passArg ? *passArg
-              : getPassOptions().getArgument(
-                  name, "Usage usage:  wasm-opt --" + name + "=WILDCARD");
+      getArgument(name, "Usage usage:  wasm-opt --" + name + "=WILDCARD");
 
     for (auto& func : module->functions) {
       if (!String::wildcardMatch(pattern, func->name.toString())) {
diff --git a/src/passes/PostEmscripten.cpp b/src/passes/PostEmscripten.cpp
@@ -214,8 +214,7 @@ struct PostEmscripten : public Pass {
     std::vector<Address> segmentOffsets; // segment index => address offset
     calcSegmentOffsets(module, segmentOffsets);
 
-    auto& options = getPassOptions();
-    auto sideModule = options.hasArgument("post-emscripten-side-module");
+    auto sideModule = hasArgument("post-emscripten-side-module");
     if (!sideModule) {
       removeData(module, segmentOffsets, "__start_em_asm", "__stop_em_asm");
       removeData(module, segmentOffsets, "__start_em_js", "__stop_em_js");
@@ -235,8 +234,7 @@ struct PostEmscripten : public Pass {
   }
 
   void removeEmJsExports(Module& module) {
-    auto& options = getPassOptions();
-    auto sideModule = options.hasArgument("post-emscripten-side-module");
+    auto sideModule = hasArgument("post-emscripten-side-module");
     EmJsWalker walker(sideModule);
     walker.walkModule(&module);
     for (const Export& exp : walker.toRemove) {
diff --git a/src/passes/PrintFunctionMap.cpp b/src/passes/PrintFunctionMap.cpp
@@ -37,7 +37,7 @@ struct PrintFunctionMap : public Pass {
   void run(Module* module) override {
     // If an argument is provided, write to that file; otherwise write to
     // stdout.
-    auto outFile = getPassOptions().getArgumentOrDefault("symbolmap", "");
+    auto outFile = getArgumentOrDefault("symbolmap", "");
     Output output(outFile, Flags::Text);
     auto& o = output.getStream();
     Index i = 0;
diff --git a/src/passes/SeparateDataSegments.cpp b/src/passes/SeparateDataSegments.cpp
@@ -33,14 +33,14 @@ struct SeparateDataSegments : public Pass {
 
   void run(Module* module) override {
     std::string outfileName =
-      getPassOptions().getArgument("separate-data-segments",
-                                   "SeparateDataSegments usage: wasm-opt "
-                                   "--separate-data-segments@FILENAME");
+      getArgument("separate-data-segments",
+                  "SeparateDataSegments usage: wasm-opt "
+                  "--separate-data-segments@FILENAME");
     Output outfile(outfileName, Flags::Binary);
-    std::string baseStr = getPassOptions().getArgument(
-      "separate-data-segments-global-base",
-      "SeparateDataSegments usage: wasm-opt "
-      "--pass-arg=separate-data-segments-global-base@NUMBER");
+    std::string baseStr =
+      getArgument("separate-data-segments-global-base",
+                  "SeparateDataSegments usage: wasm-opt "
+                  "--pass-arg=separate-data-segments-global-base@NUMBER");
     Address base = std::stoi(baseStr);
     size_t lastEnd = 0;
     for (auto& seg : module->dataSegments) {
diff --git a/src/passes/SetGlobals.cpp b/src/passes/SetGlobals.cpp
@@ -29,9 +29,9 @@ struct SetGlobals : public Pass {
   bool requiresNonNullableLocalFixups() override { return false; }
 
   void run(Module* module) override {
-    Name input = getPassRunner()->options.getArgument(
-      "set-globals",
-      "SetGlobals usage:  wasm-opt --pass-arg=set-globals@x=y,z=w");
+    Name input =
+      getArgument("set-globals",
+                  "SetGlobals usage:  wasm-opt --pass-arg=set-globals@x=y,z=w");
 
     // The input is a set of X=Y pairs separated by commas.
     String::Split pairs(input.toString(), ",");
diff --git a/src/passes/StackCheck.cpp b/src/passes/StackCheck.cpp
@@ -141,8 +141,7 @@ struct StackCheck : public Pass {
     auto stackLimitName = Names::getValidGlobalName(*module, "__stack_limit");
 
     Name handler;
-    auto handlerName =
-      getPassOptions().getArgumentOrDefault("stack-check-handler", "");
+    auto handlerName = getArgumentOrDefault("stack-check-handler", "");
     if (handlerName != "") {
       handler = handlerName;
       importStackOverflowHandler(
diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp
@@ -43,11 +43,9 @@ PassRegistry* PassRegistry::get() { return &singleton; }
 
 void PassRegistry::registerPass(const char* name,
                                 const char* description,
-                                Creator create,
-                                bool allowMultipleInstancesWithArgs) {
+                                Creator create) {
   assert(passInfos.find(name) == passInfos.end());
-  passInfos[name] =
-    PassInfo(description, create, false, allowMultipleInstancesWithArgs);
+  passInfos[name] = PassInfo(description, create, false);
 }
 
 void PassRegistry::registerTestPass(const char* name,
@@ -85,12 +83,6 @@ bool PassRegistry::isPassHidden(std::string name) {
   return passInfos[name].hidden;
 }
 
-bool PassRegistry::doesPassAllowMultipleInstancesWithArgs(std::string name) {
-  assert(passInfos.find(name) != passInfos.end());
-
-  return passInfos[name].allowMultipleInstancesWithArgs;
-}
-
 // PassRunner
 
 void PassRegistry::registerPasses() {
@@ -308,8 +300,7 @@ void PassRegistry::registerPasses() {
     createMultiMemoryLoweringWithBoundsChecksPass);
   registerPass("nm", "name list", createNameListPass);
   registerPass("name-types", "(re)name all heap types", createNameTypesPass);
-  registerPass(
-    "no-inline", "mark functions as no-inline", createNoInlinePass, true);
+  registerPass("no-inline", "mark functions as no-inline", createNoInlinePass);
   registerPass("no-full-inline",
                "mark functions as no-inline (for full inlining only)",
                createNoFullInlinePass);
@@ -1036,4 +1027,28 @@ bool PassRunner::shouldPreserveDWARF() {
   return true;
 }
 
+bool Pass::hasArgument(const std::string& key) {
+  return (key == name) ? passArg.has_value()
+                       : getPassOptions().hasArgument(key);
+}
+
+std::string Pass::getArgument(const std::string& key,
+                              const std::string& errorTextIfMissing) {
+  if (!hasArgument(key)) {
+    Fatal() << errorTextIfMissing;
+  }
+
+  return (key == name) ? *passArg
+                       : getPassOptions().getArgument(key, errorTextIfMissing);
+}
+
+std::string Pass::getArgumentOrDefault(const std::string& key,
+                                       const std::string& defaultValue) {
+  if (key == name) {
+    return passArg.value_or(defaultValue);
+  }
+
+  return getPassOptions().getArgumentOrDefault(key, defaultValue);
+}
+
 } // namespace wasm
diff --git a/src/tools/optimization-options.h b/src/tools/optimization-options.h
diff --git a/src/tools/wasm-split/wasm-split.cpp b/src/tools/wasm-split/wasm-split.cpp
diff --git a/test/lit/passes/extract-function.wast b/test/lit/passes/extract-function.wast
diff --git a/test/passes/set-globals.passes b/test/passes/set-globals.passes

Original file line number	Diff line number	Diff line change
`@@ -5451,7 +5451,9 @@ void BinaryenModuleRunPasses(BinaryenModuleRef module,`
`5451`	`5451`	`PassRunner passRunner((Module*)module);`
`5452`	`5452`	`passRunner.options = globalPassOptions;`
`5453`	`5453`	`for (BinaryenIndex i = 0; i < numPasses; i++) {`
`5454`		`- passRunner.add(passes[i]);`
	`5454`	`+ passRunner.add(passes[i]), globalPassOptions.arguments.count(passes[i]) > 0`
	`5455`	`+ ? globalPassOptions.arguments[passes[i]]`
	`5456`	`+ : std::optional<std::string>();`
`5455`	`5457`	`}`
`5456`	`5458`	`passRunner.run();`
`5457`	`5459`	`}`
`@@ -5696,7 +5698,10 @@ void BinaryenFunctionRunPasses(BinaryenFunctionRef func,`
`5696`	`5698`	`PassRunner passRunner((Module*)module);`
`5697`	`5699`	`passRunner.options = globalPassOptions;`
`5698`	`5700`	`for (BinaryenIndex i = 0; i < numPasses; i++) {`
`5699`		`- passRunner.add(passes[i]);`
	`5701`	`+ passRunner.add(passes[i],`
	`5702`	`+ globalPassOptions.arguments.count(passes[i]) > 0`
	`5703`	`+ ? globalPassOptions.arguments[passes[i]]`
	`5704`	`+ : std::optional<std::string>());`
`5700`	`5705`	`}`
`5701`	`5706`	`passRunner.runOnFunction((Function*)func);`
`5702`	`5707`	`}`
Original file line number	Diff line number	Diff line change
`@@ -46,8 +46,7 @@ struct LogExecution : public WalkerPass<PostWalker<LogExecution>> {`
`46`	`46`	`bool addsEffects() override { return true; }`
`47`	`47`
`48`	`48`	`void run(Module* module) override {`
`49`		`- auto& options = getPassOptions();`
`50`		`- loggerModule = options.getArgumentOrDefault("log-execution", "");`
	`49`	`+ loggerModule = getArgumentOrDefault("log-execution", "");`
`51`	`50`	`super::run(module);`
`52`	`51`	`}`
`53`	`52`