[NewPM][PassInstrument] Add a new kind of before-pass callback that only get called if the pass is not skipped

[ychen]

Summary

TODO

• PrintIRInstrumentation and TimePassesHandler would be using this new callback.
• "Running pass" logging will also be moved to use this callback.

[asbirlea]

llvm/include/llvm/IR/PassInstrumentation.h

77

Perhaps BeforePassRequiredFunc (or similar) is more informative that these callbacks are run for required passes?

[ychen]

Indeed. Will do.

[aeubanks]

llvm/include/llvm/IR/PassInstrumentation.h

77

"required" is sort of misleading, these callbacks will run on passes that aren't "required" as long as nothing says the pass should be skipped.
BeforePassIsNotSkippedFunc? a little verbose

[asbirlea]

llvm/include/llvm/IR/PassInstrumentation.h

77

Sure, in my mind if a pass is not skipped, it can be viewed as required. I'm ok with either renaming as long we make it more informative.

[llvm-commits]

ahh, naming is hard. I was thinking of required to run. Yes, it could also be required pass which is misleading. I'll go with BeforePassIsNotSkippedFunc which is not ambiguous.

[ychen]

• Use BeforeNonSkippedPassFunc

[aeubanks]

What about unit tests?

llvm/include/llvm/IR/PassInstrumentation.h

71

Rather than this sentence, I think there should be a comment that we call all BeforePassFuncs to determine if the pass should run or not.

[ychen]

• Add unit tests
• Add comments

[aeubanks]

lgtm

[yrouban]

llvm/include/llvm/IR/PassInstrumentation.h

76–77

I would rename BeforePass to ShouldRun.
then BeforeNonSkippedPass to BeforePass.

llvm/include/llvm/IR/PassInstrumentation.h

 /// This class manages callbacks registration, as well as provides a way for
 /// PassInstrumentation to pass control to the registered callbacks.
 class PassInstrumentationCallbacks {
 public:
   // Before/After callbacks accept IRUnits whenever appropriate, so they need
   // to take them as constant pointers, wrapped with llvm::Any.
   // For the case when IRUnit has been invalidated there is a different
   // callback to use - AfterPassInvalidated.
+  // We call all BeforePassFuncs to determine if a pass should run or not.
+  // BeforeNonSkippedPassFuncs are called only if the pass should run.
   // TODO: currently AfterPassInvalidated does not accept IRUnit, since passing
-  // already invalidated IRUnit is unsafe. There are ways to handle invalidated IRUnits
-  // in a safe way, and we might pursue that as soon as there is a useful instrumentation
-  // that needs it.
+  // already invalidated IRUnit is unsafe. There are ways to handle invalidated
+  // IRUnits in a safe way, and we might pursue that as soon as there is a
+  // useful instrumentation that needs it.
   using BeforePassFunc = bool(StringRef, Any);
+  using BeforeNonSkippedPassFunc = void(StringRef, Any);
   using AfterPassFunc = void(StringRef, Any);
   using AfterPassInvalidatedFunc = void(StringRef);
   using BeforeAnalysisFunc = void(StringRef, Any);
   using AfterAnalysisFunc = void(StringRef, Any);
 
 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 registerBeforeNonSkippedPassCallback(CallableT C) {
+    BeforeNonSkippedPassCallbacks.emplace_back(std::move(C));
+  }
+
   template <typename CallableT> void registerAfterPassCallback(CallableT C) {
     AfterPassCallbacks.emplace_back(std::move(C));
   }
 
   template <typename CallableT>
   void registerAfterPassInvalidatedCallback(CallableT C) {
     AfterPassInvalidatedCallbacks.emplace_back(std::move(C));
   }
 
   template <typename CallableT>
   void registerBeforeAnalysisCallback(CallableT C) {
     BeforeAnalysisCallbacks.emplace_back(std::move(C));
   }
 
   template <typename CallableT>
   void registerAfterAnalysisCallback(CallableT C) {
     AfterAnalysisCallbacks.emplace_back(std::move(C));
   }
 
 private:
   friend class PassInstrumentation;
 
   SmallVector<llvm::unique_function<BeforePassFunc>, 4> BeforePassCallbacks;
+  SmallVector<llvm::unique_function<BeforeNonSkippedPassFunc>, 4>
+      BeforeNonSkippedPassCallbacks;
   SmallVector<llvm::unique_function<AfterPassFunc>, 4> AfterPassCallbacks;
   SmallVector<llvm::unique_function<AfterPassInvalidatedFunc>, 4>
       AfterPassInvalidatedCallbacks;
   SmallVector<llvm::unique_function<BeforeAnalysisFunc>, 4>
       BeforeAnalysisCallbacks;
   SmallVector<llvm::unique_function<AfterAnalysisFunc>, 4>
       AfterAnalysisCallbacks;
 };
public:
   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 (ShouldRun) {
+      for (auto &C : Callbacks->BeforeNonSkippedPassCallbacks)
+        C(Pass.name(), llvm::Any(&IR));
+    }
+
     return ShouldRun;
   }
 
   /// AfterPass instrumentation point - takes \p Pass instance that has
   /// just been executed and constant reference to \p IR it operates on.
   /// \p IR is guaranteed to be valid at this point.
   template <typename IRUnitT, typename PassT>
   void runAfterPass(const PassT &Pass, const IRUnitT &IR) const {

llvm/unittests/IR/PassBuilderCallbacksTest.cpp

 
 struct MockPassInstrumentationCallbacks {
   PassInstrumentationCallbacks Callbacks;
 
   MockPassInstrumentationCallbacks() {
     ON_CALL(*this, runBeforePass(_, _)).WillByDefault(Return(true));
   }
   MOCK_METHOD2(runBeforePass, bool(StringRef PassID, llvm::Any));
+  MOCK_METHOD2(runBeforeNonSkippedPass, void(StringRef PassID, llvm::Any));
   MOCK_METHOD2(runAfterPass, void(StringRef PassID, llvm::Any));
   MOCK_METHOD1(runAfterPassInvalidated, void(StringRef PassID));
   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) {
       return this->runBeforePass(P, IR);
     });
+    Callbacks.registerBeforeNonSkippedPassCallback(
+        [this](StringRef P, llvm::Any IR) {
+          this->runBeforeNonSkippedPass(P, IR);
+        });
     Callbacks.registerAfterPassCallback(
         [this](StringRef P, llvm::Any IR) { this->runAfterPass(P, IR); });
     Callbacks.registerAfterPassInvalidatedCallback(
         [this](StringRef P) { this->runAfterPassInvalidated(P); });
     Callbacks.registerBeforeAnalysisCallback([this](StringRef P, llvm::Any IR) {
       return this->runBeforeAnalysis(P, IR);
     });
     Callbacks.registerAfterAnalysisCallback(
         [this](StringRef P, llvm::Any IR) { this->runAfterAnalysis(P, IR); });
   }
 
   void ignoreNonMockPassInstrumentation(StringRef IRName) {
     // Generic EXPECT_CALLs are needed to match instrumentation on unimportant
     // parts of a pipeline that we do not care about (e.g. various passes added
     // by default by PassBuilder - Verifier pass etc).
     // Make sure to avoid ignoring Mock passes/analysis, we definitely want
     // to check these explicitly.
     EXPECT_CALL(*this,
                 runBeforePass(Not(HasNameRegex("Mock")), HasName(IRName)))
         .Times(AnyNumber());
+    EXPECT_CALL(*this, runBeforeNonSkippedPass(Not(HasNameRegex("Mock")),
+                                               HasName(IRName)))
+        .Times(AnyNumber());
     EXPECT_CALL(*this, runAfterPass(Not(HasNameRegex("Mock")), HasName(IRName)))
         .Times(AnyNumber());
     EXPECT_CALL(*this,
                 runBeforeAnalysis(Not(HasNameRegex("Mock")), HasName(IRName)))
         .Times(AnyNumber());
     EXPECT_CALL(*this,
                 runAfterAnalysis(Not(HasNameRegex("Mock")), HasName(IRName)))
         .Times(AnyNumber());
TEST_F(ModuleCallbacksTest, InstrumentedPasses) {
 
   // PassInstrumentation calls should happen in-sequence, in the same order
   // as passes/analyses are scheduled.
   ::testing::Sequence PISequence;
   EXPECT_CALL(CallbacksHandle, runBeforePass(HasNameRegex("MockPassHandle"),
                                              HasName("<string>")))
       .InSequence(PISequence);
   EXPECT_CALL(CallbacksHandle,
+              runBeforeNonSkippedPass(HasNameRegex("MockPassHandle"),
+                                      HasName("<string>")))
+      .InSequence(PISequence);
+  EXPECT_CALL(CallbacksHandle,
               runBeforeAnalysis(HasNameRegex("MockAnalysisHandle"),
                                 HasName("<string>")))
       .InSequence(PISequence);
   EXPECT_CALL(
       CallbacksHandle,
       runAfterAnalysis(HasNameRegex("MockAnalysisHandle"), HasName("<string>")))
       .InSequence(PISequence);
   EXPECT_CALL(CallbacksHandle,
TEST_F(ModuleCallbacksTest, InstrumentedSkippedPasses) {
   // Skip all passes by returning false. Pass managers and adaptor passes are
   // also passes that observed by the callbacks.
   EXPECT_CALL(CallbacksHandle, runBeforePass(_, _))
       .WillRepeatedly(Return(false));
 
   EXPECT_CALL(AnalysisHandle, run(HasName("<string>"), _)).Times(0);
   EXPECT_CALL(PassHandle, run(HasName("<string>"), _)).Times(0);
 
-  // As the pass is skipped there is no afterPass, beforeAnalysis/afterAnalysis
-  // as well.
+  // As the pass is skipped there is no nonskippedpass/afterPass,
+  // beforeAnalysis/afterAnalysis as well.
+  EXPECT_CALL(CallbacksHandle,
+              runBeforeNonSkippedPass(HasNameRegex("MockPassHandle"), _))
+      .Times(0);
   EXPECT_CALL(CallbacksHandle, runAfterPass(HasNameRegex("MockPassHandle"), _))
       .Times(0);
   EXPECT_CALL(CallbacksHandle,
               runBeforeAnalysis(HasNameRegex("MockAnalysisHandle"), _))
       .Times(0);
   EXPECT_CALL(CallbacksHandle,
               runAfterAnalysis(HasNameRegex("MockAnalysisHandle"), _))
       .Times(0);
 
   // Order is important here. `Adaptor` expectations should be checked first
   // because the its argument contains 'PassManager' (for example:
-  // ModuleToFunctionPassAdaptor{{.*}}PassManager{{.*}}). Here only check
-  // `runAfterPass` to show that they are not skipped.
-
+  // ModuleToFunctionPassAdaptor{{.*}}PassManager{{.*}}). Check
+  // `runBeforeNonSkippedPass` and `runAfterPass` to show that they are not
+  // skipped.
+  //
   // Pass managers are not ignored.
   // 5 = (1) ModulePassManager + (2) FunctionPassMangers + (1) LoopPassManager +
   //     (1) CGSCCPassManager
+  EXPECT_CALL(CallbacksHandle,
+              runBeforeNonSkippedPass(HasNameRegex("PassManager"), _))
+      .Times(5);
+  EXPECT_CALL(
+      CallbacksHandle,
+      runBeforeNonSkippedPass(HasNameRegex("ModuleToFunctionPassAdaptor"), _))
+      .Times(1);
+  EXPECT_CALL(CallbacksHandle,
+              runBeforeNonSkippedPass(
+                  HasNameRegex("ModuleToPostOrderCGSCCPassAdaptor"), _))
+      .Times(1);
+  EXPECT_CALL(
+      CallbacksHandle,
+      runBeforeNonSkippedPass(HasNameRegex("CGSCCToFunctionPassAdaptor"), _))
+      .Times(1);
+  EXPECT_CALL(
+      CallbacksHandle,
+      runBeforeNonSkippedPass(HasNameRegex("FunctionToLoopPassAdaptor"), _))
+      .Times(1);
+
+  // The `runAfterPass` checks are the same as these of
+  // `runBeforeNonSkippedPass`.
   EXPECT_CALL(CallbacksHandle, runAfterPass(HasNameRegex("PassManager"), _))
       .Times(5);
   EXPECT_CALL(CallbacksHandle,
               runAfterPass(HasNameRegex("ModuleToFunctionPassAdaptor"), _))
       .Times(1);
   EXPECT_CALL(
       CallbacksHandle,
       runAfterPass(HasNameRegex("ModuleToPostOrderCGSCCPassAdaptor"), _))
TEST_F(FunctionCallbacksTest, InstrumentedPasses) {
   // PassInstrumentation calls should happen in-sequence, in the same order
   // as passes/analyses are scheduled.
   ::testing::Sequence PISequence;
   EXPECT_CALL(CallbacksHandle,
               runBeforePass(HasNameRegex("MockPassHandle"), HasName("foo")))
       .InSequence(PISequence);
   EXPECT_CALL(
       CallbacksHandle,
+      runBeforeNonSkippedPass(HasNameRegex("MockPassHandle"), HasName("foo")))
+      .InSequence(PISequence);
+  EXPECT_CALL(
+      CallbacksHandle,
       runBeforeAnalysis(HasNameRegex("MockAnalysisHandle"), HasName("foo")))
       .InSequence(PISequence);
   EXPECT_CALL(
       CallbacksHandle,
       runAfterAnalysis(HasNameRegex("MockAnalysisHandle"), HasName("foo")))
       .InSequence(PISequence);
   EXPECT_CALL(CallbacksHandle,
               runAfterPass(HasNameRegex("MockPassHandle"), HasName("foo")))
TEST_F(LoopCallbacksTest, InstrumentedPasses) {
   // PassInstrumentation calls should happen in-sequence, in the same order
   // as passes/analyses are scheduled.
   ::testing::Sequence PISequence;
   EXPECT_CALL(CallbacksHandle,
               runBeforePass(HasNameRegex("MockPassHandle"), HasName("loop")))
       .InSequence(PISequence);
   EXPECT_CALL(
       CallbacksHandle,
+      runBeforeNonSkippedPass(HasNameRegex("MockPassHandle"), HasName("loop")))
+      .InSequence(PISequence);
+  EXPECT_CALL(
+      CallbacksHandle,
       runBeforeAnalysis(HasNameRegex("MockAnalysisHandle"), HasName("loop")))
       .InSequence(PISequence);
   EXPECT_CALL(
       CallbacksHandle,
       runAfterAnalysis(HasNameRegex("MockAnalysisHandle"), HasName("loop")))
       .InSequence(PISequence);
   EXPECT_CALL(CallbacksHandle,
               runAfterPass(HasNameRegex("MockPassHandle"), HasName("loop")))
TEST_F(LoopCallbacksTest, InstrumentedInvalidatingPasses) {
   // PassInstrumentation calls should happen in-sequence, in the same order
   // as passes/analyses are scheduled.
   ::testing::Sequence PISequence;
   EXPECT_CALL(CallbacksHandle,
               runBeforePass(HasNameRegex("MockPassHandle"), HasName("loop")))
       .InSequence(PISequence);
   EXPECT_CALL(
       CallbacksHandle,
+      runBeforeNonSkippedPass(HasNameRegex("MockPassHandle"), HasName("loop")))
+      .InSequence(PISequence);
+  EXPECT_CALL(
+      CallbacksHandle,
       runBeforeAnalysis(HasNameRegex("MockAnalysisHandle"), HasName("loop")))
       .InSequence(PISequence);
   EXPECT_CALL(
       CallbacksHandle,
       runAfterAnalysis(HasNameRegex("MockAnalysisHandle"), HasName("loop")))
       .InSequence(PISequence);
   EXPECT_CALL(CallbacksHandle,
               runAfterPassInvalidated(HasNameRegex("MockPassHandle")))
TEST_F(CGSCCCallbacksTest, InstrumentedPasses) {
   // PassInstrumentation calls should happen in-sequence, in the same order
   // as passes/analyses are scheduled.
   ::testing::Sequence PISequence;
   EXPECT_CALL(CallbacksHandle,
               runBeforePass(HasNameRegex("MockPassHandle"), HasName("(foo)")))
       .InSequence(PISequence);
   EXPECT_CALL(
       CallbacksHandle,
+      runBeforeNonSkippedPass(HasNameRegex("MockPassHandle"), HasName("(foo)")))
+      .InSequence(PISequence);
+  EXPECT_CALL(
+      CallbacksHandle,
       runBeforeAnalysis(HasNameRegex("MockAnalysisHandle"), HasName("(foo)")))
       .InSequence(PISequence);
   EXPECT_CALL(
       CallbacksHandle,
       runAfterAnalysis(HasNameRegex("MockAnalysisHandle"), HasName("(foo)")))
       .InSequence(PISequence);
   EXPECT_CALL(CallbacksHandle,
               runAfterPass(HasNameRegex("MockPassHandle"), HasName("(foo)")))
TEST_F(CGSCCCallbacksTest, InstrumentedInvalidatingPasses) {
   // PassInstrumentation calls should happen in-sequence, in the same order
   // as passes/analyses are scheduled.
   ::testing::Sequence PISequence;
   EXPECT_CALL(CallbacksHandle,
               runBeforePass(HasNameRegex("MockPassHandle"), HasName("(foo)")))
       .InSequence(PISequence);
   EXPECT_CALL(
       CallbacksHandle,
+      runBeforeNonSkippedPass(HasNameRegex("MockPassHandle"), HasName("(foo)")))
+      .InSequence(PISequence);
+  EXPECT_CALL(
+      CallbacksHandle,
       runBeforeAnalysis(HasNameRegex("MockAnalysisHandle"), HasName("(foo)")))
       .InSequence(PISequence);
   EXPECT_CALL(
       CallbacksHandle,
       runAfterAnalysis(HasNameRegex("MockAnalysisHandle"), HasName("(foo)")))
       .InSequence(PISequence);
   EXPECT_CALL(CallbacksHandle,
               runAfterPassInvalidated(HasNameRegex("MockPassHandle")))