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

Tue Feb 23 23:21:30 PST 2021

hoy accepted this revision.
hoy added a comment.
This revision is now accepted and ready to land.

BTW, in case of duplicated functions due to multi-versioning, the profile generator may not be able to merge them as expected without properly canonicalizing function names, the compiler might be able to achieve that though. As of now, it seems that the last profile for a given canonical name always supersedes all others in the compiler reader. This may need a change for a "merge" instead.

================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:741
   void findInlinedFunctions(DenseSet<GlobalValue::GUID> &S, const Module *M,
+                            const StringMap<Function *> &SymbolMap,
                             uint64_t Threshold) const {
----------------
wmi wrote:
> hoy wrote:
> > wmi wrote:
> > > hoy wrote:
> > > > wmi wrote:
> > > > > hoy wrote:
> > > > > > Is `SymbolMap` always available to this function?  Looks like it could be null for other functions like `findFunctionSamplesAt`.
> > > > > Yes, the function is used to collect function GUID with profile but not present in current module for ThinLTO importing. That function is only used in SampleLoaderPass so SymbolMap is always available. findFunctionSamplesAt has been used more widely than just SampleLoaderPass so SymbolMap is not always available. 
> > > > As far as I understand, `SymbolMap` is always available in compiler but not tools. Does that mean tools should never do canonicalization?  Sorry, forgot to ask this earlier.
> > > Right, like we discussed here: https://reviews.llvm.org/D96932#2580530, I agree it is better to let compiler do all the canonicalization work. 
> > Gotcha. We have a couple places in llvm-profgen that call getCanonicalFnName which may not work well with unique linkage names. Do you think they should be removed?
> Yes, they should be removed. Do you think whether I should wait to commit it after llvm-profgen is changed accordingly? Is it possible to cause llvm-profgen problem or it may be fine as long as -funique-internal-linkage-names hasn't been enabled by default?
It's fine to have this in first. We already identified the unique linkage name problem with llvm-profgen and started removing the `getCanonicalFnName` calls. cc @wlei 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96932