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

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/ProfileData/InstrProf.cpp:839
+    return false;
+  if (!needsComdatForCounter(F, *(F.getParent())))
+    return false;
----------------
xur wrote:
> 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.
Yes -- that is why it is better to add the check here and document 1) why it is not safe to be on by default 2) even when it is on, there is a risk as you mentioned.


================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:410
     std::unordered_multimap<Comdat *, GlobalValue *> &ComdatMembers) {
-  if (F.getName().empty())
+  if (!canRenameComdatFunc(F))
     return false;
----------------
xur wrote:
> 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.
Right -- it is clearer this way.


https://reviews.llvm.org/D28416