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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Very high level comments here...

I don't understand the underlying design well enough to really dig into even the comments Philip made about the IRUnitT stuff. I feel like I'd just be constantly asking questions or making assumptions. INstead, mostly assking for comments to be added at this iteration.



================
Comment at: include/llvm/IR/PassInstrumentation.h:9-12
+//
+// This file defines the PassInstrumentation class that is used to provide
+// instrumentation points into the pass execution by PassManager.
+//
----------------
I'd suggest a `\file` doxygen comment here that gives an overview of the different classes provided as there are quite a few here. I think it would also help readers jump to the one they care about.


================
Comment at: include/llvm/IR/PassInstrumentation.h:43
+
+//===----------------------------------------------------------------------===//
+/// This class provides the main interface for pass instrumentation.
----------------
The separration ascii art doesn't make sense to me here... Are there two sections of the file you're breaking apart here?


================
Comment at: include/llvm/IR/PassInstrumentation.h:44-46
+/// This class provides the main interface for pass instrumentation.
+/// It handles both registration of callbacks and exposes instrumentation calls
+/// to be invoked by PassManagers.
----------------
Please attach to the actual class?


================
Comment at: include/llvm/IR/PassInstrumentation.h:96
+
+template <typename IRUnitT> class PassInstrumentation {
+  PassInstrumentationImpl *Impl;
----------------
No comments at all?


Repository:
  rL LLVM

https://reviews.llvm.org/D47858