[PATCH] D20092: [AMDGPU] Fix issues introduced by aggressive block placement

[Chuang-Yu Cheng via llvm-commits]

cycheng created this revision.
cycheng added reviewers: tstellarAMD, tjablin, kbarton.
cycheng added subscribers: hfinkel, nemanjai, amehsan, llvm-commits.
Herald added subscribers: kzhuravl, arsenm.

Patch D20017 aggressively choosing the best loop top in a loop, this introduces 2 issues for AMDGPU backend:

  # Crash issue: it breaks assumption of basic block order in shouldSkip(), e.g.
```
   bb2<--+   bb2 (From)      Block      Flow (To)
   /   \ |   bb6        => Placement => bb2 (From)
bb6 -> Flow  Flow (To)                  bb6        
```
In original code, To MBB is assumed after From MBB, so it gets crash when this is not true.

  # Unnecessary branch in fall through path, e.g.
```
   bb2<--+   Flow (Latch)  
   /   \ |   bb2  (Header) 
bb6 -> Flow  bb6
  
Flow: 
    s_cbranch_execnz bb2
    s_branch end
bb2:
    s_cbranch_execz Flow
bb6:
    s_branch Flow
end:
```

Flow can fall through bb2, so we can replace two branches into one conditional branch.

http://reviews.llvm.org/D20092

Files:
  lib/Target/AMDGPU/SILowerControlFlow.cpp

Index: lib/Target/AMDGPU/SILowerControlFlow.cpp
===================================================================
--- lib/Target/AMDGPU/SILowerControlFlow.cpp
+++ lib/Target/AMDGPU/SILowerControlFlow.cpp
@@ -130,6 +130,19 @@
 
   unsigned NumInstr = 0;
 
+  // Check whether 'To MBB' is before 'From MBB', this is possible after Block
+  // Placement Pass:
+  //    bb2<--+   bb2 (From)      Block      Flow (To)
+  //    /   \ |   bb6        => Placement => bb2 (From)
+  // bb6 -> Flow  Flow (To)                  bb6
+  for (MachineFunction::iterator MBBI = MachineFunction::iterator(From),
+                                 ToI = MachineFunction::iterator(To);
+                                 ToI != From->getParent()->end(); ++ToI) {
+    // return true so we generate conditional branch for 'From MBB'
+    if (MBBI == ToI)
+      return true;
+  }
+
   for (MachineFunction::iterator MBBI = MachineFunction::iterator(From),
                                  ToI = MachineFunction::iterator(To); MBBI != ToI; ++MBBI) {
 
@@ -295,8 +308,38 @@
           .addReg(AMDGPU::EXEC)
           .addReg(Src);
 
-  BuildMI(MBB, &MI, DL, TII->get(AMDGPU::S_CBRANCH_EXECNZ))
-    .addOperand(MI.getOperand(1));
+  // After Block Placement Pass, Latch block might before Header block
+  //    bb2<--+   Flow (Latch)  MBB
+  //    /   \ |   bb2  (Header) NextBB
+  // bb6 -> Flow  bb6
+  //
+  // If this is the case, then 'Flow MBB' can fall through 'bb2 MBB'. But we
+  // need to change branch condition for 'Flow MBB'
+
+  // Check if Latch is before Header.
+  // %Flow:
+  //   SI_LOOP %SGPR0_SGPR1, bb2, ..
+  //   S_BRANCH end
+  // %bb2:
+  //   ..
+  MachineBasicBlock &NextBB = *std::next(MachineFunction::iterator(MBB));
+  if (&NextBB == MI.getOperand(1).getMBB()) {
+    MachineInstr &NextMI = *std::next(MachineBasicBlock::iterator(MI));
+
+    assert(NextMI.getOpcode() == AMDGPU::S_BRANCH &&
+      "Next instruction of SI_LOOP should be S_BRANCH");
+
+    // Result:
+    //   s_cbranch_execz end
+    //   s_branch end
+    // The 's_branch end' is removed at Branch()
+    BuildMI(MBB, &MI, DL, TII->get(AMDGPU::S_CBRANCH_EXECZ))
+      .addOperand(NextMI.getOperand(0));
+  }
+  else {
+    BuildMI(MBB, &MI, DL, TII->get(AMDGPU::S_CBRANCH_EXECNZ))
+      .addOperand(MI.getOperand(1));
+  }
 
   MI.eraseFromParent();
 }
@@ -315,10 +358,20 @@
 }
 
 void SILowerControlFlow::Branch(MachineInstr &MI) {
+  // If these aren't equal, this is probably an infinite loop.
   if (MI.getOperand(0).getMBB() == MI.getParent()->getNextNode())
     MI.eraseFromParent();
-
-  // If these aren't equal, this is probably an infinite loop.
+  else if (&*MI.getParent()->begin() != &MI) {
+    MachineInstr &PrevMI = *std::prev(MachineBasicBlock::iterator(MI));
+
+    // Look at this pattern (see comments in Loop()):
+    //   s_cbranch_execz end
+    //   s_branch end
+    // Remove 's_branch end'
+    if (PrevMI.getOpcode() == AMDGPU::S_CBRANCH_EXECZ &&
+        PrevMI.getOperand(0).getMBB() == MI.getOperand(0).getMBB())
+      MI.eraseFromParent();
+  }
 }
 
 void SILowerControlFlow::Kill(MachineInstr &MI) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D20092.56678.patch
Type: text/x-patch
Size: 3121 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160510/36609976/attachment.bin>