[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.
+  // BeforePassRunFunc is called when we know for sure that the pass would be
+  // run. BeforePassFunc could not make such assumption.
   // 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 BeforePassRunFunc = 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 registerBeforePassRunCallback(CallableT C) {
+    BeforePassRunCallbacks.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<BeforePassRunFunc>, 4>
+      BeforePassRunCallbacks;
   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->BeforePassRunCallbacks)
+        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 {