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

Tue Feb 23 21:51:56 PST 2021

hoy added a comment.

Are the test failures related?

================
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:
> > > > 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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96932