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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:501
+    if (Probe->isDangling()) {
+      FunctionProfile.addBodySamples(Probe->Index, 0, UINT64_MAX);
+      continue;
----------------
wenlei wrote:
> wlei wrote:
> > hoy wrote:
> > > hoy wrote:
> > > > wlei wrote:
> > > > > hoy wrote:
> > > > > > hoy wrote:
> > > > > > > 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?
> > > > > > 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.
> > > > > 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`
> > > > May need to check dangling in `CSProfileGenerator::populateFunctionBoundarySamples` when updating the callsite target samples?
> > > 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`.
> > Thanks for your clarification. Will rebase it after `D95962` landed.
> Since we are using addBodySamples here, can we have probe count not equal to UINT64_MAX after the addition? Can we add assertion? 
Good catch. Changed to update the count only it doesn't exist and add the assertion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96811