[PATCH] D104267: [CSSPGO] Fix an invalid hash table reference issue in the CS preinliner.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:383
   StringSet<> ProfilesToBeRemoved;
-  // Note that StringMap order is guaranteed to be top-down order,
-  // this makes sure we make room for promoted/merged context in the
-  // map, before we move profiles in the map.
+  StringMap<FunctionSamples> ProfilesToBeAdded;
   for (auto &I : ProfileMap) {
----------------
wenlei wrote:
> Good catch - not sure how I come to incorrectly rely on the order of StringMap..
> 
> However the intention was to avoid extra copies of FunctionSamples as much as possible - these can large objects for copying. 
> 
> I think what we could do is to collect a `std::map<StringRef, StringRef>` (old context to new context) for how we should update the `StringMap`, then iterate over the (top-down) ordered `std::map` to move profile to be keyed by correct context. This way we can avoid one extra copy for each FunctionSamples that needs to be moved.
IIUC, we are trying to update the key of a promoted profile in `ProfileMap`. Since we cannot do that in place, we basically remove the current entry and add a new entry which just differs in the context key. If that's the case, not sure how to avoid copying a profile since the value field of an entry is not reference.


================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:394
+    auto Ret = ProfilesToBeAdded.try_emplace(ContextStr, FProfile);
+    (void)Ret;
     assert(Ret.second && "Conext conflict during canonicalization");
----------------
wlei wrote:
> Do we need this?
It's to make sure `Ret` is used in the release build where the following `assert` turns out nothing. Otherwise the ninja complaints `Ret` is defined by not used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104267