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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:638
+                        SampleProfileReaderItaniumRemapper *Remapper,
+                        const StringMap<Function *> *SymbolMap = nullptr) const;
 
----------------
wmi wrote:
> hoy wrote:
> > wmi wrote:
> > > hoy wrote:
> > > > How about making this a static field of `FunctionSamples`?
> > > That will look clearer, but I am not sure whether it will introduce data race problem. Look at the old problem Wenlei fixed: https://reviews.llvm.org/rG4b99b58a847c8424d9992a1e92a9f8ae7e4d7b51. The existing static members are of simple types and are all initialized with the same value for all the modules so they may be fine. SymbolMap read/write could be prone to such problem. 
> > I actually meant to make the SymbolMap pointer as a static field. Does that sound safe?
> Each module will have its own SymbolMap so the thread for one module may see other module's SymbolMap pointer. 
> 
> I think the other static vars in FunctionSamples can get away with the problem only because they are all initialized to the same value for different modules using the same profile.
Oh yeah,  SymbolMap is per module. Threading will be a problem.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96932