[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:
> hoy wrote:
> > wenlei wrote:
> > > 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?
> > `A` and `B` are context strings, right? Why does the top-down order matter, say `A` is `main->foo->bar` and `B` is `bar`?
> > 
> > Yeah, we are using a intermediate temp. Should change `ProfilesToBeAdded` to `StringMap<FunctionSamples&>` and do the addition before the deletion.  
> This is my thought - two things blocks in-place copy: 1) iterating over StringMap while updating, which is solved by iterating over name mapping `std::map<StringRef, StringRef>`; 2) not overwriting items yet to be updated, this can be solved by ordering the updates so we make room for destination key of an update before we actually update.
Still trying to follow. Say `ProfileMap` only has one entry `{"main->foo->bar", Profile}`. The context in `Profile` is "bar" and we are trying to update the key to "bar".  With an extra `std::map<StringRef, StringRef>` , we would make something like `{"main->foo->bar", "bar"}`. Then how do we proceed next? Still figuring out how to avoid copying `Profile`.


================
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:
> hoy wrote:
> > 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.
> I see, how about we use `find` instead of `try_emplace`, like `assert(ProfilesToBeAdded.find(ContextStr) == ProfilesToBeAdded.end() && "Context conflict during canonicalization");`
That looks better but may slow down the assert compiler. Actually the `(void)` usage is quite common in the code base. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104267