[AMDGPU] SILowerControlFlow::removeMBBifRedundant should not try to change MBB layout...
authoralex-t <alexander.timofeev@amd.com>
Wed, 14 Oct 2020 15:34:07 +0000 (18:34 +0300)
committeralex-t <alexander.timofeev@amd.com>
Thu, 15 Oct 2020 20:20:54 +0000 (23:20 +0300)
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.

Reviewed By: rampitec

Differential Revision: https://reviews.llvm.org/D89397

llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp
llvm/test/CodeGen/AMDGPU/collapse-endcf.mir

index a29313d..19fcc6e 100644 (file)
@@ -684,6 +684,25 @@ MachineBasicBlock *SILowerControlFlow::process(MachineInstr &MI) {
 }
 
 bool SILowerControlFlow::removeMBBifRedundant(MachineBasicBlock &MBB) {
+  auto getFallThroughSucc = [=](MachineBasicBlock * MBB) {
+    MachineBasicBlock *Ret = nullptr;
+    for (auto S : MBB->successors()) {
+      if (MBB->isLayoutSuccessor(S)) {
+        // The only fallthrough candidate
+        MachineBasicBlock::iterator I(MBB->getFirstInstrTerminator());
+        while (I != MBB->end()) {
+          if (I->isBranch() && TII->getBranchDestBlock(*I) == S)
+            // We have unoptimized branch to layout successor
+            break;
+          I++;
+        }
+        if (I == MBB->end())
+          Ret = S;
+        break;
+      }
+    }
+    return Ret;
+  };
   bool Redundant = true;
   for (auto &I : MBB.instrs()) {
     if (!I.isDebugInstr() && !I.isUnconditionalBranch())
@@ -692,25 +711,11 @@ bool SILowerControlFlow::removeMBBifRedundant(MachineBasicBlock &MBB) {
   if (Redundant) {
     MachineBasicBlock *Succ = *MBB.succ_begin();
     SmallVector<MachineBasicBlock *, 2> Preds(MBB.predecessors());
+    MachineBasicBlock *FallThrough = nullptr;
     for (auto P : Preds) {
-      P->replaceSuccessor(&MBB, Succ);
-      MachineBasicBlock::iterator I(P->getFirstInstrTerminator());
-      while (I != P->end()) {
-        if (I->isBranch()) {
-          if (TII->getBranchDestBlock(*I) == &MBB) {
-            I->getOperand(0).setMBB(Succ);
-            break;
-          }
-        }
-        I++;
-      }
-      if (I == P->end()) {
-        MachineFunction *MF = P->getParent();
-        MachineFunction::iterator InsertPt =
-            P->getNextNode() ? MachineFunction::iterator(P->getNextNode())
-                             : MF->end();
-        MF->splice(InsertPt, Succ);
-      }
+      if (getFallThroughSucc(P) == &MBB)
+        FallThrough = P;
+      P->ReplaceUsesOfBlockWith(&MBB, Succ);
     }
     MBB.removeSuccessor(Succ);
     if (LIS) {
@@ -719,6 +724,17 @@ bool SILowerControlFlow::removeMBBifRedundant(MachineBasicBlock &MBB) {
     }
     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;
   }
   return false;
index e87f1e7..4506f3d 100644 (file)
@@ -582,3 +582,119 @@ body:             |
     S_ENDPGM 0
 
 ...
+
+---
+# redundant MBB removal correctness test:
+# we can keep bb.2 fallthrough to the  new succ because after bb.3 gets removed
+# new succ (bb.4) becomes bb.2's layout successor
+name: removed_succ_fallthrough_but_layout_successor
+tracksRegLiveness: true
+machineFunctionInfo:
+  isEntryFunction: true
+body:             |
+  ; GCN-LABEL: name: removed_succ_fallthrough_but_layout_successor
+  ; GCN: bb.0:
+  ; GCN:   successors: %bb.1(0x40000000), %bb.4(0x40000000)
+  ; GCN:   [[COPY:%[0-9]+]]:sreg_64 = COPY $exec, implicit-def $exec
+  ; GCN:   [[S_AND_B64_:%[0-9]+]]:sreg_64 = S_AND_B64 [[COPY]], undef %1:sreg_64, implicit-def dead $scc
+  ; GCN:   $exec = S_MOV_B64_term killed [[S_AND_B64_]]
+  ; GCN:   S_CBRANCH_EXECZ %bb.4, implicit $exec
+  ; GCN: bb.1:
+  ; GCN:   successors: %bb.2(0x40000000), %bb.4(0x40000000)
+  ; GCN:   [[COPY1:%[0-9]+]]:sreg_64 = COPY $exec, implicit-def $exec
+  ; GCN:   [[S_AND_B64_1:%[0-9]+]]:sreg_64 = S_AND_B64 [[COPY1]], undef %3:sreg_64, implicit-def dead $scc
+  ; GCN:   $exec = S_MOV_B64_term killed [[S_AND_B64_1]]
+  ; GCN:   S_CBRANCH_EXECZ %bb.4, implicit $exec
+  ; GCN: bb.2:
+  ; GCN:   successors: %bb.4(0x80000000)
+  ; GCN: bb.4:
+  ; GCN:   successors: %bb.5(0x80000000)
+  ; GCN:   $exec = S_OR_B64 $exec, [[COPY]], implicit-def $scc
+  ; GCN: bb.5:
+  ; GCN:   S_ENDPGM 0
+  bb.0:
+    successors: %bb.1, %bb.4
+
+    %0:sreg_64 = SI_IF undef %1:sreg_64, %bb.4, implicit-def dead $exec, implicit-def dead $scc, implicit $exec
+
+  bb.1:
+    successors: %bb.2, %bb.3
+
+    %2:sreg_64 = SI_IF undef %3:sreg_64, %bb.3, implicit-def dead $exec, implicit-def dead $scc, implicit $exec
+
+  bb.2:
+
+  bb.3:
+    SI_END_CF %2:sreg_64, implicit-def dead $exec, implicit-def dead $scc, implicit $exec
+
+
+  bb.4:
+    SI_END_CF %0:sreg_64, implicit-def dead $exec, implicit-def dead $scc, implicit $exec
+
+  bb.5:
+    S_ENDPGM 0
+
+...
+
+---
+# redundant MBB removal correctness test:
+# If one of the remdundant block preds has a fallthrough path, but the only redundant block succ is not
+# going to be a layout successor to that pred after redundant block removal, we should not rearrange
+# blocks to keep pred's fallthrough path, if the succ has fallthrough path to one of it's succ too.
+
+name: deleted_succ_fallthrough_not_layout_successor
+tracksRegLiveness: true
+machineFunctionInfo:
+  isEntryFunction: true
+body:             |
+  ; GCN-LABEL: name: deleted_succ_fallthrough_not_layout_successor
+  ; GCN: bb.0:
+  ; GCN:   successors: %bb.1(0x40000000), %bb.4(0x40000000)
+  ; GCN:   [[COPY:%[0-9]+]]:sreg_64 = COPY $exec, implicit-def $exec
+  ; GCN:   [[S_AND_B64_:%[0-9]+]]:sreg_64 = S_AND_B64 [[COPY]], undef %1:sreg_64, implicit-def dead $scc
+  ; GCN:   $exec = S_MOV_B64_term killed [[S_AND_B64_]]
+  ; GCN:   S_CBRANCH_EXECZ %bb.4, implicit $exec
+  ; GCN: bb.1:
+  ; GCN:   successors: %bb.2(0x40000000), %bb.5(0x40000000)
+  ; GCN:   [[COPY1:%[0-9]+]]:sreg_64 = COPY $exec, implicit-def $exec
+  ; GCN:   [[S_AND_B64_1:%[0-9]+]]:sreg_64 = S_AND_B64 [[COPY1]], undef %3:sreg_64, implicit-def dead $scc
+  ; GCN:   $exec = S_MOV_B64_term killed [[S_AND_B64_1]]
+  ; GCN:   S_CBRANCH_EXECZ %bb.5, implicit $exec
+  ; GCN: bb.2:
+  ; GCN:   successors: %bb.5(0x80000000)
+  ; GCN:   S_BRANCH %bb.5
+  ; GCN: bb.4:
+  ; GCN:   S_ENDPGM 0
+  ; GCN: bb.5:
+  ; GCN:   successors: %bb.6(0x80000000)
+  ; GCN:   $exec = S_OR_B64 $exec, [[COPY]], implicit-def $scc
+  ; GCN: bb.6:
+  ; GCN:   successors: %bb.4(0x80000000)
+  ; GCN:   S_BRANCH %bb.4
+  bb.0:
+    successors: %bb.1, %bb.4
+
+    %0:sreg_64 = SI_IF undef %1:sreg_64, %bb.4, implicit-def dead $exec, implicit-def dead $scc, implicit $exec
+
+  bb.1:
+    successors: %bb.2, %bb.3
+
+    %2:sreg_64 = SI_IF undef %3:sreg_64, %bb.3, implicit-def dead $exec, implicit-def dead $scc, implicit $exec
+
+  bb.2:
+
+  bb.3:
+    SI_END_CF %2:sreg_64, implicit-def dead $exec, implicit-def dead $scc, implicit $exec
+    S_BRANCH %bb.5
+
+  bb.4:
+    S_ENDPGM 0
+
+
+  bb.5:
+    SI_END_CF %0:sreg_64, implicit-def dead $exec, implicit-def dead $scc, implicit $exec
+
+  bb.6:
+    S_BRANCH %bb.4
+
+...