[NewPM] Don't run before pass instrumentation on required passes

This allows those instrumentation to log when they decide to skip a
pass. This provides extra helpful info for optnone functions and also
will help with opt-bisect.

Have OptNoneInstrumentation print when it skips due to seeing optnone.

Reviewed By: asbirlea

Differential Revision: https://reviews.llvm.org/D90545

llvm/include/llvm/IR/PassInstrumentation.h

@@ -88,8 +88,9 @@ public:
   PassInstrumentationCallbacks(const PassInstrumentationCallbacks &) = delete;
   void operator=(const PassInstrumentationCallbacks &) = delete;
 
-  template <typename CallableT> void registerBeforePassCallback(CallableT C) {
-    BeforePassCallbacks.emplace_back(std::move(C));
+  template <typename CallableT>
+  void registerShouldRunOptionalPassCallback(CallableT C) {
+    ShouldRunOptionalPassCallbacks.emplace_back(std::move(C));
   }
 
   template <typename CallableT>
@@ -124,16 +125,25 @@ public:
 private:
   friend class PassInstrumentation;
 
-  SmallVector<llvm::unique_function<BeforePassFunc>, 4> BeforePassCallbacks;
+  /// These are only run on passes that are not required. They return false when
+  /// an optional pass should be skipped.
+  SmallVector<llvm::unique_function<BeforePassFunc>, 4>
+      ShouldRunOptionalPassCallbacks;
+  /// These are run on passes that are skipped.
   SmallVector<llvm::unique_function<BeforeSkippedPassFunc>, 4>
       BeforeSkippedPassCallbacks;
+  /// These are run on passes that are about to be run.
   SmallVector<llvm::unique_function<BeforeNonSkippedPassFunc>, 4>
       BeforeNonSkippedPassCallbacks;
+  /// These are run on passes that have just run.
   SmallVector<llvm::unique_function<AfterPassFunc>, 4> AfterPassCallbacks;
+  /// These are run passes that have just run on invalidated IR.
   SmallVector<llvm::unique_function<AfterPassInvalidatedFunc>, 4>
       AfterPassInvalidatedCallbacks;
+  /// These are run on analyses that are about to be run.
   SmallVector<llvm::unique_function<BeforeAnalysisFunc>, 4>
       BeforeAnalysisCallbacks;
+  /// These are run on analyses that have been run.
   SmallVector<llvm::unique_function<AfterAnalysisFunc>, 4>
       AfterAnalysisCallbacks;
 };
@@ -173,16 +183,19 @@ public:
 
   /// BeforePass instrumentation point - takes \p Pass instance to be executed
   /// and constant reference to IR it operates on. \Returns true if pass is
-  /// allowed to be executed.
+  /// allowed to be executed. These are only run on optional pass since required
+  /// passes must always be run. This allows these callbacks to print info when
+  /// they want to skip a pass.
   template <typename IRUnitT, typename PassT>
   bool runBeforePass(const PassT &Pass, const IRUnitT &IR) const {
     if (!Callbacks)
       return true;
 
     bool ShouldRun = true;
-    for (auto &C : Callbacks->BeforePassCallbacks)
-      ShouldRun &= C(Pass.name(), llvm::Any(&IR));
-    ShouldRun = ShouldRun || isRequired(Pass);
+    if (!isRequired(Pass)) {
+      for (auto &C : Callbacks->ShouldRunOptionalPassCallbacks)
+        ShouldRun &= C(Pass.name(), llvm::Any(&IR));
+    }
 
     if (ShouldRun) {
       for (auto &C : Callbacks->BeforeNonSkippedPassCallbacks)

llvm/include/llvm/Passes/StandardInstrumentations.h

@@ -59,10 +59,12 @@ private:
 
 class OptNoneInstrumentation {
 public:
+  OptNoneInstrumentation(bool DebugLogging) : DebugLogging(DebugLogging) {}
   void registerCallbacks(PassInstrumentationCallbacks &PIC);
 
 private:
-  bool skip(StringRef PassID, Any IR);
+  bool DebugLogging;
+  bool shouldRun(StringRef PassID, Any IR);
 };
 
 // Debug logging for transformation and analysis passes.
@@ -236,7 +238,8 @@ class StandardInstrumentations {
 
 public:
   StandardInstrumentations(bool DebugLogging, bool VerifyEach = false)
-      : PrintPass(DebugLogging), Verify(DebugLogging), VerifyEach(VerifyEach) {}
+      : PrintPass(DebugLogging), OptNone(DebugLogging), Verify(DebugLogging),
+        VerifyEach(VerifyEach) {}
 
   void registerCallbacks(PassInstrumentationCallbacks &PIC);
 

llvm/lib/Passes/StandardInstrumentations.cpp

@@ -347,10 +347,8 @@ void IRChangePrinter::registerCallbacks(PassInstrumentationCallbacks &PIC) {
   if (!PrintChanged)
     return;
 
-  PIC.registerBeforePassCallback([this](StringRef P, Any IR) {
-    saveIRBeforePass(IR, P);
-    return true;
-  });
+  PIC.registerBeforeNonSkippedPassCallback(
+      [this](StringRef P, Any IR) { saveIRBeforePass(IR, P); });
 
   PIC.registerAfterPassCallback(
       [this](StringRef P, Any IR, const PreservedAnalyses &) {
@@ -521,11 +519,11 @@ void PrintIRInstrumentation::registerCallbacks(
 
 void OptNoneInstrumentation::registerCallbacks(
     PassInstrumentationCallbacks &PIC) {
-  PIC.registerBeforePassCallback(
-      [this](StringRef P, Any IR) { return this->skip(P, IR); });
+  PIC.registerShouldRunOptionalPassCallback(
+      [this](StringRef P, Any IR) { return this->shouldRun(P, IR); });
 }
 
-bool OptNoneInstrumentation::skip(StringRef PassID, Any IR) {
+bool OptNoneInstrumentation::shouldRun(StringRef PassID, Any IR) {
   if (!EnableOptnone)
     return true;
   const Function *F = nullptr;
@@ -534,7 +532,12 @@ bool OptNoneInstrumentation::skip(StringRef PassID, Any IR) {
   } else if (any_isa<const Loop *>(IR)) {
     F = any_cast<const Loop *>(IR)->getHeader()->getParent();
   }
-  return !(F && F->hasOptNone());
+  bool ShouldRun = !(F && F->hasOptNone());
+  if (!ShouldRun && DebugLogging) {
+    errs() << "Skipping pass " << PassID << " on " << F->getName()
+           << " due to optnone attribute\n";
+  }
+  return ShouldRun;
 }
 
 void PrintPassInstrumentation::registerCallbacks(

llvm/test/Feature/optnone-opt.ll

@@ -4,12 +4,13 @@
 ; RUN: opt -O3 -S -debug -enable-new-pm=0 %s 2>&1 | FileCheck %s --check-prefix=O1 --check-prefix=O2O3
 ; RUN: opt -dce -gvn-hoist -loweratomic -S -debug -enable-new-pm=0 %s 2>&1 | FileCheck %s --check-prefix=MORE
 ; RUN: opt -indvars -licm -loop-deletion -loop-extract -loop-idiom -loop-instsimplify -loop-reduce -loop-reroll -loop-rotate -loop-unroll -loop-unswitch -enable-new-pm=0 -S -debug %s 2>&1 | FileCheck %s --check-prefix=LOOP
-; RUN: opt -enable-npm-optnone     -S -debug-pass-manager -enable-new-pm %s 2>&1 | FileCheck %s --check-prefix=NPM-O0
-; RUN: opt -enable-npm-optnone -O1 -S -debug-pass-manager -enable-new-pm %s 2>&1 | FileCheck %s --check-prefix=NPM-O1
-; RUN: opt -enable-npm-optnone -O2 -S -debug-pass-manager -enable-new-pm %s 2>&1 | FileCheck %s --check-prefix=NPM-O1 --check-prefix=NPM-O2O3
-; RUN: opt -enable-npm-optnone -O3 -S -debug-pass-manager -enable-new-pm %s 2>&1 | FileCheck %s --check-prefix=NPM-O1 --check-prefix=NPM-O2O3
-; RUN: opt -enable-npm-optnone -dce -gvn-hoist -loweratomic -S -debug-pass-manager -enable-new-pm %s 2>&1 | FileCheck %s --check-prefix=NPM-MORE
-; RUN: opt -enable-npm-optnone -indvars -licm -loop-deletion -loop-idiom -loop-instsimplify -loop-reduce -loop-unroll -simple-loop-unswitch -S -debug-pass-manager -enable-new-pm %s 2>&1 | FileCheck %s --check-prefix=NPM-LOOP
+; RUN: opt -enable-npm-optnone -passes='default<O0>' -S -debug-pass-manager %s 2>&1 | FileCheck %s --check-prefix=NPM-O0
+; RUN: opt -enable-npm-optnone -passes='default<O1>' -S -debug-pass-manager %s 2>&1 | FileCheck %s --check-prefix=NPM-O1
+; RUN: opt -enable-npm-optnone -passes='default<O2>' -S -debug-pass-manager %s 2>&1 | FileCheck %s --check-prefix=NPM-O1 --check-prefix=NPM-O2O3
+; RUN: opt -enable-npm-optnone -passes='default<O3>' -S -debug-pass-manager %s 2>&1 | FileCheck %s --check-prefix=NPM-O1 --check-prefix=NPM-O2O3
+; RUN: opt -enable-npm-optnone -passes='dce,gvn-hoist,loweratomic' -S -debug-pass-manager %s 2>&1 | FileCheck %s --check-prefix=NPM-MORE
+; RUN: opt -enable-npm-optnone -passes='loop(indvars,licm,loop-deletion,loop-idiom,loop-instsimplify,loop-reduce,simple-loop-unswitch),loop-unroll' -S -debug-pass-manager %s 2>&1 | FileCheck %s --check-prefix=NPM-LOOP
+; RUN: opt -enable-npm-optnone -passes='instsimplify,verify' -S -debug-pass-manager %s 2>&1 | FileCheck %s --check-prefix=NPM-REQUIRED
 
 ; REQUIRES: asserts
 
@@ -17,7 +18,7 @@
 ; optimizations on optnone functions.
 
 ; Function Attrs: noinline optnone
-define i32 @_Z3fooi(i32 %x) #0 {
+define i32 @foo(i32 %x) #0 {
 entry:
   %x.addr = alloca i32, align 4
   store i32 %x, i32* %x.addr, align 4
@@ -50,7 +51,7 @@ attributes #0 = { optnone noinline }
 ; O1-DAG: Skipping pass 'Reassociate expressions'
 ; O1-DAG: Skipping pass 'Simplify the CFG'
 ; O1-DAG: Skipping pass 'Sparse Conditional Constant Propagation'
-; NPM-O1-DAG: Skipping pass: SimplifyCFGPass on {{.*}}foo
+; NPM-O1-DAG: Skipping pass: SimplifyCFGPass on foo
 ; NPM-O1-DAG: Skipping pass: SROA
 ; NPM-O1-DAG: Skipping pass: EarlyCSEPass
 ; NPM-O1-DAG: Skipping pass: LowerExpectIntrinsicPass
@@ -81,7 +82,7 @@ attributes #0 = { optnone noinline }
 ; LOOP-DAG: Skipping pass 'Unswitch loops'
 ; LoopPassManager should not be skipped over an optnone function
 ; NPM-LOOP-NOT: Skipping pass: PassManager
-; NPM-LOOP-DAG: Skipping pass: LoopSimplifyPass on {{.*}}foo
+; NPM-LOOP-DAG: Skipping pass: LoopSimplifyPass on foo
 ; NPM-LOOP-DAG: Skipping pass: LCSSAPass
 ; NPM-LOOP-DAG: Skipping pass: IndVarSimplifyPass
 ; NPM-LOOP-DAG: Skipping pass: SimpleLoopUnswitchPass
@@ -91,3 +92,9 @@ attributes #0 = { optnone noinline }
 ; NPM-LOOP-DAG: Skipping pass: LICMPass
 ; NPM-LOOP-DAG: Skipping pass: LoopIdiomRecognizePass
 ; NPM-LOOP-DAG: Skipping pass: LoopInstSimplifyPass
+
+; NPM-REQUIRED-DAG: Skipping pass: InstSimplifyPass
+; NPM-REQUIRED-DAG: Skipping pass InstSimplifyPass on foo due to optnone attribute
+; NPM-REQUIRED-DAG: Running pass: VerifierPass
+; NPM-REQUIRED-NOT: Skipping pass: VerifyPass
+; NPM-REQUIRED-NOT: Skipping pass VerifyPass on foo due to optnone attribute

llvm/unittests/IR/PassBuilderCallbacksTest.cpp

@@ -330,9 +330,10 @@ struct MockPassInstrumentationCallbacks {
   MOCK_METHOD2(runAfterAnalysis, void(StringRef PassID, llvm::Any));
 
   void registerPassInstrumentation() {
-    Callbacks.registerBeforePassCallback([this](StringRef P, llvm::Any IR) {
-      return this->runBeforePass(P, IR);
-    });
+    Callbacks.registerShouldRunOptionalPassCallback(
+        [this](StringRef P, llvm::Any IR) {
+          return this->runBeforePass(P, IR);
+        });
     Callbacks.registerBeforeSkippedPassCallback(
         [this](StringRef P, llvm::Any IR) {
           this->runBeforeSkippedPass(P, IR);