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

Mon Jun 1 13:33:16 PDT 2020

hhb added a comment.

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...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80510