[AMDGPU] Fix multiple vreg definitions in si-lower-control-flow

[rampitec]

Summary

SI Lower control flow pass creates multiple definitions of virtual registers.

SI_IF:

%vreg3<def> = COPY %EXEC, %EXEC<imp-def>; SReg_64:%vreg3
%vreg47<def> = S_AND_B64 %vreg3, %vreg20, %SCC<imp-def,dead>; SReg_64:%vreg47,%vreg3,%vreg20
%vreg3<def> = S_XOR_B64 %vreg47, %vreg3, %SCC<imp-def,dead>; SReg_64:%vreg3,%vreg47

SI_ELSE:

%vreg2<def> = COPY %vreg0<kill>; SReg_64:%vreg2,%vreg0
%vreg2<def> = S_OR_SAVEEXEC_B64 %vreg2, %EXEC<imp-def>, %SCC<imp-def>, %EXEC<imp-use>; SReg_64:%vreg2
%vreg1<def> = COPY %vreg20<kill>; VGPR_32:%vreg1,%vreg20
%EXEC<def> = S_AND_B64 %EXEC, %vreg19<kill>, %SCC<imp-def,dead>; SReg_64:%vreg19
%vreg21<def> = COPY %vreg1<kill>; VGPR_32:%vreg21,%vreg1
%vreg2<def> = S_AND_B64 %EXEC, %vreg2, %SCC<imp-def>; SReg_64:%vreg2
%EXEC<def> = S_XOR_B64_term %EXEC, %vreg2; SReg_64:%vreg2

This is the patch to fix SSA.

[arsenm]

Do you have a testcase where this was an issue? I though I got all of these when I moved the pass to SSA. Why isn't the verifier catching this?

[rampitec]

In D26939#602033, @arsenm wrote:

Do you have a testcase where this was an issue? I though I got all of these when I moved the pass to SSA. Why isn't the verifier catching this?

Technically pass works post SSA, so it is not required to maintain SSA. It does not create errors (at least I'm not aware of errors caused by this), but results in MRI->getUniqueVRegDef() returning a null pointer. I'm working on combining two S_AND_B64 instructions in the snippet below and similar, and it if returns nullptr my code simply cannot do anything.

%vreg20<def> = S_AND_B64 %EXEC, %vreg18, %SCC<imp-def,dead>; SReg_64:%vreg20,%vreg18
%vreg46<def> = COPY %vreg19<kill>; VGPR_32:%vreg46,%vreg19
%vreg3<def> = COPY %EXEC, %EXEC<imp-def>; SReg_64:%vreg3
%vreg47<def> = S_AND_B64 %vreg3, %vreg20, %SCC<imp-def,dead>; SReg_64:%vreg47,%vreg3,%vreg20
%vreg3<def> = S_XOR_B64 %vreg47, %vreg3, %SCC<imp-def,dead>; SReg_64:%vreg3,%vreg47

[arsenm]

Oh right I forgot I moved it to SSA and then back. LGTM assuming tests pass, I thought there was some pain for maintaining live intervals without re-using

llvm/trunk/lib/Target/AMDGPU/SILowerControlFlow.cpp

void SILowerControlFlow::emitIf(MachineInstr &MI) {
 
   unsigned SaveExecReg = SaveExec.getReg();
 
   MachineOperand &ImpDefSCC = MI.getOperand(4);
   assert(ImpDefSCC.getReg() == AMDGPU::SCC && ImpDefSCC.isDef());
 
   // Add an implicit def of exec to discourage scheduling VALU after this which
   // will interfere with trying to form s_and_saveexec_b64 later.
+  unsigned CopyReg = MRI->createVirtualRegister(&AMDGPU::SReg_64RegClass);
   MachineInstr *CopyExec =
-    BuildMI(MBB, I, DL, TII->get(AMDGPU::COPY), SaveExecReg)
+    BuildMI(MBB, I, DL, TII->get(AMDGPU::COPY), CopyReg)
     .addReg(AMDGPU::EXEC)
     .addReg(AMDGPU::EXEC, RegState::ImplicitDefine);
 
   unsigned Tmp = MRI->createVirtualRegister(&AMDGPU::SReg_64RegClass);
 
   MachineInstr *And =
     BuildMI(MBB, I, DL, TII->get(AMDGPU::S_AND_B64), Tmp)
-    .addReg(SaveExecReg)
+    .addReg(CopyReg)
     //.addReg(AMDGPU::EXEC)
     .addReg(Cond.getReg());
   setImpSCCDefDead(*And, true);
 
   MachineInstr *Xor =
     BuildMI(MBB, I, DL, TII->get(AMDGPU::S_XOR_B64), SaveExecReg)
     .addReg(Tmp)
-    .addReg(SaveExecReg);
+    .addReg(CopyReg);
   setImpSCCDefDead(*Xor, ImpDefSCC.isDead());
 
   // Use a copy that is a terminator to get correct spill code placement it with
   // fast regalloc.
   MachineInstr *SetExec =
     BuildMI(MBB, I, DL, TII->get(AMDGPU::S_MOV_B64_term), AMDGPU::EXEC)
     .addReg(Tmp, RegState::Kill);
 
void SILowerControlFlow::emitIf(MachineInstr &MI) {
   MI.eraseFromParent();
 
   // FIXME: Is there a better way of adjusting the liveness? It shouldn't be
   // hard to add another def here but I'm not sure how to correctly update the
   // valno.
   LIS->removeInterval(SaveExecReg);
   LIS->createAndComputeVirtRegInterval(SaveExecReg);
   LIS->createAndComputeVirtRegInterval(Tmp);
+  LIS->createAndComputeVirtRegInterval(CopyReg);
 }
 
 void SILowerControlFlow::emitElse(MachineInstr &MI) {
   MachineBasicBlock &MBB = *MI.getParent();
   const DebugLoc &DL = MI.getDebugLoc();
 
   unsigned DstReg = MI.getOperand(0).getReg();
   assert(MI.getOperand(0).getSubReg() == AMDGPU::NoSubRegister);
 
   bool ExecModified = MI.getOperand(3).getImm() != 0;
   MachineBasicBlock::iterator Start = MBB.begin();
 
   // We are running before TwoAddressInstructions, and si_else's operands are
   // tied. In order to correctly tie the registers, split this into a copy of
   // the src like it does.
-  BuildMI(MBB, Start, DL, TII->get(AMDGPU::COPY), DstReg)
+  unsigned CopyReg = MRI->createVirtualRegister(&AMDGPU::SReg_64RegClass);
+  BuildMI(MBB, Start, DL, TII->get(AMDGPU::COPY), CopyReg)
     .addOperand(MI.getOperand(1)); // Saved EXEC
 
   // This must be inserted before phis and any spill code inserted before the
   // else.
+  unsigned SaveReg = ExecModified ?
+    MRI->createVirtualRegister(&AMDGPU::SReg_64RegClass) : DstReg;
   MachineInstr *OrSaveExec =
-    BuildMI(MBB, Start, DL, TII->get(AMDGPU::S_OR_SAVEEXEC_B64), DstReg)
-    .addReg(DstReg);
+    BuildMI(MBB, Start, DL, TII->get(AMDGPU::S_OR_SAVEEXEC_B64), SaveReg)
+    .addReg(CopyReg);
 
   MachineBasicBlock *DestBB = MI.getOperand(2).getMBB();
 
   MachineBasicBlock::iterator ElsePt(MI);
 
   if (ExecModified) {
     MachineInstr *And =
       BuildMI(MBB, ElsePt, DL, TII->get(AMDGPU::S_AND_B64), DstReg)
       .addReg(AMDGPU::EXEC)
-      .addReg(DstReg);
+      .addReg(SaveReg);
 
     if (LIS)
       LIS->InsertMachineInstrInMaps(*And);
   }
 
   MachineInstr *Xor =
     BuildMI(MBB, ElsePt, DL, TII->get(AMDGPU::S_XOR_B64_term), AMDGPU::EXEC)
     .addReg(AMDGPU::EXEC)
void SILowerControlFlow::emitElse(MachineInstr &MI) {
   LIS->InsertMachineInstrInMaps(*OrSaveExec);
 
   LIS->InsertMachineInstrInMaps(*Xor);
   LIS->InsertMachineInstrInMaps(*Branch);
 
   // src reg is tied to dst reg.
   LIS->removeInterval(DstReg);
   LIS->createAndComputeVirtRegInterval(DstReg);
+  LIS->createAndComputeVirtRegInterval(CopyReg);
+  if (ExecModified)
+    LIS->createAndComputeVirtRegInterval(SaveReg);
 
   // Let this be recomputed.
   LIS->removeRegUnit(*MCRegUnitIterator(AMDGPU::EXEC, TRI));
 }
 
 void SILowerControlFlow::emitBreak(MachineInstr &MI) {
   MachineBasicBlock &MBB = *MI.getParent();
   const DebugLoc &DL = MI.getDebugLoc();

llvm/trunk/test/CodeGen/AMDGPU/branch-condition-and.ll

 ; RUN: llc -march=amdgcn -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
 ; RUN: llc -march=amdgcn -mcpu=tonga -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
 
 ; This used to crash because during intermediate control flow lowering, there
 ; was a sequence
 ;       s_mov_b64 s[0:1], exec
 ;       s_and_b64 s[2:3], s[0:1], s[2:3] ; def & use of the same register pair
 ;       ...
 ;       s_mov_b64_term exec, s[2:3]
 ; that was not treated correctly.
 ;
 ; GCN-LABEL: {{^}}ham:
 ; GCN-DAG: v_cmp_lt_f32_e64 [[OTHERCC:s\[[0-9]+:[0-9]+\]]],
 ; GCN-DAG: v_cmp_lt_f32_e32 vcc,
 ; GCN: s_and_b64 [[AND:s\[[0-9]+:[0-9]+\]]], vcc, [[OTHERCC]]
 ; GCN: s_and_saveexec_b64 [[SAVED:s\[[0-9]+:[0-9]+\]]], [[AND]]
-; GCN: s_xor_b64 [[SAVED]], exec, [[SAVED]]
+; GCN: s_xor_b64 {{s\[[0-9]+:[0-9]+\]}}, exec, [[SAVED]]
 ;
 ; TODO: The following sequence is a bug (missing s_endpgm)!
 ;
 ; GCN: s_branch [[BB:BB[0-9]+_[0-9]+]]
 ; GCN: [[BB]]:
 ; GCN-NEXT: .Lfunc_end0:
 define amdgpu_ps void @ham(float %arg, float %arg1) #0 {
 bb:

llvm/trunk/test/CodeGen/AMDGPU/valu-i1.ll

 ; RUN: llc -march=amdgcn -verify-machineinstrs -enable-misched -asm-verbose < %s | FileCheck -check-prefix=SI %s
 
 declare i32 @llvm.amdgcn.workitem.id.x() nounwind readnone
 
 ; SI-LABEL: {{^}}test_if:
 ; Make sure the i1 values created by the cfg structurizer pass are
 ; moved using VALU instructions
 
 
 ; waitcnt should be inserted after exec modification
 ; SI: v_cmp_lt_i32_e32 vcc, 0,
-; SI-NEXT: s_and_saveexec_b64 [[SAVE:s\[[0-9]+:[0-9]+\]]], vcc
-; SI-NEXT: s_xor_b64 [[SAVE]], exec, [[SAVE]]
+; SI-NEXT: s_and_saveexec_b64 [[SAVE1:s\[[0-9]+:[0-9]+\]]], vcc
+; SI-NEXT: s_xor_b64 [[SAVE2:s\[[0-9]+:[0-9]+\]]], exec, [[SAVE1]]
 ; SI-NEXT: s_waitcnt lgkmcnt(0)
 ; SI-NEXT: ; mask branch [[FLOW_BB:BB[0-9]+_[0-9]+]]
 ; SI-NEXT: s_cbranch_execz [[FLOW_BB]]
 
 ; SI-NEXT: BB{{[0-9]+}}_1: ; %LeafBlock3
 ; SI-NOT: s_mov_b64 s[{{[0-9]:[0-9]}}], -1
 ; SI: v_mov_b32_e32 v{{[0-9]}}, -1
 ; SI: s_and_saveexec_b64
 ; SI-NEXT: s_xor_b64
 ; SI-NEXT: ; mask branch
 
 ; v_mov should be after exec modification
 ; SI: [[FLOW_BB]]:
-; SI-NEXT: s_or_saveexec_b64 [[SAVE]], [[SAVE]]
+; SI-NEXT: s_or_saveexec_b64 [[SAVE3:s\[[0-9]+:[0-9]+\]]], [[SAVE2]]
 ; SI-NEXT: v_mov_b32_e32 v{{[0-9]+}}
-; SI-NEXT: s_xor_b64 exec, exec, [[SAVE]]
+; SI-NEXT: s_xor_b64 exec, exec, [[SAVE3]]
 ; SI-NEXT: ; mask branch
 ;
 define void @test_if(i32 %b, i32 addrspace(1)* %src, i32 addrspace(1)* %dst) #1 {
 entry:
   %tid = call i32 @llvm.amdgcn.workitem.id.x() nounwind readnone
   switch i32 %tid, label %default [
     i32 0, label %case0
     i32 1, label %case1

 ; Clear exec bits for workitems that load -1s
 ; SI: [[LABEL_LOOP:BB[0-9]+_[0-9]+]]:
 ; SI: buffer_load_dword [[B:v[0-9]+]]
 ; SI: buffer_load_dword [[A:v[0-9]+]]
 ; SI-DAG: v_cmp_ne_u32_e64 [[NEG1_CHECK_0:s\[[0-9]+:[0-9]+\]]], -1, [[A]]
 ; SI-DAG: v_cmp_ne_u32_e32 [[NEG1_CHECK_1:vcc]], -1, [[B]]
 ; SI: s_and_b64 [[ORNEG1:s\[[0-9]+:[0-9]+\]]], [[NEG1_CHECK_1]], [[NEG1_CHECK_0]]
 ; SI: s_and_saveexec_b64 [[ORNEG2:s\[[0-9]+:[0-9]+\]]], [[ORNEG1]]
-; SI: s_xor_b64 [[ORNEG2]], exec, [[ORNEG2]]
+; SI: s_xor_b64 [[ORNEG3:s\[[0-9]+:[0-9]+\]]], exec, [[ORNEG2]]
 ; SI: s_cbranch_execz [[LABEL_FLOW:BB[0-9]+_[0-9]+]]
 
 ; SI: BB{{[0-9]+_[0-9]+}}: ; %bb20
 ; SI: buffer_store_dword
 ; SI: v_cmp_ge_i64_e32 [[CMP:s\[[0-9]+:[0-9]+\]|vcc]]
 ; SI: s_or_b64 [[TMP:s\[[0-9]+:[0-9]+\]]], [[CMP]], [[COND_STATE]]
 
 ; SI: [[LABEL_FLOW]]:
 ; SI-NEXT: ; in Loop: Header=[[LABEL_LOOP]]
-; SI-NEXT: s_or_b64 exec, exec, [[ORNEG2]]
-; SI-NEXT: s_or_b64 [[COND_STATE]], [[ORNEG2]], [[TMP]]
+; SI-NEXT: s_or_b64 exec, exec, [[ORNEG3]]
+; SI-NEXT: s_or_b64 [[COND_STATE]], [[ORNEG3]], [[TMP]]
 ; SI-NEXT: s_andn2_b64 exec, exec, [[COND_STATE]]
 ; SI-NEXT: s_cbranch_execnz [[LABEL_LOOP]]
 
 ; SI: BB#5
 ; SI: s_or_b64 exec, exec, [[COND_STATE]]
 
 ; SI: [[LABEL_EXIT]]:
 ; SI-NOT: [[COND_STATE]]