Skip to content

[AMDGPU] Fix sgpr to vreg_1 copy analysis #149181

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 18, 2025
Merged

Conversation

jtuyls
Copy link
Contributor

@jtuyls jtuyls commented Jul 16, 2025

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:

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
Copy link
Member

llvmbot commented Jul 16, 2025

@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:

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
+...

Copy link

⚠️ 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
Copy link
Contributor Author

jtuyls commented Jul 16, 2025

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

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add an IR testcase too

@jtuyls
Copy link
Contributor Author

jtuyls commented Jul 17, 2025

@arsenm I added an LLVM IR testcase.

; GCN-NEXT: .LBB0_8: ; %DummyReturnBlock
; GCN-NEXT: s_endpgm
._crit_edge:
%1 = tail call i32 @llvm.amdgcn.workitem.id.x()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use named values in tests

@arsenm
Copy link
Contributor

arsenm commented Jul 18, 2025

⚠️ 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

@arsenm arsenm merged commit 03fe1a4 into llvm:main Jul 18, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants