[PATCH] D156725: [CSSPGO] Improve profile staleness report for post-link time

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

> For the current sample loader, if the root or parent function profile are lost or mismatched, all its profile and its children profile are dropped, we should use the non-flattened profile(the original nested profile) to count the total mismatched samples.

How significantly does this affect staleness metric?

Maybe what we should actually do is to reuse the nested profile from those mismatched parent functions, something like run `promoteMergeNotInlinedContextSamples` on top level functions samples not used? We do reuse nested profile for not inlined callees from parent function.



================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2109
 
 void SampleProfileMatcher::populateIRLocations(
+    const Function &F, const FunctionSamples *FSForReporting,
----------------
Maybe this function needs a refactoring - break it into two functions: 1) findAnchors that only uses IR, 2) matchCallsites that also uses profile in addition to anchors. We call always call #1, but only call #2 when profile is available and reporting is enabled. 

Also if `MatchedCallsiteLocs` is only used for reporting, but not matching, can we move #2 into `countProfileMismatches`? Currently the different parts seem a bit inter-wined.. would be good have some separation.  

Maybe #2 can also be a standalone functions that covers both call site matching for report and for actual matching (like `populateProfileCallsites` on flattened profile)


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2383
   // Detect profile mismatch for profile staleness metrics report.
-  if (ReportProfileStaleness || PersistProfileStaleness) {
+  // Skip to report the metrics for the imported functions.
+  if (FSForReporting &&
----------------
nit: Skip to report -> Skip reporting, the imported functions -> imported functions


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2390
+      TotalFuncHashSamples += FSForReporting->getTotalSamples();
+      ;
+      TotalProfiledFunc++;
----------------
remove? 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2403
     uint64_t FuncProfiledCallsites = 0;
-    countProfileMismatches(FS, MatchedCallsiteLocs, FuncMismatchedCallsites,
-                           FuncProfiledCallsites);
+    countProfileMismatches(*FSForReporting, MatchedCallsiteLocs,
+                           FuncMismatchedCallsites, FuncProfiledCallsites);
----------------
This function is growing bigger. Can we move line 2384-2399 and line 2405-2414 into countProfileMismatches? These are all related to mismatch statistics. 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2421
+    // different contexts are merged together for matching.
+    FunctionSamples *FSForMatching = getFlattenedSamplesFor(F);
+    assert((!FSForReporting || (FSForReporting && FSForMatching)) &&
----------------
nit: instead of `FSForReporting` vs `FSForMatching`, just say `FS` and `FSFlattened` instead? it applies to other instances too. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156725