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

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe 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.
+///
----------------
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.


================
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;
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D47858