[PATCH] D28416: [PGO] Turn off comdat renaming in IR PGO by default

[Rong Xu via Phabricator via llvm-commits]

xur marked an inline comment as done.
xur added inline comments.


================
Comment at: lib/ProfileData/InstrProf.cpp:839
+    return false;
+  if (!needsComdatForCounter(F, *(F.getParent())))
+    return false;
----------------
davidxl wrote:
> Given the name of the function, it should probably check address taken bit as well.
We can do that, but it won't completely solve the issue as the address taken can happen in other TU.


================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:286
+  uint64_t FuncHash = Inc->getHash()->getZExtValue();
+  // No hash: don't appent it to the profile variable.
+  if (FuncHash == 0)
----------------
davidxl wrote:
> davidxl wrote:
> > appent --> append
> why there is on hash? or it is hash of a trivial function?
this can be called directly from intrinsic in a .ll file (happen some test cases), but since we now check the IRPGO bit, we can remove this.


================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:410
     std::unordered_multimap<Comdat *, GlobalValue *> &ComdatMembers) {
-  if (F.getName().empty())
+  if (!canRenameComdatFunc(F))
     return false;
----------------
davidxl wrote:
> It is clearer to check DoComdatRenaming here as well.
I can add the check, but this essentially gonna be redundant: we only collect ComdatMembers under DoComdatRenaming. And we only call canRenameComdat when ComdatMembers is not empty.
So when DoComdatRenming is false, we will not call canRenameComdat.


https://reviews.llvm.org/D28416