AMDGPU: Fix verifier error with kill intrinsic

[arsenm]

Summary

Don't create a terminator in the middle of the block.
We should probably get rid of this intrinsic.

[arsenm]

Use custom inserter to make sure kill is a terminator instruction so the post-RA pass doesn't need to worry about splitting the block

[nhaehnle]

I haven't looked at the changes in detail yet, but I'm fine with the general approach.

The reference to SI_KILL in SIWholeQuadMode should probably be removed with this change.

[arsenm]

Stop checking SI_KILL in SIWholeQuadMode

[nhaehnle]

Some small remarks, apart from that LGTM.

lib/Target/AMDGPU/SILowerControlFlow.cpp

212

The function now logically operates on a block level, so I'd change it to take MBB as a parameter instead of MI.

217

I think the first argument to shouldSkip should be the successor MBB.

That said, shouldSkip itself is a bit broken as a heuristic since it doesn't seem to follow successors itself, so perhaps it makes sense to fix this separately.

235

The SkipBB shouldn't have any successors, should it?

[arsenm]

r275203

lib/Target/AMDGPU/SILowerControlFlow.cpp

217

Yes, shouldSkip makes no sense as it is. I'm working on shouldSkip separately, moving it to a separate pass so this will all be changing anyway

235

I was treating it as a fall through block but I guess it shouldn't need it

lib/Target/AMDGPU/SIISelLowering.h

public:
   SDValue LowerReturn(SDValue Chain, CallingConv::ID CallConv, bool isVarArg,
                       const SmallVectorImpl<ISD::OutputArg> &Outs,
                       const SmallVectorImpl<SDValue> &OutVals, const SDLoc &DL,
                       SelectionDAG &DAG) const override;
 
   unsigned getRegisterByName(const char* RegName, EVT VT,
                              SelectionDAG &DAG) const override;
 
+  MachineBasicBlock *splitKillBlock(MachineInstr &MI,
+                                    MachineBasicBlock *BB) const;
+
   MachineBasicBlock *
   EmitInstrWithCustomInserter(MachineInstr &MI,
                               MachineBasicBlock *BB) const override;
   bool enableAggressiveFMAFusion(EVT VT) const override;
   EVT getSetCCResultType(const DataLayout &DL, LLVMContext &Context,
                          EVT VT) const override;
   MVT getScalarShiftAmountTy(const DataLayout &, EVT) const override;
   bool isFMAFasterThanFMulAndFAdd(EVT VT) const override;

lib/Target/AMDGPU/SIISelLowering.cpp

unsigned SITargetLowering::getRegisterByName(const char* RegName, EVT VT,
   default:
     llvm_unreachable("missing register type checking");
   }
 
   report_fatal_error(Twine("invalid type for register \""
                            + StringRef(RegName) + "\"."));
 }
 
+// If kill is not the last instruction, split the block so kill is always a
+// proper terminator.
+MachineBasicBlock *SITargetLowering::splitKillBlock(MachineInstr &MI,
+                                                    MachineBasicBlock *BB) const {
+  const SIInstrInfo *TII = getSubtarget()->getInstrInfo();
+
+  MachineBasicBlock::iterator SplitPoint(&MI);
+  ++SplitPoint;
+
+  if (SplitPoint == BB->end()) {
+    // Don't bother with a new block.
+    MI.setDesc(TII->get(AMDGPU::SI_KILL_TERMINATOR));
+    return BB;
+  }
+
+  MachineFunction *MF = BB->getParent();
+  MachineBasicBlock *SplitBB
+    = MF->CreateMachineBasicBlock(BB->getBasicBlock());
+
+  SmallSet<unsigned, 8> SplitDefRegs;
+  for (auto I = SplitPoint, E = BB->end(); I != E; ++I) {
+    for (MachineOperand &Def : I->defs())
+      SplitDefRegs.insert(Def.getReg());
+  }
+
+  // Fix the block phi references to point to the new block for the defs in the
+  // second piece of the block.
+  for (MachineBasicBlock *Succ : BB->successors()) {
+    for (MachineInstr &MI : *Succ) {
+      if (!MI.isPHI())
+        break;
+
+      for (unsigned I = 1, E = MI.getNumOperands(); I != E; I += 2) {
+        unsigned IncomingReg = MI.getOperand(I).getReg();
+        MachineOperand &FromBB = MI.getOperand(I + 1);
+        if (BB == FromBB.getMBB()) {
+          if (SplitDefRegs.count(IncomingReg))
+            FromBB.setMBB(SplitBB);
+
+          break;
+        }
+      }
+    }
+  }
+
+  MF->insert(++MachineFunction::iterator(BB), SplitBB);
+  SplitBB->splice(SplitBB->begin(), BB, SplitPoint, BB->end());
+
+
+  SplitBB->transferSuccessors(BB);
+  BB->addSuccessor(SplitBB);
+
+  MI.setDesc(TII->get(AMDGPU::SI_KILL_TERMINATOR));
+  return SplitBB;
+}
+
 MachineBasicBlock *
 SITargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
                                               MachineBasicBlock *BB) const {
   switch (MI.getOpcode()) {
   case AMDGPU::SI_INIT_M0: {
     const SIInstrInfo *TII = getSubtarget()->getInstrInfo();
     BuildMI(*BB, MI.getIterator(), MI.getDebugLoc(),
             TII->get(AMDGPU::S_MOV_B32), AMDGPU::M0)
         .addOperand(MI.getOperand(0));
     MI.eraseFromParent();
     break;
   }
-  case AMDGPU::BRANCH:
-    return BB;
   case AMDGPU::GET_GROUPSTATICSIZE: {
     const SIInstrInfo *TII = getSubtarget()->getInstrInfo();
 
     MachineFunction *MF = BB->getParent();
     SIMachineFunctionInfo *MFI = MF->getInfo<SIMachineFunctionInfo>();
     DebugLoc DL = MI.getDebugLoc();
     BuildMI(*BB, MI, DL, TII->get(AMDGPU::S_MOVK_I32))
         .addOperand(MI.getOperand(0))
         .addImm(MFI->LDSSize);
     MI.eraseFromParent();
     return BB;
   }
+  case AMDGPU::SI_KILL:
+    return splitKillBlock(MI, BB);
   default:
     return AMDGPUTargetLowering::EmitInstrWithCustomInserter(MI, BB);
   }
   return BB;
 }
 
 bool SITargetLowering::enableAggressiveFMAFusion(EVT VT) const {
   // This currently forces unfolding various combinations of fsub into fma with

lib/Target/AMDGPU/SIInstructions.td

def SI_END_CF : InstSI <
   [(int_amdgcn_end_cf i64:$saved)]
 >;
 
 } // End Uses = [EXEC], Defs = [EXEC]
 
 let Uses = [EXEC], Defs = [EXEC,VCC] in {
 def SI_KILL : InstSI <
   (outs), (ins VSrc_32:$src), "",
-  [(int_AMDGPU_kill f32:$src)]
->;
+  [(int_AMDGPU_kill f32:$src)]> {
+  let isConvergent = 1;
+  let usesCustomInserter = 1;
+}
+
+def SI_KILL_TERMINATOR : InstSI <
+  (outs), (ins VSrc_32:$src), "", []> {
+  let isTerminator = 1;
+}
+
 } // End Uses = [EXEC], Defs = [EXEC,VCC]
 
 } // End mayLoad = 1, mayStore = 1, hasSideEffects = 1
 
 def SI_PS_LIVE : InstSI <
   (outs SReg_64:$dst), (ins), "",
   [(set i1:$dst, (int_amdgcn_ps_live))]> {
   let SALU = 1;

lib/Target/AMDGPU/SILowerControlFlow.cpp

void SILowerControlFlow::Skip(MachineInstr &From, MachineOperand &To) {
   if (!shouldSkip(*From.getParent()->succ_begin(), To.getMBB()))
     return;
 
   DebugLoc DL = From.getDebugLoc();
   BuildMI(*From.getParent(), &From, DL, TII->get(AMDGPU::S_CBRANCH_EXECZ))
     .addOperand(To);
 }
 
 bool SILowerControlFlow::skipIfDead(MachineInstr &MI) {
   MachineBasicBlock &MBB = *MI.getParent();
+  MachineFunction *MF = MBB.getParent();
 
-  if (MBB.getParent()->getFunction()->getCallingConv() != CallingConv::AMDGPU_PS ||
+  if (MF->getFunction()->getCallingConv() != CallingConv::AMDGPU_PS ||
       !shouldSkip(&MBB, &MBB.getParent()->back()))
     return false;
 
-  LivePhysRegs RemainderLiveRegs(TRI);
-  RemainderLiveRegs.addLiveOuts(MBB);
+  MachineBasicBlock *SkipBB = MF->CreateMachineBasicBlock(MBB.getBasicBlock());
 
-  MachineBasicBlock *SkipBB;
-  MachineBasicBlock *RemainderBB;
-  std::tie(SkipBB, RemainderBB) = splitBlock(MBB, MI.getIterator());
+  MachineFunction::iterator BBInsertPt(&MBB);
+  ++BBInsertPt;
+
+  MF->insert(BBInsertPt, SkipBB);
+
+  assert(MBB.succ_size() == 1);
 
   const DebugLoc &DL = MI.getDebugLoc();
 
   // If the exec mask is non-zero, skip the next two instructions
   BuildMI(&MBB, DL, TII->get(AMDGPU::S_CBRANCH_EXECNZ))
-    .addMBB(RemainderBB);
+    .addMBB(&*BBInsertPt);
 
-  MBB.addSuccessor(RemainderBB);
+  SkipBB->addSuccessor(&*BBInsertPt);
+  MBB.addSuccessor(SkipBB);
 
   MachineBasicBlock::iterator Insert = SkipBB->begin();
 
   // Exec mask is zero: Export to NULL target...
   BuildMI(*SkipBB, Insert, DL, TII->get(AMDGPU::EXP))
     .addImm(0)
     .addImm(0x09) // V_008DFC_SQ_EXP_NULL
     .addImm(0)
     .addImm(1)
     .addImm(1)
     .addReg(AMDGPU::VGPR0, RegState::Undef)
     .addReg(AMDGPU::VGPR0, RegState::Undef)
     .addReg(AMDGPU::VGPR0, RegState::Undef)
     .addReg(AMDGPU::VGPR0, RegState::Undef);
 
   // ... and terminate wavefront.
   BuildMI(*SkipBB, Insert, DL, TII->get(AMDGPU::S_ENDPGM));
-
-  for (const MachineInstr &Inst : reverse(*RemainderBB))
-    RemainderLiveRegs.stepBackward(Inst);
-
-  const MachineRegisterInfo &MRI = MBB.getParent()->getRegInfo();
-  for (unsigned Reg : RemainderLiveRegs) {
-    if (MRI.isAllocatable(Reg))
-      RemainderBB->addLiveIn(Reg);
-  }
-
   return true;
 }
 
 void SILowerControlFlow::If(MachineInstr &MI) {
   MachineBasicBlock &MBB = *MI.getParent();
   DebugLoc DL = MI.getDebugLoc();
   unsigned Reg = MI.getOperand(0).getReg();
   unsigned Vcc = MI.getOperand(1).getReg();
for (I = MBB.begin(); I != MBB.end(); I = Next) {
               NextBB = std::next(BI);
               BE = MF.end();
               Next = MBB.end();
             }
           }
           EndCf(MI);
           break;
 
-        case AMDGPU::SI_KILL:
+        case AMDGPU::SI_KILL_TERMINATOR:
           if (Depth == 0) {
             if (skipIfDead(MI)) {
               NextBB = std::next(BI);
               BE = MF.end();
               Next = MBB.end();
             }
           } else
             HaveKill = true;

lib/Target/AMDGPU/SIWholeQuadMode.cpp

void SIWholeQuadMode::propagateInstruction(const MachineInstr &MI,
                                            std::vector<WorkItem>& Worklist) {
   const MachineBasicBlock &MBB = *MI.getParent();
   InstrInfo II = Instructions[&MI]; // take a copy to prevent dangling references
   BlockInfo &BI = Blocks[&MBB];
 
   // Control flow-type instructions that are followed by WQM computations
   // must themselves be in WQM.
   if ((II.OutNeeds & StateWQM) && !(II.Needs & StateWQM) &&
-      (MI.isBranch() || MI.isTerminator() || MI.getOpcode() == AMDGPU::SI_KILL)) {
+      (MI.isBranch() || MI.isTerminator())) {
     Instructions[&MI].Needs = StateWQM;
     II.Needs = StateWQM;
   }
 
   // Propagate to block level
   BI.Needs |= II.Needs;
   if ((BI.InNeeds | II.Needs) != BI.InNeeds) {
     BI.InNeeds |= II.Needs;
if (Needs && State != Needs) {
       } else {
         assert(WQMFromExec == (SavedWQMReg == 0));
         toWQM(MBB, &MI, SavedWQMReg);
         SavedWQMReg = 0;
       }
 
       State = Needs;
     }
-
-    if (MI.getOpcode() == AMDGPU::SI_KILL)
-      WQMFromExec = false;
   }
 
   if ((BI.OutNeeds & StateWQM) && State != StateWQM) {
     assert(WQMFromExec == (SavedWQMReg == 0));
     toWQM(MBB, MBB.end(), SavedWQMReg);
   } else if (BI.OutNeeds == StateExact && State != StateExact) {
     toExact(MBB, MBB.end(), 0, LiveMaskReg);
   }

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

 ; RUN: llc -march=amdgcn -verify-machineinstrs < %s | FileCheck %s
 
 ; CHECK-LABEL: {{^}}test_kill_depth_0_imm_pos:
 ; CHECK-NEXT: ; BB#0:
+; CHECK-NEXT: ; BB#1:
 ; CHECK-NEXT: s_endpgm
 define amdgpu_ps void @test_kill_depth_0_imm_pos() #0 {
   call void @llvm.AMDGPU.kill(float 0.0)
   ret void
 }
 
 ; CHECK-LABEL: {{^}}test_kill_depth_0_imm_neg:
 ; CHECK-NEXT: ; BB#0:
 ; CHECK-NEXT: s_mov_b64 exec, 0
+; CHECK-NEXT: ; BB#1:
 ; CHECK-NEXT: s_endpgm
 define amdgpu_ps void @test_kill_depth_0_imm_neg() #0 {
   call void @llvm.AMDGPU.kill(float -0.0)
   ret void
 }
 
 ; CHECK-LABEL: {{^}}test_kill_depth_var:
 ; CHECK-NEXT: ; BB#0:
 ; CHECK-NEXT: v_cmpx_le_f32_e32 vcc, 0, v0
+; CHECK-NEXT: ; BB#1:
 ; CHECK-NEXT: s_endpgm
 define amdgpu_ps void @test_kill_depth_var(float %x) #0 {
   call void @llvm.AMDGPU.kill(float %x)
   ret void
 }
 
 ; FIXME: why does the skip depend on the asm length in the same block?
 
 ; CHECK-LABEL: {{^}}test_kill_control_flow:
 ; CHECK: s_cmp_lg_i32 s{{[0-9]+}}, 0
 ; CHECK: s_cbranch_scc1 [[RETURN_BB:BB[0-9]+_[0-9]+]]
 
-; CHECK: ; BB#1:
+; CHECK-NEXT: ; BB#1:
+; CHECK: v_mov_b32_e64 v7, -1
 ; CHECK: v_nop_e64
 ; CHECK: v_nop_e64
 ; CHECK: v_nop_e64
 ; CHECK: v_nop_e64
 ; CHECK: v_nop_e64
 ; CHECK: v_nop_e64
 ; CHECK: v_nop_e64
 ; CHECK: v_nop_e64
 ; CHECK: v_nop_e64
 ; CHECK: v_nop_e64
 
-; CHECK: s_cbranch_execnz [[SPLIT_BB:BB[0-9]+_[0-9]+]]
+; CHECK: v_cmpx_le_f32_e32 vcc, 0, v7
+; CHECK-NEXT: s_cbranch_execnz [[SPLIT_BB:BB[0-9]+_[0-9]+]]
 ; CHECK-NEXT: ; BB#3:
 ; CHECK-NEXT: exp 0, 9, 0, 1, 1, v0, v0, v0, v0
 ; CHECK-NEXT: s_endpgm
 
 ; CHECK-NEXT: {{^}}[[SPLIT_BB]]:
-; CHECK-NEXT: v_cmpx_le_f32_e32 vcc, 0, v7
-; CHECK-NEXT: {{^}}BB{{[0-9]+_[0-9]+}}:
 ; CHECK-NEXT: s_endpgm
 define amdgpu_ps void @test_kill_control_flow(i32 inreg %arg) #0 {
 entry:
   %cmp = icmp eq i32 %arg, 0
   br i1 %cmp, label %bb, label %exit
 
 bb:
   %var = call float asm sideeffect "

 ; CHECK: v_nop_e64
 ; CHECK: v_nop_e64
 ; CHECK: v_nop_e64
 ; CHECK: v_nop_e64
 ; CHECK: v_nop_e64
 ; CHECK: ;;#ASMEND
 ; CHECK: v_mov_b32_e64 v8, -1
 ; CHECK: ;;#ASMEND
+; CHECK: v_cmpx_le_f32_e32 vcc, 0, v7
 ; CHECK-NEXT: s_cbranch_execnz [[SPLIT_BB:BB[0-9]+_[0-9]+]]
 
-; CHECK-NEXT: ; BB#3:
+; CHECK-NEXT: ; BB#4:
 ; CHECK-NEXT: exp 0, 9, 0, 1, 1, v0, v0, v0, v0
 ; CHECK-NEXT: s_endpgm
 
 ; CHECK-NEXT: {{^}}[[SPLIT_BB]]:
-; CHECK-NEXT: v_cmpx_le_f32_e32 vcc, 0, v7
 ; CHECK: buffer_store_dword v8
 ; CHECK: v_mov_b32_e64 v9, -2
 
 ; CHECK: {{^}}BB{{[0-9]+_[0-9]+}}:
 ; CHECK: buffer_store_dword v9
 ; CHECK-NEXT: s_endpgm
 define amdgpu_ps void @test_kill_control_flow_remainder(i32 inreg %arg) #0 {
 entry: