[AMDGPU] Fix v_swap_b32 formation on physical registers

[foad]

Summary

As explained in the comments, matchSwap matches:

mov t, x
mov x, y
// mov y, t

and turns it into:

mov t, x (t is potentially dead and move eliminated)
v_swap_b32 x, y

On physical registers we don't have full use-def chains so the check
for T being live-out was not working properly with subregs/superregs.

[arsenm]

We don't need to rely on kill flags (and in fact should not, since they are semi-deprecated). We should be using liveness scans starting from the bottom of the block which do not need kill flags

[foad]

In D101546#2726051, @arsenm wrote:

We don't need to rely on kill flags (and in fact should not, since they are semi-deprecated). We should be using liveness scans starting from the bottom of the block which do not need kill flags

I'm not relying on the kill flag being present for correctness, I'm just taking advantage of it if it does happen to be there (and assuming that it is correct if it is present). If kill flags are not present then I guess you'd need to run MachineDCE again afterwards to clean up.

llvm/lib/Target/AMDGPU/SIShrinkInstructions.cpp

580–583

Does this part make sense? I am really not too sure what kind of liveness information we can rely on here for virtual and/or physical registers.

[arsenm]

In D101546#2726096, @foad wrote:

In D101546#2726051, @arsenm wrote:

We don't need to rely on kill flags (and in fact should not, since they are semi-deprecated). We should be using liveness scans starting from the bottom of the block which do not need kill flags

I'm not relying on the kill flag being present for correctness, I'm just taking advantage of it if it does happen to be there (and assuming that it is correct if it is present). If kill flags are not present then I guess you'd need to run MachineDCE again afterwards to clean up.

I don't mean rely on for correctness, I mean use at all. Ideally we should be working to eliminate dependence on kill flags for optimizations.

[foad]

Don't use kill flags.

[foad]

We should be using liveness scans starting from the bottom of the block which do not need kill flags

To do this efficiently I guess the whole pass needs to be changed to work bottom-up in each basic block. I'd prefer to leave that for a separate patch.

[rampitec]

LGTM

[arsenm]

You can use the kill flags for now but it is something to migrate away from

llvm/lib/Target/AMDGPU/SIShrinkInstructions.cpp

if (!TRI.isVGPR(MRI, X))
     return nullptr;
 
   if (MovT.hasRegisterImplicitUseOperand(AMDGPU::M0))
     return nullptr;
 
   const unsigned SearchLimit = 16;
   unsigned Count = 0;
   bool KilledT = false;
+  bool UsedT = false;
   for (auto Iter = std::next(MovT.getIterator()),
             E = MovT.getParent()->instr_end();
        Iter != E && Count < SearchLimit && !KilledT; ++Iter, ++Count) {
 
     MachineInstr *MovY = &*Iter;
     KilledT = MovY->killsRegister(T, &TRI);
 
+    bool PrevUsedT = UsedT;
+    UsedT = UsedT || instReadsReg(MovY, T, Tsub, TRI);
+
     if ((MovY->getOpcode() != AMDGPU::V_MOV_B32_e32 &&
          MovY->getOpcode() != AMDGPU::COPY) ||
         !MovY->getOperand(1).isReg()        ||
         MovY->getOperand(1).getReg() != T   ||
         MovY->getOperand(1).getSubReg() != Tsub ||
         MovY->hasRegisterImplicitUseOperand(AMDGPU::M0))
       continue;
 
for (unsigned I = 0; I < Size; ++I) {
         MIB->RemoveOperand(MIB->getNumExplicitOperands());
         MIB->copyImplicitOps(*MBB.getParent(), *MovX);
       }
     }
     MovX->eraseFromParent();
     dropInstructionKeepingImpDefs(*MovY, TII);
     MachineInstr *Next = &*std::next(MovT.getIterator());
 
-    if (MRI.use_nodbg_empty(T)) {
+    if (T.isVirtual() ? MRI.use_nodbg_empty(T)
+                      : (MovY->getOperand(1).isKill() && !PrevUsedT)) {
+      // For physical registers: the use of T in MovY is killed, and we didn't
+      // see any other uses of T before MovY.
       dropInstructionKeepingImpDefs(MovT, TII);
     } else {
       Xop.setIsKill(false);
       for (int I = MovT.getNumImplicitOperands() - 1; I >= 0; --I ) {
         unsigned OpNo = MovT.getNumExplicitOperands() + I;
         const MachineOperand &Op = MovT.getOperand(OpNo);
         if (Op.isKill() && TRI.regsOverlap(X, Op.getReg()))
           MovT.RemoveOperand(OpNo);

llvm/test/CodeGen/AMDGPU/v_swap_b32.mir

 # GCN-NEXT: S_SETPC_B64_return
 ---
 name:            swap_phys_liveout
 body:             |
   bb.0:
     liveins: $vgpr0, $vgpr1, $sgpr30_sgpr31
     $vgpr2 = V_MOV_B32_e32 killed $vgpr0, implicit $exec
     $vgpr0 = V_MOV_B32_e32 killed $vgpr1, implicit $exec
-    $vgpr1 = V_MOV_B32_e32 killed $vgpr2, implicit $exec
+    $vgpr1 = V_MOV_B32_e32 $vgpr2, implicit $exec
     S_SETPC_B64_return $sgpr30_sgpr31, implicit $vgpr0, implicit $vgpr2, implicit $vgpr1
 ...
 
-# FIXME: should not remove the def of $vgpr2 because $vgpr2_vgpr3 is live out
 # GCN-LABEL: name: swap_phys_liveout_superreg
 # GCN: bb.0:
+# GCN-NEXT: $vgpr2 = V_MOV_B32_e32 $vgpr0, implicit $exec
 # GCN-NEXT: $vgpr0, $vgpr1 = V_SWAP_B32 $vgpr1, $vgpr0, implicit $exec
 # GCN-NEXT: S_SETPC_B64_return
 ---
 name:            swap_phys_liveout_superreg
 body:             |
   bb.0:
     liveins: $vgpr0, $vgpr1, $sgpr30_sgpr31
     $vgpr2 = V_MOV_B32_e32 killed $vgpr0, implicit $exec
     $vgpr0 = V_MOV_B32_e32 killed $vgpr1, implicit $exec
-    $vgpr1 = V_MOV_B32_e32 killed $vgpr2, implicit $exec
+    $vgpr1 = V_MOV_B32_e32 $vgpr2, implicit $exec
     S_SETPC_B64_return $sgpr30_sgpr31, implicit $vgpr0, implicit $vgpr2_vgpr3, implicit $vgpr1
 ...
 
 # GCN-LABEL: name: swap_phys_b64
 # GCN: bb.0:
 # GCN-NEXT: $vgpr0, $vgpr2 = V_SWAP_B32 $vgpr2, $vgpr0, implicit $exec
 # GCN-NEXT: $vgpr1, $vgpr3 = V_SWAP_B32 $vgpr3, $vgpr1, implicit $exec
 ---