Page MenuHomePhabricator

[AMDGPU] SILowerControlFlow::removeMBBifRedundant should not try to change MBB layout if it can fallthrough
ClosedPublic

Authored by alex-t on Wed, Oct 14, 8:39 AM.

Details

Summary

removeMBBifRedundant normally tries to keep predecessors fallthrough when removing redundant MBB.

It has to change MBBs layout to keep the new successor to immediately follow the predecessor of removed MBB.
It only may be allowed in case the new successor itself has no successors to which it fall through.

Diff Detail

Event Timeline

alex-t created this revision.Wed, Oct 14, 8:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Oct 14, 8:39 AM
alex-t requested review of this revision.Wed, Oct 14, 8:39 AM

Please address couple clang-tidy comments.

llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp
693

It can be a regular for loop.

705

Add a blank like after this.

llvm/test/CodeGen/AMDGPU/collapse-endcf.mir
643

This sentence clearly misses some punctuation.

alex-t updated this revision to Diff 298194.Wed, Oct 14, 11:08 AM

Changed according the reviewer request.

alex-t marked 3 inline comments as done.Wed, Oct 14, 11:09 AM
This revision is now accepted and ready to land.Wed, Oct 14, 11:12 AM
alex-t updated this revision to Diff 298339.Thu, Oct 15, 3:34 AM

minor bugfix

foad added a subscriber: foad.Thu, Oct 15, 5:11 AM
foad added inline comments.
llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp
689–690

You don't need to iterate to find the layout successor, just use std::next or similar.

706

Remove this and just return early if it's not redundant?

713–716

Why do we need the SmallVector? Why not just iterate over MBB.predecessors()?

alex-t added inline comments.Thu, Oct 15, 1:24 PM
llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp
689–690

This is not to find the layout successor. This check if one of the successors is also layout succesor.
Nevertheless, you a right - the loop is not necessary.

706

Thanks for good suggestion. I will address NFC in separate commit soon.
This one is already tested and need to be submitted asap.

713–716

P->ReplaceUsesOfBlockWith(&MBB, Succ) erases P from MBB.predecessors, i.e. calls std::vector.erase(P)
Since it is inside the ReplaceUsesOfBlockWith call I cannot decrement iterator afeter passing it in but before erase execution ("erase(P--)") to keep it valid. So I decided that using separate vector is easier and does not bring too much overhead - it is usually small.