Repository navigation
fix(rocprofiler-sdk): report LDS_Block_Size from the dispatch, not the code-object symbol - #12311
tianxiaojiang4 wants to merge 1 commit into
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
bff0ced to
8997981
Compare
generate_csv fills LDS_Block_Size from get_kernel_symbol(...)->group_segment_size, which is the code object's group_segment_fixed_size -- the STATIC allocation. Kernels that request group memory dynamically at dispatch declare 0 there, so the column reads 0 for them however much LDS they use. That is every Triton and Gluon kernel, and any HIP kernel using extern __shared__. rocprofiler_kernel_dispatch_info_t::group_segment_size is documented in fwd.h as "Size of group segment memory (static + runtime) required by the kernel (per work-group), in bytes. AKA: LDS size", and it is already on the record both call sites hold. Measured on gfx950 / ROCm 7.2.4 with one Triton kernel (aiter sparse MLA decode): Triton metadata shared=68608, .amdhsa_group_segment_fixed_size=0, rocpd dispatch table group_segment_size=68608, rocpd symbol table=0, CSV LDS_Block_Size=0. A control in the same trace, at::native::reduce_kernel with static __shared__, reports 512 correctly, so the column is not always zero -- it is specifically blind to dynamic allocation. Closes ROCm#12312 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8997981 to
aa02030
Compare
|
@ROCm/rocm-devtools-team — mentioning you directly because CODEOWNERS routing does not fire for Status: all policy checks pass, rebased onto current The change in one line: No rush from my side; flagging it mainly so it does not sit unnoticed. |
|
This pull request has been inactive for 25 days and will be marked as stale. If you would like to keep this PR open, please:
This PR will be automatically closed in 5 days if no further activity occurs. |
|
This pull request has been automatically closed due to inactivity (30 days with no updates). If you'd like to continue working on this, feel free to reopen the PR or create a new one. |
Closes #12312
Summary
LDS_Block_Sizein the CSV outputs is sourced from the code-object symbol, which carries onlythe static group segment. Kernels that request group memory dynamically at dispatch declare 0
there, so the column reads 0 for them no matter how much LDS they actually use — that is every
Triton and Gluon kernel, and any HIP kernel using
extern __shared__.projects/rocprofiler-sdk/source/lib/output/generateCSV.cpp, lines 300 and 635:rocprofiler_kernel_dispatch_info_t::group_segment_sizeis documented infwd.hasand it is already on the
recordboth call sites hold. This switches to it.Evidence
gfx950, ROCm 7.2.4, one Triton kernel from aiter's sparse-MLA decode, plus a control in the same
trace:
_sparse_mla(Triton, dynamic)at::native::reduce_kernel(static__shared__)shared.amdhsa_group_segment_fixed_sizegroup_segment_sizegroup_segment_sizeLDS_Block_SizeThe control matters: the column is not always zero, it is specifically blind to dynamic allocation.
Scratch_Sizeagreed between both tables (44), which is consistent with it already reading thedispatch side.
Why this looks like an oversight rather than intent
Scratch_Sizecolumn already readsrecord.dispatch_info.private_segment_size.The register columns come from the symbol because registers cannot be dynamic — so of the two
fields that can have a runtime component, LDS is the only one not taking it.
generateRocpd.cppalready writes the dispatch table'sgroup_segment_sizefrom the dispatchinfo, so rocpd output is correct today while the CSV is not — which is how the table above was
produced.
Impact
Occupancy analysis from a kernel trace is silently wrong for dynamically-allocating kernels. In our
case a kernel using 67 KiB/workgroup reported 0, which reads as unbounded occupancy; the true figure
allows 2 workgroups per CU on a 160 KiB/CU part. That sent a kernel investigation down the wrong
path for several days.
Testing
I have not been able to run
ctest— building the SDK needs a full ROCm toolchain I do not haveset up, and I did not want to gate a two-token change on it. The correctness argument is instead that
generateRocpd.cppalready reads exactly the field this switches to, and its output carries theright value (68608) for the same dispatch, as shown above. No test references
LDS_Block_Size.The policy bot also flags this as a source change without an accompanying unit test. I have not
added one because a meaningful test needs a kernel that allocates LDS dynamically plus CSV
validation, which I cannot run end to end here — I would rather say so than add a test I have not
executed. Happy to write one to your preferred shape, adjust the formatting, or split the two call
sites.
🤖 Generated with Claude Code