[AMDGPU] Fix SGPR fixing through SCC chaining

[hliao]

Summary

• During the fixing of SGPR copying from VGPR, ensure users of SCC is properly propagated, i.e.
  • only propagate through live def of SCC,
  • skip the SCC-def inst itself, and
  • stop the propagation on the other SCC-def inst after checking its SCC-use first.

[arsenm]

I'm not sure I understand the exact problem being solved here. What are the observable consequences?

llvm/test/CodeGen/AMDGPU/udivrem64.ll

172–184 ↗

(On Diff #190611)

I don't see how this is stressing this. Can you add another, more targeted test for this? The div expansion is huge

[hliao]

llvm/test/CodeGen/AMDGPU/udivrem64.ll

172–184 ↗

(On Diff #190611)

This is case where I found the long SCC chain(s). Yeah, the expansion is huge. But, to constructing a SCC chain with parts of instructions without SGPR dependency but SCC dependency is hard, also that sequence needs part of code has to use VGPR. Could you just use this minimalist LLVM IR test?

[arsenm]

llvm/test/CodeGen/AMDGPU/udivrem64.ll

172–184 ↗

(On Diff #190611)

You can also use a MIR test

[hliao]

Add minimal MIR test

[hliao]

llvm/test/CodeGen/AMDGPU/udivrem64.ll

172–184 ↗

(On Diff #190611)

added in the latest patch

[arsenm]

LGTM. I'm a bit iffy about relying on dead flags though, but I think it's OK.

llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.h

void splitScalar64BitBFE(SetVectorType &Worklist,
                            MachineInstr &Inst) const;
   void movePackToVALU(SetVectorType &Worklist,
                       MachineRegisterInfo &MRI,
                       MachineInstr &Inst) const;
 
   void addUsersToMoveToVALUWorklist(unsigned Reg, MachineRegisterInfo &MRI,
                                     SetVectorType &Worklist) const;
 
-  void
-  addSCCDefUsersToVALUWorklist(MachineInstr &SCCDefInst,
+  void addSCCDefUsersToVALUWorklist(MachineOperand &Op,
+                                    MachineInstr &SCCDefInst,
                                     SetVectorType &Worklist) const;
 
   const TargetRegisterClass *
   getDestEquivalentVGPRClass(const MachineInstr &Inst) const;
 
   bool checkInstOffsetsDoNotOverlap(MachineInstr &MIa, MachineInstr &MIb) const;
 
   unsigned findUsedSGPR(const MachineInstr &MI, int OpIndices[3]) const;
 

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

while (!Worklist.empty()) {
     Inst.setDesc(NewDesc);
 
     // Remove any references to SCC. Vector instructions can't read from it, and
     // We're just about to add the implicit use / defs of VCC, and we don't want
     // both.
     for (unsigned i = Inst.getNumOperands() - 1; i > 0; --i) {
       MachineOperand &Op = Inst.getOperand(i);
       if (Op.isReg() && Op.getReg() == AMDGPU::SCC) {
+        // Only propagate through live-def of SCC.
+        if (Op.isDef() && !Op.isDead())
+          addSCCDefUsersToVALUWorklist(Op, Inst, Worklist);
         Inst.RemoveOperand(i);
-        addSCCDefUsersToVALUWorklist(Inst, Worklist);
       }
     }
 
     if (Opcode == AMDGPU::S_SEXT_I32_I8 || Opcode == AMDGPU::S_SEXT_I32_I16) {
       // We are converting these to a BFE, so we need to add the missing
       // operands for the size and offset.
       unsigned Size = (Opcode == AMDGPU::S_SEXT_I32_I8) ? 8 : 16;
       Inst.addOperand(MachineOperand::CreateImm(0));
default:
     llvm_unreachable("unhandled s_pack_* instruction");
   }
 
   MachineOperand &Dest = Inst.getOperand(0);
   MRI.replaceRegWith(Dest.getReg(), ResultReg);
   addUsersToMoveToVALUWorklist(ResultReg, MRI, Worklist);
 }
 
-void SIInstrInfo::addSCCDefUsersToVALUWorklist(
-    MachineInstr &SCCDefInst, SetVectorType &Worklist) const {
+void SIInstrInfo::addSCCDefUsersToVALUWorklist(MachineOperand &Op,
+                                               MachineInstr &SCCDefInst,
+                                               SetVectorType &Worklist) const {
+  // Ensure that def inst defines SCC, which is still live.
+  assert(Op.isReg() && Op.getReg() == AMDGPU::SCC && Op.isDef() &&
+         !Op.isDead() && Op.getParent() == &SCCDefInst);
   // This assumes that all the users of SCC are in the same block
   // as the SCC def.
-  for (MachineInstr &MI :
-       make_range(MachineBasicBlock::iterator(SCCDefInst),
-                      SCCDefInst.getParent()->end())) {
+  for (MachineInstr &MI : // Skip the def inst itself.
+       make_range(std::next(MachineBasicBlock::iterator(SCCDefInst)),
+    // Check if SCC is used first.
+    if (MI.findRegisterUseOperandIdx(AMDGPU::SCC, false, &RI) != -1)
+      Worklist.insert(&MI);
     // Exit if we find another SCC def.
     if (MI.findRegisterDefOperandIdx(AMDGPU::SCC, false, false, &RI) != -1)
       return;
-
-    if (MI.findRegisterUseOperandIdx(AMDGPU::SCC, false, &RI) != -1)
-      Worklist.insert(&MI);
   }
 }
 
 const TargetRegisterClass *SIInstrInfo::getDestEquivalentVGPRClass(
   const MachineInstr &Inst) const {
   const TargetRegisterClass *NewDstRC = getOpRegClass(Inst, 0);
 
   switch (Inst.getOpcode()) {

llvm/trunk/test/CodeGen/AMDGPU/fix-sgpr-copies.mir

+# RUN: llc -march=amdgcn -run-pass=si-fix-sgpr-copies -o - %s | FileCheck --check-prefix=GCN %s
+
+# GCN-LABEL: name: fix-sgpr-copies
+# GCN: V_ADD_I32_e32
+# GCN: V_ADDC_U32_e32
+---
+name: fix-sgpr-copies
+body:               |
+  bb.0:
+    %0:vgpr_32 = IMPLICIT_DEF
+    %1:sreg_32 = IMPLICIT_DEF
+    %2:sreg_32 = IMPLICIT_DEF
+    %3:sreg_32 = IMPLICIT_DEF
+    %4:vgpr_32 = V_CVT_U32_F32_e64 0, %0:vgpr_32, 0, 0, implicit $exec
+    %5:sreg_32 = COPY %4:vgpr_32
+    %6:sreg_32 = S_ADD_I32 %2:sreg_32, %5:sreg_32, implicit-def $scc
+    %7:sreg_32 = S_ADDC_U32 %3:sreg_32, %1:sreg_32, implicit-def $scc, implicit $scc
+...

llvm/trunk/test/CodeGen/AMDGPU/udivrem64.ll

 ;GCN: s_endpgm
 define amdgpu_kernel void @test_urem2364(i64 addrspace(1)* %out, i64 %x, i64 %y) {
   %1 = lshr i64 %x, 41
   %2 = lshr i64 %y, 41
   %result = urem i64 %1, %2
   store i64 %result, i64 addrspace(1)* %out
   ret void
 }
+
+;FUNC-LABEL: {{^}}test_udiv_k:
+;GCN: v_mul{{.+}} v{{[0-9]+}}, v{{[0-9]+}}, 24
+;GCN: v_mul{{.+}} v{{[0-9]+}}, v{{[0-9]+}}, 24
+;GCN: v_mul{{.+}} v{{[0-9]+}}, v{{[0-9]+}}, 24
+;GCN: v_add
+;GCN: v_addc
+;GCN: v_addc
+;GCN: s_endpgm
+define amdgpu_kernel void @test_udiv_k(i64 addrspace(1)* %out, i64 %x) {
+  %result = udiv i64 24, %x
+  store i64 %result, i64 addrspace(1)* %out
+  ret void
+}