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

[Wenlei He via Phabricator via llvm-commits]

wenlei 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) {
----------------
hoy wrote:
> 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.
Here you're doing two copies, Map[A] -> temp -> Map[B]. With a `std::map<StringRef, StringRef>` for name mapping in top-down order, we should be able to do in-place copy Map[A] -> Map[B] by iterating over the name map?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104267