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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1061
 
+  void setStaleProfileMappings(LocToLocMap *M) { StaleProfileMappings = M; }
+
----------------
mtrofin wrote:
> drive-by nits:
> - s/M/LTLM or basically anything that's not suggestive of "M == Module", which is seen everywhere else in the codebase
> - can StaleProfileMappings be `const LocToLocMap`? (and same then for the parameter of this setter) - it would simplify the burden on a future reader/maintainer
> - is `StaleProfileMappings` expected to be updated multiple times? if not, could you `assert(StaleProfileMappings == null && "this should be set only once") ` - same maintainability argument.
Thanks for the feedback, all make sense, done.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1247
+
+  /// Location mappings generated by stale profile matching.
+  ///
----------------
hoy wrote:
> nit: "generated by" -> "used for"
This is the output of the matching algorithm, not to be used for the matching:)


================
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;
 };
----------------
wenlei wrote:
> How about name it `IRToProfileLocationMap` to make it more explicit. 
Sounds good, done.


================
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 "
----------------
wenlei wrote:
> nit: we always match profile regardless of staleness. how about something like `salvage-stale-profile` (or `sample-profile-salvage-stale`)
Sounds good.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:451
+  // of current build is matched to the location in the profile.
+  StringMap<LocToLocMap> FuncToMatchingsMap;
 
----------------
davidxl wrote:
> wenlei wrote:
> > 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..
> It is better make that part available to review at the same time for better context.
Ok, sorry for the confusing. I will move it to the second part.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:483
+  void runOnFunction(const Function &F, const FunctionSamples &FS);
+  void setStaleProfileMappings();
+  void setStaleProfileMappings(FunctionSamples &FS);
----------------
mtrofin wrote:
> drive-by nit: could this be called `distributeStaleProfileMappings`?
Sounds good.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:484
+  void setStaleProfileMappings();
+  void setStaleProfileMappings(FunctionSamples &FS);
+  void
----------------
mtrofin wrote:
> do these need to be public? 
Will move to private.


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


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2197
+  // Detect profile mismatch for profile staleness metrics report.
+  detectProfileMismatch(FS, MatchedCallsiteLocs);
 }
----------------
hoy wrote:
> Condition this under `ReportProfileStaleness || PersistProfileStaleness` ?
Done. Note: this is also going to be used for matching work for classic AutoFDO. For matching, it need this mismatch metrics to suggest whether to use the matching, for pseudo probe, there is the checksum but doesn't apply to AutoFDO.  Will leave when we worked on AutoFDO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147456