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

[Hongtao Yu via Phabricator via llvm-commits]

hoy updated this revision to Diff 352529.
hoy added a comment.

Addressing Wenlei and Lei's comments.


Repository:
  rG LLVM Github Monorepo

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

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
@@ -371,9 +371,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();
@@ -383,15 +381,20 @@
     // 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);
-    assert(Ret.second && "Conext conflict during canonicalization");
-    FProfile = Ret.first->second;
+    auto Ret = ProfilesToBeAdded.try_emplace(ContextStr, FProfile);
+    (void)Ret;
+    assert(Ret.second && "Context conflict during canonicalization");
 
     // Track the context profile to remove
-    ProfilesToBeRemoved.erase(ContextStr);
+    assert(!ProfilesToBeRemoved.count(ContextStr) &&
+           "Context supposed to be removed");
     ProfilesToBeRemoved.insert(I.first());
   }
 
+  for (auto &I : ProfilesToBeAdded) {
+    ProfileMap.try_emplace(I.first(), I.second);
+  }
+
   for (auto &I : ProfilesToBeRemoved) {
     ProfileMap.erase(I.first());
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D104267.352529.patch
Type: text/x-patch
Size: 3643 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210616/15d0cf32/attachment.bin>