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

[Wenlei He via Phabricator via llvm-commits]

wenlei accepted this revision.
wenlei added a comment.
This revision is now accepted and ready to land.

In D104267#2826019 <https://reviews.llvm.org/D104267#2826019>, @hoy wrote:

> I compared the performance of the new algorithm and the old implementation with extra copying with a decent large profile. It turned out the old implementation was better in speed. This is due to the fewer string hashing operations on long context string involved in the new implementation. While the new implementation could be better with shorter context string and bigger function profiles, I'm inclined to use the old one since it's easy to read and maintain.

Sounds good, thanks for giving it a try. LGTM with a minor comment.



================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:373
 void SampleContextTrimmer::canonicalizeContextProfiles() {
   StringSet<> ProfilesToBeRemoved;
+  StringMap<FunctionSamples> ProfilesToBeAdded;
----------------
This no longer needs to be a set. I used set because of the erase operation. Now a vector would do.


================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:374
   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) {
----------------
May want to add a short comment explaining why an extra map (ProfilesToBeAdded) is needed, it seem not as trivial based on our discussions..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104267