[PATCH] D12781: PGO IR-level instrumentation infrastructure

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/ProfileData/InstrProfWriter.cpp:86
@@ -84,3 +85,3 @@
     // We keep track of the max function count as we go for simplicity.
-    if (Counters[0] > MaxFunctionCount)
-      MaxFunctionCount = Counters[0];
+    if (!IsLateInstr) {
+      if (Counters[0] > MaxFunctionCount)
----------------
Counters[0] no longer mean function entry count any more, so I don't see the point of updating it. Unless we want to redefine the meaning of MaxFunctionCount to be maximum instrumented edge count. On the other hand, the later seems to be more useful as a program summary data. Please add some comments here.

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:96
@@ +95,3 @@
+RunPGOLateInstrGen("pgo-late-instr-generate", cl::init(""), cl::Hidden,
+           cl::desc("Enable generate phrase of PGO Late instrumentation"));
+
----------------
Make it clear it specifies the path of the profile data generated.

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:101
@@ +100,3 @@
+           cl::value_desc("filename"),
+           cl::desc("Enable use phrase of PGO Late instrumentation"));
+
----------------
specifies the path of the profile data

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:82
@@ +81,3 @@
+      : F(Func), M(Modu) {
+    FunctionHash = 0;
+    ProgramMaxCount = 0;
----------------
Put into the initializer

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:106
@@ +105,3 @@
+
+private:
+  Function &F;
----------------
declare private members before public ones.

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:130
@@ +129,3 @@
+  struct BBGroupInfo {
+    const BasicBlock *Group;
+    unsigned Index;
----------------
This data structure is not designed well for fast-union find algorithm. I think the Group Should be of type BBGroupInfo * (originally point to itself), not BasicBlock*.   Otherwise after union operation, the rank info of the merged BBGroup will also needs to be updated. Depending on the order of the union operation, the result can also be wrong.

Consider operation:

Union (BB2Info, BB3Info) --> BB2Info   (1)
Union (BB2Info, BB1Info) --> BB1Info   (2)

After (1), BB3Info will point to BB2, but after (2), BB2Info will be redirected to BB1, but BB3Info is not updated.


Also add another field 

BasicBlock *BB; 

to serve as the reverse mapping from BBGroupInfo to BB. This should never change after construction.

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:154
@@ +153,3 @@
+
+  typedef SmallVector<const Edge *, 2> DirectEdges;
+
----------------
Add comment about the type.

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:157
@@ +156,3 @@
+  struct CompareEdgeWeight {
+    bool operator()(const Edge *lhs, const Edge *rhs) {
+      return lhs->Weight > rhs->Weight;
----------------
make it a const method

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:164
@@ +163,3 @@
+  // It stores the profile count for each BB.
+  struct BBCount {
+    uint64_t CountValue;
----------------
Rename it to BBInfo?

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:167
@@ +166,3 @@
+    bool CountValid;
+    unsigned UnknownCountInEdge;
+    unsigned UnknownCountOutEdge;
----------------
unsigned --> uint32_t

These two fields also need more comment.

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:182
@@ +181,3 @@
+  // It stores the profile count for each edge.
+  struct EdgeCount {
+    uint64_t CountValue;
----------------
Can this be merged into the Edge class?

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:188
@@ +187,3 @@
+  };
+  DenseMap<const Edge *, EdgeCount *> EdgeCountInfo;
+
----------------
This map is not needed if count is merged into Edge class.

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:190
@@ +189,3 @@
+
+  BBGroupInfo *findandCompressGroup(const BasicBlock *BB);
+  void unionGroups(BBGroupInfo *BB1, BBGroupInfo *BB2);
----------------
findandCompressGroup -->findAndCompressGroup

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:190
@@ +189,3 @@
+
+  BBGroupInfo *findandCompressGroup(const BasicBlock *BB);
+  void unionGroups(BBGroupInfo *BB1, BBGroupInfo *BB2);
----------------
davidxl wrote:
> findandCompressGroup -->findAndCompressGroup
Missing comment for this method.

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:199
@@ +198,3 @@
+
+  BBGroupInfo *getBBGroupInfo(const BasicBlock *BB) const {
+    auto It = BBGroupInfos.find(BB);
----------------
This method should walk up the union tree and find the root of the union and return it.

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:437
@@ +436,3 @@
+
+void PGOLateInstrumentationFunc::unionGroups(BBGroupInfo *BB1,
+                                             BBGroupInfo *BB2) {
----------------
See comments above about the union structure.

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:439
@@ +438,3 @@
+                                             BBGroupInfo *BB2) {
+  const BasicBlock *BB1G = BB1->Group;
+  const BasicBlock *BB2G = BB2->Group;
----------------
Need to find the root of the union to avoid fast growing of chain length and also to get the correct ranks for the operand unions.

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:450
@@ +449,3 @@
+      BB1->Rank++;
+  }
+}
----------------
Need a union flattening/compression when the rank become too large


http://reviews.llvm.org/D12781