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

[Fedor Sergeev via Phabricator via llvm-commits]

fedor.sergeev 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.
+///
----------------
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.



================
Comment at: include/llvm/IR/PassInstrumentation.h:195
+
+  bool empty() const;
+
----------------
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().


================
Comment at: include/llvm/IR/PassInstrumentation.h:198
+private:
+  PassExecutionCounter PC;
+
----------------
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.


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



Repository:
  rL LLVM

https://reviews.llvm.org/D47858