[PATCH] D22600: [PGO] Fix profile mismatch in Comdat function with pre-inliner
David Li via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 21 13:47:14 PDT 2016
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
More information about the llvm-commits
mailing list