[PATCH] D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes.

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

In D104129#2819438 <https://reviews.llvm.org/D104129#2819438>, @wenlei wrote:

>> Be consistent with non-probe profile.
>
> AutoFDO profile usually does not fill in zeros. counts below a certain threshold is omitted to save profile size.
>
> Now in order to differentiate from unknown, the approach taken here is to always mark any known count including zero. In that sense this is different from AutoFDO (actually before this change, dangling probes are marked so zero counts can be omitted which is closer to AutoFDO for representing sparse profile).
>
>> Improving counts quality by respecting the counts already collected on the non-dangling sibling of the danling probe
>
> nit on the wording, sibling leads others to think it's a different probe under the same probe inline tree. Here, it's really just copies of the same probe (probes sharing the same Id).

Please update description: 1) not using the term sibling probe, 2), remove or update "Be consistent with non-probe profile.". 3), update "be represented by both INT64_MAX and none (missing entry in profile)"



================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:554
+    // instead of report dangling probes. This is mainly for saving the profile
+    // size. Dangling sampling can now be represented by a none/missing entry.
     for (auto &I : FrameSamples) {
----------------
hoy wrote:
> wenlei wrote:
> > Mainly for saving profile size can be a misleading comment. The main benefits we see are: 1) better profile quality when we default all missing probe to be unknown (vs previously we only treat marked probes as unknown), since we have more unknown probes than dead probes. 2) allowing probe with count to take precedence over dangling ones when merging. 
> > 
> > If doing this regress profile quality, we probably won't do it even if it leads to smaller profile size. If we go further down this route, we may end up removing `InvalidProbeCount` altogether, then saving profile size can be confusing to others as others wouldn't know where we came from. 
> Sounds good. Will update comment.
If you mention dangling in the comments, make sure it will be updated when we remove the concept of dangling altogether. 

What I actually meant is that we don't need to explain the benefit comparing to the old approach since the old approach will be completely gone. Just comment it as if we arrive at this from a clean slate, in which case there should be no place for dangling probe. The extra context is good for commit message, but it's not so relevant for comment. Here we're simply marking probes that don't have any sample hits with zero count so we can differentiate probe with known count from unknown (deleted or other reason). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104129