Skip to content

[AMDGPU] Don't DEALLOC_VGPRS from callable functions #72245

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

Closed
wants to merge 3 commits into from
Closed

[AMDGPU] Don't DEALLOC_VGPRS from callable functions #72245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48b3801
[AMDGPU] Don't DEALLOC_VGPRS from callable functions
rovka Nov 8, 2023
39f5b84
Address comments + minor cleanup
rovka Nov 8, 2023
a8249d2
Remove unused function
rovka Nov 17, 2023
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
8 changes: 6 additions & 2 deletions llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1027,6 +1027,8 @@ bool SIInsertWaitcnts::generateWaitcntInstBefore(MachineInstr &MI,
Wait.VmCnt = 0;
}

CallingConv::ID CC = MI.getMF()->getFunction().getCallingConv();

// All waits must be resolved at call return.
// NOTE: this could be improved with knowledge of all call sites or
// with knowledge of the called routines.
Expand All @@ -1039,10 +1041,12 @@ bool SIInsertWaitcnts::generateWaitcntInstBefore(MachineInstr &MI,
// Identify S_ENDPGM instructions which may have to wait for outstanding VMEM
// stores. In this case it can be useful to send a message to explicitly
// release all VGPRs before the stores have completed, but it is only safe to
// do this if there are no outstanding scratch stores.
// do this if there are no outstanding scratch stores (either from the current
// function or potentially from a caller or callee).
else if (MI.getOpcode() == AMDGPU::S_ENDPGM ||
MI.getOpcode() == AMDGPU::S_ENDPGM_SAVED) {
if (ST->getGeneration() >= AMDGPUSubtarget::GFX11 && !OptNone &&
if (ST->getGeneration() >= AMDGPUSubtarget::GFX11 &&
AMDGPU::isEntryFunctionCC(CC) && !OptNone &&
ScoreBrackets.getScoreRange(VS_CNT) != 0 &&
!ScoreBrackets.hasPendingEvent(SCRATCH_WRITE_ACCESS))
ReleaseVGPRInsts.insert(&MI);
Expand Down
28 changes: 0 additions & 28 deletions llvm/test/CodeGen/AMDGPU/llvm.amdgcn.set.inactive.chain.arg.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ define amdgpu_cs_chain void @set_inactive_chain_arg(ptr addrspace(1) %out, i32 %
; GFX11-NEXT: v_mov_b32_e32 v0, v10
; GFX11-NEXT: s_not_b32 exec_lo, exec_lo
; GFX11-NEXT: global_store_b32 v[8:9], v0, off
; GFX11-NEXT: s_nop 0
; GFX11-NEXT: s_sendmsg sendmsg(MSG_DEALLOC_VGPRS)
; GFX11-NEXT: s_endpgm
;
; GFX10-LABEL: set_inactive_chain_arg:
Expand All @@ -39,8 +37,6 @@ define amdgpu_cs_chain void @set_inactive_chain_arg(ptr addrspace(1) %out, i32 %
; GFX11_W64-NEXT: v_mov_b32_e32 v0, v10
; GFX11_W64-NEXT: s_not_b64 exec, exec
; GFX11_W64-NEXT: global_store_b32 v[8:9], v0, off
; GFX11_W64-NEXT: s_nop 0
; GFX11_W64-NEXT: s_sendmsg sendmsg(MSG_DEALLOC_VGPRS)
; GFX11_W64-NEXT: s_endpgm
;
; GFX10_W64-LABEL: set_inactive_chain_arg:
Expand Down Expand Up @@ -68,8 +64,6 @@ define amdgpu_cs_chain void @set_inactive_chain_arg_64(ptr addrspace(1) %out, i6
; GFX11-NEXT: v_mov_b32_e32 v1, v11
; GFX11-NEXT: s_not_b32 exec_lo, exec_lo
; GFX11-NEXT: global_store_b64 v[8:9], v[0:1], off
; GFX11-NEXT: s_nop 0
; GFX11-NEXT: s_sendmsg sendmsg(MSG_DEALLOC_VGPRS)
; GFX11-NEXT: s_endpgm
;
; GFX10-LABEL: set_inactive_chain_arg_64:
Expand All @@ -94,8 +88,6 @@ define amdgpu_cs_chain void @set_inactive_chain_arg_64(ptr addrspace(1) %out, i6
; GFX11_W64-NEXT: v_mov_b32_e32 v1, v11
; GFX11_W64-NEXT: s_not_b64 exec, exec
; GFX11_W64-NEXT: global_store_b64 v[8:9], v[0:1], off
; GFX11_W64-NEXT: s_nop 0
; GFX11_W64-NEXT: s_sendmsg sendmsg(MSG_DEALLOC_VGPRS)
; GFX11_W64-NEXT: s_endpgm
;
; GFX10_W64-LABEL: set_inactive_chain_arg_64:
Expand Down Expand Up @@ -133,8 +125,6 @@ define amdgpu_cs_chain void @set_inactive_chain_arg_dpp(ptr addrspace(1) %out, i
; GFX11-NEXT: s_delay_alu instid0(VALU_DEP_1)
; GFX11-NEXT: v_mov_b32_e32 v2, v1
; GFX11-NEXT: global_store_b32 v[8:9], v2, off
; GFX11-NEXT: s_nop 0
; GFX11-NEXT: s_sendmsg sendmsg(MSG_DEALLOC_VGPRS)
; GFX11-NEXT: s_endpgm
;
; GFX10-LABEL: set_inactive_chain_arg_dpp:
Expand Down Expand Up @@ -174,8 +164,6 @@ define amdgpu_cs_chain void @set_inactive_chain_arg_dpp(ptr addrspace(1) %out, i
; GFX11_W64-NEXT: s_delay_alu instid0(VALU_DEP_1)
; GFX11_W64-NEXT: v_mov_b32_e32 v2, v1
; GFX11_W64-NEXT: global_store_b32 v[8:9], v2, off
; GFX11_W64-NEXT: s_nop 0
; GFX11_W64-NEXT: s_sendmsg sendmsg(MSG_DEALLOC_VGPRS)
; GFX11_W64-NEXT: s_endpgm
;
; GFX10_W64-LABEL: set_inactive_chain_arg_dpp:
Expand Down Expand Up @@ -233,8 +221,6 @@ define amdgpu_cs_chain void @set_inactive_chain_arg_call(ptr addrspace(1) %out,
; GISEL11-NEXT: s_delay_alu instid0(VALU_DEP_1)
; GISEL11-NEXT: v_mov_b32_e32 v0, v12
; GISEL11-NEXT: global_store_b32 v[41:42], v0, off
; GISEL11-NEXT: s_nop 0
; GISEL11-NEXT: s_sendmsg sendmsg(MSG_DEALLOC_VGPRS)
; GISEL11-NEXT: s_endpgm
;
; DAGISEL11-LABEL: set_inactive_chain_arg_call:
Expand Down Expand Up @@ -265,8 +251,6 @@ define amdgpu_cs_chain void @set_inactive_chain_arg_call(ptr addrspace(1) %out,
; DAGISEL11-NEXT: s_delay_alu instid0(VALU_DEP_1)
; DAGISEL11-NEXT: v_mov_b32_e32 v0, v12
; DAGISEL11-NEXT: global_store_b32 v[41:42], v0, off
; DAGISEL11-NEXT: s_nop 0
; DAGISEL11-NEXT: s_sendmsg sendmsg(MSG_DEALLOC_VGPRS)
; DAGISEL11-NEXT: s_endpgm
;
; GISEL10-LABEL: set_inactive_chain_arg_call:
Expand Down Expand Up @@ -380,8 +364,6 @@ define amdgpu_cs_chain void @set_inactive_chain_arg_call(ptr addrspace(1) %out,
; GISEL11_W64-NEXT: s_delay_alu instid0(VALU_DEP_1)
; GISEL11_W64-NEXT: v_mov_b32_e32 v0, v12
; GISEL11_W64-NEXT: global_store_b32 v[41:42], v0, off
; GISEL11_W64-NEXT: s_nop 0
; GISEL11_W64-NEXT: s_sendmsg sendmsg(MSG_DEALLOC_VGPRS)
; GISEL11_W64-NEXT: s_endpgm
;
; DAGISEL11_W64-LABEL: set_inactive_chain_arg_call:
Expand Down Expand Up @@ -419,8 +401,6 @@ define amdgpu_cs_chain void @set_inactive_chain_arg_call(ptr addrspace(1) %out,
; DAGISEL11_W64-NEXT: s_delay_alu instid0(VALU_DEP_1)
; DAGISEL11_W64-NEXT: v_mov_b32_e32 v0, v12
; DAGISEL11_W64-NEXT: global_store_b32 v[41:42], v0, off
; DAGISEL11_W64-NEXT: s_nop 0
; DAGISEL11_W64-NEXT: s_sendmsg sendmsg(MSG_DEALLOC_VGPRS)
; DAGISEL11_W64-NEXT: s_endpgm
;
; GISEL10_W64-LABEL: set_inactive_chain_arg_call:
Expand Down Expand Up @@ -538,8 +518,6 @@ define amdgpu_cs_chain void @set_inactive_chain_arg_last_vgpr(ptr addrspace(1) %
; GISEL11-NEXT: s_delay_alu instid0(VALU_DEP_1)
; GISEL11-NEXT: v_mov_b32_e32 v0, v12
; GISEL11-NEXT: global_store_b32 v[41:42], v0, off
; GISEL11-NEXT: s_nop 0
; GISEL11-NEXT: s_sendmsg sendmsg(MSG_DEALLOC_VGPRS)
; GISEL11-NEXT: s_endpgm
;
; DAGISEL11-LABEL: set_inactive_chain_arg_last_vgpr:
Expand Down Expand Up @@ -570,8 +548,6 @@ define amdgpu_cs_chain void @set_inactive_chain_arg_last_vgpr(ptr addrspace(1) %
; DAGISEL11-NEXT: s_delay_alu instid0(VALU_DEP_1)
; DAGISEL11-NEXT: v_mov_b32_e32 v0, v12
; DAGISEL11-NEXT: global_store_b32 v[41:42], v0, off
; DAGISEL11-NEXT: s_nop 0
; DAGISEL11-NEXT: s_sendmsg sendmsg(MSG_DEALLOC_VGPRS)
; DAGISEL11-NEXT: s_endpgm
;
; GISEL10-LABEL: set_inactive_chain_arg_last_vgpr:
Expand Down Expand Up @@ -685,8 +661,6 @@ define amdgpu_cs_chain void @set_inactive_chain_arg_last_vgpr(ptr addrspace(1) %
; GISEL11_W64-NEXT: s_delay_alu instid0(VALU_DEP_1)
; GISEL11_W64-NEXT: v_mov_b32_e32 v0, v12
; GISEL11_W64-NEXT: global_store_b32 v[41:42], v0, off
; GISEL11_W64-NEXT: s_nop 0
; GISEL11_W64-NEXT: s_sendmsg sendmsg(MSG_DEALLOC_VGPRS)
; GISEL11_W64-NEXT: s_endpgm
;
; DAGISEL11_W64-LABEL: set_inactive_chain_arg_last_vgpr:
Expand Down Expand Up @@ -724,8 +698,6 @@ define amdgpu_cs_chain void @set_inactive_chain_arg_last_vgpr(ptr addrspace(1) %
; DAGISEL11_W64-NEXT: s_delay_alu instid0(VALU_DEP_1)
; DAGISEL11_W64-NEXT: v_mov_b32_e32 v0, v12
; DAGISEL11_W64-NEXT: global_store_b32 v[41:42], v0, off
; DAGISEL11_W64-NEXT: s_nop 0
; DAGISEL11_W64-NEXT: s_sendmsg sendmsg(MSG_DEALLOC_VGPRS)
; DAGISEL11_W64-NEXT: s_endpgm
;
; GISEL10_W64-LABEL: set_inactive_chain_arg_last_vgpr:
Expand Down
24 changes: 24 additions & 0 deletions llvm/test/CodeGen/AMDGPU/release-vgprs.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
define amdgpu_ps void @global_atomic() { ret void }
define amdgpu_ps void @image_atomic() { ret void }
define amdgpu_ps void @global_store_optnone() noinline optnone { ret void }
define amdgpu_gfx void @gfx_function() { ret void }
define void @ccc_function() { ret void }
...

---
Expand Down Expand Up @@ -556,3 +558,25 @@ body: |
S_WAITCNT_VSCNT undef $sgpr_null, 0
S_ENDPGM 0
...

---
name: gfx_function
body: |
bb.0:
; CHECK-LABEL: name: gfx_function
; CHECK-NOT: S_SENDMSG 3
; CHECK: S_ENDPGM 0
GLOBAL_STORE_DWORD undef renamable $vgpr0_vgpr1, killed renamable $vgpr1, 0, 4, implicit $exec
S_ENDPGM 0
...

---
name: ccc_function
body: |
bb.0:
; CHECK-LABEL: name: ccc_function
; CHECK-NOT: S_SENDMSG 3
; CHECK: S_ENDPGM 0
GLOBAL_STORE_DWORD undef renamable $vgpr0_vgpr1, killed renamable $vgpr1, 0, 4, implicit $exec
S_ENDPGM 0
...
Copy link
Contributor

Choose a reason for hiding this comment

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

Tangentially related but do we avoid inserting waits before unreachable in noreturn functions?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think unreachable exists in MIR does it? I would expect to see an MBB ending with a tail call, and SIInsertWaitcnts certainly should not insert any waits after the tail call.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you end up with an MBB ending in nothing, which may or may not also happen to be at the end of the function

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'm not sure if we have any special handling for this. How much do we care about it? Are you concerned about the code size? I'm guessing if we reach an unreachable then waiting a little extra isn't the biggest problem...