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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

This is looking really, really good. Some comments below.



================
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.
+///
----------------
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....


================
Comment at: include/llvm/IR/PassInstrumentation.h:195
+
+  bool empty() const;
+
----------------
Why expose this? We can trivially implement the empty case anyways?


================
Comment at: include/llvm/IR/PassInstrumentation.h:198
+private:
+  PassExecutionCounter PC;
+
----------------
Why include this here? Seems like the counter stuff could be separate and just managed by the callbacks when wired up?


================
Comment at: include/llvm/IR/PassManager.h:465
+      // false).
+      if (PI && !PI->runBeforePass(Passes[Idx].get(), IR))
+        continue;
----------------
Now that we have multiple uses of `Passes[Idx]`, I would hoist it into a variable.


================
Comment at: include/llvm/IR/PassManager.h:543
+public:
+  PassInstrumentationProxy(PassInstrumentation *_PI) : PI(_PI) {}
+
----------------
No need for `_` here (or anywhere else here). (And `_PI` is a reserved name.)


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


Repository:
  rL LLVM

https://reviews.llvm.org/D47858