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

[Fedor Sergeev via Phabricator via llvm-commits]

fedor.sergeev added a comment.

Thanks for a deep review.



================
Comment at: include/llvm/IR/PassInstrumentation.h:17
+///     pass managers to call on. It is light on copy since it is queries
+///     through the analysis framework as a result of the analysis.
+///
----------------
chandlerc wrote:
> Not sure this explanation of why the copy is cheap quite parses for me.
It doesnt quite parse for me as well :)
Pehaps I meant "since it is being queried through the analysis framework".
Will try to reword it in a more straightforward way.


================
Comment at: include/llvm/IR/PassInstrumentation.h:134-135
+  // instrumentation callbacks
+  SmallVector<std::function<BeforePassFunc>, 4> BeforePassCallbacks;
+  SmallVector<std::function<AfterPassFunc>, 4> AfterPassCallbacks;
+};
----------------
chandlerc wrote:
> I would suggest using `llvm::unique_function` throughout this code to support callbacks with move-only captures.
> 
> I generally suspect we should default to `llvm::unique_function` over `std::function` at this point until a need for copy arises.
hmm... did not know about llvm::unique_function.


================
Comment at: include/llvm/IR/PassManager.h:568-571
+/// Pseudo-analysis pass that exposes the \c PassInstrumentation to pass
+/// managers. Goes before AnalysisManager definition to provide its
+/// internals (e.g PassInstrumentationAnalysis::ID) for use there if needed.
+class PassInstrumentationAnalysis
----------------
chandlerc wrote:
> I think we should either move all the instrumentation stuff into this header, or find a way to pull this into the separate header. I think the latter is possible but will require some refactorings. Can you leave a FIXME for now so that we come back to this?
I'm afraid I dont quite follow. Pull *what* into the separate header?
PassInstrumentationAnalysis definition altogether? or anything else?

And I dont like the idea of putting instrumentation stuff into this header.
It is already quite packed with different concepts.
Instrumentation looks distinct enough to deserve its own header.


================
Comment at: include/llvm/Passes/PassBuilder.h:564-568
+  /// Calls registration function \p C on PassInstrumentationCallbacks object
+  /// to register pass instrumentation callbacks there.
+  ///
+  /// \Returns false if there is no pass instrumentation object available.
+  bool registerPassInstrumentation(
----------------
chandlerc wrote:
> I'm somewhat curious what the expected usage of this method is? It isn't obvious to me at all...
It allows to use PassBuilder as sort of a central place for registering all the callbacks, w/o polluting Passbuilder with all the interfaces.
Particularly useful for plugins, I believe.
You can see how it is being used in -print-after-all and -pass-times implementation followups or in the unittests.

Alternative solution would be to introduce PassBuilder::getPassInstrumentationCallbacks() method and use it for registration purposes, but I did not really like that (and yes, getPassInstrumentationCallbacks should have been dropped).


================
Comment at: unittests/IR/PassBuilderCallbacksTest.cpp:268-272
+    // 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).
+    EXPECT_CALL(*this, runBeforePass(_, _)).Times(AnyNumber());
+    EXPECT_CALL(*this, runAfterPass(_, _)).Times(AnyNumber());
----------------
chandlerc wrote:
> Instead of this, you can use `::testing::NiceMock<MockPassInstrumentationCallbacks>` in places where you want to ignore these. The way you've written it, even if users *want* to check for unexpected calls to these routines, they won't be able to do so.
To be honest, my gmock voodoo is weak at best.
I was under impression that my InstrumentedSkippedPasses subtests check exactly for the routines not being called. Did I fail to test that?
(I will go read about NiceMock and see what I can do here)


================
Comment at: unittests/IR/PassBuilderCallbacksTest.cpp:283-284
 
+/// Helper for HasName matcher that returns getName both for IRUnit and
+/// for IRUnit pointer wrapper into llvm::Any (wrapped by PassInstrumentation).
+template <typename IRUnitT> StringRef getName(const IRUnitT &IR) {
----------------
chandlerc wrote:
> Maybe leave a FIXME here?
> 
> We should design a good matcher for `llvm::Any` and then we should be able to compose things. But this is fine for now.
Do you mean a good matcher for llvm::Any or a good generic unwrapper from llvm::Any into IRUnit?
Since llvm::Any is just "any", I find it hard to imagine a good generic matcher for it...


================
Comment at: unittests/IR/PassBuilderCallbacksTest.cpp:423-424
+      PIC.registerBeforePassCallback(
+          std::bind(&MockPassInstrumentationCallbacks::runBeforePass,
+                    &PICallbacks, _1, _2));
+      PIC.registerAfterPassCallback(
----------------
chandlerc wrote:
> `std::bind` should generally be avoided at this point. We can just use lambdas to delegate here.
> 
> And see my comment on `registerPassInstrumentian` above: I think instead we should just register the callbacks first, and hand the object into the pass builder.
> 
> Notably, I think you can have the mock *contain* a `PassInstrumentationCallbacks` object that it pre-registers with lambdas that delegate to the mock, and then just pass that into `PassBuilder` constructor.
> register the callbacks first
What about instrumentation that plugins want to install?
RegisterPassBuilderCallbacks does not have access to anything else except PassBuilder.

> pre-registers with lambdas that delegate to the mock
Hmm... here again my weak gtest/gmock knowledge fails me.
I dont see how can I pass control to fixture constructor from mock side inside , say, TEST_F(ModuleCallbacksTest, InstrumentedPasses) - isnt it in action only when Test object (and its PassBuilder member) have already been constructed?
Care to drop a quick sketch of this idea? 


================
Comment at: unittests/IR/PassBuilderCallbacksTest.cpp:463-464
+
+  {
+    testing::InSequence InstrumentationExpectationsSequenced;
+    EXPECT_CALL(PICallbacks, runBeforePass(HasNameRegex("MockPassHandle"),
----------------
chandlerc wrote:
> I find it simpler to define a `Sequence` object and then connect it to each expectation with the `.InSequence` methods. This makes it fairly easy to avoid a proliferation of scopes to wire these things up. 
> 
> You'll also want to say how *many* calls here, which will make more sense once you make the change I suggest above w.r.t. the number.
Ok, will try the sequence thing.
And about how many calls - I thought EXPECT_CALL by default means Times(1)?


Repository:
  rL LLVM

https://reviews.llvm.org/D47858