[PATCH] D47858: [New PM] Introducing PassInstrumentation framework

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM (provided Philip is still happy here, I know I asked to remove his plugin hooks).

I think this is good to go in. I want to keep pushing on the tuple based analysis results stuff, but I'm happy to do that in the follow-up change. I've left some much more minor comments below. If they make sense, feel free to land this with them addressed and we'll get to the follow-ups. =D

Thanks again *so much* for the huge effort working on this. I know it was really complex, and I'm really happy with the result.



================
Comment at: include/llvm/IR/PassInstrumentation.h:127
+
+  /// AfterPass instrumentation point - takes \p PassID instance that has
+  /// just been executed and constant reference to IR it operates on.
----------------
Slightly stale comment re. `PassID`.


================
Comment at: unittests/IR/PassBuilderCallbacksTest.cpp:324-330
+  void registerPassInstrumentation() {
+    Callbacks.registerBeforePassCallback([this](StringRef P, llvm::Any IR) {
+      return this->runBeforePass(P, IR);
+    });
+    Callbacks.registerAfterPassCallback(
+        [this](StringRef P, llvm::Any IR) { this->runAfterPass(P, IR); });
+  }
----------------
Do you just want to do this in the constructor?


================
Comment at: unittests/IR/PassBuilderCallbacksTest.cpp:332-343
+  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,
----------------
This is nice!


Repository:
  rL LLVM

https://reviews.llvm.org/D47858