Page MenuHomePhabricator

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

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



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.


It can be a regular for loop.


Add a blank like after this.


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.

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


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


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

alex-t added inline comments.Thu, Oct 15, 1:24 PM

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.


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


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.