[PATCH] D129205: [BasicBlockUtils] Allow splitting predecessors with callbr terminators

Thu Jul 7 06:59:13 PDT 2022

nikic added a comment.

In D129205#3635611 <https://reviews.llvm.org/D129205#3635611>, @jyknight wrote:

> In D129205#3633528 <https://reviews.llvm.org/D129205#3633528>, @efriedma wrote:
>
>> If we don't support using arbitrary blockaddresses as destinations, can we stop using blockaddress to represent the destinations?  The presence of blockaddress blocks optimizations even if we aren't imposing the full control flow restrictions.
>
> Just to be clear: the original intent of the LLVM IR design here was that it would be valid to store a blockaddress into some memory, and then call that stored address inside a callbr asm. Thus the callbr asm might or might not even have _any_ blockaddress inputs itself -- that a blockaddress was an argument to the callbr had no special significance vs the blockaddress anywhere else. All possible destinations the callbr could jump to must be listed as indirect labels, however.
>
> I do think that was a reasonable design choice to make, but we've already moved halfway away from it in a sort of ad-hoc fashion...

Yes, that was the original design, which essentially made callbr the same as indirectbr. I assume that this design was later discarded because this generality is not necessary (or particularly useful) to support asm goto. At the same time, giving callbr the same semantics as indirectbr prevents many CFG optimizations and increases the chances of miscompiles and assertion failures for any CFG transform that does not have explicit handling for callbr. It's a reasonable design in principle, but not probably not a good tradeoff for its intended use-case.

I've put up https://reviews.llvm.org/D129288 to decouple `callbr` representation from blockaddresses entirely (on the IR level), which should allow us to remove the remaining callbr-related special cases, and also make it clearer that callbr cannot be used with arbitrary blockaddresses.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129205