[AMDGPU] Tidy up conditional branches from early termination

[critson]

Summary

Replace conditional branches with unconditional ones where the
value of SCC is known apriori.

[foad]

llvm/lib/Target/AMDGPU/SIInsertSkips.cpp

227

I'm confused by this. Why can't you generate an unconditional branch to the early exit block from a BB that happened to have two or more successors?

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.wqm.demote.ll

11

The v_cmp is dead, and so is the whole %bb.1 block. Would you need to run another MachineDCE to get rid of them?

Alternatively could we get rid of dead code at the IR level? Maybe the AMDGPU instCombineIntrinsic could spot that the argument is false and call setDoesNotReturn() on the call?

[critson]

• Incorporate code that should have been in this patch.

[arsenm]

I'm not sure this is the right pass for this (I've been hoping to move everything out of this pass eventually)

llvm/lib/Target/AMDGPU/SIInsertSkips.cpp

195

Comment what the point of this is

201–202

Invert and early return?

205–206

Comment for the instruction pattern

228–238

Why isn't BranchFolding handling this case?

[critson]

In D94748#2507651, @arsenm wrote:

I'm not sure this is the right pass for this (I've been hoping to move everything out of this pass eventually)

I agree, I think SIPreEmitPeephole is probably a better place for this.

llvm/lib/Target/AMDGPU/SIInsertSkips.cpp

227

I am not sure why I wrote this either.
I retested without it and could not produce any problems.

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.wqm.demote.ll

11

Adding a late MachineDCE does clean up the v_cmp, but not %bb.1. I notice that in some of the other examples it returns other unreachable instructions. It might be interesting to test results of this on a large collection of shaders.

I am not sure if setting "does not return" is valid, as this will only terminate the whole shader if every lane is demoted. This is true in uniform control flow, but this intrinsic is valid in non-uniform control flow too.

[critson]

While the pattern this optimises for occurs a fair bit in trivial lit test code, after evaluating game shaders I can find only a handful of examples (4 in 10000 shaders).
So I do not think it is worth adding code for.

llvm/lib/Target/AMDGPU/SIInsertSkips.cpp

private:
   MachineBasicBlock *EarlyExitBlock = nullptr;
   bool EarlyExitClearsExec = false;
 
   bool shouldSkip(const MachineBasicBlock &From,
                   const MachineBasicBlock &To) const;
 
   void ensureEarlyExitBlock(MachineBasicBlock &MBB, bool ClearExec);
 
+  bool tidySCCDef(MachineInstr &MI);
   void earlyTerm(MachineInstr &MI);
 
   bool skipMaskBranch(MachineInstr &MI, MachineBasicBlock &MBB);
 
 public:
   static char ID;
 
   unsigned MovOpc;
static void splitBlock(MachineBasicBlock &MBB, MachineInstr &MI,
   for (MachineBasicBlock *Succ : SplitBB->successors()) {
     DTUpdates.push_back({DomTreeT::Insert, SplitBB, Succ});
     DTUpdates.push_back({DomTreeT::Delete, &MBB, Succ});
   }
   DTUpdates.push_back({DomTreeT::Insert, &MBB, SplitBB});
   MDT->getBase().applyUpdates(DTUpdates);
 }
 
+bool SIInsertSkips::tidySCCDef(MachineInstr &MI) {
+  MachineBasicBlock &MBB = *MI.getParent();
+
+  // Peek at the previous instruction in case this can be unconditional
+  assert(MI.getIterator() != MBB.begin());
+  auto Prev = std::prev(MI.getIterator());
+  if (Prev->getOpcode() == AMDGPU::S_ANDN2_B32 ||
+      Prev->getOpcode() == AMDGPU::S_ANDN2_B64) {
+    auto Src0 = Prev->getOperand(1);
+    auto Src1 = Prev->getOperand(2);
+    if (Src0.isReg() && Src0.getReg() == ExecReg && Src1.isReg() &&
+        Src1.getReg() == ExecReg) {
+      // SCC will always be 0; use unconditional branch
+      Register Dst = Prev->getOperand(0).getReg();
+      // Simplify S_ANDN2, remove entirely for exec, as it is set in exit block
+      if (Dst != ExecReg) {
+        BuildMI(MBB, Prev, Prev->getDebugLoc(), TII->get(MovOpc), Dst)
+            .addImm(0);
+      }
+      Prev->eraseFromParent();
+      return true;
+    }
+  }
+  return false;
+}
+
 void SIInsertSkips::earlyTerm(MachineInstr &MI) {
   MachineBasicBlock &MBB = *MI.getParent();
   const DebugLoc DL = MI.getDebugLoc();
 
   ensureEarlyExitBlock(MBB, true);
 
-  auto BranchMI = BuildMI(MBB, MI, DL, TII->get(AMDGPU::S_CBRANCH_SCC0))
-                      .addMBB(EarlyExitBlock);
+  // Can we make branch unconditional?
+  bool ReplaceSuccessor = MBB.succ_size() <= 1;
+  if (ReplaceSuccessor)
+    ReplaceSuccessor = tidySCCDef(MI);
+
+  MachineInstr *BranchMI = nullptr;
+  if (ReplaceSuccessor) {
+    // Branch is always taken
+    BranchMI =
+        BuildMI(MBB, MI, DL, TII->get(AMDGPU::S_BRANCH)).addMBB(EarlyExitBlock);
+  } else {
+    BranchMI = BuildMI(MBB, MI, DL, TII->get(AMDGPU::S_CBRANCH_SCC0))
+  }
+
   auto Next = std::next(MI.getIterator());
 
   if (Next != MBB.end() && !Next->isTerminator())
     splitBlock(MBB, *BranchMI, MDT);
 
+  MachineBasicBlock *OldSuccessor = nullptr;
+  if (ReplaceSuccessor && !MBB.succ_empty()) {
+    OldSuccessor = *MBB.succ_begin();
+    MBB.replaceSuccessor(OldSuccessor, EarlyExitBlock);
+  } else {
     MBB.addSuccessor(EarlyExitBlock);
+  }
+
+  // Update MDT
   MDT->getBase().insertEdge(&MBB, EarlyExitBlock);
+  if (OldSuccessor)
+    MDT->getBase().deleteEdge(&MBB, OldSuccessor);
 }
 
 // Returns true if a branch over the block was inserted.
 bool SIInsertSkips::skipMaskBranch(MachineInstr &MI,
                                    MachineBasicBlock &SrcMBB) {
   MachineBasicBlock *DestBB = MI.getOperand(0).getMBB();
 
   if (!shouldSkip(**SrcMBB.succ_begin(), *DestBB))
for (I = MBB.begin(); I != MBB.end(); I = Next) {
       default:
         break;
       }
     }
   }
 
   for (MachineInstr *Instr : EarlyTermInstrs) {
     // Early termination in GS does nothing
-    if (MF.getFunction().getCallingConv() != CallingConv::AMDGPU_GS)
+    if (MF.getFunction().getCallingConv() != CallingConv::AMDGPU_GS) {
       earlyTerm(*Instr);
+    } else {
+      tidySCCDef(*Instr);
+    }
     Instr->eraseFromParent();
   }
   EarlyTermInstrs.clear();
   EarlyExitBlock = nullptr;
 
   return MadeChange;
 }

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.wqm.demote.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -march=amdgcn -mcpu=tonga -verify-machineinstrs < %s | FileCheck -check-prefix=SI %s
 ; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck -check-prefix=GFX9 %s
 ; RUN: llc -march=amdgcn -mcpu=gfx1010 -mattr=+wavefrontsize32,-wavefrontsize64 -verify-machineinstrs < %s | FileCheck -check-prefixes=GFX10-32 %s
 ; RUN: llc -march=amdgcn -mcpu=gfx1010 -mattr=-wavefrontsize32,+wavefrontsize64 -verify-machineinstrs < %s | FileCheck -check-prefixes=GFX10-64 %s
 
 define amdgpu_ps void @static_exact(float %arg0, float %arg1) {
 ; SI-LABEL: static_exact:
 ; SI:       ; %bb.0: ; %.entry
 ; SI-NEXT:    v_cmp_gt_f32_e32 vcc, 0, v0
-; SI-NEXT:    s_andn2_b64 exec, exec, exec
-; SI-NEXT:    s_cbranch_scc0 BB0_2
+; SI-NEXT:    s_branch BB0_2
 ; SI-NEXT:  ; %bb.1: ; %.entry
 ; SI-NEXT:    s_mov_b64 exec, 0
 ; SI-NEXT:    v_cndmask_b32_e64 v0, 0, 1.0, vcc
 ; SI-NEXT:    exp mrt1 v0, v0, v0, v0 done vm
 ; SI-NEXT:    s_endpgm
 ; SI-NEXT:  BB0_2:
 ; SI-NEXT:    s_mov_b64 exec, 0
 ; SI-NEXT:    exp null off, off, off, off done vm
 ; SI-NEXT:    s_endpgm
 ;
 ; GFX9-LABEL: static_exact:
 ; GFX9:       ; %bb.0: ; %.entry
 ; GFX9-NEXT:    v_cmp_gt_f32_e32 vcc, 0, v0
-; GFX9-NEXT:    s_andn2_b64 exec, exec, exec
-; GFX9-NEXT:    s_cbranch_scc0 BB0_2
+; GFX9-NEXT:    s_branch BB0_2
 ; GFX9-NEXT:  ; %bb.1: ; %.entry
 ; GFX9-NEXT:    s_mov_b64 exec, 0
 ; GFX9-NEXT:    v_cndmask_b32_e64 v0, 0, 1.0, vcc
 ; GFX9-NEXT:    exp mrt1 v0, v0, v0, v0 done vm
 ; GFX9-NEXT:    s_endpgm
 ; GFX9-NEXT:  BB0_2:
 ; GFX9-NEXT:    s_mov_b64 exec, 0
 ; GFX9-NEXT:    exp null off, off, off, off done vm
 ; GFX9-NEXT:    s_endpgm
 ;
 ; GFX10-32-LABEL: static_exact:
 ; GFX10-32:       ; %bb.0: ; %.entry
 ; GFX10-32-NEXT:    v_cmp_gt_f32_e32 vcc_lo, 0, v0
-; GFX10-32-NEXT:    s_andn2_b32 exec_lo, exec_lo, exec_lo
-; GFX10-32-NEXT:    s_cbranch_scc0 BB0_2
+; GFX10-32-NEXT:    s_branch BB0_2
 ; GFX10-32-NEXT:  ; %bb.1: ; %.entry
 ; GFX10-32-NEXT:    s_mov_b32 exec_lo, 0
 ; GFX10-32-NEXT:    v_cndmask_b32_e64 v0, 0, 1.0, vcc_lo
 ; GFX10-32-NEXT:    exp mrt1 v0, v0, v0, v0 done vm
 ; GFX10-32-NEXT:    s_endpgm
 ; GFX10-32-NEXT:  BB0_2:
 ; GFX10-32-NEXT:    s_mov_b32 exec_lo, 0
 ; GFX10-32-NEXT:    exp null off, off, off, off done vm
 ; GFX10-32-NEXT:    s_endpgm
 ;
 ; GFX10-64-LABEL: static_exact:
 ; GFX10-64:       ; %bb.0: ; %.entry
 ; GFX10-64-NEXT:    v_cmp_gt_f32_e32 vcc, 0, v0
-; GFX10-64-NEXT:    s_andn2_b64 exec, exec, exec
-; GFX10-64-NEXT:    s_cbranch_scc0 BB0_2
+; GFX10-64-NEXT:    s_branch BB0_2
 ; GFX10-64-NEXT:  ; %bb.1: ; %.entry
 ; GFX10-64-NEXT:    s_mov_b64 exec, 0
 ; GFX10-64-NEXT:    v_cndmask_b32_e64 v0, 0, 1.0, vcc
 ; GFX10-64-NEXT:    exp mrt1 v0, v0, v0, v0 done vm
 ; GFX10-64-NEXT:    s_endpgm
 ; GFX10-64-NEXT:  BB0_2:
 ; GFX10-64-NEXT:    s_mov_b64 exec, 0
 ; GFX10-64-NEXT:    exp null off, off, off, off done vm

llvm/test/CodeGen/AMDGPU/skip-if-dead.ll

 ; RUN: llc -march=amdgcn -verify-machineinstrs -simplifycfg-require-and-preserve-domtree=1 < %s | FileCheck %s
 
 ; CHECK-LABEL: {{^}}test_kill_depth_0_imm_pos:
 ; CHECK-NEXT: ; %bb.0:
 ; CHECK-NEXT: s_endpgm
 define amdgpu_ps void @test_kill_depth_0_imm_pos() #0 {
   call void @llvm.amdgcn.kill(i1 true)
   ret void
 }
 
 ; CHECK-LABEL: {{^}}test_kill_depth_0_imm_neg:
 ; CHECK-NEXT: ; %bb.0:
-; CHECK-NEXT: s_andn2_b64 exec, exec, exec
-; CHECK-NEXT: s_cbranch_scc0 [[EXIT_BB:BB[0-9]+_[0-9]+]]
+; CHECK-NEXT: s_branch [[EXIT_BB:BB[0-9]+_[0-9]+]]
 ; CHECK-NEXT: s_endpgm
 ; CHECK-NEXT: [[EXIT_BB]]:
 ; CHECK-NEXT: s_mov_b64 exec, 0
 ; CHECK-NEXT: exp null off, off, off, off done vm
 ; CHECK-NEXT: s_endpgm
 define amdgpu_ps void @test_kill_depth_0_imm_neg() #0 {
   call void @llvm.amdgcn.kill(i1 false)
   ret void