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

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe added a comment.

Back from vacation, and had time for a closer look.

My major concern right now is ownership. In your current state, `PassBuilder` owns both all the callbacks as well as the state of the instrumentation (i.e., the counter). As it were, PassBuilder is in fact more of a transient factory that you can throw a way after you've set up your pipeline. Or you might even use it to make more than one pipeline. In both cases ownership is a problem.

Instead, I believe it makes for nicer separation if PassBuilder becomes purely the entry point for callback registration, and all bookkeeping happens within the pipeline. IMO,  a good place to put this would be the Module-level Analysis itself, which is a singleton wrt. a single compilation. Federation between different IRUnits can happen via proxies, which adds some amount of indirections, but I still think that's manageable.

Lastly, I think this deserves unittests. You can check out PassBuilderCallbacksTests or LoopPassManagerTests for inspiration on how to mock out passes and analyses. This might also be a perfect moment to nudge @chandlerc in the direction of https://reviews.llvm.org/D39678 to simplify this :)


Repository:
  rL LLVM

https://reviews.llvm.org/D47858