[PATCH] D96932: [SampleFDO] Support enabling -funique-internal-linkage-name

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:852
+        // -funique-internal-linkage-names and contains ".__uniq." suffix.
+        CanonName = getCanonicalFnName(CanonName, "selected");
+      }
----------------
wenlei wrote:
> If this is ok for inlinee, is it ok for standalone functions as well? I think this SymbolMap lookup has lead to extra function argument in quite a few places, so wondering how critical is that given that it still doesn't cover the inlined cases.
> 
> IIUC, with SecFlagUniqSuffix plus using "selected" policy and the fix for canonicalization upon symbolMap insertion, this patch already supported migration to turn on -funique-internal-linkage-name without regression, if c++/go mix isn't a concern, which sounds like a separate issue for the future. And if we want to cover c++/go mix, perhaps something that handles both inline and non-inline cases would be needed?
> 
Right, currently if we simply use "selected" strategy for all here, it is of no problem in terms of supporting -funique-internal-linkage-names. Using SymbolMap here is a  best effort but like you said we need to add extra function argument in a few places. Using "selected" strategy by default here and leaving the change of using SymbolMap till the point when we really need it sounds good with me. 

Fundamentally I think compiler should keep declaration when a function is all inlined. The inlined and deleted function could have its name referred in debug information, and it is possible their function attributes will be needed for multiple usages.     


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96932/new/

https://reviews.llvm.org/D96932