[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:97-107
+/// This class is an opaque wrapper over any new pass manager's pass instance.
+///
+/// Note that new pass manager lacks a particular hierarchy of classes and uses
+/// tricks to store pass instances. Thus we need to handle both actual pass
+/// instances (say, for PassManager instances inside the PassManager itself) and
+/// pass instances wrapped by PassManager into PassModel/PassConcept objects.
+///
----------------
philip.pfaffe wrote:
> fedor.sergeev wrote:
> > chandlerc wrote:
> > > There is a lot of complexity here which seems largely designed to try and produce the opaque pointer. Especially as that pointer isn't really "reliable", is this really that important?
> > > 
> > > What breaks if we simply give the callback the name of the pass? Then we can just have the instrumentation object have templates that take a generic `PassT &P` and then do `P.name()` to get the name. This should work both with concrete passes and with all of the `PassConcept` objects (the real case).
> > > 
> > > I think the tuple of (name, IR unit) is about the most precise we can make the identity in the API.
> > > 
> > > This doesn't prevent the instrumentation callback from doing things like keeping counters and such internally to differentiate what is going on....
> > The problem is that "name" is not going to give callbacks a way to differentiate between *instances* of passes.
> > And, say, -time-passes functionality needs to collect timings per-instance.
> > 
> How would a callback with a template argument look like, and how would you store it?
> 
> Can you do it without another Concept/Model pair capturing the Pass interface? Because I don't think that'd save a lot in terms of complexity.
It's not clear that time-passes needs this functionality.

My problem is that I think this overpromises something that we cannot actually deliver: This cannot capture the *iteration* count either, and that is just as much of a factor as the position in the pipeline.

If we want detailed statistics for timing, we should collect the time of *each* run (and the IR unit it ran over) and show them separately. For summary statistics, I think it just being the name is good in that it sets reasonable expectations.


================
Comment at: include/llvm/IR/PassInstrumentation.h:97-107
+/// This class is an opaque wrapper over any new pass manager's pass instance.
+///
+/// Note that new pass manager lacks a particular hierarchy of classes and uses
+/// tricks to store pass instances. Thus we need to handle both actual pass
+/// instances (say, for PassManager instances inside the PassManager itself) and
+/// pass instances wrapped by PassManager into PassModel/PassConcept objects.
+///
----------------
chandlerc wrote:
> philip.pfaffe wrote:
> > fedor.sergeev wrote:
> > > chandlerc wrote:
> > > > There is a lot of complexity here which seems largely designed to try and produce the opaque pointer. Especially as that pointer isn't really "reliable", is this really that important?
> > > > 
> > > > What breaks if we simply give the callback the name of the pass? Then we can just have the instrumentation object have templates that take a generic `PassT &P` and then do `P.name()` to get the name. This should work both with concrete passes and with all of the `PassConcept` objects (the real case).
> > > > 
> > > > I think the tuple of (name, IR unit) is about the most precise we can make the identity in the API.
> > > > 
> > > > This doesn't prevent the instrumentation callback from doing things like keeping counters and such internally to differentiate what is going on....
> > > The problem is that "name" is not going to give callbacks a way to differentiate between *instances* of passes.
> > > And, say, -time-passes functionality needs to collect timings per-instance.
> > > 
> > How would a callback with a template argument look like, and how would you store it?
> > 
> > Can you do it without another Concept/Model pair capturing the Pass interface? Because I don't think that'd save a lot in terms of complexity.
> It's not clear that time-passes needs this functionality.
> 
> My problem is that I think this overpromises something that we cannot actually deliver: This cannot capture the *iteration* count either, and that is just as much of a factor as the position in the pipeline.
> 
> If we want detailed statistics for timing, we should collect the time of *each* run (and the IR unit it ran over) and show them separately. For summary statistics, I think it just being the name is good in that it sets reasonable expectations.
The method on PassInstrumentation can be the template, and the callback just gets the `StringRef` for the name.


================
Comment at: include/llvm/IR/PassInstrumentation.h:195
+
+  bool empty() const;
+
----------------
fedor.sergeev wrote:
> chandlerc wrote:
> > Why expose this? We can trivially implement the empty case anyways?
> The intent is to avoid overhead of instrumentation framework as much as possible for the case when there are no callbacks registered.
> Current implementation provides nullptr to the proxy analysis instead of the empty Instrumentation object.
> It saves some minor cycles on dereferencing and iteration through empty vectors.
> 
> First implementation had no need for empty() since PassBuilder was managing the object and avoided creating it altogether if no registrations requests were given. Current implementation manages PassInstrumentation outside of PassBuilder, so PassBuilder needs empty().
I thikn you should keep simplifying.

The cost of an empty for loop in the instrumentation is going to be tiny. We shouldn't add complexity to "avoid" it, especially when that complexity is a wider interface.


================
Comment at: include/llvm/IR/PassInstrumentation.h:198
+private:
+  PassExecutionCounter PC;
+
----------------
fedor.sergeev wrote:
> chandlerc wrote:
> > Why include this here? Seems like the counter stuff could be separate and just managed by the callbacks when wired up?
> Oh, well... had an idea on making counters independent from callbacks decisions on skipping some passes, but then realized it is hardly possible overall. So yes, it can be made separate.
> And then I hardly need to introduce this class at all here.
It still seems useful to hold the callbacks and handle the dispatch (with a template).


================
Comment at: include/llvm/IR/PassManager.h:760-768
+  /// Get the PassInstrumentation object as a result of
+  /// PassInstrumentationProxy.
+  PassInstrumentation *getPassInstrumentation(IRUnitT &IR,
+                                              ExtraArgTs... ExtraArgs) {
+    // avoid failing when analysis is not registered (mostly for tests).
+    if (!isRegisteredAnalysis<PassInstrumentationProxy>())
+      return nullptr;
----------------
philip.pfaffe wrote:
> fedor.sergeev wrote:
> > chandlerc wrote:
> > > This coupling with the analysis manager seems really awkward. It makes it no longer "just an analysis". I'd really like to get back to that.
> > > 
> > > I see at least two ways to do this:
> > > 
> > > 1) just bite the bullet and require that if you use an analysis manager, you have to register the instrumentation
> > > 2) Write a utility function that implements this but isn't part of the analysis manager.
> > > 
> > > I lean slightly toward #1, but maybe its way more awkward than I'm imagining? It would have the advantage of simplifying the code because yo uwould "always" have a (potentially trivial) instrumentation object.
> > "just an analysis" is perhaps not fully true anyway, since it is the only analysis that is being explicitly queried in all the generic parts of the code, even in generic PassManager.
> > If we decide #1 then we essentially legalize it not being "just an analysis". And I'm fine with it.
> > It will require adding registration to all the PassManager tests that do not use PassBuilder currently (not a big deal either, just a hassle).
> > 
> I'd +1 the first way, too. When using a PassManager most of the time you're not free to run around with an empty AnalysisManager anyways, so the burden would be low, IMO.
Yeah, the more I think, the more this can just be an analysis.

We can have a default constructor that leaves all the callbacks empty. It won't do anything interesting. But the code will be simple and work.

Then users can register more interesting ones as they like.


Repository:
  rL LLVM

https://reviews.llvm.org/D47858