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

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

It would have been better to start a new review for this, instead of reopening one that has already been pushed.



================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:686
+    auto *S = B->getNextNode();
+    if (S && B->isSuccessor(S)) {
+      // The only fallthrough candidate
----------------
Return early if this condition is *not* true.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:693
+          // We have unoptimized branch to layout successor
+          break;
       }
----------------
Return early here.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:695
       }
+      if (I == E)
+        Ret = S;
----------------
Just return S. Then there's no need for Ret.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:705
   }
-  if (Redundant) {
-    MachineBasicBlock *Succ = *MBB.succ_begin();
-    SmallVector<MachineBasicBlock *, 2> Preds(MBB.predecessors());
-    MachineBasicBlock *FallThrough = nullptr;
-    for (auto P : Preds) {
-      if (getFallThroughSucc(P) == &MBB)
-        FallThrough = P;
-      P->ReplaceUsesOfBlockWith(&MBB, Succ);
-    }
-    MBB.removeSuccessor(Succ);
-    if (LIS) {
-      for (auto &I : MBB.instrs())
-        LIS->RemoveMachineInstrFromMaps(I);
-    }
-    MBB.clear();
-    MBB.eraseFromParent();
-    if (FallThrough && !FallThrough->isLayoutSuccessor(Succ)) {
-      MachineFunction *MF = FallThrough->getParent();
-      if (!getFallThroughSucc(Succ)) {
-        MachineFunction::iterator InsertPt(FallThrough->getNextNode());
-        MF->splice(InsertPt, Succ);
-      } else
-        BuildMI(*FallThrough, FallThrough->end(),
-                FallThrough->findBranchDebugLoc(), TII->get(AMDGPU::S_BRANCH))
-            .addMBB(Succ);
-    }
-
-    return true;
+  MachineBasicBlock *Succ = *MBB.succ_begin();
+  MachineBasicBlock *FallThrough = nullptr;
----------------
Assert that MBB has exactly one successor?


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:711
+    if (GetFallThroughSucc(P) == &MBB)
+      FallThrough = P;
+    P->ReplaceUsesOfBlockWith(&MBB, Succ);
----------------
Assert !FallThrough here (i.e. assert that there is at most one block that falls through into MBB)?


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:724
+    MachineBasicBlock *InsertBefore = FallThrough->getNextNode();
+    if (InsertBefore && !GetFallThroughSucc(Succ)) {
+      MachineFunction::iterator InsertPt(InsertBefore);
----------------
Why do you have to test `InsertBefore` here? Can't you always use std::next(FallThrough) as the insert point? Even if FallThrough is the last BB, the insert point should be valid -- it'll be MF.end().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89397