[PATCH] D51276: [New PM][PassTiming] implement -time-passes for the new pass manager

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe added a comment.

Some inline comments.



================
Comment at: include/llvm/IR/PassTimingInfo.h:23
 #include "llvm/ADT/StringRef.h"
+#include "llvm/IR/PassInstrumentation.h"
 #include "llvm/Support/Timer.h"
----------------
You could forward declare `PassInstrumentationCallbacks` instead.


================
Comment at: include/llvm/IR/PassTimingInfo.h:85
+template <>
+class PassTimingInfo<StringRef> {
+  public:
----------------
What if someone wants a legacy  StringRef PassTimingInfo. This should just be a new standalone type, especially since PassTimingInfo is somewhat of a template "interface" that your specialization doesn't fulfill anymore.


================
Comment at: include/llvm/IR/PassTimingInfo.h:104
+
+  /// Prints out timing information and then resets the timers.
+  void print();
----------------
The implementation doesn't reset.


================
Comment at: include/llvm/IR/PassTimingInfo.h:127
+  /// Destructor handles the print action if it has not been handled before.
+  ~TimePassesHandler();
+
----------------
Why is this explicit when it doesn't do anything?


================
Comment at: include/llvm/IR/PassTimingInfo.h:150
+
+  static bool matchPassManager(StringRef PassID);
+
----------------
A private static can just be a free static function.


================
Comment at: include/llvm/IR/PassTimingInfo.h:154
+
+  std::unique_ptr<PassTimingInfo<StringRef>> TimingInfo;
+};
----------------
Why doesn't this contain the object directly?


================
Comment at: lib/Passes/PassBuilder.cpp:60
 #include "llvm/IR/PassManager.h"
+#include "llvm/IR/PassTimingInfo.h"
 #include "llvm/IR/Verifier.h"
----------------
Is this include actually used?


Repository:
  rL LLVM

https://reviews.llvm.org/D51276