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

[Hongtao Yu via Phabricator via llvm-commits]

hoy created this revision.
Herald added subscribers: wenlei, hiraditya, eraman.
hoy requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

We were using a `StringMap` object to store all profiles to be emitted. The object is basically an unordered hash table, therefore updating it in the process of trasvering it may cause issue since the underlying bucket array could change.

I'm also moving the `csspgo-preinliner` switch around so that no context tri will be constructed (by the constructor of `CSPreInliner`) when the switch is off.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104267

Files:
  llvm/lib/ProfileData/SampleProf.cpp
  llvm/tools/llvm-profgen/CSPreInliner.cpp
  llvm/tools/llvm-profgen/ProfileGenerator.cpp


Index: llvm/tools/llvm-profgen/ProfileGenerator.cpp
===================================================================
--- llvm/tools/llvm-profgen/ProfileGenerator.cpp
+++ llvm/tools/llvm-profgen/ProfileGenerator.cpp
@@ -48,6 +48,11 @@
     cl::desc("Keep the last K frames while merging cold profile. 1 means the "
              "context-less base profile"));
 
+static cl::opt<bool> EnableCSPreInliner(
+    "csspgo-preinliner", cl::Hidden, cl::init(false),
+    cl::desc("Run a global pre-inliner to merge context profile based on "
+             "estimated global top-down inline decisions"));
+
 extern cl::opt<int> ProfileSummaryCutoffCold;
 
 using namespace llvm;
@@ -401,7 +406,8 @@
 
   // Run global pre-inliner to adjust/merge context profile based on estimated
   // inline decisions.
-  CSPreInliner(ProfileMap, HotCountThreshold, ColdCountThreshold).run();
+  if (EnableCSPreInliner)
+    CSPreInliner(ProfileMap, HotCountThreshold, ColdCountThreshold).run();
 
   // Trim and merge cold context profile using cold threshold above;
   SampleContextTrimmer(ProfileMap)
Index: llvm/tools/llvm-profgen/CSPreInliner.cpp
===================================================================
--- llvm/tools/llvm-profgen/CSPreInliner.cpp
+++ llvm/tools/llvm-profgen/CSPreInliner.cpp
@@ -16,11 +16,6 @@
 using namespace llvm;
 using namespace sampleprof;
 
-static cl::opt<bool> EnableCSPreInliner(
-    "csspgo-preinliner", cl::Hidden, cl::init(false),
-    cl::desc("Run a global pre-inliner to merge context profile based on "
-             "estimated global top-down inline decisions"));
-
 // The switches specify inline thresholds used in SampleProfileLoader inlining.
 // TODO: the actual threshold to be tuned here because the size here is based
 // on machine code not LLVM IR.
@@ -179,9 +174,6 @@
 }
 
 void CSPreInliner::run() {
-  if (!EnableCSPreInliner)
-    return;
-
 #ifndef NDEBUG
   auto printProfileNames = [](StringMap<FunctionSamples> &Profiles,
                               bool IsInput) {
Index: llvm/lib/ProfileData/SampleProf.cpp
===================================================================
--- llvm/lib/ProfileData/SampleProf.cpp
+++ llvm/lib/ProfileData/SampleProf.cpp
@@ -380,9 +380,7 @@
 
 void SampleContextTrimmer::canonicalizeContextProfiles() {
   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) {
     FunctionSamples &FProfile = I.second;
     StringRef ContextStr = FProfile.getNameWithContext();
@@ -392,18 +390,23 @@
     // Use the context string from FunctionSamples to update the keys of
     // ProfileMap. They can get out of sync after context profile promotion
     // through pre-inliner.
-    auto Ret = ProfileMap.try_emplace(ContextStr, FProfile);
+    auto Ret = ProfilesToBeAdded.try_emplace(ContextStr, FProfile);
+    (void)Ret;
     assert(Ret.second && "Conext conflict during canonicalization");
-    FProfile = Ret.first->second;
 
     // Track the context profile to remove
-    ProfilesToBeRemoved.erase(ContextStr);
+    assert(!ProfilesToBeRemoved.count(ContextStr) &&
+           "Conext supposed to be removed");
     ProfilesToBeRemoved.insert(I.first());
   }
 
   for (auto &I : ProfilesToBeRemoved) {
     ProfileMap.erase(I.first());
   }
+
+  for (auto &I : ProfilesToBeAdded) {
+    ProfileMap.try_emplace(I.first(), I.second);
+  }
 }
 
 std::error_code ProfileSymbolList::write(raw_ostream &OS) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D104267.352001.patch
Type: text/x-patch
Size: 3635 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210614/0e16f50e/attachment.bin>