[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.
   //