[PATCH] D12781: PGO IR-level instrumentation infrastructure

[Rong Xu via llvm-commits]

xur marked 8 inline comments as done.

================
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)
----------------
davidxl wrote:
> 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.
This is for the FE instrumentation profiles. For late instrumentation, we track the maximum instrumented edge. It will be used in profile-use.

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:130
@@ +129,3 @@
+  struct BBGroupInfo {
+    const BasicBlock *Group;
+    unsigned Index;
----------------
davidxl wrote:
> 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.
For this example, BB3Info points to BB2 in the unionGroups step. This is ok, as in the findandComporessGroup (this is where we get the group info), there is loop to traverse the group info (in this case BB2's groupinfo) to find the root. When we compress the path, all the groupinfo in the path will be set to root.

So there is no correctness issues.

But I agree with you that using member Group pointing to BBGroupInfo rather BasicBlock is more efficient -- it avoids one map lookup.

I changed the this part of the code.

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:182
@@ +181,3 @@
+  // It stores the profile count for each edge.
+  struct EdgeCount {
+    uint64_t CountValue;
----------------
davidxl wrote:
> Can this be merged into the Edge class?
Yes. This can be done.
The reason for the split is that this EdgeCount only used in profile-use.
It's the same as BBCount used in profile-use.
I'll merge EdgeCount to Edge to avoid the map.

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


http://reviews.llvm.org/D12781