[PATCH] D100235: [CSSPGO][llvm-profgen] Always report dangling probes for frames with real samples.

[Wenlei He via Phabricator via llvm-commits]

wenlei accepted this revision.
wenlei added a comment.
This revision is now accepted and ready to land.

lgtm, thanks!



================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:518
+    // Ignore dangling probes since they will be reported later if needed.
+    if (Probe->isDangling())
+      continue;
----------------
wlei wrote:
> Try to understand this.
> 
> If pass2's probe is dangling, then return 0 no matter what in the profile(pass1);
> 
> If pass2's probe is not dangling and reading from profile gets a dangling probe(pass1), then return invalid count(UINT64_MAX), (this case happen because later the BB of the probe will be removed)
> 
> is that right? 
> 
> So this change make sure all the future removed BB will be identified as dangling so that the inference algorithm can work on this?
> 
> is this issue also coming from the rebalancing algorithm? 
> 
> like we have TrueBranch :0 and FalseBranch: 0, the rebalancing algorithm can't work on this.
> 
> but if we have TrueBranch:0 and FalseBranch:dangling, then it can be rebalanced.
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
By return, you meant the return value of `SampleProfileLoader::getProbeWeight`?

For inference, I think what matters is whether probe is dangling from pass1. 


================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.h:48
   };
-  std::unordered_map<InlineSite, std::unique_ptr<PseudoProbeInlineTree>,
-                     InlineSiteHash>
-      Children;
+  using ChildrenType =
+      std::unordered_map<InlineSite, std::unique_ptr<PseudoProbeInlineTree>,
----------------
nit: a more reflective type name, something like `InlinedProbesMap`, `InlinedProbeTreeMap`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100235