[PATCH] D51276: [New PM][PassTiming] implement -time-passes for the new pass manager
Philip Pfaffe via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 25 03:08:32 PDT 2018
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
More information about the llvm-commits
mailing list