[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

121

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

121

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

121

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

121

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

 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Support/Allocator.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cstdint>
 #include <list>
 #include <map>
 #include <set>
 #include <sstream>
 #include <string>
 #include <system_error>
 #include <unordered_map>
 #include <utility>
 
+extern llvm::cl::opt<bool> GenCSNestedProfile;
+extern llvm::cl::opt<bool> GenerateMergedBaseProfiles;
+
 namespace llvm {
 
 const std::error_category &sampleprof_category();
 
 enum class sampleprof_error {
   success = 0,
   bad_magic,
   unsupported_version,

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();
   }
   for (const auto &I : FS.getBodySamples()) {
     uint64_t Count = I.second.getSamples();
       addCount(Count);
   }
+
+  // Do not recount callee samples if they are already merged into their base
+  // profiles. This can happen during the generation of CS nested profile.
+  if (!GenCSNestedProfile || !GenerateMergedBaseProfiles)
     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);

llvm/lib/ProfileData/SampleProf.cpp

cl::desc("Cutoff value about how many symbols in profile symbol list "
              "will be used. This is very useful for performance debugging"));
 
 cl::opt<bool> GenerateMergedBaseProfiles(
     "generate-merged-base-profiles", cl::init(true), cl::ZeroOrMore,
     cl::desc("When generating nested context-sensitive profiles, always "
              "generate extra base profile for function with all its context "
              "profiles merged into it."));
 
+cl::opt<bool> GenCSNestedProfile(
+    "gen-cs-nested-profile", cl::Hidden, cl::init(false),
+    cl::desc("Generate nested function profiles for CSSPGO"));
+
 namespace llvm {
 namespace sampleprof {
 SampleProfileFormat FunctionSamples::Format;
 bool FunctionSamples::ProfileIsProbeBased = false;
 bool FunctionSamples::ProfileIsCSFlat = false;
 bool FunctionSamples::ProfileIsCSNested = false;
 bool FunctionSamples::UseMD5 = false;
 bool FunctionSamples::HasUniqSuffix = true;

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 --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
 
 
 ; 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

 ; 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/tools/llvm-profdata/llvm-profdata.cpp

cl::opt<unsigned> SupplMinSizeThreshold(
       "suppl-min-size-threshold", cl::init(10), cl::Hidden,
       cl::desc("If the size of a function is smaller than the threshold, "
                "assume it can be inlined by PGO early inliner and it won't "
                "be adjusted based on sample profile."));
   cl::opt<unsigned> InstrProfColdThreshold(
       "instr-prof-cold-threshold", cl::init(0), cl::Hidden,
       cl::desc("User specified cold threshold for instr profile which will "
                "override the cold threshold got from profile summary. "));
-  cl::opt<bool> GenCSNestedProfile(
-      "gen-cs-nested-profile", cl::Hidden, cl::init(false),
-      cl::desc("Generate nested function profiles for CSSPGO"));
   cl::opt<std::string> DebugInfoFilename(
       "debug-info", cl::init(""),
       cl::desc("Use the provided debug info to correlate the raw profile."));
 
   cl::ParseCommandLineOptions(argc, argv, "LLVM profile data merger\n");
 
   WeightedFileVector WeightedInputs;
   for (StringRef Filename : InputFilenames)

llvm/tools/llvm-profgen/ProfileGenerator.cpp

 static cl::opt<bool> UpdateTotalSamples(
     "update-total-samples", llvm::cl::init(false),
     llvm::cl::desc(
         "Update total samples by accumulating all its body samples."),
     llvm::cl::Optional);
 
 extern cl::opt<int> ProfileSummaryCutoffHot;
 
-static cl::opt<bool> GenCSNestedProfile(
-    "gen-cs-nested-profile", cl::Hidden, cl::init(false),
-    cl::desc("Generate nested function profiles for CSSPGO"));
-
 using namespace llvm;
 using namespace sampleprof;
 
 namespace llvm {
 namespace sampleprof {
 
 // Initialize the MaxCompressionSize to -1 which means no size limit
 int32_t CSProfileGenerator::MaxCompressionSize = -1;