[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.

I like how simple this is becoming!

Comments inline.



================
Comment at: include/llvm/IR/PassTimingInfo.h:154
+
+  std::unique_ptr<PassTimingInfo<StringRef>> TimingInfo;
+};
----------------
fedor.sergeev wrote:
> philip.pfaffe wrote:
> > Why doesn't this contain the object directly?
> To avoid creating TimingInfo (its TimeGroup member in particular) if -time-passes is not enabled.
But why? TimerGroup's initialization doesn't really do anything important, and you pay with complexity, an allocation, and all the assertions below. 


================
Comment at: include/llvm/IR/PassTimingInfo.h:44
+/// Pass Timing info specifically for the new pass manager.
+/// Intended to be owned by TimePassesHandler.
+class PassTiming {
----------------
The comment should instead explain what this class does and how it's intended to be used.


================
Comment at: include/llvm/IR/PassTimingInfo.h:47
+public:
+  /// Value of this type is capable of uniquely identifying pass invocations.
+  using UniqPassID = std::pair<StringRef, unsigned>;
----------------
Explain the semantics of the ID.


================
Comment at: include/llvm/IR/PassTimingInfo.h:51
+private:
+  DenseMap<UniqPassID, Timer *>
+      TimingData;                     ///< Map of timers for pass invocations
----------------
PassTiming owns all Timer instances, doesn't it? Store a value instead of a new'ed pointer.


================
Comment at: include/llvm/IR/PassTimingInfo.h:52
+  DenseMap<UniqPassID, Timer *>
+      TimingData;                     ///< Map of timers for pass invocations
+  StringMap<unsigned> PassIDCountMap; ///< Map that counts invocations of passes
----------------
Comments on extra line instead?


================
Comment at: include/llvm/IR/PassTimingInfo.h:78
+
+class TimePassesHandler {
+  bool Enabled;
----------------
Class comments?


================
Comment at: include/llvm/IR/PassTimingInfo.h:79
+class TimePassesHandler {
+  bool Enabled;
+
----------------
This goes below, to the rest of the private members.


================
Comment at: include/llvm/Passes/PassBuilder.h:22
 #include "llvm/IR/PassManager.h"
+#include "llvm/IR/PassTimingInfo.h"
 #include "llvm/Transforms/Instrumentation.h"
----------------
Superfluous include.


================
Comment at: lib/IR/PassTimingInfo.cpp:145
 PassTimingInfo *PassTimingInfo::TheTimeInfo;
+
 } // namespace legacy
----------------
Extra whitespace.


================
Comment at: lib/Passes/PassBuilder.cpp:60
 #include "llvm/IR/PassManager.h"
+#include "llvm/IR/PassTimingInfo.h"
 #include "llvm/IR/Verifier.h"
----------------
philip.pfaffe wrote:
> Is this include actually used?
Marked as done but the include is still there


================
Comment at: lib/Passes/StandardInstrumentations.cpp:111
     PassInstrumentationCallbacks &PIC) {
+  // print-before instrumentation
   if (llvm::shouldPrintBeforePass())
----------------
I don't think these comments here actually add anything.


Repository:
  rL LLVM

https://reviews.llvm.org/D51276