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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


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


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


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


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

```
  for (auto &IR : IRLocations) { 
     bool IsAnchorMatched = false;
     ....
   }
```
For each location, it needs to check if it's an anchor then if it's matched, maybe the name is confusing, changed to `IsMatchedAnchor`.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2362
+    // The matching result will be saved to IRToProfileLocationMap.
+    auto &IRToProfileLocationMap = getOrCreateIRToProfileLocationMap(F);
+
----------------
wenlei wrote:
> 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.
Yes, here the matching should run only once per function, makes sense to just create a map.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2339
+  if (ReportProfileStaleness || PersistProfileStaleness) {
+    uint64_t _TotalProfiledCallsites = TotalProfiledCallsites;
+    uint64_t _NumMismatchedCallsites = NumMismatchedCallsites;
----------------
wenlei wrote:
> 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`.
> 
> 
yeah, it's clearer this way, done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147545