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

Fri Jan 27 12:00:53 PST 2023

wenlei 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;
----------------
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). 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142747