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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
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:
> > 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). 
> I can remove the dangling term now, but feel that we need another term for the missing probes, or to mention why zero is reported. Maybe something like "Reporting zero for non-executed probes. The compiler will infer counts for deleted probes"? 
Yeah something like "Explicitly assign zero count for remaining probes without sample hits. This is to differentiate from removed probes whose count is unknown and will be inferred in the compiler". Missing probe is probe optimized away, it should be intuitive. Not sure if we need a special term for that..

One thing is the removal of the term about dangling; the other is we don't need to explain all the benefit when the base will be gone. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104129