[PATCH] D142747: [Pseudo Probe] Do not instrument EH blocks.
Hongtao Yu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jan 27 12:24:57 PST 2023
hoy added inline comments.
================
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:
> > > 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.
> Ok, but if the purpose is to prevent predecessor from cycle to be considered non-EH. How about landingpad->A->B->A, where there's no self edge to A, but B would prevent A from being marked EH? The need for self cycle special handling looks fishy.. We either handle all cycles, or don't..
Yeah, I just remembered the issue was also raised up in https://reviews.llvm.org/D131824 so we ended up with a more complicated implementation. Sounds like I should reuse that code here. Then making a shared version makes sense.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142747/new/
https://reviews.llvm.org/D142747
More information about the llvm-commits
mailing list