[PATCH] D142747: [Pseudo Probe] Do not instrument EH blocks.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

In D142747#4086978 <https://reviews.llvm.org/D142747#4086978>, @modimo wrote:

> We've got the same EH-ness propagation code in MFS already: https://reviews.llvm.org/D131824. Consider merging the code?

Oh I just remembered that code. They're basically the same algorithm. But given that IR and MIR routines are usually implemented separately otherwise templated code would be needed, I'd like to keep as is. WDYT?



================
Comment at: llvm/lib/Transforms/IPO/SampleProfileProbe.cpp:300
   for (auto &BB : *F) {
-    BlockProbeIds[&BB] = ++LastProbeId;
+    if (!KnownColdBlocks.contains(&BB))
+      BlockProbeIds[&BB] = ++LastProbeId;
----------------
wenlei wrote:
> hoy wrote:
> > wlei wrote:
> > > hoy wrote:
> > > > wlei wrote:
> > > > > Since this skipped some BBs and might assign different id to the same BB, it seems to me this will cause some profile matching issue? say, it's good for on-demand profiling, but for the profile in the production, it still used the old number of BB.
> > > > This is very good point! Yeah, the downwards compatibility is being screwed up there. I guess we can give blocks the same original id, but then it's always going to be like that. Fortunately this change is so far only needed for llvm-15 and there should be no legacy profiles.
> > > Good to know it's only needed for llvm-15, just I think in this case people need to remind that when upgrading to llvm-15 it should use a new fresh profile, using the profile collected from the old version could cause some regression due to this.  Incremental PGO might detect/mitigate this issue if happens :)
> > Yeah, platform upgrade should always come with a new round of profile refresh.
> Do we require probe Id to be consecutive? If not, skipping blocks we exclude actually isn't a bad idea? It makes probe Id more stable and it gives us flexibility to make changes in this area without breaking compatibility (i.e. we could revert this change in llvm-15, or even exclude other cold blocks later if needed with minimum impact). 

>  (i.e. we could revert this change in llvm-15, or even exclude other cold blocks later if needed with minimum impact).

The concern is valid. Probe ids do not need to be consecutive. I'll make it downwards compatible.



================
Comment at: llvm/lib/Transforms/IPO/SampleProfileProbe.cpp:278-279
+      for (auto *Predecessor : predecessors(Successor)) {
+        if (Predecessor == Successor)
+          continue;
+        if (!EHBlocks.contains(Predecessor)) {
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > Is this necessary? If Predecessor is non-EH, we will break out the loop later; if it's EH, line 274 should have skipped this Predecessor/Successor already. 
> > It's necessary if the successor forms a self-loop. In such case, the self back edge should be ignored.
> What I'm saying is the self back edge will be ignored without this check (see reasoning above), or am I wrong? 
Without this check the self back edge is going to be ignored in a different way such that the block will never be considered EH-only.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142747