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

[aeubanks]

Summary

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.

[asbirlea]

llvm/include/llvm/IR/PassInstrumentation.h

196

Could you add a doxygen comment that the BeforePassCallbacks are only called if the pass is not required around the declaration of BeforePassCallbacks?
Would be useful to have comments on each of the vectors of callbacks.

[aeubanks]

add doxygen comments

[nikic]

llvm/include/llvm/IR/PassInstrumentation.h

196

Maybe also rename it to BeforeRequiredPassCallbacks?

[aeubanks]

rename to ShouldRunOptionalPassCallbacks

llvm/include/llvm/IR/PassInstrumentation.h

 
 public:
   PassInstrumentationCallbacks() {}
 
   /// Copying PassInstrumentationCallbacks is not intended.
   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>
   void registerBeforeSkippedPassCallback(CallableT C) {
     BeforeSkippedPassCallbacks.emplace_back(std::move(C));
   }
 
   template <typename CallableT>
public:
   template <typename CallableT>
   void registerAfterAnalysisCallback(CallableT C) {
     AfterAnalysisCallbacks.emplace_back(std::move(C));
   }
 
 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;
 };
 
 /// This class provides instrumentation entry points for the Pass Manager,
 /// doing calls to callbacks registered in PassInstrumentationCallbacks.
 class PassInstrumentation {
   PassInstrumentationCallbacks *Callbacks;
public:
   /// should at least match the life-time of corresponding
   /// PassInstrumentationAnalysis (which usually is till the end of current
   /// compilation).
   PassInstrumentation(PassInstrumentationCallbacks *CB = nullptr)
       : Callbacks(CB) {}
 
   /// 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)
+    if (!isRequired(Pass)) {
+      for (auto &C : Callbacks->ShouldRunOptionalPassCallbacks)
         ShouldRun &= C(Pass.name(), llvm::Any(&IR));
-    ShouldRun = ShouldRun || isRequired(Pass);
+    }
 
     if (ShouldRun) {
       for (auto &C : Callbacks->BeforeNonSkippedPassCallbacks)
         C(Pass.name(), llvm::Any(&IR));
     } else {
       for (auto &C : Callbacks->BeforeSkippedPassCallbacks)
         C(Pass.name(), llvm::Any(&IR));
     }

llvm/include/llvm/Passes/StandardInstrumentations.h

private:
   /// Stack of Module description, enough to print the module after a given
   /// pass.
   SmallVector<PrintModuleDesc, 2> ModuleDescStack;
   bool StoreModuleDesc = false;
 };
 
 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.
 class PrintPassInstrumentation {
 public:
   PrintPassInstrumentation(bool DebugLogging) : DebugLogging(DebugLogging) {}
   void registerCallbacks(PassInstrumentationCallbacks &PIC);
 
class StandardInstrumentations {
   PreservedCFGCheckerInstrumentation PreservedCFGChecker;
   IRChangePrinter PrintChangedIR;
   VerifyInstrumentation Verify;
 
   bool VerifyEach;
 
 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);
 
   TimePassesHandler &getTimePasses() { return TimePasses; }
 };
 } // namespace llvm
 
 #endif

llvm/lib/Passes/StandardInstrumentations.cpp

 IRChangePrinter::IRChangePrinter() : Out(dbgs()) {}
 
 IRChangePrinter::~IRChangePrinter() {}
 
 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 &) {
         handleIRAfterPass(IR, P);
       });
   PIC.registerAfterPassInvalidatedCallback(
       [this](StringRef P, const PreservedAnalyses &) {
         handleInvalidatedPass(P);
PIC.registerAfterPassInvalidatedCallback(
         [this](StringRef P, const PreservedAnalyses &) {
           this->printAfterPassInvalidated(P);
         });
   }
 }
 
 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;
   if (any_isa<const Function *>(IR)) {
     F = any_cast<const Function *>(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(
     PassInstrumentationCallbacks &PIC) {
   if (!DebugLogging)
     return;
 
   std::vector<StringRef> SpecialPasses = {"PassManager"};

llvm/test/Feature/optnone-opt.ll

 ; RUN: opt     -S -debug -enable-new-pm=0 %s 2>&1 | FileCheck %s --check-prefix=O0
 ; RUN: opt -O1 -S -debug -enable-new-pm=0 %s 2>&1 | FileCheck %s --check-prefix=O1
 ; RUN: opt -O2 -S -debug -enable-new-pm=0 %s 2>&1 | FileCheck %s --check-prefix=O1 --check-prefix=O2O3
 ; 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
 
 ; This test verifies that we don't run target independent IR-level
 ; 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
   br label %while.cond
 
 while.cond:                                       ; preds = %while.body, %entry
   %0 = load i32, i32* %x.addr, align 4
   %dec = add nsw i32 %0, -1

 
 ; IR passes run at -O1 and higher.
 ; O1-DAG: Skipping pass 'Aggressive Dead Code Elimination'
 ; O1-DAG: Skipping pass 'Combine redundant instructions'
 ; O1-DAG: Skipping pass 'Early CSE'
 ; 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
 ; NPM-O1-DAG: Skipping pass: PromotePass
 ; NPM-O1-DAG: Skipping pass: InstCombinePass
 
 ; Additional IR passes run at -O2 and higher.
 ; O2O3-DAG: Skipping pass 'Global Value Numbering'

 ; LOOP-DAG: Skipping pass 'Recognize loop idioms'
 ; LOOP-DAG: Skipping pass 'Reroll loops'
 ; LOOP-DAG: Skipping pass 'Rotate Loops'
 ; LOOP-DAG: Skipping pass 'Simplify instructions in loops'
 ; LOOP-DAG: Skipping pass 'Unroll loops'
 ; 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
 ; NPM-LOOP-DAG: Skipping pass: LoopUnrollPass
 ; NPM-LOOP-DAG: Skipping pass: LoopStrengthReducePass
 ; NPM-LOOP-DAG: Skipping pass: LoopDeletionPass
 ; 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

struct MockPassInstrumentationCallbacks {
   MOCK_METHOD3(runAfterPass,
                void(StringRef PassID, llvm::Any, const PreservedAnalyses &PA));
   MOCK_METHOD2(runAfterPassInvalidated,
                void(StringRef PassID, const PreservedAnalyses &PA));
   MOCK_METHOD2(runBeforeAnalysis, void(StringRef PassID, llvm::Any));
   MOCK_METHOD2(runAfterAnalysis, void(StringRef PassID, llvm::Any));
 
   void registerPassInstrumentation() {
-    Callbacks.registerBeforePassCallback([this](StringRef P, llvm::Any 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);
         });
     Callbacks.registerBeforeNonSkippedPassCallback(
         [this](StringRef P, llvm::Any IR) {
           this->runBeforeNonSkippedPass(P, IR);
         });