-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[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
Conversation
@llvm/pr-subscribers-backend-amdgpu Author: Jorn Tuyls (jtuyls) ChangesThe added test is reduced from a kernel function that hits the assertion:
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:
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
+...
|
|
There was a problem hiding this 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
@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() |
There was a problem hiding this comment.
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
Also should fix this |
The added test is reduced from a kernel function that hits the assertion:
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
llvm-project/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
Line 645 in 94382c8
llvm-project/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
Line 782 in 94382c8
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.