[AutoFDO] Use getHeadSamplesEstimate instead of getTotalSamples to compute profile callsite staleness

[wlei]

Summary

Fix two issues for profile staleness report.

1. It should be more accurate to use the sum of all entry count(getHeadSamplesEstimate) for the callsite samples than the total samples, since even the top-level callsite is mismatched, it does affect the inlining but it can still be merged into base profile and used later.

2. I accidentally missed to persist the num of mismatched callsite into binary.

Also added the asm testing to test the decoding of the section.

[hoy]

llvm/lib/Transforms/IPO/SampleProfile.cpp

2187

nit: NumMismatchedCallsite -> NumMismatchedCallsites, TotalProfiledCallsite -> TotalProfiledCallsites

[wlei]

addressing feedback.

[wenlei]

I thought the idea is to compute the % of samples being dropped due to mismatch? in this case, all samples from callsite will be dropped, so I actually don't see a problem with using getTotalSamples. Yes you can argue that they can be merged with base profile, but then that argument applies to getHeadSamplesEstimate too?

[wlei]

In D140063#3998253, @wenlei wrote:

I thought the idea is to compute the % of samples being dropped due to mismatch? in this case, all samples from callsite will be dropped, so I actually don't see a problem with using getTotalSamples. Yes you can argue that they can be merged with base profile, but then that argument applies to getHeadSamplesEstimate too?

Yeah, it's tricky to quantify the dropped callsite samples's impact. I was thinking in a reversed way, if we use getTotalSamples, I feel like the total samples are completely dropped(not merged into top-level profile), in fact it's still in use. And for getHeadSamplesEstimate, I feel it's like only the callsite call/jump's samples are dropped, but I admit that this's also not accurate, the missing inlining from big total samples could affect more on perf. I don't have strong opinion on this.

[hoy]

In D140063#3998381, @wlei wrote:

In D140063#3998253, @wenlei wrote:

I thought the idea is to compute the % of samples being dropped due to mismatch? in this case, all samples from callsite will be dropped, so I actually don't see a problem with using getTotalSamples. Yes you can argue that they can be merged with base profile, but then that argument applies to getHeadSamplesEstimate too?

Yeah, it's tricky to quantify the dropped callsite samples's impact. I was thinking in a reversed way, if we use getTotalSamples, I feel like the total samples are completely dropped(not merged into top-level profile), in fact it's still in use. And for getHeadSamplesEstimate, I feel it's like only the callsite call/jump's samples are dropped, but I admit that this's also not accurate, the missing inlining from big total samples could affect more on perf. I don't have strong opinion on this.

Reporting mismatched samples based on getHeadSamplesEstimate sounds a bit more accurate to me, thought it's not perfect either. The direct effect of mismatched callsite samples is likely a missing inlining. Reporting callee total samples for this may not give a good signal, especially when the callee is really big but the callsite isn't very hot.

We have some internal services showing a very high callsite mismatch rate like 30%. Wondering if that could be related.

[wlei]

In D140063#3998253, @wenlei wrote:

I thought the idea is to compute the % of samples being dropped due to mismatch? in this case, all samples from callsite will be dropped, so I actually don't see a problem with using getTotalSamples. Yes you can argue that they can be merged with base profile, but then that argument applies to getHeadSamplesEstimate too?

Or since it's arguable, needed more discussion/experiments, I can make a separate patch for that, make this to fix the obvious issues first.

[wenlei]

In D140063#3998381, @wlei wrote:

In D140063#3998253, @wenlei wrote:

I thought the idea is to compute the % of samples being dropped due to mismatch? in this case, all samples from callsite will be dropped, so I actually don't see a problem with using getTotalSamples. Yes you can argue that they can be merged with base profile, but then that argument applies to getHeadSamplesEstimate too?

Yeah, it's tricky to quantify the dropped callsite samples's impact. I was thinking in a reversed way, if we use getTotalSamples, I feel like the total samples are completely dropped(not merged into top-level profile), in fact it's still in use. And for getHeadSamplesEstimate, I feel it's like only the callsite call/jump's samples are dropped, but I admit that this's also not accurate, the missing inlining from big total samples could affect more on perf. I don't have strong opinion on this.

I guess it depends on how we define "callsite samples" -- is it about the actual call count, or about the total samples of callees. It has some ambiguity. Maybe keep this change, but call it mismatched call site count instead?

[wenlei]

In D140063#3998407, @hoy wrote:

In D140063#3998381, @wlei wrote:

In D140063#3998253, @wenlei wrote:

I thought the idea is to compute the % of samples being dropped due to mismatch? in this case, all samples from callsite will be dropped, so I actually don't see a problem with using getTotalSamples. Yes you can argue that they can be merged with base profile, but then that argument applies to getHeadSamplesEstimate too?

Yeah, it's tricky to quantify the dropped callsite samples's impact. I was thinking in a reversed way, if we use getTotalSamples, I feel like the total samples are completely dropped(not merged into top-level profile), in fact it's still in use. And for getHeadSamplesEstimate, I feel it's like only the callsite call/jump's samples are dropped, but I admit that this's also not accurate, the missing inlining from big total samples could affect more on perf. I don't have strong opinion on this.

Reporting mismatched samples based on getHeadSamplesEstimate sounds a bit more accurate to me, thought it's not perfect either. The direct effect of mismatched callsite samples is likely a missing inlining. Reporting callee total samples for this may not give a good signal, especially when the callee is really big but the callsite isn't very hot.

We have some internal services showing a very high callsite mismatch rate like 30%. Wondering if that could be related.

Logically I think a call site focused metric makes sense. But the way CallsiteSamples is defined led people to think this is total samples. I think perhaps we just need to be explicit in the naming, so it's clear that we're tracking call site counts, not call site samples..

[wlei]

In D140063#3998437, @wenlei wrote:

In D140063#3998407, @hoy wrote:

In D140063#3998381, @wlei wrote:

In D140063#3998253, @wenlei wrote:

I thought the idea is to compute the % of samples being dropped due to mismatch? in this case, all samples from callsite will be dropped, so I actually don't see a problem with using getTotalSamples. Yes you can argue that they can be merged with base profile, but then that argument applies to getHeadSamplesEstimate too?

Yeah, it's tricky to quantify the dropped callsite samples's impact. I was thinking in a reversed way, if we use getTotalSamples, I feel like the total samples are completely dropped(not merged into top-level profile), in fact it's still in use. And for getHeadSamplesEstimate, I feel it's like only the callsite call/jump's samples are dropped, but I admit that this's also not accurate, the missing inlining from big total samples could affect more on perf. I don't have strong opinion on this.

Reporting mismatched samples based on getHeadSamplesEstimate sounds a bit more accurate to me, thought it's not perfect either. The direct effect of mismatched callsite samples is likely a missing inlining. Reporting callee total samples for this may not give a good signal, especially when the callee is really big but the callsite isn't very hot.

We have some internal services showing a very high callsite mismatch rate like 30%. Wondering if that could be related.

Logically I think a call site focused metric makes sense. But the way CallsiteSamples is defined led people to think this is total samples. I think perhaps we just need to be explicit in the naming, so it's clear that we're tracking call site counts, not call site samples..

Sounds good to change to the callsite counts. CallsiteSamples in the nested profile is indeed the total samples of the callee, which is a confusing name.

[wenlei]

actually you already record it as NumMismatchedCallsites, so I think it should be good. :)

[wlei]

In D140063#3998590, @wenlei wrote:

actually you already record it as NumMismatchedCallsites, so I think it should be good. :)

Ah, Okay! Also avoid the Num and Count name confusing :)

llvm/lib/Transforms/IPO/SampleProfile.cpp

 
 // Sample profile matching - fuzzy match.
 class SampleProfileMatcher {
   Module &M;
   SampleProfileReader &Reader;
   const PseudoProbeManager *ProbeManager;
 
   // Profile mismatching statstics.
-  uint64_t TotalProfiledCallsite = 0;
-  uint64_t NumMismatchedCallsite = 0;
+  uint64_t TotalProfiledCallsites = 0;
+  uint64_t NumMismatchedCallsites = 0;
   uint64_t MismatchedCallsiteSamples = 0;
   uint64_t TotalCallsiteSamples = 0;
   uint64_t TotalProfiledFunc = 0;
   uint64_t NumMismatchedFuncHash = 0;
   uint64_t MismatchedFuncHashSamples = 0;
   uint64_t TotalFuncHashSamples = 0;
 
 public:
void SampleProfileMatcher::detectProfileMismatch(const Function &F,
   for (auto &I : FS.getBodySamples()) {
     const LineLocation &Loc = I.first;
     if (isInvalidLineOffset(Loc.LineOffset))
       continue;
 
     uint64_t Count = I.second.getSamples();
     if (!I.second.getCallTargets().empty()) {
       TotalCallsiteSamples += Count;
-      TotalProfiledCallsite++;
+      TotalProfiledCallsites++;
       if (!MatchedCallsiteLocs.count(Loc)) {
         MismatchedCallsiteSamples += Count;
-        NumMismatchedCallsite++;
+        NumMismatchedCallsites++;
       }
     }
   }
 
   for (auto &I : FS.getCallsiteSamples()) {
     const LineLocation &Loc = I.first;
     if (isInvalidLineOffset(Loc.LineOffset))
       continue;
 
     uint64_t Count = 0;
     for (auto &FM : I.second) {
-      Count += FM.second.getTotalSamples();
+      Count += FM.second.getHeadSamplesEstimate();
     }
     TotalCallsiteSamples += Count;
-    TotalProfiledCallsite++;
+    TotalProfiledCallsites++;
     if (!MatchedCallsiteLocs.count(Loc)) {
       MismatchedCallsiteSamples += Count;
-      NumMismatchedCallsite++;
+      NumMismatchedCallsites++;
     }
   }
 }
 
 void SampleProfileMatcher::detectProfileMismatch() {
   for (auto &F : M) {
     if (F.isDeclaration() || !F.hasFnAttribute("use-sample-profile"))
       continue;
     FunctionSamples *FS = Reader.getSamplesFor(F);
     if (!FS)
       continue;
     detectProfileMismatch(F, *FS);
   }
 
   if (ReportProfileStaleness) {
     if (FunctionSamples::ProfileIsProbeBased) {
       errs() << "(" << NumMismatchedFuncHash << "/" << TotalProfiledFunc << ")"
              << " of functions' profile are invalid and "
              << " (" << MismatchedFuncHashSamples << "/" << TotalFuncHashSamples
              << ")"
              << " of samples are discarded due to function hash mismatch.\n";
     }
-    errs() << "(" << NumMismatchedCallsite << "/" << TotalProfiledCallsite
+    errs() << "(" << NumMismatchedCallsites << "/" << TotalProfiledCallsites
            << ")"
            << " of callsites' profile are invalid and "
            << "(" << MismatchedCallsiteSamples << "/" << TotalCallsiteSamples
            << ")"
            << " of samples are discarded due to callsite location mismatch.\n";
   }
 
   if (PersistProfileStaleness) {
     LLVMContext &Ctx = M.getContext();
     MDBuilder MDB(Ctx);
 
     SmallVector<std::pair<StringRef, uint64_t>> ProfStatsVec;
     if (FunctionSamples::ProfileIsProbeBased) {
       ProfStatsVec.emplace_back("NumMismatchedFuncHash", NumMismatchedFuncHash);
       ProfStatsVec.emplace_back("TotalProfiledFunc", TotalProfiledFunc);
       ProfStatsVec.emplace_back("MismatchedFuncHashSamples",
                                 MismatchedFuncHashSamples);
       ProfStatsVec.emplace_back("TotalFuncHashSamples", TotalFuncHashSamples);
     }
+
+    ProfStatsVec.emplace_back("NumMismatchedCallsites", NumMismatchedCallsites);
+    ProfStatsVec.emplace_back("TotalProfiledCallsites", TotalProfiledCallsites);
     ProfStatsVec.emplace_back("MismatchedCallsiteSamples",
                               MismatchedCallsiteSamples);
     ProfStatsVec.emplace_back("TotalCallsiteSamples", TotalCallsiteSamples);
 
     auto *MD = MDB.createLLVMStats(ProfStatsVec);
     auto *NMD = M.getOrInsertNamedMetadata("llvm.stats");
     NMD->addOperand(MD);
   }

llvm/test/Transforms/SampleProfile/profile-mismatch.ll

 ; REQUIRES: x86_64-linux
 ; RUN: opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/profile-mismatch.prof -report-profile-staleness -persist-profile-staleness -S 2>%t -o %t.ll
 ; RUN: FileCheck %s --input-file %t
 ; RUN: FileCheck %s --input-file %t.ll -check-prefix=CHECK-MD
 ; RUN: llc < %t.ll -filetype=obj -o %t.obj
 ; RUN: llvm-objdump --section-headers %t.obj | FileCheck %s --check-prefix=CHECK-OBJ
+; RUN: llc < %t.ll -filetype=asm -o - | FileCheck %s --check-prefix=CHECK-ASM
 
-; CHECK: (2/3) of callsites' profile are invalid and (20/30) of samples are discarded due to callsite location mismatch.
+; CHECK: (2/3) of callsites' profile are invalid and (15/25) of samples are discarded due to callsite location mismatch.
 
-; CHECK-MD: ![[#]] = !{!"MismatchedCallsiteSamples", i64 20, !"TotalCallsiteSamples", i64 30}
+; CHECK-MD: ![[#]] = !{!"NumMismatchedCallsites", i64 2, !"TotalProfiledCallsites", i64 3, !"MismatchedCallsiteSamples", i64 15, !"TotalCallsiteSamples", i64 25}
 
 ; CHECK-OBJ: .llvm_stats
 
+; CHECK-ASM: .section  .llvm_stats,"",@progbits
+; CHECK-ASM: .byte 22
+; CHECK-ASM: .ascii  "NumMismatchedCallsites"
+; CHECK-ASM: .byte 4
+; CHECK-ASM: .ascii  "Mg=="
+; CHECK-ASM: .byte 22
+; CHECK-ASM: .ascii  "TotalProfiledCallsites"
+; CHECK-ASM: .byte 4
+; CHECK-ASM: .ascii  "Mw=="
+; CHECK-ASM: .byte 25
+; CHECK-ASM: .ascii  "MismatchedCallsiteSamples"
+; CHECK-ASM: .byte 4
+; CHECK-ASM: .ascii  "MTU="
+; CHECK-ASM: .byte 20
+; CHECK-ASM: .ascii  "TotalCallsiteSamples"
+; CHECK-ASM: .byte 4
+; CHECK-ASM: .ascii  "MjU="
+
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 @x = dso_local global i32 0, align 4, !dbg !0
 
 ; Function Attrs: nounwind uwtable
 define dso_local i32 @foo(i32 noundef %x) #0 !dbg !12 {
 entry:

llvm/test/Transforms/SampleProfile/pseudo-probe-profile-mismatch.ll

 ; REQUIRES: x86_64-linux
 ; RUN: opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/pseudo-probe-profile-mismatch.prof -report-profile-staleness -persist-profile-staleness -S 2>%t -o %t.ll
 ; RUN: FileCheck %s --input-file %t
 ; RUN: FileCheck %s --input-file %t.ll -check-prefix=CHECK-MD
 ; RUN: llc < %t.ll -filetype=obj -o %t.obj
 ; RUN: llvm-objdump --section-headers %t.obj | FileCheck %s --check-prefix=CHECK-OBJ
+; RUN: llc < %t.ll -filetype=asm -o - | FileCheck %s --check-prefix=CHECK-ASM
 
 ; CHECK: (1/3) of functions' profile are invalid and (10/50) of samples are discarded due to function hash mismatch.
 ; CHECK: (2/3) of callsites' profile are invalid and (20/30) of samples are discarded due to callsite location mismatch.
 
-; CHECK-MD: ![[#]] = !{!"NumMismatchedFuncHash", i64 1, !"TotalProfiledFunc", i64 3, !"MismatchedFuncHashSamples", i64 10, !"TotalFuncHashSamples", i64 50, !"MismatchedCallsiteSamples", i64 20, !"TotalCallsiteSamples", i64 30}
+; CHECK-MD: ![[#]] = !{!"NumMismatchedFuncHash", i64 1, !"TotalProfiledFunc", i64 3, !"MismatchedFuncHashSamples", i64 10, !"TotalFuncHashSamples", i64 50, !"NumMismatchedCallsites", i64 2, !"TotalProfiledCallsites", i64 3, !"MismatchedCallsiteSamples", i64 20, !"TotalCallsiteSamples", i64 30}
 
 ; CHECK-OBJ: .llvm_stats
 
+; CHECK-ASM: .section  .llvm_stats,"",@progbits
+; CHECK-ASM: .byte 21
+; CHECK-ASM: .ascii  "NumMismatchedFuncHash"
+; CHECK-ASM: .byte 4
+; CHECK-ASM: .ascii  "MQ=="
+; CHECK-ASM: .byte 17
+; CHECK-ASM: .ascii  "TotalProfiledFunc"
+; CHECK-ASM: .byte 4
+; CHECK-ASM: .ascii  "Mw=="
+; CHECK-ASM: .byte 25
+; CHECK-ASM: .ascii  "MismatchedFuncHashSamples"
+; CHECK-ASM: .byte 4
+; CHECK-ASM: .ascii  "MTA="
+; CHECK-ASM: .byte 20
+; CHECK-ASM: .ascii  "TotalFuncHashSamples"
+; CHECK-ASM: .byte 4
+; CHECK-ASM: .ascii  "NTA="
+; CHECK-ASM: .byte 22
+; CHECK-ASM: .ascii  "NumMismatchedCallsites"
+; CHECK-ASM: .byte 4
+; CHECK-ASM: .ascii  "Mg=="
+; CHECK-ASM: .byte 22
+; CHECK-ASM: .ascii  "TotalProfiledCallsites"
+; CHECK-ASM: .byte 4
+; CHECK-ASM: .ascii  "Mw=="
+; CHECK-ASM: .byte 25
+; CHECK-ASM: .ascii  "MismatchedCallsiteSamples"
+; CHECK-ASM: .byte 4
+; CHECK-ASM: .ascii  "MjA="
+; CHECK-ASM: .byte 20
+; CHECK-ASM: .ascii  "TotalCallsiteSamples"
+; CHECK-ASM: .byte 4
+; CHECK-ASM: .ascii  "MzA="
+
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 @x = dso_local global i32 0, align 4, !dbg !0
 
 ; Function Attrs: nounwind uwtable
 define dso_local i32 @foo(i32 noundef %x) #0 !dbg !16 {
 entry: