[PATCH] D92998: [CSSPGO][llvm-profgen] Pseudo probe based CS profile generation

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/test/tools/llvm-profgen/noinline-cs-pseudoprobe.test:13-14
+; CHECK-NEXT: 1: 15
+; CHECK-NEXT: 2: 0
+; CHECK-NEXT: 3: 0
+; CHECK-NEXT: 4: 15
----------------
wmi wrote:
> hoy wrote:
> > wmi wrote:
> > > wlei wrote:
> > > > wmi wrote:
> > > > > In [main:2 @ foo], probes with 0 count are not dumped. In which case probe with 0 count will be dumped?
> > > > The 0 count is for a dangling probe case to distinguish a missing count, see the ProfileGenerator.cpp:420
> > > > 
> > > > Copied the comments for you to refer:
> > > > 
> > > > ```
> > > >     // Drop the samples collected for a dangling probe since it's misleading.
> > > >     // We still report the probe but with a special zero count. The compiler
> > > >     // won't trust the zero count and will rely on the counts inference
> > > >     // algorithm to get the probe a reasonable count. Note that a zero count is
> > > >     // different from a missing count, where the latter really tells the
> > > >     // compiler that a probe is never executed.
> > > > ```
> > > > Note that a zero count is different from a missing count, where the latter really tells the compiler that a probe is never executed.
> > > 
> > > That is contrary to the debug info based profile where zero count for a line means the line is never executed. Missing count for a line means compiler has to infer the count. Why the probe based profile is implemented in such way?
> > Good question. The current counts inference algorithm in sample profile loader doesn't seem to differentiate a missing sample from a zero sample. For example, in `SampleProfileLoader::propagateThroughEdges`, accesses to `BlockWeights` are not preceded with a check. Therefore a missing entry in `BlockWeights` will be created as a zero entry. 
> > 
> > With pseudo probes, a missing sample really means the probe is not executed. A zero sample, on the other hand, wouldn't be created by the profile generator. We are leveraging zero sample to represent a special type of probe, called `dangling` probe, which will need the compiler to infer its count. We were thinking about using UINT64_MAX instead, but UINT64_MAX literally is a legal number of samples.
> > 
> > The compiler change that dangles probes haven't been sent out yet. @wlei I think we should exclude this detail from this change for now. But it's better to discuss here if this approach makes sense.
> > For example, in SampleProfileLoader::propagateThroughEdges, accesses to BlockWeights are not preceded with a check. Therefore a missing entry in BlockWeights will be created as a zero entry.
> 
> IIRC, VisitedBlocks is used to differentiate missing count with zero count BB.
> 
> > With pseudo probes, a missing sample really means the probe is not executed. 
> > We are leveraging zero sample to represent a special type of probe, called dangling probe, which will need the compiler to infer its count.
> 
> I think we discussed it in another patch so I understand for probe, a missing sample means the probe is not executed. It is a little weird to leverage zero sample to tell compiler to infer the count. UINT64_MAX looks better to me because there cannot be any real sample counter with value being UINT64_MAX. In addition, using zero is misleading when we read the profile -- for a location which has zero count, it is actually not zero from compiler's perspective.
> 
> 
> IIRC, VisitedBlocks is used to differentiate missing count with zero count BB.
Thanks for pointing that out. I misread that code.

Sounds like in order to reuse the existing inference algorithm, we will need to give probes without any samples an explicit zero count in `BlockWeights`. 

Agreed that using `UINT64_MAX` looks more clear than using zero.

 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92998