[CSSPGO][llvm-profgen] Change sample count of dangling probe in llvm-profgen

[wlei]

Summary

Change to use UINT64_MAX for sample count of dangling probe from llvm-profgen side. The compiler will identify this for further process, please refer to https://reviews.llvm.org/D95962 for the details.

[wenlei]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

512

Do we still want to count samples on dangling probe towards total samples? Without deduplication for dangling probes, we could have multiple dangling probes in the same block and counting samples covering these probe repeatedly may cause bloated total samples?

[hoy]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

512

Good point. Since the samples collected on dangling probes are invalid, I would not count it against total samples.

[wlei]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

512

I see, thanks for your suggestion.

How about the count for isEntry for line506, I guess it's the original count not the zero? Or we won't have dangling probe which is the entry probe?

[hoy]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

512

That's a good point. It's possible to have a dangling entry probe and we should bail out here. Or we can just return right after adding body samples for dangling probes.

[wlei]

remove count towards total sample for dangling probes

llvm/tools/llvm-profgen/ProfileGenerator.cpp

512

I see. Since the sample count are invalid, we shouldn't count for total sample nor using it to infer inliner's sample count. Thanks for your clarification.

[hoy]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

501

Actually I UINT64_MAX may cause overflow to total samples. Even if it doesn't, profile merge may overflow too. That's one of the reasons we were using 0 as a special count. @wmi What do you think about keeping using 0?

[hoy]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

501

I talked to other folks and they like using UINT64_MAX instead of 0 to be less confusing. @wlei we may need to fix the places that accumulate total samples and set entry count to not using UINT64_MAX as the sample count.

[wlei]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

501

Thanks for the sharing. So for profgen side, we can keep this patch, right? see line 501, it only adds the body sample and doesn't accumulate the total sample.

For compiler side, we need to avoid the addition when meeting the UINT64_MAX

[hoy]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

501

May need to check dangling in CSProfileGenerator::populateFunctionBoundarySamples when updating the callsite target samples?

[hoy]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

501

I was wrong. The current implementation looks good. We are not propagating UINT64_MAX anywhere except for adding it as a body sample.

On the compiler side, the merging of dangling probes are taken care of here: https://reviews.llvm.org/D95962 .

Please rebase this change on top of D95962 to share the definition of FunctionSamples::InvalidProbeCount.

[wlei]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

501

Thanks for your clarification. Will rebase it after D95962 landed.

[wenlei]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

501

Since we are using addBodySamples here, can we have probe count not equal to UINT64_MAX after the addition? Can we add assertion?

[wlei]

address reviewers' feedback

[wlei]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

501

Good catch. Changed to update the count only it doesn't exist and add the assertion.

[wenlei]

lgtm.

[hoy]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

501

This is a good point. Probes with same ID may have both dangling and non-dangling copies. Using SampleRecord::merge should be safe, like R->merge(unctionSamples::InvalidProbeCount, 1)

[wlei]

use merge function to update body samples for probe

[wlei]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

501

Thanks for your reminding. I see, then we should also consider non-dangling case, so added a helper function addBodySamplesForProbe for this. See whether this looks good for you?

[hoy]

lgtm, thx!

llvm/test/tools/llvm-profgen/inline-cs-pseudoprobe.test

 ; RUN: llvm-profgen --perfscript=%S/Inputs/inline-cs-pseudoprobe.perfscript --binary=%S/Inputs/inline-cs-pseudoprobe.perfbin --output=%t --show-unwinder-output --csprof-cold-thres=0 | FileCheck %s --check-prefix=CHECK-UNWINDER
 ; RUN: FileCheck %s --input-file %t
 
 ; CHECK:     [main:2 @ foo]:74:0
 ; CHECK-NEXT: 2: 15
 ; CHECK-NEXT: 3: 15
 ; CHECK-NEXT: 4: 14
 ; CHECK-NEXT: 5: 1
 ; CHECK-NEXT: 6: 15
 ; CHECK-NEXT: 8: 14 bar:14
 ; CHECK-NEXT: !CFGChecksum: 138950591924
-; CHECK-NEXT:[main:2 @ foo:8 @ bar]:56:14
+; CHECK-NEXT:[main:2 @ foo:8 @ bar]:28:14
 ; CHECK-NEXT: 1: 14
-; CHECK-NEXT: 2: 14
-; CHECK-NEXT: 3: 14
+; CHECK-NEXT: 2: 18446744073709551615
+; CHECK-NEXT: 3: 18446744073709551615
 ; CHECK-NEXT: 4: 14
 ; CHECK-NEXT: !CFGChecksum: 72617220756
 
 
 ; CHECK-UNWINDER:      Binary(inline-cs-pseudoprobe.perfbin)'s Range Counter:
 ; CHECK-UNWINDER-EMPTY:
 ; CHECK-UNWINDER-NEXT:   (800, 858): 1
 ; CHECK-UNWINDER-NEXT:   (80e, 82b): 1

llvm/test/tools/llvm-profgen/noinline-cs-pseudoprobe.test

 ; RUN: llvm-profgen --perfscript=%S/Inputs/noinline-cs-pseudoprobe.perfscript --binary=%S/Inputs/noinline-cs-pseudoprobe.perfbin --output=%t --show-unwinder-output --csprof-cold-thres=0 | FileCheck %s --check-prefix=CHECK-UNWINDER
 ; RUN: FileCheck %s --input-file %t
 
 ; CHECK:     [main:2 @ foo]:75:0
 ; CHECK-NEXT: 2: 15
 ; CHECK-NEXT: 3: 15
 ; CHECK-NEXT: 4: 15
 ; CHECK-NEXT: 6: 15
 ; CHECK-NEXT: 8: 15 bar:15
 ; CHECK-NEXT: !CFGChecksum: 138950591924
-; CHECK-NEXT:[main:2 @ foo:8 @ bar]:60:15
+; CHECK-NEXT:[main:2 @ foo:8 @ bar]:30:15
 ; CHECK-NEXT: 1: 15
-; CHECK-NEXT: 2: 15
-; CHECK-NEXT: 3: 15
+; CHECK-NEXT: 2: 18446744073709551615
+; CHECK-NEXT: 3: 18446744073709551615
 ; CHECK-NEXT: 4: 15
 ; CHECK-NEXT: !CFGChecksum: 72617220756
 
 
 ; CHECK-UNWINDER:      Binary(noinline-cs-pseudoprobe.perfbin)'s Range Counter:
 ; CHECK-UNWINDER-NEXT: main:2
 ; CHECK-UNWINDER-NEXT:   (79e, 7bf): 15
 ; CHECK-UNWINDER-NEXT:   (7c4, 7cf): 15

llvm/tools/llvm-profgen/ProfileGenerator.cpp

void PseudoProbeCSProfileGenerator::populateBodySamplesWithProbes(
   // the Address2ProbeMap
   extractProbesFromRange(RangeCounter, ProbeCounter, Binary);
   for (auto PI : ProbeCounter) {
     const PseudoProbe *Probe = PI.first;
     uint64_t Count = PI.second;
     FunctionSamples &FunctionProfile =
         getFunctionProfileForLeafProbe(ContextStrStack, Probe, Binary);
 
+    // Use InvalidProbeCount(UINT64_MAX) to mark sample count for a dangling
+    // probe. Dangling probes are the probes associated to an empty block. With
+    // this place holder, sample count on dangling probe will not be trusted by
+    // the compiler and it will rely on the counts inference algorithm to get
+    // the probe a reasonable count.
+    if (Probe->isDangling()) {
+      ErrorOr<uint64_t> R = FunctionProfile.findSamplesAt(Probe->Index, 0);
+      if (!R) {
+        FunctionProfile.addBodySamples(Probe->Index, 0,
+                                       FunctionSamples::InvalidProbeCount);
+      } else {
+        assert(R.get() == FunctionSamples::InvalidProbeCount &&
+               "Dangling probe count should be UINT64_MAX.");
+      }
+      continue;
+    }
     FunctionProfile.addBodySamples(Probe->Index, 0, Count);
     FunctionProfile.addTotalSamples(Count);
     if (Probe->isEntry()) {
       FunctionProfile.addHeadSamples(Count);
       // Look up for the caller's function profile
       const auto *InlinerDesc = Binary->getInlinerDescForProbe(Probe);
       if (InlinerDesc != nullptr) {
         // Since the context id will be compressed, we have to use callee's
         // context id to infer caller's context id to ensure they share the
         // same context prefix.