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

Tue Feb 23 22:41:36 PST 2021

wmi added a comment.

In D96932#2583927 <https://reviews.llvm.org/D96932#2583927>, @hoy wrote:

> Are the test failures related?

Thanks for reminding me. I don't have the test failure locally. The error says "Unrecognized sample profile encoding format". I guess it is because those profiles are binaries and the diff  cannot be applied on them properly.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96932