[PATCH] D12781: PGO IR-level instrumentation infrastructure

Wed Nov 25 15:17:11 PST 2015

silvas added a comment.

I took another pass through, so some of the comments are nits. But most of the comments on the tests in particular are quite significant and not just "nits".


================
Comment at: lib/Transforms/IPO/LLVMBuild.txt:23
@@ -22,2 +22,2 @@
 library_name = ipo
-required_libraries = Analysis Core InstCombine IRReader Linker Object ProfileData Scalar Support TransformUtils Vectorize
+required_libraries = Analysis Core InstCombine IRReader Linker Object ProfileData Scalar Support TransformUtils Vectorize Instrumentation
----------------
Why does IPO now depend on Instrumentation, but didn't previously?
What is different about the PGO passes vs the other instrumentation passes?

================
Comment at: lib/Transforms/Instrumentation/CFGMST.h:128
@@ +127,3 @@
+        AllEdges.begin(), AllEdges.end(),
+        [](const std::unique_ptr<Edge> &lhs, const std::unique_ptr<Edge> &rhs) {
+          return lhs->Weight > rhs->Weight;
----------------
Naming convention.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1
@@ +1,2 @@
+//===- PGOInstru.cpp - PGO Instrumentation --------===//
+//
----------------
The length of this line does not match the one below.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:11
@@ +10,3 @@
+// This file implements PGO instrumentation using a minimum spanning tree based
+// on the  following paper.
+//   [1] Donald E. Knuth, Francis R. Stevenson. Optimal measurement of points
----------------
typo: two spaces after 'the' should be just one space.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:31
@@ +30,3 @@
+// annotates the branch weight.
+// These two passes are mutually exclusive, and they are called at the same
+// compilation point (so they see the same IR). For PGOInstrumentationGen,
----------------
Why are they mutually exclusive? Weren't you wanting to the the count profile in MST computation eventually?

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:34
@@ +33,3 @@
+// the real work is done instrumentOneFunc(). For PGOInstrumentationUse, the
+// real work in done in class PGOUseFunc and the profile is opened in module
+// level and passed to each PGOUseFunc instance.
----------------
typo: "is done"

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:41
@@ +40,3 @@
+// BBInfo contains auxiliary information for a BB. These two classes are used
+// in PGOGenFunc. Class PGOUseEdge and UseBBInfo are the derived class of
+// PGOEdge and BBInfo, respectively. They contains extra data structure used
----------------
class `PGOGenFunc` is no longer present, please update the comment.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:45
@@ +44,3 @@
+// The MST implementation is in Class CFGMST.
+//
+//===----------------------------------------------------------------------===//
----------------
These header comments easily go out of date during patch review. I would recommend reading the comment from top to bottom and verifying that it is up to date.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:86
@@ +85,3 @@
+static cl::opt<std::string>
+    PGOProfileFile("pgo-profile-file", cl::init(""), cl::Hidden,
+                   cl::value_desc("filename"),
----------------
This option is only for testing, so please rename it to make that clear (also update the description).

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:280
@@ +279,3 @@
+
+  BasicBlock *SrcBB = const_cast<BasicBlock *>(E->SrcBB);
+  BasicBlock *DestBB = const_cast<BasicBlock *>(E->DestBB);
----------------
Do you still need these const_cast<>'s after r253733?

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:493
@@ +492,3 @@
+
+// Set the count value for the unknown edges. There should be one and only one
+// unknown edge in Edges vector.
----------------
You say "edges" (plural) here but then say "There should be one and only one".

================
Comment at: test/Transforms/PGOProfile/branch1_gen.ll:7
@@ +6,3 @@
+
+define i32 @_Z9test_br_1i(i32 %i) {
+entry:
----------------
This would be more readable with a non-mangled name. (same for the other tests)

================
Comment at: test/Transforms/PGOProfile/branch2_use.ll:10
@@ +9,3 @@
+  br i1 %cmp, label %if.then, label %if.else
+; CHECK: !prof ![[bw0:[0-9]+]]
+
----------------
Verify that it is attached to the instruction you expect (CHECK-SAME may be useful).

(same for the other "use" tests)

================
Comment at: test/Transforms/PGOProfile/criticaledge_gen.ll:53
@@ +52,3 @@
+sw.default:
+; CHECK: call void @llvm.instrprof.increment(i8* getelementptr inbounds ([23 x i8], [23 x i8]* @__llvm_profile_name__Z17test_criticalEdgeii, i32 0, i32 0), i64 82323253069, i32 8, i32 7)
+  %call6 = call i32 @_ZL3bari(i32 32)
----------------
Will this test fail if the call ends up in the previous or next BB?

I would recommend, for each BB, having a CHECK line verifying the BB name, followed by either CHECK or CHECK-NOT verifying the presence or absence of the increment call.

================
Comment at: test/Transforms/PGOProfile/criticaledge_use.ll:10
@@ +9,3 @@
+    i32 1, label %sw.bb
+    i32 2, label %sw.bb1
+    i32 3, label %sw.bb3
----------------
Please have the names consistent with the switch values.

================
Comment at: test/Transforms/PGOProfile/criticaledge_use.ll:25
@@ +24,3 @@
+sw.bb:
+  %call = call i32 @_ZL3bari(i32 2)
+  br label %sw.epilog
----------------
This test case can be simplified further. At the very least, the function `bar` is not needed.

================
Comment at: test/Transforms/PGOProfile/criticaledge_use.ll:49
@@ +48,3 @@
+  br i1 %cmp7, label %if.then8, label %if.end9
+; CHECK: !prof ![[bw2:[0-9]+]]
+
----------------
The convention for FileCheck variable captures is all upper case (e.g. `[[BW2:[0-9]+]]`).
Also, can you use more semantic names instead of numbers? E.g. `BW_<name>` instead of `BW<number>`.

================
Comment at: test/Transforms/PGOProfile/loop2_use.ll:5
@@ +4,3 @@
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @_Z13test_do_whilei(i32 %n) {
----------------
What part of the code is this test trying to cover that is not covered by loop3 or loop1? Do we need this test?

================
Comment at: test/Transforms/PGOProfile/switch_gen.ll:6
@@ +5,3 @@
+; CHECK: @__llvm_profile_name__Z13test_switch_1i = private constant [18 x i8] c"_Z13test_switch_1i"
+
+define i32 @_Z13test_switch_1i(i32 %i) {
----------------
You can merge the _gen and _use files by using FileCheck's option --check-prefix (e.g. --check-prefix=GEN or --check-prefix=USE).

Actually, doing this is necessary so that there is a single point of truth for the IR used by both.

================
Comment at: test/Transforms/PGOProfile/switch_use.ll:16
@@ +15,3 @@
+sw.bb:
+  %add = add nsw i32 %i, 2
+  br label %sw.epilog
----------------
These `add` instructions are not needed. Just make the function return void.


http://reviews.llvm.org/D12781



