-
Notifications
You must be signed in to change notification settings - Fork 13.3k
[WIP][AMDGPU] Fix emitting illegal COPY #131752
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -899,6 +899,55 @@ bool SIFixSGPRCopies::lowerSpecialCase(MachineInstr &MI, | |
MI, MI.getDebugLoc())) { | ||
I = std::next(I); | ||
MI.eraseFromParent(); | ||
} else { | ||
// At this point, if we still have a VGPR → SGPR copy, it is completely | ||
// illegal. We assume that it was intentionally introduced and should be | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it safe to assume? For example, #130443 can also introduce VGPR to SGPR copy but that doesn't seem to be the case here. This fix can only fix one case IIUC so the comment needs to be adapted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd go with no. I don't want to make semantic guesses in this pass, and require inserting these up front where required. We're already suffering from the assumed usage of SCC in this pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agree with you. It arises at many places after enabling the AMDGPUUniformIntrinsicCombine pass into llc pipeline PR: #128687, which optimizes certain intrinsic by propagating uniform values directly to their uses. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think it is similar case here as llvm/test/CodeGen/AMDGPU/fix-illegal-copy.ll? |
||
// replaced with the READFIRSTLANE to ensure correctness. | ||
const TargetRegisterClass *SRC = MRI->getRegClass(SrcReg); | ||
const TargetRegisterClass *DRC = TRI->getPhysRegBaseClass(DstReg); | ||
ArrayRef<int16_t> SrcIndices = TRI->getRegSplitParts(SRC, 4); | ||
ArrayRef<int16_t> DstIndices = TRI->getRegSplitParts(DRC, 4); | ||
assert(SrcIndices.size() == DstIndices.size() && | ||
"Register tuple should match"); | ||
MachineInstr *FirstMI = nullptr, *LastMI = nullptr; | ||
// If SrcReg is virtual register can we get the sub reg? one way to handle | ||
// this If def is copy fold the def src. This should not work in case of | ||
// src is and AGPR. Since direct copy from agpr to sgpr is not allowed? | ||
MachineInstr *DefMI = MRI->getVRegDef(SrcReg); | ||
// Get the def SrcReg | ||
Register DefSrcReg = DefMI->getOperand(1).getReg(); | ||
// If not a register tuple replace the opcode itself. | ||
if (SrcIndices.size() == 1) { | ||
MI.setDesc(TII->get(AMDGPU::V_READFIRSTLANE_B32)); | ||
MI.addOperand(*MI.getParent()->getParent(), | ||
MachineOperand::CreateReg(AMDGPU::EXEC, false, true)); | ||
return true; | ||
} | ||
|
||
if (!DefMI || !DefMI->isCopy() || TRI->isAGPR(*MRI, DefSrcReg)) | ||
return true; | ||
|
||
for (unsigned Idx = 0; Idx < SrcIndices.size(); ++Idx) { | ||
int16_t SubIdx = SrcIndices[Idx]; | ||
Register DefSrcSubReg = TRI->getSubReg(DefSrcReg, SubIdx); | ||
Register DstSubReg = TRI->getSubReg(DstReg, SubIdx); | ||
assert(DstSubReg && DefSrcSubReg && "Failed to find subregs!"); | ||
LastMI = BuildMI(*MI.getParent(), I, MI.getDebugLoc(), | ||
TII->get(AMDGPU::V_READFIRSTLANE_B32), DstSubReg) | ||
.addReg(DefSrcSubReg) | ||
.addReg(DefSrcReg, RegState::Implicit); | ||
if (!FirstMI) | ||
FirstMI = LastMI; | ||
} | ||
assert(FirstMI && LastMI); | ||
|
||
FirstMI->addOperand( | ||
MachineOperand::CreateReg(DstReg, true /*IsDef*/, true /*IsImp*/)); | ||
|
||
LastMI->addRegisterKilled(DefSrcReg, TRI); | ||
I = std::next(I); | ||
MI.eraseFromParent(); | ||
DefMI->eraseFromParent(); | ||
} | ||
return true; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5 | ||
; RUN: llc < %s -mtriple=amdgcn -mcpu=gfx1100 -verify-machineinstrs -stop-after=finalize-isel | FileCheck %s -check-prefixes=GFX11 | ||
|
||
define amdgpu_ps i32 @s_copysign_f32_bf16(float inreg %mag, bfloat inreg %sign.bf16) { | ||
; GFX11-LABEL: name: s_copysign_f32_bf16 | ||
; GFX11: bb.0 (%ir-block.0): | ||
; GFX11-NEXT: liveins: $sgpr0, $sgpr1 | ||
; GFX11-NEXT: {{ $}} | ||
; GFX11-NEXT: [[COPY:%[0-9]+]]:sgpr_32 = COPY $sgpr1 | ||
; GFX11-NEXT: [[COPY1:%[0-9]+]]:sgpr_32 = COPY $sgpr0 | ||
; GFX11-NEXT: [[V_LSHLREV_B32_e64_:%[0-9]+]]:vgpr_32 = V_LSHLREV_B32_e64 16, [[COPY]], implicit $exec | ||
; GFX11-NEXT: [[S_MOV_B32_:%[0-9]+]]:sreg_32 = S_MOV_B32 2147483647 | ||
; GFX11-NEXT: [[V_BFI_B32_e64_:%[0-9]+]]:vgpr_32 = V_BFI_B32_e64 killed [[S_MOV_B32_]], [[COPY1]], killed [[V_LSHLREV_B32_e64_]], implicit $exec | ||
; GFX11-NEXT: $sgpr0 = V_READFIRSTLANE_B32 [[V_BFI_B32_e64_]], implicit $exec | ||
; GFX11-NEXT: SI_RETURN_TO_EPILOG $sgpr0 | ||
%sign = fpext bfloat %sign.bf16 to float | ||
%op = call float @llvm.copysign.f32(float %mag, float %sign) | ||
%cast = bitcast float %op to i32 | ||
ret i32 %cast | ||
} |
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.
Not a good idea to use
→
in the comment.->
is preferred.