[AMDGPU][GFX908] IndirectCopyToAGPR: Confirm modified register is dst reg of accvgpr_write

[jrbyrnes]

Summary

IndirectCopyToAGPR should be reworked as to avoid optimizing during copy lowering. However, as it stands, the code is buggy. This patch replaces the call to definesRegister with modifiesRegister, and confirms that the dest reg of the found accvgpr_write is in fact the src reg of our copy.

[arsenm]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

591

This would be illegal, a def always must be a register

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

926–929

Should precommit this test to show the diff here

1005

Extra whitespace change

[jrbyrnes]

Remove redundant check + rebase on top of precommit test.

The test shows that indirectCopyToAGPR has incorrectly determined that (at the time of COPY agpr1_agpr2) we can use vgpr0 as having the same bits as agpr1 AND agpr2 (in reality, of the two, it is only consistent with agpr1). We incorrectly make the determination by our assumption of what "definesRegister" means.

[arsenm]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

589–590

This comment is also stating the obvious

591

The assert is also redundant, getReg does that for you

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

1005

Still extra whitespace change

[jrbyrnes]

Address comments

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

static void indirectCopyToAGPR(const SIInstrInfo &TII,
 
   // First try to find defining accvgpr_write to avoid temporary registers.
   // In the case of copies of overlapping AGPRs, we conservatively do not
   // reuse previous accvgpr_writes. Otherwise, we may incorrectly pick up
   // an accvgpr_write used for this same copy due to implicit-defs
   if (!RegsOverlap) {
     for (auto Def = MI, E = MBB.begin(); Def != E; ) {
       --Def;
-      if (!Def->definesRegister(SrcReg, &RI))
+
+      if (!Def->modifiesRegister(SrcReg, &RI))
         continue;
-      if (Def->getOpcode() != AMDGPU::V_ACCVGPR_WRITE_B32_e64)
+
+      if (Def->getOpcode() != AMDGPU::V_ACCVGPR_WRITE_B32_e64 ||
+          Def->getOperand(0).getReg() != SrcReg)
         break;
 
       MachineOperand &DefOp = Def->getOperand(1);
       assert(DefOp.isReg() || DefOp.isImm());
 
       if (DefOp.isReg()) {
         bool SafeToPropagate = true;
         // Check that register source operand is not clobbered before MI.
         // Immediate operands are always safe to propagate.
         for (auto I = Def; I != MI && SafeToPropagate; ++I)
           if (I->modifiesRegister(DefOp.getReg(), &RI))
             SafeToPropagate = false;
 

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

body:             |
   bb.0:
     liveins: $agpr0_agpr1
     ; GFX908-LABEL: name: a2_to_a2_implicit_defs
     ; GFX908: liveins: $agpr0_agpr1
     ; GFX908-NEXT: {{  $}}
     ; GFX908-NEXT: $vgpr1 = V_ACCVGPR_READ_B32_e64 $agpr1, implicit $exec, implicit $agpr0_agpr1
     ; GFX908-NEXT: $agpr2 = V_ACCVGPR_WRITE_B32_e64 $vgpr1, implicit $exec, implicit-def $agpr1_agpr2
     ; GFX908-NEXT: $vgpr0 = V_ACCVGPR_READ_B32_e64 $agpr0, implicit $exec, implicit $agpr0_agpr1
-    ; GFX908-NEXT: $agpr1 = V_ACCVGPR_WRITE_B32_e64 $vgpr0, implicit $exec, implicit $exec, implicit-def $agpr1_agpr2
-    ; GFX908-NEXT: $agpr4 = V_ACCVGPR_WRITE_B32_e64 $vgpr0, implicit $exec, implicit-def $agpr3_agpr4, implicit $agpr1_agpr2
-    ; GFX908-NEXT: $agpr3 = V_ACCVGPR_WRITE_B32_e64 $vgpr0, implicit $exec, implicit killed $agpr1_agpr2, implicit $exec
+    ; GFX908-NEXT: $agpr1 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr0, implicit $exec, implicit $exec, implicit-def $agpr1_agpr2
+    ; GFX908-NEXT: $vgpr0 = V_ACCVGPR_READ_B32_e64 $agpr2, implicit $exec, implicit $agpr1_agpr2
+    ; GFX908-NEXT: $agpr4 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr0, implicit $exec, implicit-def $agpr3_agpr4
+    ; GFX908-NEXT: $vgpr255 = V_ACCVGPR_READ_B32_e64 killed $agpr1, implicit $exec, implicit killed $agpr1_agpr2
+    ; GFX908-NEXT: $agpr3 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr255, implicit $exec, implicit $exec
     ; GFX90A-LABEL: name: a2_to_a2_implicit_defs
     ; GFX90A: liveins: $agpr0_agpr1
     ; GFX90A-NEXT: {{  $}}
     ; GFX90A-NEXT: $vgpr1 = V_ACCVGPR_READ_B32_e64 $agpr1, implicit $exec, implicit $agpr0_agpr1
     ; GFX90A-NEXT: $agpr2 = V_ACCVGPR_WRITE_B32_e64 $vgpr1, implicit $exec, implicit-def $agpr1_agpr2
     ; GFX90A-NEXT: $vgpr0 = V_ACCVGPR_READ_B32_e64 $agpr0, implicit $exec, implicit $agpr0_agpr1
     ; GFX90A-NEXT: $agpr1 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr0, implicit $exec, implicit $exec, implicit-def $agpr1_agpr2
     ; GFX90A-NEXT: $agpr4 = V_ACCVGPR_MOV_B32 $agpr2, implicit $exec, implicit-def $agpr3_agpr4, implicit $agpr1_agpr2
     ; GFX90A-NEXT: $agpr3 = V_ACCVGPR_MOV_B32 $agpr1, implicit $exec, implicit killed $agpr1_agpr2, implicit $exec
     ; GFX940-LABEL: name: a2_to_a2_implicit_defs
     ; GFX940: liveins: $agpr0_agpr1
     ; GFX940-NEXT: {{  $}}
     ; GFX940-NEXT: $vgpr1 = V_ACCVGPR_READ_B32_e64 $agpr1, implicit $exec, implicit $agpr0_agpr1
     ; GFX940-NEXT: $agpr2 = V_ACCVGPR_WRITE_B32_e64 $vgpr1, implicit $exec, implicit-def $agpr1_agpr2
     ; GFX940-NEXT: $vgpr0 = V_ACCVGPR_READ_B32_e64 $agpr0, implicit $exec, implicit $agpr0_agpr1
     ; GFX940-NEXT: $agpr1 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr0, implicit $exec, implicit $exec, implicit-def $agpr1_agpr2
     ; GFX940-NEXT: $agpr4 = V_ACCVGPR_MOV_B32 $agpr2, implicit $exec, implicit-def $agpr3_agpr4, implicit $agpr1_agpr2
     ; GFX940-NEXT: $agpr3 = V_ACCVGPR_MOV_B32 $agpr1, implicit $exec, implicit killed $agpr1_agpr2, implicit $exec
     $vgpr1 = V_ACCVGPR_READ_B32_e64 $agpr1, implicit $exec, implicit $agpr0_agpr1
     $agpr2 = V_ACCVGPR_WRITE_B32_e64 $vgpr1, implicit $exec, implicit-def $agpr1_agpr2
     $vgpr0 = V_ACCVGPR_READ_B32_e64 $agpr0, implicit $exec, implicit $agpr0_agpr1
     $agpr1 = V_ACCVGPR_WRITE_B32_e64 killed $vgpr0, implicit $exec, implicit $exec, implicit-def $agpr1_agpr2
     $agpr3_agpr4 = COPY killed $agpr1_agpr2, implicit $exec
 ...
 
 ---
 name:            a3_to_a3_nonoverlap_kill
 tracksRegLiveness: true
 body:             |
   bb.0:
bb.0:
     ; GFX940-NEXT: $agpr2 = V_ACCVGPR_MOV_B32 $agpr3, implicit $exec, implicit $agpr1_agpr2_agpr3
     ; GFX940-NEXT: $vgpr1 = V_ACCVGPR_READ_B32_e64 $agpr1, implicit $exec
     ; GFX940-NEXT: S_ENDPGM 0, implicit $agpr0_agpr1_agpr2, implicit $vgpr1
     $agpr0_agpr1_agpr2 = COPY killed $agpr1_agpr2_agpr3
     $vgpr1 = COPY $agpr1
     S_ENDPGM 0, implicit $agpr0_agpr1_agpr2, implicit $vgpr1
 ...
 
 ---
 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 killed $vgpr1, implicit $exec, implicit-def $agpr2_agpr3_agpr4_agpr5