[AMDGPU] Fix sgpr to vreg_1 copy analysis

[jtuyls]

The added test is reduced from a kernel function that hits the assertion:

llvm-project/llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:3624: const llvm::TargetRegisterClass* llvm::SIRegisterInfo::getEquivalentSGPRClass(const llvm::TargetRegisterClass*) const: Assertion `SRC && "Invalid register class size"' failed.

This happens because the vgpr register size is 1 bit and this is not supported in getEquivalentSGPRClass. This PR fixes it by moving tryChangeVGPRtoSGPRinCopy behind the isSGPRToVGPRCopy check. This seems sensible as the same is done at other places in this file where tryChangeVGPRtoSGPRinCopy is called:

• llvm-project/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

  Line 305 in 94382c8

  if (tryChangeVGPRtoSGPRinCopy(CopyUse, TRI, TII))

• llvm-project/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

  Line 645 in 94382c8

  if (tryChangeVGPRtoSGPRinCopy(MI, TRI, TII))

• llvm-project/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

  Line 782 in 94382c8

  tryChangeVGPRtoSGPRinCopy(*MI, TRI, TII);

Additionally, this fix seems to work for the kernel in which I hit this. However, let me know if this is not the correct approach.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: Jorn Tuyls (jtuyls)

Changes

The added test is reduced from a kernel function that hits the assertion:

llvm-project/llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:3624: const llvm::TargetRegisterClass* llvm::SIRegisterInfo::getEquivalentSGPRClass(const llvm::TargetRegisterClass*) const: Assertion `SRC && "Invalid register class size"' failed.

This happens because the vgpr register size is 1 bit and this is not supported in getEquivalentSGPRClass. This PR fixes it by moving tryChangeVGPRtoSGPRinCopy behind the isSGPRToVGPRCopy check. This seems sensible as the same is done at other places in this file where tryChangeVGPRtoSGPRinCopy is called:

• llvm-project/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

  Line 305 in 94382c8

  if (tryChangeVGPRtoSGPRinCopy(CopyUse, TRI, TII))

• llvm-project/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

  Line 645 in 94382c8

  if (tryChangeVGPRtoSGPRinCopy(MI, TRI, TII))

• llvm-project/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

  Line 782 in 94382c8

  tryChangeVGPRtoSGPRinCopy(*MI, TRI, TII);

Additionally, this fix seems to work for the kernel in which I hit this. However, let me know if this is not the correct approach.

---

Full diff: https://github.com/llvm/llvm-project/pull/149181.diff

2 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp (+12-7)
• (added) llvm/test/CodeGen/AMDGPU/sgpr-to-vreg1-copy.mir (+31)

diff --git a/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp b/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
index 9b5a46395695d..343d203cbe0de 100644
--- a/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
+++ b/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
@@ -946,13 +946,18 @@ void SIFixSGPRCopies::analyzeVGPRToSGPRCopy(MachineInstr* MI) {
 
     // Copies and REG_SEQUENCE do not contribute to the final assembly
     // So, skip them but take care of the SGPR to VGPR copies bookkeeping.
-    if (Inst->isCopy() || Inst->isRegSequence()) {
-      if (TRI->isVGPR(*MRI, Inst->getOperand(0).getReg())) {
-        if (!Inst->isCopy() ||
-            !tryChangeVGPRtoSGPRinCopy(*Inst, TRI, TII)) {
-          Info.NumSVCopies++;
-          continue;
-        }
+    if (Inst->isRegSequence() &&
+        TRI->isVGPR(*MRI, Inst->getOperand(0).getReg())) {
+      Info.NumSVCopies++;
+      continue;
+    }
+    if (Inst->isCopy()) {
+      const TargetRegisterClass *SrcRC, *DstRC;
+      std::tie(SrcRC, DstRC) = getCopyRegClasses(*Inst, *TRI, *MRI);
+      if (isSGPRToVGPRCopy(SrcRC, DstRC, *TRI) &&
+          !tryChangeVGPRtoSGPRinCopy(*Inst, TRI, TII)) {
+        Info.NumSVCopies++;
+        continue;
       }
     }
 
diff --git a/llvm/test/CodeGen/AMDGPU/sgpr-to-vreg1-copy.mir b/llvm/test/CodeGen/AMDGPU/sgpr-to-vreg1-copy.mir
new file mode 100644
index 0000000000000..2daea2b2eeb74
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/sgpr-to-vreg1-copy.mir
@@ -0,0 +1,31 @@
+# RUN: llc -mtriple=amdgcn -run-pass si-fix-sgpr-copies -verify-machineinstrs -o - %s | FileCheck -check-prefix=GCN %s
+
+---
+name:            copy_to_vreg_1
+tracksRegLiveness: true
+body:             |
+  ; GCN-LABEL: name: copy_to_vreg_1
+  ; GCN: bb.0:
+  ; GCN-NEXT:   successors: %bb.1(0x80000000)
+  ; GCN-NEXT:   liveins: $vgpr0, $vgpr1
+  ; GCN-NEXT: {{  $}}
+  ; GCN-NEXT:   [[V_CVT_U32_F32_e64:%[0-9]+]]:vgpr_32 = nofpexcept V_CVT_U32_F32_e64 0, killed $vgpr0, 0, 0, implicit $mode, implicit $exec
+  ; GCN-NEXT:   [[IMPLICIT_DEF:%[0-9]+]]:sreg_32 = IMPLICIT_DEF
+  ; GCN-NEXT:   [[COPY1:%[0-9]+]]:vgpr_32 = COPY $vgpr1
+  ; GCN-NEXT:   [[V_CMP_GT_U32_e64:%[0-9]+]]:sreg_64_xexec = samesign V_CMP_GT_U32_e64 [[V_CVT_U32_F32_e64]], killed [[COPY1]], implicit $exec
+  ; GCN-NEXT:   [[VREG1:%[0-9]+]]:vreg_1 = COPY [[V_CMP_GT_U32_e64]]
+  ; GCN-NEXT: {{  $}}
+  ; GCN-NEXT: bb.1:
+  ; GCN-NEXT:   S_ENDPGM 0
+  bb.0:
+    liveins: $vgpr0, $vgpr1
+      %0:vgpr_32 = nofpexcept V_CVT_U32_F32_e64 0, killed $vgpr0, 0, 0, implicit $mode, implicit $exec
+      %1:sreg_32 = COPY %0:vgpr_32
+      %2:sreg_32 = COPY $vgpr1
+      samesign S_CMP_GT_U32 %1:sreg_32, killed %2:sreg_32, implicit-def $scc
+      %3:sreg_64 = COPY $scc
+      %4:vreg_1 = COPY %3:sreg_64
+
+  bb.1:
+      S_ENDPGM 0
+...

[github-actions]

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Developer Policy and LLVM Discourse for more information.

[jtuyls]

Tagging a couple of people that did recent reviews of related logic: @Sisyph @arsenm @jayfoad, could I get some help with a review?

[arsenm]

Can you add an IR testcase too

[jtuyls]

@arsenm I added an LLVM IR testcase.

[arsenm]

Should use named values in tests

[arsenm]

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo. Please turn off Keep my email addresses private setting in your account. See LLVM Developer Policy and LLVM Discourse for more information.

Also should fix this