[PATCH] D80510: Update BFI when handling inlined landing / eh pad

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added a comment.

In D80510#2067235 <https://reviews.llvm.org/D80510#2067235>, @hhb wrote:

> In D80510#2065193 <https://reviews.llvm.org/D80510#2065193>, @mtrofin wrote:
>
> > Shouldn't this happen as part of BasicBlock::splitBasicBlock? Or probably BasicBlockUtils.cpp (llvm::SplitBlock).
> >
> > There should also probably be a call to BlockFrequencyInfoImpl::forgetBlock that we're missing somewhere, that triggers this in the first place.
> >
> > This is a good find, and I think we should identify the places that introduce fragility (i.e. deletions of BBs without telling BFI to forget - there seems to have been a design about this, BFICallbackVH, can't seem to find uses of it though)
>
>
> I do want to put this into splitBasicBlock and other related functions. But it seems too intrusive. Consider the situation that all analysis passes adding a parameter to splitBasicBlock...
>
> There are probably many places where forgetBlock is missing. Given that forgetBlock is not even exposed through BlockFrequencyInfo.
>
> But yes this fix is not reliable. All places where a new block is added, need to also update BFI. There's no way to guarantee that.
>
> A couple of ways to fix:
>
> 1. Remove BFI "defaults to 0" behavior. This way if a new block is not added to BFI, compiler will likely crash and we will notice. But many other places depends on default 0 and difficult to change.
> 2. Make sure removed blocks are also removed from BFI. We may need some event mechanism. I'll check BFICallbackVH.
> 3. Use some other identifier that will not be reused, instead of pointer. It will be a huge change.
> 4. Add a unit test and enable -check-bfi-unknown-block-queries for it. This is probably the only viable way to go...


I believe point 2 is critically important, and points 1 or 4 are "nice to have": we need to remove the source of non-determinism due to BB pointers being used as keys, and not removed. If a map has a key value "p1" from a BB1, and BB1 is deleted, it is possible, and non-deterministic, that a BB2 be allocated at p, and this would appear as a known BB. -check-bfi-unknown-block-queries would only help with the resulting BB from a split not having BFI data.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80510