- Notifications
You must be signed in to change notification settings - Fork14.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.cppindex 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.mirnew file mode 100644index 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. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| ; 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
Uh oh!
There was an error while loading.Please reload this page.
Also should fix this |
03fe1a4 intollvm:mainUh oh!
There was an error while loading.Please reload this page.
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 in94382c8
llvm-project/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
Line 645 in94382c8
llvm-project/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
Line 782 in94382c8
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.