[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:464
+  // target.
+  static constexpr const char *IndirectCalleeName = "_indirect_call_";
+
----------------
this can actually be a valid c mangle name.. we need something more distinctive..  `*UnkownIndirectCallee`? 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2214
+    bool CallsiteIsMatched = false;
+    // Since indirect call does not have the CalleeName, check
+    // conservatively if callsite in the profile is a callsite location.
----------------
nit: the CalleeName -> CalleeName.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2216
+    // conservatively if callsite in the profile is a callsite location.
+    // This is to avoid nums of false positive since otherwise all the
+    // indirect call samples will be reported as mismatching.
----------------
nit: avoid nums of false positive ->

reduce num of false positives

 or 

avoid false positive


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2263
 void SampleProfileMatcher::populateProfileCallsites(
-    const FunctionSamples &FS,
+    const std::map<LineLocation, StringSet<>> ProfileLocations,
     StringMap<std::set<LineLocation>> &CalleeToCallsitesMap) {
----------------
This is passed by value, missing `&`? 


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


================
Comment at: llvm/test/Transforms/SampleProfile/profile-mismatch.ll:27
 ; CHECK-ASM: .byte 4
-; CHECK-ASM: .ascii  "MTU="
+; CHECK-ASM: .ascii  "MjU="
 ; CHECK-ASM: .byte 20
----------------
what causes this change? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158817