Fix opt/shrink levels when running the optimizer multiple times, Part 2 (#5787)

This is a followup to #5333 . That fixed the selection of which passes to run, but
forgot to also fix the global state of the current optimize/shrink levels. This PR
fixes that. As a result, running -O3 -Oz will now work as expected: the first -O3
will run the right passes (as #5333 fixed) and while running them, the global
optimize/shrinkLevels will be -O3 (and not -Oz), which this PR fixes.

A specific result of this is that -O3 -Oz used to inline less, since the invocation
of inlining during -O3 thought we were optimizing for size. The new test verifies
that we do fully inline in the first -O3 now.

Author: kripken

CHANGELOG.md

@@ -15,6 +15,14 @@ full changeset diff at the end of each section.
 Current Trunk
 -------------
 
+- Fix a bug where e.g. -O3 -Oz ran the -O3 with the opt levels of -Oz, which
+  could inhibit inlining, for example. While this is a bugfix, it affects how
+  commandline options are interpreted, so if you depended on the old behavior
+  this may be a breaking change. That is, the old behavior made -O3 -Oz run the
+  first -O3 with -Oz's opt levels, and the new behavior is to run -O3 with the
+  proper (-O3) opt levels. This is a followup to #5333 from a previous release.
+  (#5787)
+
 v113
 ----
 

src/pass.h

@@ -388,9 +388,6 @@ struct PassRunner {
   // Whether this pass runner has run. A pass runner should only be run once.
   bool ran = false;
 
-  // Passes in |options.passesToSkip| that we have seen and skipped.
-  std::unordered_set<std::string> skippedPasses;
-
   void runPass(Pass* pass);
   void runPassOnFunction(Pass* pass, Function* func);
 

src/passes/pass.cpp

@@ -698,9 +698,6 @@ void PassRunner::run() {
   assert(!ran);
   ran = true;
 
-  // As we run passes, we'll notice which we skip.
-  skippedPasses.clear();
-
   static const int passDebug = getPassDebug();
   // Emit logging information when asked for. At passDebug level 1+ we log
   // the main passes, while in 2 we also log nested ones. Note that for
@@ -821,20 +818,6 @@ void PassRunner::run() {
     }
     flush();
   }
-
-  if (!isNested) {
-    // All the passes the user requested to skip should have been seen, and
-    // skipped. If not, the user may have had a typo in the name of a pass to
-    // skip, and we will warn. (We don't do this in a nested runner because
-    // those are used for various internal tasks inside passes, which would lead
-    // to many spurious warnings.)
-    for (auto pass : options.passesToSkip) {
-      if (!skippedPasses.count(pass)) {
-        std::cerr << "warning: --" << pass << " was requested to be skipped, "
-                  << "but it was not found in the passes that were run.\n";
-      }
-    }
-  }
 }
 
 void PassRunner::runOnFunction(Function* func) {
@@ -956,7 +939,6 @@ void PassRunner::runPass(Pass* pass) {
   assert(!pass->isFunctionParallel());
 
   if (options.passesToSkip.count(pass->name)) {
-    skippedPasses.insert(pass->name);
     return;
   }
 
@@ -980,7 +962,6 @@ void PassRunner::runPassOnFunction(Pass* pass, Function* func) {
   assert(pass->isFunctionParallel());
 
   if (options.passesToSkip.count(pass->name)) {
-    skippedPasses.insert(pass->name);
     return;
   }
 

src/tools/optimization-options.h

@@ -328,32 +328,52 @@ struct OptimizationOptions : public ToolOptions {
   bool runningPasses() { return passes.size() > 0; }
 
   void runPasses(Module& wasm) {
-    PassRunner passRunner(&wasm, passOptions);
-    if (debug) {
-      passRunner.setDebug(true);
-    }
-    for (auto& pass : 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;
+    std::unique_ptr<PassRunner> passRunner;
+
+    // Flush anything in the current pass runner, and then reset it to a fresh
+    // state so it is ready for new things.
+    auto flushAndReset = [&]() {
+      if (passRunner) {
+        passRunner->run();
       }
-      if (pass.shrinkLevel) {
-        passRunner.options.shrinkLevel = *pass.shrinkLevel;
+      passRunner = std::make_unique<PassRunner>(&wasm, passOptions);
+      if (debug) {
+        passRunner->setDebug(true);
       }
+    };
 
+    flushAndReset();
+
+    for (auto& pass : passes) {
       if (pass.name == DEFAULT_OPT_PASSES) {
-        passRunner.addDefaultOptimizationPasses();
+        // This is something like -O3 or -Oz. We must run this now, in order to
+        // set the proper opt and shrink levels. To do that, first reset the
+        // runner so that anything already queued is run (since we can only run
+        // after those things).
+        flushAndReset();
+
+        // -O3/-Oz etc. always set their own optimize/shrinkLevels.
+        assert(pass.optimizeLevel);
+        assert(pass.shrinkLevel);
+        passRunner->options.optimizeLevel = *pass.optimizeLevel;
+        passRunner->options.shrinkLevel = *pass.shrinkLevel;
+
+        // Run our optimizations now, and reset the runner so that the default
+        // pass options are used later (and not the temporary optimize/
+        // shrinkLevels we just set).
+        passRunner->addDefaultOptimizationPasses();
+        flushAndReset();
       } else {
-        passRunner.add(pass.name);
-      }
+        // This is a normal pass. Add it to the queue for execution.
+        passRunner->add(pass.name);
 
-      // Revert back to the default levels, if we changed them.
-      passRunner.options.optimizeLevel = oldOptimizeLevel;
-      passRunner.options.shrinkLevel = oldShrinkLevel;
+        // Normal passes do not set their own optimize/shrinkLevels.
+        assert(!pass.optimizeLevel);
+        assert(!pass.shrinkLevel);
+      }
     }
-    passRunner.run();
+
+    flushAndReset();
   }
 };
 

test/lit/passes/O3_Oz.wast

@@ -0,0 +1,45 @@
+;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
+
+;; RUN: foreach %s %t wasm-opt -O3 -Oz -S -o - | filecheck %s
+
+(module
+  (func $inline.me (param $x i32) (result i32)
+    (i32.add
+      (local.get $x)
+      (i32.const 2)
+    )
+  )
+
+  (func "export" (param $x i32) (result i32)
+    ;; $inline.me is called twice, so we do not always inline it like called-
+    ;; once functions are. -Oz is too cautious to inline such things that may
+    ;; end up increasing total code size, but we are running -O3 -Oz here and so
+    ;; the first -O3 will inline there. That is, this test verifies that the
+    ;; later -Oz does not affect the earlier -O3 (which it could, if -Oz set
+    ;; global state that -O3 then reads to see the optimization and shrink
+    ;; levels).
+    (i32.add
+      (call $inline.me
+        (local.get $x)
+      )
+      (call $inline.me
+        (local.get $x)
+      )
+    )
+  )
+)
+;; CHECK:      (type $i32_=>_i32 (func (param i32) (result i32)))
+
+;; CHECK:      (export "export" (func $1))
+
+;; CHECK:      (func $1 (; has Stack IR ;) (param $0 i32) (result i32)
+;; CHECK-NEXT:  (i32.add
+;; CHECK-NEXT:   (local.tee $0
+;; CHECK-NEXT:    (i32.add
+;; CHECK-NEXT:     (local.get $0)
+;; CHECK-NEXT:     (i32.const 2)
+;; CHECK-NEXT:    )
+;; CHECK-NEXT:   )
+;; CHECK-NEXT:   (local.get $0)
+;; CHECK-NEXT:  )
+;; CHECK-NEXT: )