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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:451
+  // the profile.
+  StringMap<LocToLocMap> FuncToMatchingsMap;
 
----------------
nit: FuncToMatchingsMap->FuncMappings? 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2208
+    StringMap<std::set<LineLocation>> &CalleeToCallsitesMap,
+    LocToLocMap &IRToProfileLocationMap) {
+  auto InsertMatching = [&](const LineLocation &From, const LineLocation &To) {
----------------
Can/should we assert `IRToProfileLocationMap` is empty to begin with? i.e. we shouldn't be populating the same map multiple times? 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2229
+        const auto &Candidate = *ProfileAnchors->second.begin();
+        ProfileAnchors->second.erase(Candidate);
+        InsertMatching(Loc, Candidate);
----------------
If you provide `erase` with an iterator instead of an element, it would save a find..


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2256
+    // Match forwards for non-anchor locations.
+    if (!IsAnchorMatched) {
+      uint32_t CandidateLineOffset = Loc.LineOffset + LocationDelta;
----------------
It looks to me that `IsAnchorMatched` can be replaced with `!IRToProfileLocationMap.empty()`?


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2362
+    // The matching result will be saved to IRToProfileLocationMap.
+    auto &IRToProfileLocationMap = getOrCreateIRToProfileLocationMap(F);
+
----------------
I'm wondering if we should just create a map here, and change the `getOrCreateIRToProfileLocationMap` API into a simple getter? 

There seems to be very clear separation as to when we need to create and when we need to get, since there's no case for on-demand creation. Hence, simple/separate API might be cleaner, and less error-prone.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2339
+  if (ReportProfileStaleness || PersistProfileStaleness) {
+    uint64_t _TotalProfiledCallsites = TotalProfiledCallsites;
+    uint64_t _NumMismatchedCallsites = NumMismatchedCallsites;
----------------
wlei wrote:
> wenlei wrote:
> > A bit confused by the use of `_TotalProfiledCallsites` and `_NumMismatchedCallsites`, what are you trying to do with the two additional variables? 
> Because the `TotalProfiledCallsites` and `NumMismatchedCallsites` is the sum for the whole module functions not one function, so here for function level debug print, use a location variable to do it.
Ok, now I see what's going on. The code as is can be confusing (both the naming of variables, and the subtraction involved to get per-function stats). 

I suggest we make `countProfileMismatches` take two `int&` input through parameters, so it doesn't change global state as a way to output, which is inconsistent with the way it takes func as input through parameters.

Then use `FuncMismatchedCallsites`, `FuncProfiledCallsites` to pass into `countProfileMismatches` to get its output. At caller side, we accumulate `FuncMismatchedCallsites`, `FuncProfiledCallsites` on to `TotalProfiledCallsites`, `TotalProfiledCallsites`.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147545