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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: include/llvm/IR/PassInstrumentation.h:85-91
+  void registerBeforePassCallback(llvm::unique_function<BeforePassFunc> &&C) {
+    BeforePassCallbacks.emplace_back(std::move(C));
+  }
+
+  void registerAfterPassCallback(llvm::unique_function<AfterPassFunc> &&C) {
+    AfterPassCallbacks.emplace_back(std::move(C));
+  }
----------------
My suggestion was to go a step further, and have these be template methods that can accept an arbitrary callable (IE, a template).

If they get called with an existing `unique_function`, then it'll work like what you have here, but if they get called with a lambda (common case), they will construct the `unique_function` in place in the vector from the lambda.


================
Comment at: include/llvm/IR/PassInstrumentation.h:103-105
+  /// Callbacks object is not owned by PassInstrumentation, its life-time
+  /// should at least match the life-time of corresponding PassInstrumentationAnalysis
+  /// (which usually is till the end of current compilation).
----------------
I would sink this comment to the constructor, as that is the public API that requires the lifetime. This is just the implementation detail of it.


================
Comment at: include/llvm/IR/PassInstrumentation.h:120
+
+    bool shouldRun = true;
+    for (auto &C : Callbacks->BeforePassCallbacks)
----------------
Naming conventions: `ShouldRun`.


================
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.
+///
----------------
fedor.sergeev wrote:
> 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.
"light" i think is still less clear than "clear".


================
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
----------------
fedor.sergeev wrote:
> 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.
I agree that it is best if the instrumentation stuff can be in its own header.

what seems good to extract to a separate header is the `PassInstrumentationAnalysis`. I'm not suggesting you do that in this patch, just leave a FIXME.


================
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(
----------------
philip.pfaffe wrote:
> fedor.sergeev wrote:
> > 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).
> For plugins I'd really like  to retain some means of accessing the Callbacks registry through PassBuilder. To keep it off the PassBuilder API, maybe we should pass it in the plugin APIs `RegisterPassBuilderCallbacks` instead? Could be done in a followup change.
I'd prefer to sort out what makes sense for plugins in a separate patch.

I think it is better (until we know what plugins actually need) to simply have users do their registration on the `PassInstrumentationCallbacks` object they give to the `PassBuilder`. For example, we don't expose any hooks to manipulate the `TargetMachine` from the `PassBuilder` -- instead, clients set it up directly themselves.


================
Comment at: unittests/IR/PassBuilderCallbacksTest.cpp:423-424
+      PIC.registerBeforePassCallback(
+          std::bind(&MockPassInstrumentationCallbacks::runBeforePass,
+                    &PICallbacks, _1, _2));
+      PIC.registerAfterPassCallback(
----------------
fedor.sergeev wrote:
> 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? 
The callbacks object isn't *copied* into the PassBuilder -- you can leave it as a member of the fixture and directly register things onto it?

As I mention above -- I would suggest we sort out the good plugin API when we're actually building the plugin API. Even if it means we end up changing this code because it becomes easier to write it in a different way, that seems OK. Because then, we will *actually* have plugin stuff to look at, rather than hypothesizing how it might look.


================
Comment at: unittests/IR/PassBuilderCallbacksTest.cpp:463-464
+
+  {
+    testing::InSequence InstrumentationExpectationsSequenced;
+    EXPECT_CALL(PICallbacks, runBeforePass(HasNameRegex("MockPassHandle"),
----------------
fedor.sergeev wrote:
> fedor.sergeev wrote:
> > 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)?
> Did the sequencing change, still leaving EXPECT_CALL w/o Times(1) for brevity.
So, this `EXPECT_CALL` does the .Times(1), but once that expectation is saturated, there is still another expectation from the constructor that can accept as many calls as you make. So I would expect this to not fail if you call it *too many* times, only if you call it *too few*.


Repository:
  rL LLVM

https://reviews.llvm.org/D47858