[PATCH] D12781: PGO IR-level instrumentation infrastructure

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/ProfileData/InstrProfWriter.cpp:86-94
@@ -84,4 +85,11 @@
     // 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)
+        MaxFunctionCount = Counters[0];
+    } else {
+      for (size_t I = 0, E = Counters.size(); I < E; ++I) {
+        if (Counters[I] > MaxFunctionCount)
+          MaxFunctionCount = Counters[I];
+      }
+    }
     return instrprof_error::success;
----------------
Please add a comment.

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:117
@@ +116,3 @@
+  // (when Removed == true).
+  std::vector<Edge *> AllEdges;
+
----------------
make it std::vector<std::unique_ptr<Edge>>

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:150
@@ +149,3 @@
+  };
+  DenseMap<const BasicBlock *, BBInfo *> BBInfos;
+
----------------
make it DenseMap<const BasicBlock *, std::unique_ptr<BBInfo>> BBInfos;

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:272
@@ +271,3 @@
+  // Free Edge in AllEdges.
+  for (auto &EI : AllEdges)
+    delete EI;
----------------
No need for explicit delete with unique_ptr

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:336
@@ +335,3 @@
+
+  do {
+    std::pair<const BasicBlock *, succ_const_iterator> &Top = VisitStack.back();
----------------
You can use the ReversePostOrderTraversal Class defined in ADT/PostOrderIterator.h. Actually does the order of traverse matter here?

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:349
@@ +348,3 @@
+                                            const_cast<BasicBlock *>(BB));
+      if (SuccNum >= 2)
+        Weight = 3;
----------------
What is the rationale of this heuristic?

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:351
@@ +350,3 @@
+        Weight = 3;
+      if (SuccNum == 0)
+        Weight = 0;
----------------
Why not compute BlockFrequencyInfo and use the real static Edge frequency (from BB freq and edge prob)?  For Critical edge, the weight can be increased to infinite.

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:493
@@ +492,3 @@
+// the potential name conflict.
+void PGOLateInstrumentationFunc::setFuncName(
+    StringRef Name, GlobalValue::LinkageTypes Linkage) {
----------------
As the name linkage function, the name globalization function also needs to be a common interface which FE instrumentation can share in the future.

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:516
@@ +515,3 @@
+
+void PGOLateInstrumentationFunc::createFuncNameVar(
+    GlobalValue::LinkageTypes Linkage) {
----------------
This code is shared with FE based instrumentation and it should be refactored as a utility function. Suggested location: include/llvm/Transformation/Instrumentation.h.

Converting FE to use the common interface can be done as a follow-up patch

================
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:666
@@ +665,3 @@
+  // Read the counter array from file.
+  std::unique_ptr<IndexedInstrProfReader> PGOReader;
+  auto ReaderOrErr = IndexedInstrProfReader::create(ProfileFileName);
----------------
Make reader a shared object at module level.


http://reviews.llvm.org/D12781