[PATCH] D22600: [PGO] Fix profile mismatch in Comdat function with pre-inliner

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:338
@@ +337,3 @@
+  // For AvailableExternallyLinkage functions.
+  if (!F.hasComdat())
+    return true;
----------------
Perhaps add an assert about the linkage?

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:341
@@ +340,3 @@
+
+  // Only handle those Comdat groups that only containing one function (OK to
+  // have have other variables/aliases).
----------------
Add // FIXME and some explanation of the limitation.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:342
@@ +341,3 @@
+  // Only handle those Comdat groups that only containing one function (OK to
+  // have have other variables/aliases).
+  Comdat *C = F.getComdat();
----------------
have have --> have 

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:344
@@ +343,3 @@
+  Comdat *C = F.getComdat();
+  for (auto &&CM : make_range(ComdatMembers.equal_range(C))) {
+    if (!dyn_cast<Function>(CM.second))
----------------
Also explain why having other variables are ok?

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:365
@@ +364,3 @@
+  Module *M = F.getParent();
+  // For AvailableExternallyLinkage functions, changed to Comdat.
+  if (!F.hasComdat()) {
----------------
--> change the linkage to LinkOnceODR and put them into comdat. This is because after renaming, there is no backup external copy available for the function.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:367
@@ +366,3 @@
+  if (!F.hasComdat()) {
+    NewComdat = M->getOrInsertComdat(StringRef(NewFuncName));
+    F.setLinkage(GlobalValue::LinkOnceODRLinkage);
----------------
Add assertion about availableExternally linkage.

================
Comment at: test/Transforms/PGOProfile/comdat_internal.ll:7
@@ -6,2 +6,3 @@
 $foo = comdat any
+; CHECK: $foo.12884901887 = comdat any
 
----------------
Can you use wild card and defined a variable for the hash val? There is no need to test the actual alue.

================
Comment at: test/Transforms/PGOProfile/indirect_call_profile.ll:16
@@ -15,3 +15,3 @@
 ; GEN: entry:
-; GEN-NEXT: call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 12884901887, i32 1, i32 0)
+; GEN-NEXT: call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 281487861612543, i32 1, i32 0)
   %tmp = load void ()*, void ()** @bar, align 8
----------------
Same here


https://reviews.llvm.org/D22600