[CSSPGO] Do not recount callee samples when computing profile summary for nested CS profile.

[hoy]

Summary

When generating nested CS profile with all calling contexts of a function duplicated into a base profile under --generate-merged-base-profiles, do not recount callee samples when computing profile summary. This fixes the profile summary mismatch between flat cs profile and nested cs profile, for both extbinary and text format.

[hoy]

Updating D119494: [CSSPGO] Do not recount callee samples when computing profile summary for nested CS profile.

[wenlei]

llvm/lib/ProfileData/ProfileSummaryBuilder.cpp

128

We should not have ProfileSummaryBuilder depending on profile generation flags from llvm-profgen. You moved the flags out off llvm-profgen to avoid cyclic dependency, but this is still hacky.

If we want to this specific behavior of ProfileSummaryBuilder, i.e. whether callee samples should be counted, we can have ProfileSummaryBuilder expose a proper knob for that.

In this case, maybe we could also consider actually setting a duplicated bit/attribute for those duplicated callee profile, then ProfileSummaryBuilder can naturally skip the duplicated one, which should be equivalent.

[hoy]

llvm/lib/ProfileData/ProfileSummaryBuilder.cpp

128

For extbiny profile, ProfileSummaryBuilder is called in profile writing time, which is still in the SampleProf library. The command line switches being checked here is because we would like to correct the summary computation during profile writing time. Maybe we can expose a writer flag to for this instead.

A function profile may have both inlined samples and uninlined samples and it's hard to tell them apart once they are merged. We could add an extra attribute to the extbin profile for GenerateMergedBaseProfiles which can be checked here, similar to ProfileIsCSNested. This seems cleaner. WDYT?

[hoy]

llvm/lib/ProfileData/ProfileSummaryBuilder.cpp

128

Actually an attribute for each callee profile may work better. It can help text profile too.

[hoy]

Switching to using per-context attribute.

[wenlei]

lgtm, thanls.

llvm/lib/ProfileData/ProfileSummaryBuilder.cpp

128

For extbiny profile, ProfileSummaryBuilder is called in profile writing time, which is still in the SampleProf library.

It is ok for llvm-profgen to use ProfileSummaryBuilder, but ProfileSummaryBuilder should not be coupled with anything specific to profile generation. Checking the switches made it coupled with profile generation specifics - that's the hacky part, it breaks proper layering.

The command line switches being checked here is because we would like to correct the summary computation during profile writing time.

I understand the practical part of it, i.e. why you want to do this. I'm arguing the structure of the implementation, not why this is needed.

[wenlei]

llvm/lib/ProfileData/ProfileSummaryBuilder.cpp

113–116

nit:

} else if (FS.getContext().hasAttribute(sampleprof::ContextDuplicatedIntoBase)) {
  return;
}

[hoy]

Updating D119494: [CSSPGO] Do not recount callee samples when computing profile summary for nested CS profile.

llvm/include/llvm/ProfileData/SampleProf.h

enum ContextStateMask {
   MergedContext = 0x8     // Profile for context merged into base profile
 };
 
 // Attribute of context associated with FunctionSamples
 enum ContextAttributeMask {
   ContextNone = 0x0,
   ContextWasInlined = 0x1,      // Leaf of context was inlined in previous build
   ContextShouldBeInlined = 0x2, // Leaf of context should be inlined
+  ContextDuplicatedIntoBase =
+      0x4, // Leaf of context is duplicated into the base profile
 };
 
 // Represents a context frame with function name and line location
 struct SampleContextFrame {
   StringRef FuncName;
   LineLocation Location;
 
   SampleContextFrame() : Location(0, 0) {}

llvm/lib/ProfileData/ProfileSummaryBuilder.cpp

 // To compute the detailed summary, we consider each line containing samples as
 // equivalent to a block with a count in the instrumented profile.
 void SampleProfileSummaryBuilder::addRecord(
     const sampleprof::FunctionSamples &FS, bool isCallsiteSample) {
   if (!isCallsiteSample) {
     NumFunctions++;
     if (FS.getHeadSamples() > MaxFunctionCount)
       MaxFunctionCount = FS.getHeadSamples();
+  } else {
+    // Do not recount callee samples if they are already merged into their base
+    // profiles. This can happen to CS nested profile.
+    if (FS.getContext().hasAttribute(sampleprof::ContextDuplicatedIntoBase))
+      return;
   }
   for (const auto &I : FS.getBodySamples()) {
     uint64_t Count = I.second.getSamples();
       addCount(Count);
   }
   for (const auto &I : FS.getCallsiteSamples())
     for (const auto &CS : I.second)
       addRecord(CS.second, true);
 }
 
 // The argument to this method is a vector of cutoff percentages and the return
 // value is a vector of (Cutoff, MinCount, NumCounts) triplets.
 void ProfileSummaryBuilder::computeDetailedSummary() {
   if (DetailedSummaryCutoffs.empty())
     return;
   llvm::sort(DetailedSummaryCutoffs);
   auto Iter = CountFrequencies.begin();
   const auto End = CountFrequencies.end();
 

llvm/lib/ProfileData/SampleProf.cpp

for (auto &It : Node.AllChildFrames) {
     }
 
     // Separate child profile to be a standalone profile, if the current parent
     // profile doesn't exist. This is a duplicating operation when the child
     // profile is already incorporated into the parent which is still useful and
     // thus done optionally. It is seen that duplicating context profiles into
     // base profiles improves the code quality for thinlto build by allowing a
     // profile in the prelink phase for to-be-fully-inlined functions.
-    if (!NodeProfile || GenerateMergedBaseProfiles)
+    if (!NodeProfile) {
       ProfileMap[ChildProfile->getContext()].merge(*ChildProfile);
+    } else if (GenerateMergedBaseProfiles) {
+      ProfileMap[ChildProfile->getContext()].merge(*ChildProfile);
+      auto &SamplesMap = NodeProfile->functionSamplesAt(ChildNode.CallSiteLoc);
+      SamplesMap[ChildProfile->getName().str()].getContext().setAttribute(
+          ContextDuplicatedIntoBase);
+    }
 
     // Contexts coming with a `ContextShouldBeInlined` attribute indicate this
     // is a preinliner-computed profile.
     if (OrigChildContext.hasAttribute(ContextShouldBeInlined))
       FunctionSamples::ProfileIsCSNested = true;
 
     // Remove the original child profile.
     ProfileMap.erase(OrigChildContext);
   }
 }
 
 void CSProfileConverter::convertProfiles() { convertProfiles(RootFrame); }

llvm/test/tools/llvm-profdata/cs-sample-nested-profile.test

 RUN: llvm-profdata merge --sample --text -output=%t.proftext %S/Inputs/cs-sample-preinline.proftext --gen-cs-nested-profile=1 -generate-merged-base-profiles=0
 RUN: FileCheck %s < %t.proftext --match-full-lines --strict-whitespace
 RUN: llvm-profdata merge --sample --text -output=%t.probe.proftext %S/Inputs/cs-sample-preinline-probe.proftext --gen-cs-nested-profile=1 -generate-merged-base-profiles=0
 RUN: FileCheck %s < %t.probe.proftext --match-full-lines --strict-whitespace -check-prefix=PROBE
 RUN: llvm-profdata merge --sample --extbinary -output=%t.profbin %S/Inputs/cs-sample-preinline.proftext --gen-cs-nested-profile=1 -generate-merged-base-profiles=0
 RUN: llvm-profdata merge --sample --text -output=%t2.proftext %t.profbin
 RUN: FileCheck %s < %t2.proftext --match-full-lines --strict-whitespace
 RUN: llvm-profdata show --sample -show-sec-info-only %t.profbin | FileCheck %s -check-prefix=PREINLINE
-RUN: llvm-profdata merge --sample --text -output=%t.proftext %S/Inputs/cs-sample-preinline.proftext --gen-cs-nested-profile=1 -generate-merged-base-profiles=1
-RUN: FileCheck %s < %t.proftext --match-full-lines --strict-whitespace -check-prefix=RECOUNT
+RUN: llvm-profdata merge --sample --text -output=%t3.proftext %S/Inputs/cs-sample-preinline.proftext --gen-cs-nested-profile=1 -generate-merged-base-profiles=1
+RUN: FileCheck %s < %t3.proftext --match-full-lines --strict-whitespace -check-prefix=RECOUNT
+RUN: llvm-profdata merge --sample --extbinary -output=%t2.profbin %S/Inputs/cs-sample-preinline.proftext --gen-cs-nested-profile=1 -generate-merged-base-profiles=1
+RUN: llvm-profdata show -sample -detailed-summary %S/Inputs/cs-sample-preinline.proftext | FileCheck %s -check-prefix=SUMMARY
+RUN: llvm-profdata show -sample -detailed-summary %t2.profbin | FileCheck %s -check-prefix=SUMMARY
+RUN: llvm-profdata show -sample -detailed-summary %t3.proftext | FileCheck %s -check-prefix=SUMMARY
 
 
 ; CHECK:main:1968679:12
 ; CHECK-NEXT: 2: 24
 ; CHECK-NEXT: 3: 28 _Z5funcAi:18
 ; CHECK-NEXT: 3.1: 28 _Z5funcBi:30
 ; CHECK-NEXT: 3: _Z5funcAi:1467398
 ; CHECK-NEXT:  0: 10

 ; RECOUNT-NEXT:  1: 10 _Z8funcLeafi:11
 ; RECOUNT-NEXT:  3: 24
 ; RECOUNT-NEXT:  1: _Z8funcLeafi:1467299
 ; RECOUNT-NEXT:   0: 6
 ; RECOUNT-NEXT:   1: 6
 ; RECOUNT-NEXT:   3: 287884
 ; RECOUNT-NEXT:   4: 287864 _Z3fibi:315608
 ; RECOUNT-NEXT:   15: 23
-; RECOUNT-NEXT:   !Attributes: 2
-; RECOUNT-NEXT:  !Attributes: 2
+; RECOUNT-NEXT:   !Attributes: 6
+; RECOUNT-NEXT:  !Attributes: 6
 ; RECOUNT-NEXT: 3.1: _Z5funcBi:500973
 ; RECOUNT-NEXT:  0: 19
 ; RECOUNT-NEXT:  1: 19 _Z8funcLeafi:20
 ; RECOUNT-NEXT:  3: 12
 ; RECOUNT-NEXT:  1: _Z8funcLeafi:500853
 ; RECOUNT-NEXT:   0: 15
 ; RECOUNT-NEXT:   1: 15
 ; RECOUNT-NEXT:   3: 74946
 ; RECOUNT-NEXT:   4: 74941 _Z3fibi:82359
 ; RECOUNT-NEXT:   10: 23324
 ; RECOUNT-NEXT:   11: 23327 _Z3fibi:25228
 ; RECOUNT-NEXT:   15: 11
-; RECOUNT-NEXT:   !Attributes: 2
-; RECOUNT-NEXT:  !Attributes: 2
+; RECOUNT-NEXT:   !Attributes: 6
+; RECOUNT-NEXT:  !Attributes: 6
 ; RECOUNT-NEXT:_Z8funcLeafi:1968152:31
 ; RECOUNT-NEXT: 0: 21
 ; RECOUNT-NEXT: 1: 21
 ; RECOUNT-NEXT: 3: 362830
 ; RECOUNT-NEXT: 4: 362805 _Z3fibi:397967
 ; RECOUNT-NEXT: 10: 23324
 ; RECOUNT-NEXT: 11: 23327 _Z3fibi:25228
 ; RECOUNT-NEXT: 15: 34
 ; RECOUNT-NEXT: !Attributes: 2
 ; RECOUNT-NEXT:_Z5funcAi:1467398:11
 ; RECOUNT-NEXT: 0: 10
 ; RECOUNT-NEXT: 1: 10 _Z8funcLeafi:11
 ; RECOUNT-NEXT: 3: 24
 ; RECOUNT-NEXT: 1: _Z8funcLeafi:1467299
 ; RECOUNT-NEXT:  0: 6
 ; RECOUNT-NEXT:  1: 6
 ; RECOUNT-NEXT:  3: 287884
 ; RECOUNT-NEXT:  4: 287864 _Z3fibi:315608
 ; RECOUNT-NEXT:  15: 23
-; RECOUNT-NEXT:  !Attributes: 2
+; RECOUNT-NEXT:  !Attributes: 6
 ; RECOUNT-NEXT: !Attributes: 2
 ; RECOUNT-NEXT:_Z5funcBi:501213:32
 ; RECOUNT-NEXT: 0: 32
 ; RECOUNT-NEXT: 1: 32 _Z8funcLeafi:20
 ; RECOUNT-NEXT: 3: 12
 ; RECOUNT-NEXT: 1: _Z8funcLeafi:500853
 ; RECOUNT-NEXT:  0: 15
 ; RECOUNT-NEXT:  1: 15
 ; RECOUNT-NEXT:  3: 74946
 ; RECOUNT-NEXT:  4: 74941 _Z3fibi:82359
 ; RECOUNT-NEXT:  10: 23324
 ; RECOUNT-NEXT:  11: 23327 _Z3fibi:25228
 ; RECOUNT-NEXT:  15: 11
-; RECOUNT-NEXT:  !Attributes: 2
+; RECOUNT-NEXT:  !Attributes: 6
 
 ; PROBE:main:1968679:12
 ; PROBE-NEXT: 2: 24
 ; PROBE-NEXT: 3: 28 _Z5funcAi:18
 ; PROBE-NEXT: 3.1: 28 _Z5funcBi:30
 ; PROBE-NEXT: 3: _Z5funcAi:1467398
 ; PROBE-NEXT:  0: 10
 ; PROBE-NEXT:  1: 10 _Z8funcLeafi:11

 ; PROBE-NEXT: !CFGChecksum: 563125815542069
 ; PROBE-NEXT:_Z5funcBi:240:13
 ; PROBE-NEXT: 0: 13
 ; PROBE-NEXT: 1: 13
 ; PROBE-NEXT: !CFGChecksum: 563022570642068
 
 
 ; PREINLINE: ProfileSummarySection {{.*}} Flags: {context-nested}
+
+
+; SUMMARY:      Total functions: 4
+; SUMMARY-NEXT: Maximum function count: 32
+; SUMMARY-NEXT: Maximum block count: 362830
+; SUMMARY-NEXT: Total number of blocks: 16
+; SUMMARY-NEXT: Total count: 772562
+; SUMMARY-NEXT: Detailed summary:
+; SUMMARY-NEXT: 1 blocks with count >= 362830 account for 1 percentage of the total counts.
+; SUMMARY-NEXT: 1 blocks with count >= 362830 account for 10 percentage of the total counts.
+; SUMMARY-NEXT: 1 blocks with count >= 362830 account for 20 percentage of the total counts.
+; SUMMARY-NEXT: 1 blocks with count >= 362830 account for 30 percentage of the total counts.
+; SUMMARY-NEXT: 1 blocks with count >= 362830 account for 40 percentage of the total counts.
+; SUMMARY-NEXT: 2 blocks with count >= 362805 account for 50 percentage of the total counts.
+; SUMMARY-NEXT: 2 blocks with count >= 362805 account for 60 percentage of the total counts.
+; SUMMARY-NEXT: 2 blocks with count >= 362805 account for 70 percentage of the total counts.
+; SUMMARY-NEXT: 2 blocks with count >= 362805 account for 80 percentage of the total counts.
+; SUMMARY-NEXT: 2 blocks with count >= 362805 account for 90 percentage of the total counts.
+; SUMMARY-NEXT: 3 blocks with count >= 23327 account for 95 percentage of the total counts.
+; SUMMARY-NEXT: 4 blocks with count >= 23324 account for 99 percentage of the total counts.
+; SUMMARY-NEXT: 4 blocks with count >= 23324 account for 99.9 percentage of the total counts.
+; SUMMARY-NEXT: 11 blocks with count >= 24 account for 99.99 percentage of the total counts.
+; SUMMARY-NEXT: 16 blocks with count >= 10 account for 99.999 percentage of the total counts.
+; SUMMARY-NEXT: 16 blocks with count >= 10 account for 99.9999 percentage of the total counts.

llvm/test/tools/llvm-profgen/cs-preinline.test

 ; CHECK-PREINL-NEST:     foo:393:0
 ; CHECK-PREINL-NEST-NEXT: 2.1: 14
 ; CHECK-PREINL-NEST-NEXT: 3: 15
 ; CHECK-PREINL-NEST-NEXT: 3.1: 14 bar:14
 ; CHECK-PREINL-NEST-NEXT: 3.2: 1
 ; CHECK-PREINL-NEST-NEXT: 65526: 14
 ; CHECK-PREINL-NEST-NEXT: 3.1: bar:84
 ; CHECK-PREINL-NEST-NEXT:  1: 14
-; CHECK-PREINL-NEST-NEXT:  !Attributes: 3
+; CHECK-PREINL-NEST-NEXT:  !Attributes: 7