[llvm] Branch folding: Avoid infinite loop with indirect target blocks (PR #96956)

[via llvm-commits]

v01dXYZ wrote:

I would like to replace the `PrevBB->isSuccessor(&*FallThrough)` with `(PrevTBB == &*FallThrough) || (PrevFBB == &*FallThrough)`. I think it's the cause of the infinite loop. 

It seems to be related to the fact that `analyzeBranch(PrevBB, ...)` ignore the instructions for `INVOKE`/`INLINEASM_BR`  while those constructs still affect the successors/predecessors of the blocks. That's because those instructions are not terminators.

Using `isSuccessor` to check if a Block is a target of the terminating branch is faulty.

If the return value is as if the branch is analyzable, given a successor of `PrevBB`, it does not mean the branch refers to this specific successor. BTW if the branch is analyzable, the branch exists (`PrevBB` could not be branchless because of previous checks ensuring `PrevBB` does not fallthough).

For example, in the case of `PrevBB` having a JMP to `MBB` at the end (EBB0, EBB1 are blocks only used by an `INLINEASM_BR`).  When using `PrevBB->isSuccessor(&*FallThrough)`, the transform is applied and `MBB` is moved at the end. The next two iterations will do the same for EBB0, EBB1. After the third iteration, we are back at the initial placement. Thus, the infinite loop.

```
    +----------+               +----------+
    | PrevBB   |--+            | PrevBB   |--+
    | JMP MBB  |  |            | JMP MBB  |  |
    +----------+  |            +----------+  |
         |        |          _______/        |
         v        |         /                |
    +----------+  |         |  +----------+  |
    |   MBB    |  |         |  | EBB0     |<-+
    +----------+  |   ==>   |  +----------+  |
                  |         |                |
    +----------+  |         |  +----------+  |
    | EBB0     |<-+         |  | EBB0     |<-+
    +----------+  |         |  +----------+
                  |         |
    +----------+  |         |  +----------+
    | EBB1     |<-+         +->| MBB      |
    +----------+               +----------+
```

`analyzeBranch` returns `false` and set only `PrevTBB` pointing to `MBB` while not setting `PrevFBB`. Since `PrevTBB != FallThrough`, the transform is not applied when using `PrevTBB`/`PrevFBB` instead of `PrevBB->isSuccessor(...)`.

Keeping the check about `FallThrough` not being a EHPad block is not absolutely necessary, but it prevents uselessly calling the expensive `analyzeBranch` since a catchpad could only be reached from an `INVOKE`.

As Indirect Targets could be reached from a branch, the check about not being an Indirect Target is conservative to prevent the infinite loop from reappearing.


https://github.com/llvm/llvm-project/pull/96956