[PATCH] D158817: [CSSPGO] Refactoring populateIRLocations and countProfileMismatches

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2379-2416
   std::map<LineLocation, StringRef> IRLocations;
-  // Extract profile matching anchors and profile mismatch metrics in the IR.
-  populateIRLocations(F, FS, MatchedCallsiteLocs, IRLocations);
+  populateIRLocations(F, IRLocations);
+  // Anchors for profile. it's a map from callsite location to set of callee
+  // name.
+  std::map<LineLocation, StringSet<>> ProfileLocations;
+  populateProfileLocations(FS, ProfileLocations);
 
----------------
wlei wrote:
> wenlei wrote:
> > Trying to organize the code a bit and keep high level flow clean, how about this:
> > 
> > ```
> > // ..
> > std::map<LineLocation, StringRef> IRAnchors;
> > findIRAnchors(FS, IRAnchors);
> > 
> > // ..
> > std::map<LineLocation, StringSet<>> ProfileAnchors;
> > findProfileAnchors(FS, ProfileLocations);
> > 
> > // Detect profile mismatch for profile staleness metrics report.
> > if (ReportProfileStaleness || PersistProfileStaleness) {
> >   countProfileMismatches(FS, IRAnchors, ProfileAnchors, ...)
> > }
> > 
> > if (IsFuncHashMismatch && SalvageStaleProfile) {
> >    runStaleProfileMatching(IRAnchors, ProfileAnchors, getIRToProfileLocationMap(F));
> > }
> > ```
> > 
> > Accordingly, we would need to move `populateProfileCallsites` in `runStaleProfileMatching`. Maybe even inline expand `runStaleProfileMatching` there without a function call since the function is now simplified? Also move all statistics into `countProfileMismatches`.
> > 
> > Inside `runStaleProfileMatching`, we'd also rename `ProfileAnchors` to `MatchedAnchors`. 
> That would looks much cleaner, thanks for the suggestion! done accordingly.
> 
> Only one things is that the name `IRAnchors`, it is not only the `Anchors`, we also need the non-call locations whose callee name is empty. but I understand separating the non-call locations and the anchors doesn't look neat. I don't have strong option on this, maybe this is straightforward to understand.
Yeah, I think the term Anchor is abstract enough that including non-call is also okay.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158817