[PATCH] D147545: [CSSPGO] Stale profile matching(part 2)

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1846
       ++NumMismatchedProfile;
-      return false;
+      if (!SalvageStaleProfile)
+        return false;
----------------
wlei wrote:
> davidxl wrote:
> > nit: probably rename the option to be 'resync-stale-profile' to make the meaning more accurate.
> Thanks for the name suggestion. so "resync the profile according to current build source code", it sounds good to me, @wenlei see if you are good for this name since you suggested the salvage-stale-profile 
not a big deal but maybe `recover-stale-profile` is a middle ground? the intention was to use a name that tells users what the switch does on a high level (salvage/recover), rather than how it's implemented (match/sync). 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2237-2243
+    if (!IsMatched) {
+      uint32_t CandidateLineOffset = Loc.LineOffset + LocationDelta;
+      LineLocation Candidate(CandidateLineOffset, Loc.Discriminator);
+      InsertMatching(Loc, Candidate);
+      LLVM_DEBUG(dbgs() << "Location is matched from " << Loc << " to "
+                        << Candidate << "\n");
+    }
----------------
wlei wrote:
> wlei wrote:
> > wenlei wrote:
> > > wlei wrote:
> > > > wenlei wrote:
> > > > > This doesn't seem to do what you intended to achieve, or at least what the comment says
> > > > > 
> > > > > > non-anchor location match is based on the offset to the last matched anchor.
> > > > > 
> > > > > If last matched anchor doesn't exist, should we still match anything? 
> > > > > 
> > > > > Consider a case where a function is completely changed, and there's no matched callsite at call, in which case we should probably still throw away its profile instead of matching everything with LocationDelta == 0?
> > > > > 
> > > > > 
> > > > Sorry if my description is unclear.
> > > > 
> > > > My implementation assumes a special non-callsite anchor: `0`, i.e. the beginning of the function. Without the fuzzy matching, it's just this only one anchor matched, and all the others(non-anchor) use the  `LocationDelta = 0` to do the matching, which  means the match source is equal to the target.
> > > > 
> > > > >Consider a case where a function is completely changed, and there's no matched callsite at call, in which case we should probably still throw away its profile instead of matching everything with LocationDelta == 0?
> > > > 
> > > > Say:
> > > > ```
> > > > IR locations:      [1...100 101(foo)]
> > > > Profile locations: [1...100 101(bar)]
> > > > ```
> > > > So my intuition is even without any callsite anchor, matching the leading location would be better than dropping them. (remembering current pseudo-probe implementation, the bb ids all come first, it's not uncommon)
> > > > 
> > > > 
> > > > 
> > > Hmm.. I thought if there's no matching call site, that would mean the function has completely changed, so we can't match probes/blocks in a reasonable way. I can see what you have can help matching leaf functions without call sites.
> > > 
> > > But there needs to be something to make sure when the change is too big, we bail out instead of doing low confidence matching. Maybe we should bail out on zero matched call site + different number of probes/blocks? 
> > Good point, I will add some check(like a threshold) to make sure the very bad match to be bailed out and drop the profiles.
> I found to do this we need to get some metrics to define how big the change is, also need to test the perf number and tune the threshold, I will handle this in a separate diff.
sounds good. 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2262-2264
+  std::set<LineLocation> AllLocations;
+  // Direct call's callee name will be used as the matching anchor.
+  std::unordered_map<LineLocation, StringRef, LineLocationHash> DirectCallSites;
----------------
wlei wrote:
> wenlei wrote:
> > Wondering if we can simplify by merging the two containers, i.e use `std::map<LineLocation, StringRef>`, and empty StringRef value for non-direct-callsite, then we avoid the 2nd map, and also avoid the map look up in `runStaleProfileMatching`. It may use slightly more memory, but feels a bit cleaner. 
> > 
> > Or even something like `std::map<LineLocation, std::pair<StringRef, bool>>` to have `MatchedCallsiteLocs` merged in as well. These containers feel a bit duplicated.
> Good point!  going to remove the `MatchedCallsiteLocs `, so changed to use `std::map<LineLocation, StringRef>` 
Looks like MatchedCallsiteLocs is still there? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147545