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

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

Thanks for working on this. Among all deleted probes, if the majority of them are actually dangling (i.e removed due to hoisting, merging like optimization as opposed to dce), it makes sense to default to dangling and explicitly marking non-dangling. Previously we assumed that majority of deleted probes are not dangled, hence we choose to mark dangled ones explicitly.

It seems that our assumption has been wrong, which is why this change actually improves profile quality.

> This enabled dangling probe to be represented by both INT64_MAX and none (missing entry in profile).

Why do we still want to allow INT64_MAX representation of dangling probe? Would it be cleaner to only use missing probe to represent dangled probes?

> Reducing compiler work to track dangling probes. However, for probes that are real dead and removed, we still need the compiler to identify them so that they can be reported as zero-count, instead of mistreated as dangling probes.

While explicitly marking real dead probes can be a follow up patch. Should we remove `moveAndDanglePseudoProbes` completely together with this patch?

Moreover, since we treat missing probe the same as dangling probe in getProbeWeight and hence in profile inference, it seems we no longer need the concept of dangling. All we need is unknown (missing) vs known (explicitly marked 0 in the future for dead code, or probe with a real non-zero count). The term dangling come from the pass1 optimization side, dangle reflects how a probe looks after its containing block is removed, now if we don't need to track such probes from pass1 optimization side, it seems the whole concept of dangling can go away to simply things?



================
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) {
----------------
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. 


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:558
       for (auto *Probe : I.first->getProbes()) {
-        if (Probe->isDangling()) {
-          FunctionProfile->addBodySamplesForProbe(
-              Probe->Index, FunctionSamples::InvalidProbeCount);
-        }
+        if (!Probe->isDangling())
+          FunctionProfile->addBodySamplesForProbe(Probe->Index, 0);
----------------
Not that we don't do anything for dangling probes, can we remove the `moveAndDanglePseudoProbes` from compiler too? These will be missing, and not having `0` without any special handling. 

Furthermore, this is the only place we generate `InvalidProbeCount`, with this change, all the special casing for `InvalidProbeCount` can be removed too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104129