Skip to content

[AMDGPU] Use immediates for stack accesses in chain funcs #71913

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.

Sign up for GitHub

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 1 commit into from
Nov 14, 2023
Merged

[AMDGPU] Use immediates for stack accesses in chain funcs #71913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions llvm/lib/Target/AMDGPU/SIFrameLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1093,14 +1093,12 @@ void SIFrameLowering::emitPrologue(MachineFunction &MF,
if (FuncInfo->isChainFunction()) {
// Functions with the amdgpu_cs_chain[_preserve] CC don't receive a SP, but
// are free to set one up if they need it.
// FIXME: We shouldn't need to set SP just for the stack objects (we should
// use 0 as an immediate offset instead).
bool UseSP = requiresStackPointerReference(MF) || MFI.hasStackObjects();
bool UseSP = requiresStackPointerReference(MF);
if (UseSP) {
assert(StackPtrReg != AMDGPU::SP_REG);

BuildMI(MBB, MBBI, DL, TII->get(AMDGPU::S_MOV_B32), StackPtrReg)
.addImm(0);
.addImm(MFI.getStackSize() * getScratchScaleFactor(ST));
}
}

Expand All @@ -1115,7 +1113,8 @@ void SIFrameLowering::emitPrologue(MachineFunction &MF,
Register FramePtrRegScratchCopy;
if (!HasFP && !hasFP(MF)) {
// Emit the CSR spill stores with SP base register.
emitCSRSpillStores(MF, MBB, MBBI, DL, LiveUnits, StackPtrReg,
emitCSRSpillStores(MF, MBB, MBBI, DL, LiveUnits,
FuncInfo->isChainFunction() ? Register() : StackPtrReg,
FramePtrRegScratchCopy);
} else {
// CSR spill stores will use FP as base register.
Expand Down Expand Up @@ -1799,10 +1798,11 @@ static bool frameTriviallyRequiresSP(const MachineFrameInfo &MFI) {
bool SIFrameLowering::hasFP(const MachineFunction &MF) const {
const MachineFrameInfo &MFI = MF.getFrameInfo();

// For entry functions we can use an immediate offset in most cases, so the
// presence of calls doesn't imply we need a distinct frame pointer.
// For entry & chain functions we can use an immediate offset in most cases,
// so the presence of calls doesn't imply we need a distinct frame pointer.
if (MFI.hasCalls() &&
!MF.getInfo<SIMachineFunctionInfo>()->isEntryFunction()) {
!MF.getInfo<SIMachineFunctionInfo>()->isEntryFunction() &&
!MF.getInfo<SIMachineFunctionInfo>()->isChainFunction()) {
Comment on lines +1804 to +1805
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth inventing isEntryOrChainFunction, or some other name that captures the commonality?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe. I'll see if I can throw another patch together for that :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say it's not worth it unless we find a good name for it. isBottomOfStackFunction? Not too fond of that, to be honest, but at least it captures the meaning.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, thanks, I'll think some more about the name. My brain's first reaction to this one was "what's a stack function and why would something be the bottom of it?" (it makes sense when compared to the existing helpers, but on its own it sounds a bit off).

Also IMO we have potential for a lot of confusion just with the existing isShader, isEntryFunction and isModuleEntryFunction, and it wouldn't help to introduce more of this kind of vague terminology (unless we explain it properly in the comments, which I'm a bit shy to do myself for the existing helpers). It would be nice to separate the different aspects of "entry"-ness into helpers with more expressive names, but I don't have a very good idea about how to go about that.

// All offsets are unsigned, so need to be addressed in the same direction
// as stack growth.

Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ int SIMachineFunctionInfo::getScavengeFI(MachineFrameInfo &MFI,
const SIRegisterInfo &TRI) {
if (ScavengeFI)
return *ScavengeFI;
if (isEntryFunction()) {
if (isEntryFunction() || isChainFunction()) {
ScavengeFI = MFI.CreateFixedObject(
TRI.getSpillSize(AMDGPU::SGPR_32RegClass), 0, false);
} else {
Expand Down
8 changes: 4 additions & 4 deletions llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,11 +499,11 @@ SIRegisterInfo::getLargestLegalSuperClass(const TargetRegisterClass *RC,
Register SIRegisterInfo::getFrameRegister(const MachineFunction &MF) const {
const SIFrameLowering *TFI = ST.getFrameLowering();
const SIMachineFunctionInfo *FuncInfo = MF.getInfo<SIMachineFunctionInfo>();
// During ISel lowering we always reserve the stack pointer in entry
// During ISel lowering we always reserve the stack pointer in entry and chain
// functions, but never actually want to reference it when accessing our own
// frame. If we need a frame pointer we use it, but otherwise we can just use
// an immediate "0" which we represent by returning NoRegister.
if (FuncInfo->isEntryFunction()) {
if (FuncInfo->isEntryFunction() || FuncInfo->isChainFunction()) {
return TFI->hasFP(MF) ? FuncInfo->getFrameOffsetReg() : Register();
}
return TFI->hasFP(MF) ? FuncInfo->getFrameOffsetReg()
Expand Down Expand Up @@ -1649,7 +1649,7 @@ void SIRegisterInfo::buildSpillLoadStore(
if (UseVGPROffset && ScratchOffsetReg) {
MIB.addReg(ScratchOffsetReg);
} else {
assert(FuncInfo->isEntryFunction());
assert(FuncInfo->isEntryFunction() || FuncInfo->isChainFunction());
MIB.addImm(0);
}
}
Expand Down Expand Up @@ -2424,7 +2424,7 @@ bool SIRegisterInfo::eliminateFrameIndex(MachineBasicBlock::iterator MI,

bool IsMUBUF = TII->isMUBUF(*MI);

if (!IsMUBUF && !MFI->isEntryFunction()) {
if (!IsMUBUF && !MFI->isEntryFunction() && !MFI->isChainFunction()) {
// Convert to a swizzled stack address by scaling by the wave size.
// In an entry function/kernel the offset is already swizzled.
bool IsSALU = isSGPRClass(TII->getOpRegClass(*MI, FIOperandNum));
Expand Down
79 changes: 66 additions & 13 deletions llvm/test/CodeGen/AMDGPU/amdgpu-cs-chain-cc.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,65 @@ define amdgpu_cs_chain void @amdgpu_cs_chain_spill(<24 x i32> inreg %sgprs, <24
ret void
}

define amdgpu_cs_chain void @alloca_and_call() {
; GISEL-GFX11-LABEL: alloca_and_call:
; GISEL-GFX11: ; %bb.0: ; %.entry
; GISEL-GFX11-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
; GISEL-GFX11-NEXT: v_mov_b32_e32 v0, 42
; GISEL-GFX11-NEXT: s_mov_b32 s0, use@abs32@lo
; GISEL-GFX11-NEXT: s_mov_b32 s1, use@abs32@hi
; GISEL-GFX11-NEXT: s_mov_b32 s32, 16
; GISEL-GFX11-NEXT: scratch_store_b32 off, v0, off offset:4
; GISEL-GFX11-NEXT: v_mov_b32_e32 v0, 4
; GISEL-GFX11-NEXT: s_swappc_b64 s[30:31], s[0:1]
; GISEL-GFX11-NEXT: s_endpgm
;
; GISEL-GFX10-LABEL: alloca_and_call:
; GISEL-GFX10: ; %bb.0: ; %.entry
; GISEL-GFX10-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
; GISEL-GFX10-NEXT: v_mov_b32_e32 v0, 42
; GISEL-GFX10-NEXT: s_mov_b64 s[0:1], s[48:49]
; GISEL-GFX10-NEXT: s_mov_b32 s4, use@abs32@lo
; GISEL-GFX10-NEXT: s_mov_b32 s5, use@abs32@hi
; GISEL-GFX10-NEXT: s_mov_b64 s[2:3], s[50:51]
; GISEL-GFX10-NEXT: buffer_store_dword v0, off, s[48:51], 0 offset:4
; GISEL-GFX10-NEXT: v_mov_b32_e32 v0, 4
; GISEL-GFX10-NEXT: s_movk_i32 s32, 0x200
; GISEL-GFX10-NEXT: s_swappc_b64 s[30:31], s[4:5]
; GISEL-GFX10-NEXT: s_endpgm
;
; DAGISEL-GFX11-LABEL: alloca_and_call:
; DAGISEL-GFX11: ; %bb.0: ; %.entry
; DAGISEL-GFX11-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
; DAGISEL-GFX11-NEXT: v_mov_b32_e32 v0, 42
; DAGISEL-GFX11-NEXT: s_mov_b32 s1, use@abs32@hi
; DAGISEL-GFX11-NEXT: s_mov_b32 s0, use@abs32@lo
; DAGISEL-GFX11-NEXT: s_mov_b32 s32, 16
; DAGISEL-GFX11-NEXT: scratch_store_b32 off, v0, off offset:4
; DAGISEL-GFX11-NEXT: v_mov_b32_e32 v0, 4
; DAGISEL-GFX11-NEXT: s_swappc_b64 s[30:31], s[0:1]
; DAGISEL-GFX11-NEXT: s_endpgm
;
; DAGISEL-GFX10-LABEL: alloca_and_call:
; DAGISEL-GFX10: ; %bb.0: ; %.entry
; DAGISEL-GFX10-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
; DAGISEL-GFX10-NEXT: v_mov_b32_e32 v0, 42
; DAGISEL-GFX10-NEXT: s_mov_b64 s[0:1], s[48:49]
; DAGISEL-GFX10-NEXT: s_mov_b32 s5, use@abs32@hi
; DAGISEL-GFX10-NEXT: s_mov_b32 s4, use@abs32@lo
; DAGISEL-GFX10-NEXT: s_mov_b64 s[2:3], s[50:51]
; DAGISEL-GFX10-NEXT: buffer_store_dword v0, off, s[48:51], 0 offset:4
; DAGISEL-GFX10-NEXT: v_mov_b32_e32 v0, 4
; DAGISEL-GFX10-NEXT: s_movk_i32 s32, 0x200
; DAGISEL-GFX10-NEXT: s_swappc_b64 s[30:31], s[4:5]
; DAGISEL-GFX10-NEXT: s_endpgm
.entry:
%v = alloca [3 x i32], addrspace(5)
store i32 42, ptr addrspace(5) %v
call amdgpu_gfx void @use(ptr addrspace(5) %v)
ret void
}

define amdgpu_cs void @cs_to_chain(<3 x i32> inreg %a, <3 x i32> %b) {
; GISEL-GFX11-LABEL: cs_to_chain:
; GISEL-GFX11: ; %bb.0:
Expand Down Expand Up @@ -807,9 +866,8 @@ define amdgpu_cs_chain void @amdgpu_cs_chain_dont_realign_stack(i32 %idx) {
; GISEL-GFX11-NEXT: s_mov_b32 s1, 2
; GISEL-GFX11-NEXT: s_mov_b32 s0, 1
; GISEL-GFX11-NEXT: v_lshlrev_b32_e32 v0, 4, v8
; GISEL-GFX11-NEXT: s_mov_b32 s32, 0
; GISEL-GFX11-NEXT: s_delay_alu instid0(VALU_DEP_1) | instid1(SALU_CYCLE_1)
; GISEL-GFX11-NEXT: v_add_nc_u32_e32 v4, s32, v0
; GISEL-GFX11-NEXT: s_delay_alu instid0(VALU_DEP_1)
; GISEL-GFX11-NEXT: v_add_nc_u32_e32 v4, 32, v0
; GISEL-GFX11-NEXT: v_dual_mov_b32 v0, s0 :: v_dual_mov_b32 v3, s3
; GISEL-GFX11-NEXT: v_dual_mov_b32 v1, s1 :: v_dual_mov_b32 v2, s2
; GISEL-GFX11-NEXT: scratch_store_b128 v4, v[0:3], off dlc
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really related to your patch, but shouldn't the immediate offset 32 be folded into this store?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think @ruiling was looking into some of these, but I'm not sure if he has exactly this case covered.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the Add has NoUnsignedWrap being set, it will be folded. Will double check this later.

Expand All @@ -819,14 +877,12 @@ define amdgpu_cs_chain void @amdgpu_cs_chain_dont_realign_stack(i32 %idx) {
; GISEL-GFX10-LABEL: amdgpu_cs_chain_dont_realign_stack:
; GISEL-GFX10: ; %bb.0:
; GISEL-GFX10-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
; GISEL-GFX10-NEXT: s_mov_b32 s32, 0
; GISEL-GFX10-NEXT: v_lshlrev_b32_e32 v0, 4, v8
; GISEL-GFX10-NEXT: v_lshrrev_b32_e64 v2, 5, s32
; GISEL-GFX10-NEXT: v_mov_b32_e32 v1, 1
; GISEL-GFX10-NEXT: v_mov_b32_e32 v2, 2
; GISEL-GFX10-NEXT: v_mov_b32_e32 v3, 3
; GISEL-GFX10-NEXT: v_mov_b32_e32 v4, 4
; GISEL-GFX10-NEXT: v_add_nc_u32_e32 v0, v2, v0
; GISEL-GFX10-NEXT: v_mov_b32_e32 v2, 2
; GISEL-GFX10-NEXT: v_add_nc_u32_e32 v0, 32, v0
; GISEL-GFX10-NEXT: buffer_store_dword v1, v0, s[48:51], 0 offen
; GISEL-GFX10-NEXT: s_waitcnt_vscnt null, 0x0
; GISEL-GFX10-NEXT: buffer_store_dword v2, v0, s[48:51], 0 offen offset:4
Expand All @@ -840,24 +896,21 @@ define amdgpu_cs_chain void @amdgpu_cs_chain_dont_realign_stack(i32 %idx) {
; DAGISEL-GFX11-LABEL: amdgpu_cs_chain_dont_realign_stack:
; DAGISEL-GFX11: ; %bb.0:
; DAGISEL-GFX11-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
; DAGISEL-GFX11-NEXT: s_mov_b32 s32, 0
; DAGISEL-GFX11-NEXT: v_dual_mov_b32 v0, 1 :: v_dual_mov_b32 v1, 2
; DAGISEL-GFX11-NEXT: v_dual_mov_b32 v2, 3 :: v_dual_mov_b32 v3, 4
; DAGISEL-GFX11-NEXT: v_lshl_add_u32 v4, v8, 4, s32
; DAGISEL-GFX11-NEXT: v_lshl_add_u32 v4, v8, 4, 32
; DAGISEL-GFX11-NEXT: scratch_store_b128 v4, v[0:3], off dlc
; DAGISEL-GFX11-NEXT: s_waitcnt_vscnt null, 0x0
; DAGISEL-GFX11-NEXT: s_endpgm
;
; DAGISEL-GFX10-LABEL: amdgpu_cs_chain_dont_realign_stack:
; DAGISEL-GFX10: ; %bb.0:
; DAGISEL-GFX10-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
; DAGISEL-GFX10-NEXT: s_mov_b32 s32, 0
; DAGISEL-GFX10-NEXT: v_mov_b32_e32 v0, 4
; DAGISEL-GFX10-NEXT: v_lshrrev_b32_e64 v2, 5, s32
; DAGISEL-GFX10-NEXT: v_lshl_add_u32 v1, v8, 4, 32
; DAGISEL-GFX10-NEXT: v_mov_b32_e32 v2, 3
; DAGISEL-GFX10-NEXT: v_mov_b32_e32 v3, 2
; DAGISEL-GFX10-NEXT: v_mov_b32_e32 v4, 1
; DAGISEL-GFX10-NEXT: v_lshl_add_u32 v1, v8, 4, v2
; DAGISEL-GFX10-NEXT: v_mov_b32_e32 v2, 3
; DAGISEL-GFX10-NEXT: buffer_store_dword v0, v1, s[48:51], 0 offen offset:12
; DAGISEL-GFX10-NEXT: s_waitcnt_vscnt null, 0x0
; DAGISEL-GFX10-NEXT: buffer_store_dword v2, v1, s[48:51], 0 offen offset:8
Expand Down
Loading