[AMDGPU] Fix using physical registers in vector instructions

[Flakebi]

Summary

We can only get the register classes of virtual registers, not of
physical registers. The previous code crashed for vector instructions that
use physical registers.

[Flakebi]

Remove accidentally added empty line

[arsenm]

Need test and I would expect a physical register to never appear in this context

[foad]

Test case? Is this related to the atomic optimizer and/or the ballot intrinsic?

[Flakebi]

I’ll add a test case. Yes, this is related to the atomic optimizer and ballot intrinsic. There we get e.g. %0:vgpr_32 = V_MBCNT_LO_U32_B32_e64 $exec_lo, 0, implicit $exec.

[arsenm]

In D75857#1912899, @Flakebi wrote:

I’ll add a test case. Yes, this is related to the atomic optimizer and ballot intrinsic. There we get e.g. %0:vgpr_32 = V_MBCNT_LO_U32_B32_e64 $exec_lo, 0, implicit $exec.

That should probably be reading a copy from exec_lo

[Flakebi]

Add test

[arsenm]

I still think theses should not be seeing physical register operands, and it would be better to fix this by avoiding that situation

[Flakebi]

In D75857#1917137, @arsenm wrote:

I still think theses should not be seeing physical register operands, and it would be better to fix this by avoiding that situation

Why is introducing a copy a better fix? And does this mean I should return a COPY when doing (ballot 1) -> EXEC/EXEC_LO here? https://reviews.llvm.org/D65088#C1855323NL8643

Wouldn’t a copy use an SGPR and increase the SGPR-count? Then we would generate less optimal code, right?

[arsenm]

In D75857#1918810, @Flakebi wrote:

In D75857#1917137, @arsenm wrote:

I still think theses should not be seeing physical register operands, and it would be better to fix this by avoiding that situation

Why is introducing a copy a better fix? And does this mean I should return a COPY when doing (ballot 1) -> EXEC/EXEC_LO here? https://reviews.llvm.org/D65088#C1855323NL8643

Wouldn’t a copy use an SGPR and increase the SGPR-count? Then we would generate less optimal code, right?

It should be eliminated later. Introducing physical register constraints earlier is generally bad. I would also worry about earlier passes not expected a non-implicit exec use

[Flakebi]

Thanks Matt, I’ll use CopyFromReg.

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

if (!MO.isReg()) {
       }
 
       --LiteralLimit;
       --ConstantBusLimit;
       legalizeOpWithMove(MI, Idx);
       continue;
     }
 
-    if (RI.hasAGPRs(MRI.getRegClass(MO.getReg())) &&
-        !isOperandLegal(MI, Idx, &MO)) {
+    bool IsSGPR = false;
+    bool IsAGPR = false;
+    if (MO.getReg().isVirtual()) {
+      IsAGPR = RI.hasAGPRs(MRI.getRegClass(MO.getReg()));
+      IsSGPR = RI.isSGPRClass(MRI.getRegClass(MO.getReg()));
+    } else {
+      IsAGPR = RI.isAGPR(MRI, MO.getReg());
+      IsAGPR = RI.isSGPRReg(MRI, MO.getReg());
+    }
+
+    if (IsAGPR && !isOperandLegal(MI, Idx, &MO)) {
       legalizeOpWithMove(MI, Idx);
       continue;
     }
 
-    if (!RI.isSGPRClass(MRI.getRegClass(MO.getReg())))
+    if (!IsSGPR)
       continue; // VGPRs are legal
 
     // We can use one SGPR in each VOP3 instruction prior to GFX10
     // and two starting from GFX10.
     if (SGPRsUsed.count(MO.getReg()))
       continue;
     if (ConstantBusLimit > 0) {
       SGPRsUsed.insert(MO.getReg());
for (unsigned i = 0; i < 3; ++i) {
     // constraints?
     const TargetRegisterClass *OpRC = RI.getRegClass(Desc.OpInfo[Idx].RegClass);
     bool IsRequiredSGPR = RI.isSGPRClass(OpRC);
     if (IsRequiredSGPR)
       return MO.getReg();
 
     // If this could be a VGPR or an SGPR, Check the dynamic register class.
     Register Reg = MO.getReg();
+    if (Reg.isVirtual()) {
       const TargetRegisterClass *RegRC = MRI.getRegClass(Reg);
       if (RI.isSGPRClass(RegRC))
         UsedSGPRs[i] = Reg;
+    } else {
+      if (RI.isSGPRReg(MRI, Reg))
+        UsedSGPRs[i] = Reg;
+    }
   }
 
   // We don't have a required SGPR operand, so we have a bit more freedom in
   // selecting operands to move.
 
   // Try to select the most used SGPR. If an SGPR is equal to one of the
   // others, we choose that.
   //

llvm/test/CodeGen/AMDGPU/vop3-physical-reg.mir

+# RUN: llc -march=amdgcn -mcpu=kaveri -run-pass=si-peephole-sdwa -o - %s | FileCheck -check-prefix=CI -check-prefix=GCN %s
+# RUN: llc -march=amdgcn -mcpu=fiji -run-pass=si-peephole-sdwa -o - %s | FileCheck -check-prefix=VI -check-prefix=GCN %s
+# RUN: llc -march=amdgcn -mcpu=gfx900 -run-pass=si-peephole-sdwa -o - %s | FileCheck -check-prefix=GFX9 -check-prefix=GCN %s
+# RUN: llc -march=amdgcn -mcpu=gfx1010 -run-pass=si-peephole-sdwa -o - %s | FileCheck -check-prefix=GFX10 -check-prefix=GCN %s
+
+# Allow physical registers as vop3 operands
+
+# GCN-LABEL: {{^}}name: vop3_reg
+
+# GCN: %{{[0-9]+}}:vgpr_32 = V_BCNT_U32_B32_e64 $exec_lo, 0, implicit-def $vcc, implicit $exec
+# GCN: %{{[0-9]+}}:vgpr_32 = V_BCNT_U32_B32_e64 $exec_hi, 0, implicit-def $vcc, implicit $exec
+# GCN: %{{[0-9]+}}:vgpr_32 = V_MBCNT_LO_U32_B32_e64 $exec_lo, 0, implicit-def $vcc, implicit $exec
+# GCN: %{{[0-9]+}}:vgpr_32 = V_MBCNT_HI_U32_B32_e64 $exec_hi, %{{[0-9]+}}, implicit-def $vcc, implicit $exec
+
+---
+name:            vop3_reg
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: vreg_64 }
+  - { id: 1, class: vgpr_32 }
+  - { id: 2, class: vgpr_32 }
+  - { id: 3, class: vgpr_32 }
+  - { id: 4, class: vgpr_32 }
+  - { id: 9, class: sreg_64 }
+body:             |
+  bb.0:
+    liveins: $vgpr0_vgpr1, $vgpr2, $sgpr30_sgpr31
+
+    %9 = COPY $sgpr30_sgpr31
+    %0 = COPY $vgpr0_vgpr1
+
+    %1 = V_BCNT_U32_B32_e64 $exec_lo, 0, implicit-def $vcc, implicit $exec
+    %2 = V_BCNT_U32_B32_e64 $exec_hi, 0, implicit-def $vcc, implicit $exec
+    %3 = V_MBCNT_LO_U32_B32_e64 $exec_lo, 0, implicit-def $vcc, implicit $exec
+    %4 = V_MBCNT_HI_U32_B32_e64 $exec_hi, %3, implicit-def $vcc, implicit $exec
+
+    FLAT_STORE_DWORD %0, %1, 0, 0, 0, 0, implicit $exec, implicit $flat_scr :: (store 4)
+    FLAT_STORE_DWORD %0, %2, 0, 0, 0, 0, implicit $exec, implicit $flat_scr :: (store 4)
+    FLAT_STORE_DWORD %0, %3, 0, 0, 0, 0, implicit $exec, implicit $flat_scr :: (store 4)
+    FLAT_STORE_DWORD %0, %4, 0, 0, 0, 0, implicit $exec, implicit $flat_scr :: (store 4)
+    $sgpr30_sgpr31 = COPY %9
+    S_SETPC_B64_return $sgpr30_sgpr31