[AMDGPU][GFX908] Only consider explicit defs of src reg in indirect agpr copy

[jrbyrnes]

Summary

When doing indirect copy to AGPR, we parse all previous instructions in attempt to find an accvgpr_write which has a dest that holds the src of the copy -- we can use this instead of reconstructing a separate accvgpr_write. However, after https://github.com/llvm/llvm-project/commit/663f16684d1a5f129874b7be9e1f486682977bfa we add implicit defs of the wide AGPR to AGPR spills. Thus, we previously flag accvgpr_writes as holding some particular subreg of the src of the copy, when, in fact, they hold some other subreg of the src. Ideally, we would not need to attach implicit_defs for each agpr spill. This patch resolves the bug until we have a better way of doing things. It modifies indirectCopyToAGPR s.t. it only considers accvgpr_writes that explicitly define the vgpr.

[arsenm]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

577

Assuming the intent of implicit defs makes me nervous. They're there for liveness representation we don't want to just ignore. What does this solve exactly? I thought we always reserved a register for this now, so you don't need to actually scan for anything

[jrbyrnes]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

577

We don't need to scan for anything -- it is just an optimization. When copying agprs on gfx908, this code is used to pick up the case where we already have an accvgpr_write that does the same thing as one we are about to create. However, it is incorrectly picking up accvgpr_writes because we have attached to spills implicit def of the entire wide agpr.

So when lowering
agpr0_agpr1 = copy agpr2_agpr3,
we will find
vgprx = v_accvgpr_write agpr2, implicit-def agpr2_agpr3
and incorrectly use vgprx as holding agpr3 due to implicit-def.

If we don't find any such accvgpr_write, then we use a temp register to copy over. The register we use for this takes into account the reserved registers to find an unused one. The other patch was to mark vgpr used for agpr spills as reserved so the temp didn't clobber.

I think this case is niche enough to be able to assume the intent of implicit-def. With this patch we at least err on the side of caution in that we only apply optimization if the found instruction explicitly defines our reg. Furthermore, if we do find such a reg, we check instructions in between that def and our use for any modifications to this reg (including implicit defs).

[arsenm]

This isn’t the right place to apply optimizations. I’ve wanted to delete all of this code.

Copy lowering should be as straightforward as possible. What we have now is doing a liveness scan for each copy, which is crazy. We should either avoid this situation in the first place by applying implicit VGPR virtual register defs so the allocator ensures one is free at this point, or optimize all the AGPR copies at once after the fact

[jrbyrnes]

This isn’t the right place to apply optimizations. I’ve wanted to delete all of this code.

Copy lowering should be as straightforward as possible. What we have now is doing a liveness scan for each copy, which is crazy. We should either avoid this situation in the first place by applying implicit VGPR virtual register defs so the allocator ensures one is free at this point, or optimize all the AGPR copies at once after the fact

indirectCopyToAGPR performs two optimizations when lowering copies.

1. reuse existing accvgpr_write instead of creating a new one. If we find a match, we can reuse accvgpr_write into our dest, rather than accvpgr_read the source into vgpr, then accvgpr_write that vgpr int our dest. This involves scanning to find accvgpr_writes then to check if candidate duplicate accvpgr_writes are modified.
2. allocate vgprs used in the copy via round-robin of a triplet. This is to avoid a hazard. This involves scanning via RS.

Since the goal of expand-postra-pseudos is to lower pseudos and not apply these optimizations, it makes sense to move these elsewhere. Perhaps it makes the most sense to have a pass to do pseudo expansion cleanup (i.e. handle both of these, and more if desired)? Attaching implicit virtual vgpr defs may enable 2., but I think there would be no guarantee that they would all be allocated in the desired fashion. I think this would allow us to get rid of VGPRForAGPR copy though.

Regardless, in facilitating 1., even in a separate pass, I think that having implicit-defs of agprs attached to instructions that don't define them will pose problems, and we may end up wanting the explicitlyDefines functionality.

I propose to use this patch to provide fix for relatively high priority correctness issue, and introduce explicitlyDefines. Then, in separate work, clean up the indirectCopyToAGPR (potentially via new pass). Otherwise, I can handle above concerns here.

[jrbyrnes]

Abandon in favor of https://reviews.llvm.org/D141101

llvm/include/llvm/CodeGen/MachineInstr.h

   /// If TargetRegisterInfo is passed, then it also checks
   /// if there is a def of a super-register.
   /// NOTE: It's ignoring subreg indices on virtual registers.
   bool definesRegister(Register Reg,
                        const TargetRegisterInfo *TRI = nullptr) const {
     return findRegisterDefOperandIdx(Reg, false, false, TRI) != -1;
   }
 
+  bool explicitlyDefinesRegister(Register Reg,
+                                 const TargetRegisterInfo *TRI = nullptr) const {
+    return findRegisterDefOperandIdx(Reg, false, false, TRI, true) != -1;
+  }
+
   /// Return true if the MachineInstr modifies (fully define or partially
   /// define) the specified register.
   /// NOTE: It's ignoring subreg indices on virtual registers.
   bool modifiesRegister(Register Reg,
                         const TargetRegisterInfo *TRI = nullptr) const {
     return findRegisterDefOperandIdx(Reg, false, true, TRI) != -1;
   }
 

   /// Returns the operand index that is a def of the specified register or
   /// -1 if it is not found. If isDead is true, defs that are not dead are
   /// skipped. If Overlap is true, then it also looks for defs that merely
   /// overlap the specified register. If TargetRegisterInfo is non-null,
   /// then it also checks if there is a def of a super-register.
   /// This may also return a register mask operand when Overlap is true.
   int findRegisterDefOperandIdx(Register Reg,
                                 bool isDead = false, bool Overlap = false,
-                                const TargetRegisterInfo *TRI = nullptr) const;
+                                const TargetRegisterInfo *TRI = nullptr,
+                                bool isExplicit = false) const;
 
   /// Wrapper for findRegisterDefOperandIdx, it returns
   /// a pointer to the MachineOperand rather than an index.
   MachineOperand *
   findRegisterDefOperand(Register Reg, bool isDead = false,
                          bool Overlap = false,
                          const TargetRegisterInfo *TRI = nullptr) {
     int Idx = findRegisterDefOperandIdx(Reg, isDead, Overlap, TRI);

llvm/lib/CodeGen/MachineInstr.cpp

 }
 
 /// findRegisterDefOperandIdx() - Returns the operand index that is a def of
 /// the specified register or -1 if it is not found. If isDead is true, defs
 /// that are not dead are skipped. If TargetRegisterInfo is non-null, then it
 /// also checks if there is a def of a super-register.
 int
 MachineInstr::findRegisterDefOperandIdx(Register Reg, bool isDead, bool Overlap,
-                                        const TargetRegisterInfo *TRI) const {
+                                        const TargetRegisterInfo *TRI, bool isExplicit) const {
   bool isPhys = Register::isPhysicalRegister(Reg);
   for (unsigned i = 0, e = getNumOperands(); i != e; ++i) {
     const MachineOperand &MO = getOperand(i);
     // Accept regmask operands when Overlap is set.
     // Ignore them when looking for a specific def operand (Overlap == false).
     if (isPhys && Overlap && MO.isRegMask() && MO.clobbersPhysReg(Reg))
       return i;
-    if (!MO.isReg() || !MO.isDef())
+    if (!MO.isReg() || !MO.isDef() || (isExplicit && MO.isImplicit()))
       continue;
     Register MOReg = MO.getReg();
     bool Found = (MOReg == Reg);
     if (!Found && TRI && isPhys && Register::isPhysicalRegister(MOReg)) {
       if (Overlap)
         Found = TRI->regsOverlap(MOReg, Reg);
       else
         Found = TRI->isSubRegister(MOReg, Reg);

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

static void indirectCopyToAGPR(const SIInstrInfo &TII,
   assert(AMDGPU::AGPR_32RegClass.contains(DestReg) &&
          "Destination register of the copy should be an AGPR.");
 
   const SIRegisterInfo &RI = TII.getRegisterInfo();
 
   // First try to find defining accvgpr_write to avoid temporary registers.
   for (auto Def = MI, E = MBB.begin(); Def != E; ) {
     --Def;
-    if (!Def->definesRegister(SrcReg, &RI))
+    if (!Def->explicitlyDefinesRegister(SrcReg, &RI))
       continue;
     if (Def->getOpcode() != AMDGPU::V_ACCVGPR_WRITE_B32_e64)
       break;
 
     MachineOperand &DefOp = Def->getOperand(1);
     assert(DefOp.isReg() || DefOp.isImm());
 
     if (DefOp.isReg()) {

llvm/test/CodeGen/AMDGPU/accvgpr-copy-implicit-def.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -march=amdgcn -mcpu=gfx908 -run-pass=postrapseudos -verify-machineinstrs -o - %s | FileCheck -check-prefix=GFX908 %s
+
+---
+name: no_free_vgprs_for_copy_a32_to_a32
+tracksRegLiveness: true
+body: |
+  bb.0:
+  liveins: $sgpr0_sgpr1_sgpr2_sgpr3
+    ; GFX908-LABEL: name: no_free_vgprs_for_copy_a32_to_a32
+    ; GFX908: liveins: $sgpr0_sgpr1_sgpr2_sgpr3
+    ; GFX908-NEXT: {{  $}}
+    ; GFX908-NEXT: $vgpr127 = BUFFER_LOAD_DWORD_OFFSET $sgpr0_sgpr1_sgpr2_sgpr3, 0, 260, 0, 0, implicit $exec
+    ; GFX908-NEXT: $agpr0 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr127, implicit $exec, implicit-def $agpr0_agpr1
+    ; GFX908-NEXT: $vgpr127 = BUFFER_LOAD_DWORD_OFFSET $sgpr0_sgpr1_sgpr2_sgpr3, 0, 264, 0, 0, implicit $exec
+    ; GFX908-NEXT: $agpr1 = V_ACCVGPR_WRITE_B32_e64 $vgpr127, implicit $exec, implicit-def $agpr0_agpr1
+    ; GFX908-NEXT: $agpr5 = V_ACCVGPR_WRITE_B32_e64 $vgpr127, implicit $exec, implicit-def $agpr4_agpr5, implicit $agpr0_agpr1
+    ; GFX908-NEXT: $vgpr0 = V_ACCVGPR_READ_B32_e64 killed $agpr0, implicit $exec, implicit killed $agpr0_agpr1
+    ; GFX908-NEXT: $agpr4 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr0, implicit $exec, implicit $exec
+    ; GFX908-NEXT: S_ENDPGM 0, amdgpu_allvgprs
+  $vgpr127 = BUFFER_LOAD_DWORD_OFFSET $sgpr0_sgpr1_sgpr2_sgpr3, 0, 260, 0, 0, implicit $exec
+  $agpr0 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr127, implicit $exec, implicit-def $agpr0_agpr1
+  $vgpr127 = BUFFER_LOAD_DWORD_OFFSET $sgpr0_sgpr1_sgpr2_sgpr3, 0, 264, 0, 0, implicit $exec
+  $agpr1 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr127, implicit $exec, implicit-def $agpr0_agpr1
+  $agpr4_agpr5 = COPY killed $agpr0_agpr1, implicit $exec
+  S_ENDPGM 0, amdgpu_allvgprs
+...

llvm/test/CodeGen/AMDGPU/accvgpr-copy.mir

 tracksRegLiveness: true
 body:             |
   bb.0:
     liveins: $agpr1_agpr2_agpr3
     ; GFX908-LABEL: name: a3_to_a3_overlap_kill
     ; GFX908: liveins: $agpr1_agpr2_agpr3
     ; GFX908-NEXT: {{  $}}
     ; GFX908-NEXT: $vgpr255 = V_ACCVGPR_READ_B32_e64 $agpr1, implicit $exec, implicit $agpr1_agpr2_agpr3
-    ; GFX908-NEXT: $agpr0 = V_ACCVGPR_WRITE_B32_e64 $vgpr255, implicit $exec, implicit-def $agpr0_agpr1_agpr2
-    ; GFX908-NEXT: $agpr1 = V_ACCVGPR_WRITE_B32_e64 $vgpr255, implicit $exec, implicit $agpr1_agpr2_agpr3
+    ; GFX908-NEXT: $agpr0 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr255, implicit $exec, implicit-def $agpr0_agpr1_agpr2
+    ; GFX908-NEXT: $vgpr0 = V_ACCVGPR_READ_B32_e64 $agpr2, implicit $exec, implicit $agpr1_agpr2_agpr3
+    ; GFX908-NEXT: $agpr1 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr0, implicit $exec
     ; GFX908-NEXT: $vgpr2 = V_ACCVGPR_READ_B32_e64 $agpr3, implicit $exec, implicit $agpr1_agpr2_agpr3
     ; GFX908-NEXT: $agpr2 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr2, implicit $exec
     ; GFX908-NEXT: $vgpr1 = V_ACCVGPR_READ_B32_e64 $agpr1, implicit $exec
     ; GFX908-NEXT: S_ENDPGM 0, implicit $agpr0_agpr1_agpr2, implicit $vgpr1
     ; GFX90A-LABEL: name: a3_to_a3_overlap_kill
     ; GFX90A: liveins: $agpr1_agpr2_agpr3
     ; GFX90A-NEXT: {{  $}}
     ; GFX90A-NEXT: $agpr0 = V_ACCVGPR_MOV_B32 $agpr1, implicit $exec, implicit-def $agpr0_agpr1_agpr2, implicit $agpr1_agpr2_agpr3

 ---
 name:            a4_to_a4
 tracksRegLiveness: true
 body:             |
   bb.0:
     ; GFX908-LABEL: name: a4_to_a4
     ; GFX908: $agpr0_agpr1_agpr2_agpr3 = IMPLICIT_DEF
     ; GFX908-NEXT: $vgpr1 = V_ACCVGPR_READ_B32_e64 $agpr3, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3
-    ; GFX908-NEXT: $agpr5 = V_ACCVGPR_WRITE_B32_e64 $vgpr1, implicit $exec, implicit-def $agpr2_agpr3_agpr4_agpr5
-    ; GFX908-NEXT: $agpr4 = V_ACCVGPR_WRITE_B32_e64 $vgpr1, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3
+    ; GFX908-NEXT: $agpr5 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr1, implicit $exec, implicit-def $agpr2_agpr3_agpr4_agpr5
+    ; GFX908-NEXT: $vgpr0 = V_ACCVGPR_READ_B32_e64 $agpr2, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3
+    ; GFX908-NEXT: $agpr4 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr0, implicit $exec
     ; GFX908-NEXT: $vgpr255 = V_ACCVGPR_READ_B32_e64 $agpr1, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3
     ; GFX908-NEXT: $agpr3 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr255, implicit $exec
-    ; GFX908-NEXT: $vgpr2 = V_ACCVGPR_READ_B32_e64 $agpr0, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3
-    ; GFX908-NEXT: $agpr2 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr2, implicit $exec, implicit $exec
+    ; GFX908-NEXT: $vgpr1 = V_ACCVGPR_READ_B32_e64 $agpr0, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3
+    ; GFX908-NEXT: $agpr2 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr1, implicit $exec, implicit $exec
     ; GFX908-NEXT: S_ENDPGM 0, implicit $agpr2_agpr3_agpr4_agpr5
     ; GFX90A-LABEL: name: a4_to_a4
     ; GFX90A: $agpr0_agpr1_agpr2_agpr3 = IMPLICIT_DEF
     ; GFX90A-NEXT: $agpr5 = V_ACCVGPR_MOV_B32 $agpr3, implicit $exec, implicit-def $agpr2_agpr3_agpr4_agpr5, implicit $agpr0_agpr1_agpr2_agpr3
     ; GFX90A-NEXT: $agpr4 = V_ACCVGPR_MOV_B32 $agpr2, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3
     ; GFX90A-NEXT: $agpr3 = V_ACCVGPR_MOV_B32 $agpr1, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3
     ; GFX90A-NEXT: $agpr2 = V_ACCVGPR_MOV_B32 $agpr0, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3, implicit $exec
     ; GFX90A-NEXT: S_ENDPGM 0, implicit $agpr2_agpr3_agpr4_agpr5

 tracksRegLiveness: true
 body:             |
   bb.0:
     liveins: $agpr0_agpr1_agpr2_agpr3
     ; GFX908-LABEL: name: a4_to_a4_overlap
     ; GFX908: liveins: $agpr0_agpr1_agpr2_agpr3
     ; GFX908-NEXT: {{  $}}
     ; GFX908-NEXT: $vgpr1 = V_ACCVGPR_READ_B32_e64 $agpr3, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3
-    ; GFX908-NEXT: $agpr5 = V_ACCVGPR_WRITE_B32_e64 $vgpr1, implicit $exec, implicit-def $agpr2_agpr3_agpr4_agpr5
-    ; GFX908-NEXT: $agpr4 = V_ACCVGPR_WRITE_B32_e64 $vgpr1, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3
+    ; GFX908-NEXT: $agpr5 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr1, implicit $exec, implicit-def $agpr2_agpr3_agpr4_agpr5
+    ; GFX908-NEXT: $vgpr0 = V_ACCVGPR_READ_B32_e64 $agpr2, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3
+    ; GFX908-NEXT: $agpr4 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr0, implicit $exec
     ; GFX908-NEXT: $vgpr255 = V_ACCVGPR_READ_B32_e64 $agpr1, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3
     ; GFX908-NEXT: $agpr3 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr255, implicit $exec
-    ; GFX908-NEXT: $vgpr2 = V_ACCVGPR_READ_B32_e64 $agpr0, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3
-    ; GFX908-NEXT: $agpr2 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr2, implicit $exec, implicit $exec
+    ; GFX908-NEXT: $vgpr1 = V_ACCVGPR_READ_B32_e64 $agpr0, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3
+    ; GFX908-NEXT: $agpr2 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr1, implicit $exec, implicit $exec
     ; GFX908-NEXT: S_ENDPGM 0, implicit $agpr0, implicit $agpr1, implicit $agpr2, implicit $agpr3, implicit $agpr4, implicit $agpr5
     ; GFX90A-LABEL: name: a4_to_a4_overlap
     ; GFX90A: liveins: $agpr0_agpr1_agpr2_agpr3
     ; GFX90A-NEXT: {{  $}}
     ; GFX90A-NEXT: $agpr5 = V_ACCVGPR_MOV_B32 $agpr3, implicit $exec, implicit-def $agpr2_agpr3_agpr4_agpr5, implicit $agpr0_agpr1_agpr2_agpr3
     ; GFX90A-NEXT: $agpr4 = V_ACCVGPR_MOV_B32 $agpr2, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3
     ; GFX90A-NEXT: $agpr3 = V_ACCVGPR_MOV_B32 $agpr1, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3
     ; GFX90A-NEXT: $agpr2 = V_ACCVGPR_MOV_B32 $agpr0, implicit $exec, implicit $agpr0_agpr1_agpr2_agpr3, implicit $exec