[PATCH] D147456: [AutoFDO] Stale profile matching(part 1)

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

Thanks for working on this. Looks good overall, except a few naming nits.



================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:845
+  // same location.
+  const LineLocation &remapLoc(const LineLocation &Loc) const {
+    if (!StaleProfileMappings)
----------------
similarly, `mapIRLocToProfileLoc`?


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1264
+  /// be remapped to 1 and find the location of bar in the profile.
+  LocToLocMap *StaleProfileMappings = nullptr;
 };
----------------
How about name it `IRToProfileLocationMap` to make it more explicit. 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:134
+static cl::opt<bool> SampleProfileMatching(
+    "sample-profile-matching", cl::Hidden, cl::init(false),
+    cl::desc("Run stale profile matching algorithm and use the remapped "
----------------
nit: we always match profile regardless of staleness. how about something like `salvage-stale-profile` (or `sample-profile-salvage-stale`)


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:451
+  // of current build is matched to the location in the profile.
+  StringMap<LocToLocMap> FuncToMatchingsMap;
 
----------------
hoy wrote:
> Where is this DS populated? Is it in a separate patch?
same question. I saw it being populated in the 2nd patch, but the split between the two patches is a bit weird if its population is left out. not a big deal though as long as the entire stack works..


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2100
 
-void SampleProfileMatcher::detectProfileMismatch(const Function &F,
-                                                 const FunctionSamples &FS) {
+void SampleProfileMatcher::detectProfileMismatch(
+    const FunctionSamples &FS,
----------------
With the refactoring, this function is now narrower, and perhaps we could rename it to something like `countProfileMismatches` to reflect the narrowed scope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147456