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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Some more minor code comments inline...

While I'm fine for it to arrive in a follow-up patch, I do think we'll want a mechanism to aggregate by pass name, and I suspect that should even be the default, with the per-invocation timings available by passing more detailed flags. (In many cases, the per-invocation will be *massive* due to the number of functions in the module.)



================
Comment at: include/llvm/IR/PassTimingInfo.h:49
+  /// to all the instance of a given pass) + sequential invocation counter.
+  using UniqPassID = std::pair<StringRef, unsigned>;
+
----------------
Maybe `PassInvocationID`? That makes the name more directly explain what it does.


================
Comment at: include/llvm/IR/PassTimingInfo.h:55
+  /// Map of timers for pass invocations
+  DenseMap<UniqPassID, Timer*> TimingData;
+
----------------
Why not `std::unique_ptr<Timer>`?


================
Comment at: include/llvm/IR/PassTimingInfo.h:71
+    for (auto &I : TimingData)
+      delete I.getSecond();
+  }
----------------
I would suggest using idiomatic STL APIs here rather than the (quite odd) APIs from `DenseMap`. Common pattern is:

```
  for (auto &KV : Timingdata)
    delete KV.second;
```


================
Comment at: include/llvm/IR/PassTimingInfo.h:94
+public:
+  /// do not initialize timing info for starters
+  TimePassesHandler(bool Enabled = TimePassesIsEnabled) : Enabled(Enabled) {}
----------------
This comment seems more like a fragment...


================
Comment at: include/llvm/IR/PassTimingInfo.h:114
+
+  // timer helpers
+  void startTimer(StringRef PassID);
----------------
Don't think this comment really helps.


================
Comment at: include/llvm/IR/PassTimingInfo.h:120
+  SmallVector<Timer *, 8> TimerStack; //< Stack of currently active timers.
+  PassTiming TimingInfo;              //< Timers mapped to the passes.
+};
----------------
What is the advantage of having two separate types here? (Not a rhetorical question, just isn't obvious to me on reading the code.)


================
Comment at: lib/Passes/StandardInstrumentations.cpp:111
     PassInstrumentationCallbacks &PIC) {
+  // print-before instrumentation
   if (llvm::shouldPrintBeforePass())
----------------
fedor.sergeev wrote:
> philip.pfaffe wrote:
> > I don't think these comments here actually add anything.
> if you grep the sources for the name of the option you will find this.
> Other than that - yes, it does not add anything to the code readability.
Maybe if they were full prose it would be more clear? As it is, these are I think mostly helpful given context that you have implicitly in your head. I suspect expanding them into prose will either make them both clear and useful to readers, or will make it more obvious that they are just repeating what the code already clearly states it is doing.


Repository:
  rL LLVM

https://reviews.llvm.org/D51276