Fix opt/shrink levels when running the optimizer multiple times (#5333)

Previously -O3 -O1 would run -O1 twice since the last flag set the global opt level
to 1, and then all invocations of the optimizer pipeline read that. This makes each
pipeline define its own opt level.

This has been a long-standing annoyance, which wasn't much noticed except that
with wasm GC there is more of a need to run the optimization pipeline more than
once. And sometimes it is nice to run different levels.

4 files changed, 88 insertions, 15 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6c7f6f2fb..466e98e77 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -15,6 +15,14 @@ full changeset diff at the end of each section.
 Current Trunk
 -------------
 
+- Optimization sequences like `-O3 -Os` now do the expected thing and run `-O3`
+  followed by `-Os`. Previously the last of them set the defaults that were used
+  by all executions, so `-O3 -Os` was equivalent to `-Os -Os`. (There is no
+  change to the default optimization level that other passes can see. For
+  example, `--precompute-propagate -O2 -O1` will run `--precompute-propagate`
+  at opt level `1`, as the global default is set to `2` and then overridden to
+  `1`. The only change is that the passes run by `-O2` will actually run `-O2`
+  now, while before they'd use the global default which made them do `-O1`.)
 - Add `--closed-world` flag. This enables more optimizations in GC mode as it
   lets us assume that we can change types inside the module.
 - The isorecursive WasmGC type system (i.e. --hybrid) is now the default to
diff --git a/src/tools/optimization-options.h b/src/tools/optimization-options.h
index ff9bb973d..eaa90f55f 100644
--- a/src/tools/optimization-options.h
+++ b/src/tools/optimization-options.h
@@ -30,7 +30,36 @@ struct OptimizationOptions : public ToolOptions {
   static constexpr const int OS_OPTIMIZE_LEVEL = 2;
   static constexpr const int OS_SHRINK_LEVEL = 1;
 
-  std::vector<std::string> passes;
+  // Information to run a pass, as requested by a commandline flag.
+  struct PassInfo {
+    // The name of the pass to run.
+    std::string name;
+
+    // The optimize and shrink levels to run the pass with, if specified. If not
+    // specified then the defaults are used.
+    std::optional<int> optimizeLevel;
+    std::optional<int> shrinkLevel;
+
+    PassInfo(std::string name) : name(name) {}
+    PassInfo(const char* name) : name(name) {}
+    PassInfo(std::string name, int optimizeLevel, int shrinkLevel)
+      : name(name), optimizeLevel(optimizeLevel), shrinkLevel(shrinkLevel) {}
+  };
+
+  std::vector<PassInfo> passes;
+
+  // Add a request to run all the default opt passes. They are run with the
+  // current opt and shrink levels specified, which are read from passOptions.
+  //
+  // Each caller to here sets the opt and shrink levels before, which provides
+  // the right values for us to read. That is, -Os etc. sets the default opt
+  // level, so that the last of -O3 -Os will override the previous default, but
+  // also we note the current opt level for when we run the pass, so that the
+  // sequence -O3 -Os will run -O3 and then -Os, and not -Os twice.
+  void addDefaultOptPasses() {
+    passes.push_back(PassInfo{
+      DEFAULT_OPT_PASSES, passOptions.optimizeLevel, passOptions.shrinkLevel});
+  }
 
   constexpr static const char* OptimizationOptionsCategory =
     "Optimization options";
@@ -51,7 +80,7 @@ struct OptimizationOptions : public ToolOptions {
             PassOptions::DEFAULT_OPTIMIZE_LEVEL == OS_OPTIMIZE_LEVEL &&
               PassOptions::DEFAULT_SHRINK_LEVEL == OS_SHRINK_LEVEL,
             "Help text states that -O is equivalent to -Os but now it isn't.");
-          passes.push_back(DEFAULT_OPT_PASSES);
+          addDefaultOptPasses();
         })
       .add("",
            "-O0",
@@ -71,7 +100,7 @@ struct OptimizationOptions : public ToolOptions {
            [this](Options*, const std::string&) {
              passOptions.optimizeLevel = 1;
              passOptions.shrinkLevel = 0;
-             passes.push_back(DEFAULT_OPT_PASSES);
+             addDefaultOptPasses();
            })
       .add(
         "",
@@ -82,7 +111,7 @@ struct OptimizationOptions : public ToolOptions {
         [this](Options*, const std::string&) {
           passOptions.optimizeLevel = 2;
           passOptions.shrinkLevel = 0;
-          passes.push_back(DEFAULT_OPT_PASSES);
+          addDefaultOptPasses();
         })
       .add("",
            "-O3",
@@ -93,7 +122,7 @@ struct OptimizationOptions : public ToolOptions {
            [this](Options*, const std::string&) {
              passOptions.optimizeLevel = 3;
              passOptions.shrinkLevel = 0;
-             passes.push_back(DEFAULT_OPT_PASSES);
+             addDefaultOptPasses();
            })
       .add("",
            "-O4",
@@ -105,7 +134,7 @@ struct OptimizationOptions : public ToolOptions {
            [this](Options*, const std::string&) {
              passOptions.optimizeLevel = 4;
              passOptions.shrinkLevel = 0;
-             passes.push_back(DEFAULT_OPT_PASSES);
+             addDefaultOptPasses();
            })
       .add("",
            "-Os",
@@ -115,7 +144,7 @@ struct OptimizationOptions : public ToolOptions {
            [this](Options*, const std::string&) {
              passOptions.optimizeLevel = OS_OPTIMIZE_LEVEL;
              passOptions.shrinkLevel = OS_SHRINK_LEVEL;
-             passes.push_back(DEFAULT_OPT_PASSES);
+             addDefaultOptPasses();
            })
       .add("",
            "-Oz",
@@ -125,7 +154,7 @@ struct OptimizationOptions : public ToolOptions {
            [this](Options*, const std::string&) {
              passOptions.optimizeLevel = 2;
              passOptions.shrinkLevel = 2;
-             passes.push_back(DEFAULT_OPT_PASSES);
+             addDefaultOptPasses();
            })
       .add("--optimize-level",
            "-ol",
@@ -293,7 +322,7 @@ struct OptimizationOptions : public ToolOptions {
 
   bool runningDefaultOptimizationPasses() {
     for (auto& pass : passes) {
-      if (pass == DEFAULT_OPT_PASSES) {
+      if (pass.name == DEFAULT_OPT_PASSES) {
         return true;
       }
     }
@@ -308,11 +337,25 @@ struct OptimizationOptions : public ToolOptions {
       passRunner.setDebug(true);
     }
     for (auto& pass : passes) {
-      if (pass == DEFAULT_OPT_PASSES) {
+      // We apply the pass's intended opt and shrink levels, if any.
+      auto oldOptimizeLevel = passRunner.options.optimizeLevel;
+      auto oldShrinkLevel = passRunner.options.shrinkLevel;
+      if (pass.optimizeLevel) {
+        passRunner.options.optimizeLevel = *pass.optimizeLevel;
+      }
+      if (pass.shrinkLevel) {
+        passRunner.options.shrinkLevel = *pass.shrinkLevel;
+      }
+
+      if (pass.name == DEFAULT_OPT_PASSES) {
         passRunner.addDefaultOptimizationPasses();
       } else {
-        passRunner.add(pass);
+        passRunner.add(pass.name);
       }
+
+      // Revert back to the default levels, if we changed them.
+      passRunner.options.optimizeLevel = oldOptimizeLevel;
+      passRunner.options.shrinkLevel = oldShrinkLevel;
     }
     passRunner.run();
   }
diff --git a/src/tools/wasm-opt.cpp b/src/tools/wasm-opt.cpp
index 538496ecf..effd085bc 100644
--- a/src/tools/wasm-opt.cpp
+++ b/src/tools/wasm-opt.cpp
@@ -59,9 +59,10 @@ static std::string runCommand(std::string command) {
 #endif
 }
 
-static bool willRemoveDebugInfo(const std::vector<std::string>& passes) {
+static bool
+willRemoveDebugInfo(const std::vector<OptimizationOptions::PassInfo>& passes) {
   for (auto& pass : passes) {
-    if (PassRunner::passRemovesDebugInfo(pass)) {
+    if (PassRunner::passRemovesDebugInfo(pass.name)) {
       return true;
     }
   }
@@ -250,8 +251,11 @@ int main(int argc, const char* argv[]) {
     // If the user asked to print the module, print it even if invalid,
     // as otherwise there is no way to print the broken module (the pass
     // to print would not be reached).
-    if (std::find(options.passes.begin(), options.passes.end(), "print") !=
-        options.passes.end()) {
+    if (std::any_of(options.passes.begin(),
+                    options.passes.end(),
+                    [](const OptimizationOptions::PassInfo& info) {
+                      return info.name == "print";
+                    })) {
       std::cout << wasm << '\n';
     }
     Fatal() << message;
diff --git a/test/unit/test_passes.py b/test/unit/test_passes.py
index 2fe7f6263..25f844757 100644
--- a/test/unit/test_passes.py
+++ b/test/unit/test_passes.py
@@ -47,3 +47,21 @@ class PassesTest(utils.BinaryenTestCase):
                         self.assertIn(pass_, passes)
                     else:
                         self.assertNotIn(pass_, passes)
+
+    def test_O3_O1(self):
+        # When we run something like -O3 -O1 we should run -O3 followed by -O1
+        # (and not -O1 -O1, which would be the case if the last commandline
+        # argument set a global opt level flag that was then used by all
+        # invocations of the full opt pipeline).
+
+        # A pass that runs in -O3 but not -O1
+        PASS_IN_O3_ONLY = 'precompute-propagate'
+
+        # That pass is run in -O3 and -O3 -O1 etc. but not -O1 or -O1 -O1
+        self.assertIn(PASS_IN_O3_ONLY, self.get_passes_run(['-O3']))
+        self.assertIn(PASS_IN_O3_ONLY, self.get_passes_run(['-O3', '-O1']))
+        self.assertIn(PASS_IN_O3_ONLY, self.get_passes_run(['-O1', '-O3']))
+        self.assertIn(PASS_IN_O3_ONLY, self.get_passes_run(['-O3', '-O3']))
+
+        self.assertNotIn(PASS_IN_O3_ONLY, self.get_passes_run(['-O1']))
+        self.assertNotIn(PASS_IN_O3_ONLY, self.get_passes_run(['-O1', '-O1']))