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

[Zhang Kang via Phabricator via llvm-commits]

ZhangKang marked 2 inline comments as done.
ZhangKang added inline comments.
Herald added a subscriber: shchenz.


================
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);
----------------
efriedma wrote:
> 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?
Ok, I will modify the comment and add the test.


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

https://reviews.llvm.org/D63972