[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:632
+  /// If \p SymbolMap is not nullptr, findFunctionSamplesAt will do
+  /// canonication for \p CalleeName. If \p SymbolMap is not provided, user
+  /// has to make sure \p CalleeName has already been canonicalized or has no
----------------
hoy wrote:
> Nit: canonicalization? 
Thanks, fixed.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:638
+                        SampleProfileReaderItaniumRemapper *Remapper,
+                        const StringMap<Function *> *SymbolMap = nullptr) const;
 
----------------
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. 


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


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1747
   }
+  assert(SymbolMap.count("") == 0 &&
+         "No empty StringRef should be added in SymbolMap");
----------------
hoy wrote:
> Nit: "" -> StringRef() ?
Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96932