[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:1850
     }
     ++NumMatchedProfile;
   } else {
----------------
As mentioned below, I think we should still throw away profiles that can't be matched due to missing call site anchors. 


================
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");
+    }
----------------
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?




================
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;
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1844
       LLVM_DEBUG(
           dbgs() << "Profile is invalid due to CFG mismatch for Function "
+                 << F.getName() << "\n");
----------------
Unrelated to this change, but it may be useful to have a way for users to print out functions with stale profile without needing debug compiler. 

Often times users ask this: is this change going to invalidate profile for this function? having a switch may help them self-diagnose to get an answer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147545