Repository navigation
perf(rocr): shadow XGMI device batches, read timestamps with one load - #14338
Merged
Merged
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
saleelk
marked this pull request as ready for review
October 27, 2026 05:28
saleelk
requested review from
atgutier,
cfreeamd,
dayatsin-amd,
kentrussell and
shwetagkhatri
as code owners
October 27, 2026 05:28
saleelk
force-pushed
the
users/saleelk/FixShadowSignal
branch
from
October 27, 2026 06:17
2fdb2f4 to
161fea1
Compare
cfreeamd
reviewed
Oct 27, 2026
cfreeamd
left a comment
Contributor
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
The SSE timestamp load and the shadow-allocation refactor are structurally sound; amd_signal_t is aligned(64) and amd_signal is SharedSignal's first member, so the _mm_load_si128 address is 32-byte aligned at runtime.
saleelk
force-pushed
the
users/saleelk/FixShadowSignal
branch
from
October 27, 2026 22:21
161fea1 to
c98bac9
Compare
Shadow device-memory signal batches on XGMI-attached GPUs. hsa_amd_signal_batch_create keeps a host-side copy of each member's Signal* for batches whose arena the host reaches across a link, so that Signal::Convert resolves a handle without reading the slot's id and core_signal from device memory. The copy was only made when the GPU's stores need an HDP flush (PcieStoresNeedFlush), which is false when the CPU is attached over XGMI. On those systems every signal API call on a device-memory batch member still read two fields across XGMI before the value. Shadow every batch in a discrete GPU's device pool instead. Host reads of a discrete GPU's memory cross PCIe or XGMI either way; whether stores need an HDP flush only decides how stores and read-modify-writes are made, which is unchanged. An APU's device pools are not shadowed: the host shares that memory, so reading the slot costs no link round trip. Read a signal's dispatch timestamps with one load. SharedSignal::GetRawTs read start_ts and end_ts with two 8-byte loads, so for a signal in device memory hsa_amd_profiling_get_dispatch_time and _get_async_copy_time cost two uncached round trips. The two fields fill one aligned 16-byte chunk of amd_signal_t, and one 16-byte load reads it in a single round trip on x86-64. static_asserts pin the layout and the 16-byte alignment of amd_signal in SharedSignal that the load relies on. On MI300X a host read of device memory costs 1.1 us for 8 or 16 bytes and 2.2 us for 32 or 64. MI300X: hsa_amd_profiling_get_dispatch_time on a device-memory signal takes 1.15-1.27 us instead of 2.24-2.47 us. With the CLR built-in profiler (a signal per dispatch) and device-memory queue signals, a 100-node graph launch enqueues in 351 us instead of 463 us, and a single launch plus sync takes 10.5 us instead of 11.7 us. Host-memory signals are unchanged.
saleelk
force-pushed
the
users/saleelk/FixShadowSignal
branch
from
October 27, 2026 23:13
c98bac9 to
fc34b80
Compare
cfreeamd
approved these changes
Oct 27, 2026
dayatsin-amd
self-requested a review
October 28, 2026 02:29
dayatsin-amd
approved these changes
Oct 28, 2026
Contributor
TheRock Submodule Bump ActivityNewest first
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Two host-side costs remain for signals in device memory after #14115:
Signal*soSignal::Convertresolves a device-memory batch handle without reading the slot'sidandcore_signalacross the link. The copy was only made whenGpuAgent::PcieStoresNeedFlush()is true, which is!xgmi_cpu_gpu_. On XGMI-attached systems,every signal API call on a device-memory batch member still read two fields across XGMI.
SharedSignal::GetRawTsreadsstart_tsandend_tswith two 8-byte loads, sohsa_amd_profiling_get_dispatch_timeandhsa_amd_profiling_get_async_copy_timecost two uncached reads per device-memory signal. CLR'sbuilt-in profiler (
GPU_CLR_PROFILE_OUTPUT) attaches a signal to every dispatch, so this cost ispaid once per kernel.
Technical Details
hsa_amd_signal_batch_createshadows every batch whose arena is in a discrete GPU's device pool(
SignalBatch::Createtakes the decision asshadow_members). The host reads a discrete GPU'smemory across PCIe or XGMI either way.
host_access_via_hdpis unchanged: it still decides HDPflushes after stores and emulated read-modify-writes, which only apply over PCIe.
properties().Integrated) are not shadowed: the host shares that memory,so reading the slot costs no link round trip. Before, an APU's batches were shadowed when its CPU
link was not reported as XGMI.
SharedSignal::GetRawTsreads both timestamps with one 16-byte SSE2 load on x86-64. They fill onealigned 16-byte chunk of
amd_signal_t.static_asserts pin the layout and the alignment theload relies on:
alignof(amd_signal_t) >= 16andamd_signalat a 16-byte offset inSharedSignal. Both signal allocators align toSharedSignal. Other architectures keep the twoloads.
A host read of device memory on MI300X: 8-byte load 1.1 us, 16-byte load 1.1 us (one transaction),
32- or 64-byte load 2.2 us (two transactions).
Test Plan
*Signal*and IPC tests.hip_launch_latency -k -gwithGPU_CLR_PROFILE_OUTPUTset, CLR device-memory queue signals onand off, against
develop's ROCr; kernel durations from the traces compared.HSA_ENABLE_SDMA=1and0, checking copydurations in the trace (
hsa_amd_profiling_get_async_copy_time).Test Result
MI300X (PCIe host):
mismatches).
developand thisbranch interleaved:
hsa_amd_profiling_get_dispatch_time2.24-2.47 -> 1.15-1.27 us;hsa_signal_load_relaxedunchanged at 1.06-1.25 us. Host-memory signals: 6-7 ns both.launch + sync 11.7 -> 10.5 us. Host-memory signals unchanged.
(3.328 us) on both ROCr builds; copy durations consistent with SDMA on and off.
Not run on an XGMI-attached system or an APU. On XGMI, device batches now take the shadow path
that #14115 measured over PCIe, where dropping the slot's
idandcore_signalreads tookhsa_signal_load_relaxedfrom 3.5 to 1.1 us; the XGMI saving is two reads across XGMI per call,not measured here.
Submission Checklist