[PATCH] D12781: PGO IR-level instrumentation infrastructure

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: include/llvm/Support/CFGMST.h:78
@@ +77,3 @@
+  // Give BB, return the auxiliary information.
+  const std::unique_ptr<BBInfo> &getBBInfo(const BasicBlock *BB) const {
+    auto It = BBInfos.find(BB);
----------------
Return BBInfo& instead of unique_ptr<BBInfo>&. It is more efficient for uses later .

================
Comment at: include/llvm/Support/CFGMST.h:81
@@ +80,3 @@
+    assert(It != BBInfos.end());
+    return It->second;
+  }
----------------
assert It->second.get() != null.

Return *It->second.get().

================
Comment at: include/llvm/Support/CFGMST.h:94
@@ +93,3 @@
+
+    // Special handling for single BB functions.
+    if (succ_empty(BB)) {
----------------
Add more comments explaining here why need two duplicate fake edges.

================
Comment at: include/llvm/Support/CFGMST.h:104
@@ +103,3 @@
+      uint64_t Weight;
+      if (int successors = TI->getNumSuccessors()) {
+        for (int i = 0; i != successors; ++i) {
----------------
Redundant guard.

================
Comment at: include/llvm/Support/CFGMST.h:105
@@ +104,3 @@
+      if (int successors = TI->getNumSuccessors()) {
+        for (int i = 0; i != successors; ++i) {
+          bool Critical = false;
----------------
Make successors and i 'unsigned' type.

================
Comment at: include/llvm/Support/CFGMST.h:110
@@ +109,3 @@
+            Weight =
+                BPI->getEdgeProbability(BB, TargetBB).scale(1000 * BBWeight);
+          else
----------------
Use a macro for 1000? CRITICAL_EDGE_MULTIPLIER? Also handling overflow situation?

================
Comment at: include/llvm/Support/CFGMST.h:113
@@ +112,3 @@
+            Weight = BPI->getEdgeProbability(BB, TargetBB).scale(BBWeight);
+          // Weight = BlockFrequency::getMaxFrequency();
+          addEdge(BB, TargetBB, Weight)->IsCritical = Critical;
----------------
Stale code.

================
Comment at: include/llvm/Support/CFGMST.h:127
@@ +126,3 @@
+  // Sort CFG edges based on its weight.
+  void sortEdges() {
+    std::sort(AllEdges.begin(), AllEdges.end(), CompareEdgeWeight());
----------------
--> sortEdgesByWeight

================
Comment at: include/llvm/Support/CFGMST.h:146
@@ +145,3 @@
+          if (unionGroups(Ei->SrcBB, Ei->DestBB))
+            Ei->InMST = true;
+        }
----------------
Should this be marked unconditionally here? Otherwise, we can simply boost the weight for such edges even higher without special treatment here?

================
Comment at: include/llvm/Support/CFGMST.h:184
@@ +183,3 @@
+  // Add an edge to AllEdges with weight W.
+  const std::unique_ptr<Edge> &addEdge(const BasicBlock *Src,
+                                       const BasicBlock *Dest, unsigned W = 2) {
----------------
Make Edge* the return type for efficiency.

================
Comment at: include/llvm/Support/CFGMST.h:185
@@ +184,3 @@
+  const std::unique_ptr<Edge> &addEdge(const BasicBlock *Src,
+                                       const BasicBlock *Dest, unsigned W = 2) {
+    if (BBInfos.find(Src) == BBInfos.end()) {
----------------
Use a const var for default weight.

================
Comment at: include/llvm/Support/CFGMST.h:186
@@ +185,3 @@
+                                       const BasicBlock *Dest, unsigned W = 2) {
+    if (BBInfos.find(Src) == BBInfos.end()) {
+      unsigned Index = BBInfos.size();
----------------
Combine the find + insert pattern into one insert (with pair of source BB and null ptr). If the map is updated, a iterator is returned that can be updated with unique_ptr reset.

================
Comment at: include/llvm/Support/CFGMST.h:191
@@ +190,3 @@
+    }
+    if (BBInfos.find(Dest) == BBInfos.end()) {
+      unsigned Index = BBInfos.size();
----------------
Same here.

================
Comment at: include/llvm/Support/CFGMST.h:197
@@ +196,3 @@
+    AllEdges.emplace_back(new Edge(Src, Dest, W));
+    return AllEdges[AllEdges.size() - 1];
+  }
----------------
return AllEdges.back().get();

================
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:
----------------
silvas wrote:
> Why "IR"? Are you planning to generalize this to Machine? For now just leave off "IR" (here and elsewhere).
There was an earlier suggestion to change 'Late' in  PGOLateInstrumentation to 'IR'.  To add more to the bikeshed, I suggest change the class name to

class FuncPGOInstrumentation {
};

================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:181
@@ +180,3 @@
+  // The Minimum Spanning Tree of function CFG.
+  CFGMST<Edge, BBInfo> Mst;
+
----------------
Mst --> MST or MinSpanTree.

================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:185
@@ +184,3 @@
+  // Return NULL if no BB to be instrumented.
+  BasicBlock *getInstrBB(const std::unique_ptr<Edge> &E);
+
----------------
BasicBlock *getInstrBB(const Edge &E);


http://reviews.llvm.org/D12781