[PATCH] D134152: [SimplifyCFG][TranformUtils]Do not simplify away a trivial basic block if both this block and at least one of its predecessors are loop latches.

Wed Sep 28 00:34:36 PDT 2022

mingmingl added a comment.

In D134152#3819747 <https://reviews.llvm.org/D134152#3819747>, @davidxl wrote:

> Perhaps add a TODO in the comment part for the longer term solution suggested by nikic. Otherwise LGTM

Thanks. Added a FIXME and quoted nikic's comment there.

In D134152#3809106 <https://reviews.llvm.org/D134152#3809106>, @nikic wrote:

> The current representation of loop metadata (using a loop latch terminator attachment) is known to be fundamentally broken. Loop latches are not uniquely associated with loops (both in that a latch can be part of multiple loops and a loop may have multiple latches). Loop headers are. The solution to this problem is also known: Add support for basic block metadata, and attach loop metadata to the loop header.
>
> I don't think adding these kinds of hacks is going to scale much further -- someone needs to actually address the root problem.

Thanks for the input! It makes sense to me that current representation won't scale well (from the history around this line, to first count for loop headers, then loop latches, then about the number of predecessors).

I didn't reply though, after davidxl@ beat me to it (that fixing loop metadata representation is a long shot to preserve the inner-loop metadata).

For efficiency considerations,

1. Regarding the scope, this patch only makes a difference when 'BB' has only one predecessor, in the sense that ToT won't simplify it away when 'BB' has more than two predecessors (as a loop latch)
2. branch-folder in the codegen pipeline <https://github.com/llvm/llvm-project/blob/daf51682b439096eb62b6bb378b3a8f55a593e55/llvm/lib/CodeGen/BranchFolding.cpp#L9> is likely forward branches to unconditional branches to destination directly if this trivial block presents by then.
3. I did a loadtest on one representative internal benchmark and didn't see regressions.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134152