[PATCH] D12781: PGO IR-level instrumentation infrastructure
Sean Silva via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 6 21:10:59 PST 2015
silvas added a comment.
This is looking really good!
My biggest concern overall about the implementation is that there seems to be overuse of classes (unnecessary inheritance; unusual use of constructors). It creates quite a maze.
I think that in the process of cleaning up the awkward `PGOIRInstrumentationGenFunc Func(F, &M, BPI, BFI);` to be free functions (see comment inline) you will find many simplifications to the entire implementation.
Remember, the purpose of a constructor is to initialize the invariants for the class's data members. Don't "do an algorithm" in a constructor or execute "steps" of a procedural computation.
================
Comment at: include/llvm/Transforms/Instrumentation.h:182
@@ +181,3 @@
+
+// set the function name and prefix module name for local linkage to avoid
+// the potential name conflict.
----------------
Use correct capitalization http://llvm.org/docs/CodingStandards.html#commenting
================
Comment at: include/llvm/Transforms/Instrumentation.h:184
@@ +183,3 @@
+// the potential name conflict.
+static inline std::string PGOSetFuncName(Module &M, StringRef Name,
+ GlobalValue::LinkageTypes Linkage) {
----------------
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
(here and elsewhere)
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:10
@@ +9,3 @@
+//
+// This pass implements IR level instrumentation for PGO.
+//
----------------
"IR level" is redundant since this is in lib/Transforms.
Please give an overview here of the structure of the file. (2 module passes: "use" and "gen"). Also explain their relationship, sharing data structures, etc.
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:124
@@ +123,3 @@
+
+namespace llvm {
+/// \brief An MST based instrumentation for PGO
----------------
There's a lot of things here that don't seem to be used externally. So use an anonymous namespace.
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:161
@@ +160,3 @@
+ // Return the information string of this object.
+ virtual const std::string infoString() const {
+ return "Index=" + std::to_string(Index);
----------------
Do you actually need dynamic dispatch?
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:167
@@ +166,3 @@
+// This class implements the CFG edges. Note the CFG can be a multi-graph.
+template <class Edge, class BBInfo> class PGOIRInstrumentationFunc {
+private:
----------------
Why "IR"? Are you planning to generalize this to Machine? For now just leave off "IR" (here and elsewhere).
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:193
@@ +192,3 @@
+ PGOIRInstrumentationFunc(Function &Func, Module *Modu,
+ BranchProbabilityInfo *_BPI = nullptr,
+ BlockFrequencyInfo *_BFI = nullptr)
----------------
Switch to using a trailing underscore. Names starting with underscore followed by a capital letter are reserved for the implementation (it is undefined behavior to name a variable like that).
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:295
@@ +294,3 @@
+ for (unsigned i = 0, j = 0, E = Mst.AllEdges.size(); i < E; i++) {
+ const std::unique_ptr<PGOInstrumentationEdge> &Ei = Mst.AllEdges[i];
+ BasicBlock *InstrBB = getInstrBB(Ei);
----------------
Avoid `std::unique_ptr<T> &`. For a simple non-owning handle, use a `T *`. If you don't need to store the handle and the value is guaranteed non-null, consider using `T &`.
Same applies in many other parts of this patch where `std::unique_ptr<T> &` is used.
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:337
@@ +336,3 @@
+// This class stores the auxiliary information for each BB.
+struct PGOInstrumentationUseBBInfo : public PGOInstrumentationBBInfo {
+ uint64_t CountValue;
----------------
This class is local to this file, right? No need to use such a large name; makes it sound like this is for an external interface or something.
(same comment applies to many other things in this file; I think that the module passes themselves are the only things that really need "PGOInstrumentation" prefix on their name)
Look at e.g. lib/Transforms/SROA.cpp with e.g. names like `Slice` or `AllocaSlices`.
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:340
@@ +339,3 @@
+ bool CountValid;
+ uint32_t UnknownCountInEdge;
+ uint32_t UnknownCountOutEdge;
----------------
I don't think wrapping behavior is intended, so avoid unsigned types.
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:368
@@ +367,3 @@
+static uint64_t
+sumEdgeCount(const SmallVector<PGOInstrumentationUseEdge *, 2> Edges) {
+ uint64_t Total = 0;
----------------
Nit: ArrayRef
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:440
@@ +439,3 @@
+ BasicBlock *InstrBB = getInstrBB(Ei);
+ if (!InstrBB) {
+ continue;
----------------
Nit: no braces.
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:505
@@ +504,3 @@
+ return false;
+ } else
+ NumOfPGOFunc++;
----------------
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:517
@@ +516,3 @@
+
+ getBBInfo(reinterpret_cast<BasicBlock *>(NULL))->UnknownCountOutEdge = 2;
+ getBBInfo(reinterpret_cast<BasicBlock *>(NULL))->UnknownCountInEdge = 2;
----------------
nullptr.
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:524
@@ +523,3 @@
+ // This IR instrumentation handles it differently from Clang-based
+ // instrumentation.
+ ProgramMaxCount = PGOReader->getMaximumFunctionCount();
----------------
In what way? And why does it matter to know that here?
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:563
@@ +562,3 @@
+ for (auto &BB : F)
+ It = BBInReverseOrder.insert(It, &BB);
+
----------------
Just use `reverse` adapter from `llvm/ADT/STLExtras.h`
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:627
@@ +626,3 @@
+ unsigned Size = BBCountIt->OutEdges.size();
+ SmallVector<unsigned, 2> EdgeCounts;
+ EdgeCounts.resize(Size);
----------------
Just declare this as `SmallVector<unsigned, 2> EdgeCounts(Size, 0);`
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:631
@@ +630,3 @@
+ EdgeCounts[s] = 0;
+ uint64_t MaxCount;
+ for (unsigned s = 0; s < Size; s++) {
----------------
Is this ever initialized?
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:632
@@ +631,3 @@
+ uint64_t MaxCount;
+ for (unsigned s = 0; s < Size; s++) {
+ const PGOInstrumentationUseEdge *E = BBCountIt->OutEdges[s];
----------------
Can you use a range-for?
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:638
@@ +637,3 @@
+ continue;
+ unsigned SuccNum = GetSuccessorNumber(const_cast<BasicBlock *>(SrcBB),
+ const_cast<BasicBlock *>(DestBB));
----------------
Can you fix GetSuccessorNumber to take a `const BasicBlock *`? (hopefully it is a pretty obvious fix and there is no need for pre-commit review).
Then you can get rid of all these const_cast's.
================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:665
@@ +664,3 @@
+ &(getAnalysis<BlockFrequencyInfoWrapperPass>(F).getBFI());
+ PGOIRInstrumentationGenFunc Func(F, &M, BPI, BFI);
+ }
----------------
This is extremely awkward (declaring a local variable as a way to do a stateful mutating operation). Just use a free function. Same thing for the similar line with `PGOIRInstrumentationUseFunc` below.
http://reviews.llvm.org/D12781
More information about the llvm-commits
mailing list