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

[Alexander via Phabricator via llvm-commits]

alex-t added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:689-690
+    MachineBasicBlock *Ret = nullptr;
+    for (auto *S : MBB->successors()) {
+      if (MBB->isLayoutSuccessor(S)) {
+        // The only fallthrough candidate
----------------
foad wrote:
> You don't need to iterate to find the layout successor, just use std::next or similar.
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.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:707
+
   bool Redundant = true;
   for (auto &I : MBB.instrs()) {
----------------
foad wrote:
> Remove this and just return early if it's not redundant?
Thanks for good suggestion. I will address NFC in separate commit soon.
This one is already tested and need to be submitted asap.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:714-716
     SmallVector<MachineBasicBlock *, 2> Preds(MBB.predecessors());
+    MachineBasicBlock *FallThrough = nullptr;
     for (auto P : Preds) {
----------------
foad wrote:
> Why do we need the SmallVector? Why not just iterate over MBB.predecessors()?
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89397