[PATCH] D120335: [llvm-profgen] Generating probe-based non-CS profile.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:944
+    // doesn't belong to current context, filter them out.
+    if (!Probe->isBlock() || Count == 0)
+      continue;
----------------
wlei wrote:
> hoy wrote:
> > wlei wrote:
> > > `Probe.isBlock()` check is hoisted from `extractProbesFromRange` to this for CS profile but Why doesn't add this check back for non-CS profile?
> > So we don't count callsite probes here since they are handled in `populateBoundarySamplesWithProbes`. Otherwise they will be double counted.
> > 
> > We don't do this for non-CS probe profile. This is to be consistent with non-CS dwarf implementation where we report zero count in `populateBoundarySamplesForAllFunctions`. I guess I can also do some refactoring to unify the two implementations. 
> > 
> Yeah, I understand for `count==0`. Here I'm asking for whether we should add `Probe->isBlock()` for non-CS probe profile. My understanding is  for body sample, we only accumulate total sample for Block profile? otherwise, the count from callsite probes will affect the total samples.
For non-CS, I'm using `count==0` for probe to be consistent with dwarf, i.e, both block probes and call probes are counted by  `populateBodySamples`.

For CS, only block probes are counted by `populateBodySamples`, call probes are counted by `populateBoundarySamples`.

That's the inconsistency we should probably fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120335