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

Fri Jan 27 11:34:23 PST 2023

hoy added inline comments.

================
Comment at: llvm/lib/Transforms/IPO/SampleProfileProbe.cpp:300
   for (auto &BB : *F) {
-    BlockProbeIds[&BB] = ++LastProbeId;
+    if (!KnownColdBlocks.contains(&BB))
+      BlockProbeIds[&BB] = ++LastProbeId;
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142747