[PATCH] D63972: [PowerPC] Do the Early Return for the li and unconditional branch

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:2741
+        if (((TBB == &*F->begin()) || !std::prev(I)->isSuccessor(TBB)) &&
+             (TailDup.isSimpleBB(ChainBB) || (TBB->pred_size() == 1))) {
+          TII->removeBranch(*ChainBB);
----------------
ZhangKang wrote:
> efriedma wrote:
> > If you have a block that ends in an unconditional branch, and the successor block has exactly one predecessor and no fallthrough successors, you can merge the two blocks; that's straightforward.  But I don't see how the isSimpleBB check is related to that, and the isSuccessor() check seems redundant.
> Here, I will move all the instructions of TBB to ChainBB and remove TBB, if `TBB->pred_size() > 1`, I must ensure that ChainBB has only an unconditonal branch.
> For below example:
> ```
> BB:
>    XOR 3, 3, 4
>    B TBB
> ...
> ChainBB:
>    LI 3, 1
>    B TBB
> ...
> TBB:
>    ADD 3, 3, 4
>    BLR
> ```
> TBB has two predecessors, I cannot move all the instructions of TBB to ChainBB and remove TBB.  Of course, if I reserve TBB, I can also do the tail-duplication, but it will increase the size of program or has other side-effect, for those complex pattern, it need to be done by the `tail-duplicaton` pass. And here, I only optimize the pattern may be created by the `block-placement` pass.
> 
> But if ChainBB has only uncontional branch, I can do it.
> ```
> BB:
>    XOR 3, 3, 4
>    B TBB
> ...
> ChainBB:
>    B TBB
> ...
> TBB:
>    ADD 3, 3, 4
>    BLR
> ```
> Can be optimized to:
> ```
> BB:
>    XOR 3, 3, 4
>    B ChainBB
> ...
> ChainBB:
>    ADD 3, 3, 4
>    BLR
> ```
> 
> Also `TBB->pred_size() == 1`, the only one predcessor is ChainBB, I will not care whether ChainBB has only one branch.
Oh, I didn't realize you were modifying the predecessors of TBB to branch to ChainBB instead.  That's fine, I guess.  Please update the comment with a more complete description of the transform, and please include tests which cover both the single-predecessor and multiple-predecessor transforms.

Also, a minor suggestion: instead of `std::prev(I)->isSuccessor(TBB)`, maybe call  `std::prev(I)->canFallThrough()` instead?


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

https://reviews.llvm.org/D63972