Repository navigation
rocr: serve swap copies with a shader blit where the DMA engine cannot - #14190
shwetagkhatri wants to merge 4 commits into
Conversation
cfreeamd
left a comment
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
Scope: projects/rocr-runtime/ only. The feature shape looks coherent — SwapCopy added as an optional per-ISA shader, BlitKernel gains an N-entry SubmitSwapCopyCommand, and GpuAgent::DmaCopySwap / DmaCopyBatchFallback route to the shader blit where the SDMA engine lacks a usable swap sub-op (gfx125+ correctly excluded). The AssembleShader → SelectASICShader() / IsShaderAvailable() refactor is a clean single-source-of-truth split. test_categories.yaml updated in all three profiles. SPDX headers present on all four new files.
[Required] PR title rocr: serve swap copies with a shader blit where the DMA engine cannot does not follow type(component): convention — e.g. feat(rocr): serve swap copies with a shader blit where the DMA engine cannot.
[Required] All four new files carry Copyright (c) Advanced Micro Devices — the standard AMD form uses the © glyph: Copyright © Advanced Micro Devices, Inc., or its affiliates.. Applies to copy_test_context.h, swap_copy_tests.cc, swap_copy_multi_entry.cc, and blit_swapCopy.s.
The PR description is empty — for a change that rewrites a shared BlitKernel primitive (AcquireWriteIndex) and introduces a new engine-selection path, a brief description and any benchmark data would be helpful.
| } | ||
|
|
||
| uint64_t write_index = queue_->AddWriteIndexAcqRel(num_packet); | ||
| uint64_t BlitKernel::AcquireWriteIndex(uint32_t num_packet) { |
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
[Important] This rewrites AcquireWriteIndex from an unconditional AddWriteIndexAcqRel (advance-then-wait) to a wait-then-CAS-publish loop. The new logic correctly avoids a lost-wakeup/stale-packet hazard. However, AcquireWriteIndex is also on the pre-existing SubmitLinearCopyCommand path (write_index = AcquireWriteIndex at the linear-copy call site), so this is a concurrency-semantics change to every BlitKernel copy, not just the new swap path. Worth calling out explicitly in the PR description and verifying the ordinary linear-copy path has been exercised with the change (a concurrency change to a shared hot primitive is the riskiest part of this PR, bundled into a swap-feature change).
| std::atomic<uint32_t> remaining; | ||
| }; | ||
|
|
||
| DeferredSubmit* op = new DeferredSubmit{std::move(submit), {}}; |
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
[Question] DeferUntilDepsResolved heap-allocates DeferredSubmit and frees it only when the last dependency handler fires. What happens if the runtime begins shutdown while a batch is deferred — i.e. the dependency signals never reach 0 before the async signal handler threads are joined/torn down? The op object and captured buffer-list vectors would leak, and the deferred submit() could fire against an already-torn-down blit queue. Is there a guarantee that all pending deps resolve (or that SetAsyncSignalHandler registrations are drained) before BlitKernel/queue teardown?
| // packet is made visible to the packet processor. Relaxed is sufficient: the | ||
| // release fence in ReleaseWriteIndex orders this ahead of the doorbell, and | ||
| // no dispatch exists to observe it before then. | ||
| if (total_dispatches > 1) out_signal.AddRelaxed(total_dispatches - 1); |
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
[Question] Completion-signal ordering on the deferred path (pattern:completion-signal-ordering). The direct path looks correct: out_signal.AddRelaxed(total_dispatches - 1) before ReleaseWriteIndex's release fence + doorbell. On the deferred path, the lambda re-enters SubmitSwapCopyCommand with no_deps, re-derives total_dispatches, and re-raises the signal inside the handler thread while the caller may already be waiting. Is there any window where a decrement from an early dispatch of reservation N races the AddRelaxed accounting for reservation N+1? A note on why the per-reservation raise-before-publish keeps the count monotonically correct would settle it.
| @@ -0,0 +1,634 @@ | |||
| /* | |||
| * Copyright (c) Advanced Micro Devices, Inc., or its affiliates. | |||
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
[Required] License header uses Copyright (c) Advanced Micro Devices — the standard AMD form uses the © glyph: Copyright © Advanced Micro Devices, Inc., or its affiliates.. Same fix needed in copy_test_context.h, swap_copy_multi_entry.cc, and blit_swapCopy.s.
| @@ -90,6 +90,8 @@ test_categories: | |||
| - "rocrtstFunc.GpuDiscoveryDeprecatedDoorbellTest" | |||
| - "DoorbellTypeValidation.*" | |||
| - "BroadcastCapabilityQuery.*" | |||
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
[Question] SwapCopyTest.* / SwapCopyMultiEntry.* are registered here across all three profiles, but I don't see a matching update to platform_config.yaml. Tests are TEST_F suites (no main.cc / RunGenericTest() wiring needed), but please confirm whether platform_config.yaml also needs an entry for these suites to run in CI.
A swap exchanges the contents of two buffers. Only some DMA engines carry a swap sub-op, and until now an agent whose engine did not was left rejecting the request outright, even though the same exchange is expressible as a compute dispatch the agent can run. Add a swap blit shader and route to it by capability. The DMA engine keeps the op on the parts that already reach it; an agent whose engine does not carry the sub-op falls through to the shader rather than returning an error for an operation the agent can complete. The capability claim is deliberately narrower than the packet definition: a part may encode a swap sub-op without the engine retiring it, and an engine that accepts a swap it never completes stalls the queue rather than returning a status the caller can act on. Routing for parts already served by the native packet is left exactly as it was. This commit only adds a path for agents that previously had none, so no agent moves off an engine it was using before. The blit entry point takes a batch of address pairs because that is the shape the batch copy API and its caller use; the single-pair form forwards to it and is non-virtual so an implementation cannot support one shape and reject the other. Batches larger than one queue reservation are split across reservations, and a split batch waits for its dependent signals off-queue first: barriers at the head of the first reservation would otherwise wait on signals only the caller can satisfy, inside the call the caller must return from to satisfy them. The shader addresses memory per 16-byte chunk plus a byte tail, so the fallback has no alignment requirement of its own and does not inherit the DMA engine's. rocrtst coverage for both the single-pair and batch forms, including the multi-reservation split and its deferred-dependency path, lands in a companion commit in this series rather than here, so that this commit stays runtime-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The blit kernel advanced the queue write index by the full reservation and only then waited for room. Between those two steps the write index sat more than one ring beyond the read index, which the queue's producer contract does not permit, and the slots it named aliased packets the packet processor had not consumed yet. A submission that reserved the whole ring while a whole ring was already outstanding then waited for a drain that could not happen, because its own reservation had aliased the work it was waiting on. Wait for space first and take the slots with a compare-and-swap, so the write index is never published more than one ring beyond the read index. A full ring stays a legal reservation, which is the bound the AQL queue's own producer already uses. This also narrows the window in which the packet processor can observe a slot a reservation has claimed but not yet filled. It does not close that window: the packet body is still written after the index is published, exactly as before. A reservation larger than the ring can never be satisfied at all. Add a predicate for that and have every submit path reject it with an out of resources status instead of waiting forever, checked before the completion signal is raised so a rejected submission leaves no counted but unsent work behind. Reproduced and fixed on GFX12.5, where multi entry swap copies make whole ring reservations; the same reservation sizing is used on earlier GFX families. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The swap shader fallback and the ring reservation that serves it landed without any test of their own: the runtime commits beneath this one change only runtime sources, so nothing in the suite exercises the path they add and a regression in it would reach a release unobserved. Add two suites. The single-pair suite states the operation's contract - after a swap each buffer holds what the other held - across sizes from one byte to a megabyte, over aligned and unaligned lengths, and through a double swap that must restore the original contents. The batch suite drives the list form, where several thousand entries exceed what one queue reservation holds, so the split across reservations and the completion-signal accounting that split requires are what is under test, together with the shader's trailing-byte phase for lengths that are not a multiple of sixteen. The dependent-signal case asserts the completion signal is unresolved rather than that it still holds the value it was created with: the fan-out routing raises the count once per engine group before submitting, so a value above one is legitimate mid-flight and only zero means done. The same case states the requirement a second way, engine-independently, by checking that no byte has moved while the dependency is still blocked. The submission wrapper screens no argument of its own. A wrapper that rejected a null pointer or a zero length before the call would answer the negative cases itself, leaving the runtime's own validation unexercised and the suite passing whether or not that validation exists. For the same reason every negative case supplies a real completion signal, since the entry point validates the signal before it reaches any per-type field and a zero handle would be refused by the signal check rather than by the argument the case names. Nothing here skips. A skipped test is reported as passing, so an environment that supplied no device or could not obtain its buffers would otherwise be indistinguishable from a clean run; a device that cannot satisfy the test is reported as a failure to test, with the allocation status and the entry it failed on. Both suites are registered in the standard, comprehensive and full tiers. An unregistered suite is built and never run, which looks the same as a suite that passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The swap capability flag is claimed by gfx94x+ only, so routing on it alone sends gfx125+ to the shader blit even though its SDMA engine reaches the swap packet through a separate condition. Exclude gfx125+ from the fallback so it keeps the engine path, and restore the matching term in the fan-out validation so the op is not refused one level up. Parts that claim neither are unaffected and still reach the shader blit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
77786ae to
07e91fa
Compare
39d627e to
32d0492
Compare
JIRA ID : AILIROCR-24
This is PR 2 of 3 carved out of #6185. It is stacked on PR1, the broadcast
destination ceiling query, and must merge after it. PR3, the broadcast shader
blit fallback, is stacked on this one and reuses the blit-kernel infrastructure
introduced here.
WHAT THIS CHANGES
A swap copy exchanges the contents of two buffers. Where the SDMA engine has no
usable swap sub-op, the request previously failed outright. This adds a shader
blit path that performs the exchange, and routes to it when the engine cannot.
The routing decision is in GpuAgent::DmaCopySwap (amd_gpu_agent.cpp:2156-2160).
An agent is treated as native-capable when its SDMA blit either claims the swap
sub-op or is gfx125+, which reaches the swap packet through its own condition
rather than through the capability flag. Everything else goes to
DmaCopyBatchFallback and the shader blit rather than being refused outright.
Not unconditionally, though: that fallback's LINEAR_SWAP case still returns
HSA_STATUS_ERROR_INVALID_ARGUMENT at amd_gpu_agent.cpp:2272-2273 when the
fallback blit itself reports no swap support. The change is that refusal is now
decided on the shader blit's capability rather than on the engine's. Both the
scalar and the list form route there, so a batched swap is not left without a
route.
gfx125+ is deliberately excluded from the fallback. It has a working engine
path today, and moving it onto the shader would be a behaviour change this PR
does not need in order to deliver the feature. Three sites carry that exclusion:
The first is new: develop has no DmaCopySwap routing gate at all, so there is no
develop condition for it to match. The other two are unchanged, confirmed by
their absence from git diff 26fb5d9 39d627e.
The submit guard at amd_blit_sdma.cpp:1385 is unreachable in this tree. That is
established by enumeration rather than inspection: SubmitBodies has exactly two
call sites, both inside DmaCopyFanOutOp, and a LINEAR_SWAP op reaches them only
through DmaCopySwap, whose sole caller is amd_gpu_agent.cpp:2385. Every such
path has already passed the :2156 gate, which admits only agents for which the
guard's condition is false. It is left in place as a backstop. Reviewers should
not read it as live.
ONE LIMIT ON THE EXCLUSION, and it is a NEW EXPOSURE THIS PR INTRODUCES. State
it plainly rather than leaving a reviewer to find it: the exclusion holds with
SDMA up, and with SDMA disabled it does not hold at all.
Two routes carry a gfx12.5 swap past the :2156 gate under HSA_ENABLE_SDMA=0:
What is NOT pre-existing is where that lands. On develop, LINEAR_SWAP sits in
the fallback's reject group alongside broadcast, indirect and rect
(develop:2135, returning HSA_STATUS_ERROR_INVALID_ARGUMENT at develop:2144), so
develop REFUSES a gfx12.5 swap under SDMA=0. This PR adds the LINEAR_SWAP case
(:2245-2286) that instead routes it to BlitKernel::SubmitSwapCopyCommand - which
this body states below is byte-identical to the function that hung on A0. The
case carries no gfx125+ carve-out; it gates on the shader blit's
SupportsSwapCopyCommand() instead.
So the net effect is introduced here: under SDMA=0 on gfx12.5, develop refuses
and this PR dispatches onto the shader swap path. The :2156 exclusion covers the
SDMA-up configuration, which is the one the hang below was measured in. It is
not a guarantee that gfx12.5 never reaches the shader swap path, and under
SDMA=0 it specifically does.
The swap capability flag itself is lifted out of its enclosing block and
commented (amd_blit_sdma.cpp:233). This is not a value change: develop computes
swap_supported_ = (minor >= 4) nested inside else if (major == 9), which is the
same predicate. It is written explicitly so the claim is readable at the
assignment rather than inferred from an enclosing block.
The ring reservation commit is a correctness fix that belongs with this work
rather than after it: space is reserved before the write index is published, so
a consumer cannot observe an index pointing at space that was never reserved.
SIZE
VALIDATION
Measured 2026-10-19, before the rebase onto develop 26fb5d9 and before the
gfx125+ exclusion commit. Read the limits in WHAT IS NOT VALIDATED below before
quoting any of it.
The word "base" below does not name one tree, and the difference is deliberate:
origin/develop was NOT used as the gfx1200 control, by choice. It sits two
commits behind this branch's parent, and those two commits are PR1 plus a build
fix. PR1 and PR2 both modify amd_blit_sdma.cpp, amd_gpu_agent.cpp,
amd_gpu_agent.h and test_categories.yaml, so a develop control would have folded
PR1's 8 files into the delta and made the result unattributable. HEAD~3 isolates
this PR's three commits exactly. The consequence for reading the tables: the
two nv44-ss swap rows, both nv44-ss throughput rows and the whole 152-test
regression table are against 5ca80d13222, not against develop. The four splinter
swap rows are against e24fb12.
The fallback is only reachable where the agent is not native-capable. That
predicate is false for every gfx9 part at minor 4 or above, so on gfx942 and
gfx950 this code is unreachable in default configuration and a green there
carries no information about the feature. Both configurations were therefore
measured.
Swap suite, 16 tests, SwapCopyTest 10 plus SwapCopyMultiEntry 6:
The last row is the important one and is why the gfx942 default arm proves
nothing: the BASE runtime also scores 16/16 there, because the pre-existing SDMA
swap services every case. Only the HSA_ENABLE_SDMA=0 pair on that box separates
the arms.
On gfx1200 the fallback is the default path, so the base runtime fails with no
environment override at all. 7 of the 11 failures record status 4097,
HSA_STATUS_ERROR_INVALID_ARGUMENT. The other 4 fail at a different assertion
(swap_copy_multi_entry.cc:228 and :328) which prints the status in hex and whose
value the run log does not capture, so those 4 are reported as failures without
a recorded status rather than folded into the 4097 count.
The five tests that pass on the base runtime are the same five on both boxes,
and they are exactly the degenerate cases that require no byte exchange:
The partition falling exactly where the mechanism predicts is the evidence that
the feature, and not some unrelated difference between the arms, is what the
test is measuring.
Throughput, 100MB swap. splinter figures are medians of repeated runs measured
standalone; the same report measures 9.3006 GB/s for the same case in full-suite
context, so these are not comparable to suite-context numbers:
No speedup ratio is quoted against the gfx1200 base arm because it cannot
perform the operation at all; there is no comparison to make. The two splinter
default arms agreeing to six significant figures, from two separately built
runtimes with different md5s, is further evidence that the default arm on that
box ran the pre-existing SDMA path rather than this one.
Full rocrtst regression on gfx1200:
No test passes on the base arm and fails on PR2. 19 self-skips per arm,
identical sets, none of them swap tests.
gfx12.5 - WHY IT IS EXCLUDED, AND WHAT IS AND IS NOT KNOWN
The swap shader path has a recorded hang on gfx1250, which is why this PR keeps
gfx125+ on the engine path rather than routing it to the fallback. The evidence
needs stating precisely, because it does not say what a summary of it would
suggest.
What was measured. On gfx1250 A0 (heliosr-1b114-c07-1, 2026-10-19), 2 of the 6
SwapCopyMultiEntry tests hung: TC_SWAP_ME_003 and TC_SWAP_ME_004. The other 4
passed. TC_SWAP_ME_003 was reproduced 4 times out of 4.
The tree matters and it is this one. The A0 tree was f15c34cfdf12, which is this
branch's pre-rebase head 4e25a82f717 plus exactly the three broadcast commits -
confirmed by ancestry, not by labelling. So the hang was measured on a tree that
CONTAINS this PR's commits, with the broadcast work on top. It was not measured
on some neighbouring branch. What it was not measured on is this branch's
CURRENT head, which did not exist then and which now carries the gfx125+
exclusion that keeps gfx12.5 off the shader path with SDMA up - the very
configuration the hang was recorded in. That exclusion is the change this PR
makes in response to the hang, and it is unvalidated.
The run's SDMA configuration is not stated in prose anywhere in the report, so
it is read off the backtrace instead: frame #4 is GpuAgent::DmaCopyBatch at
amd_gpu_agent.cpp:2413, which in the tested tree f15c34cfdf12 is the
DmaCopySwap(op, dep_signals) call - downstream of the !isSDMA() short-circuit at
:2357. Had SDMA been disabled, frame #4 would have read :2358. The control arm
routing to the native SDMA engine and passing ME_003 and ME_004 corroborates it.
Captured under RelWithDebInfo, stable across two samples 20 s apart:
The spin at amd_blit_kernel.cpp:929 requires
write_index + num_packet - read_index <= queue_size. With those values
5 + 64 - 0 = 69 > 64, permanently, and the loop has no timeout and no backoff.
ReservationFits admits num_packet == queue_size exactly, so a full-queue
reservation passes the fit check and then waits for the read index to catch the
write index completely. first_reservation TRUE rules out spanning as the cause.
Capping the reservation at queue_size - 1 was built, loaded and still hung; the
shortfall is 5 slots, not 1.
What this does NOT establish, stated because the obvious reading is wrong:
What IS established about this branch: BlitKernel::SubmitSwapCopyCommand is
byte-identical between this branch and the A0-tested tree - same md5,
f17ac3664fb5644d7c4179b63d454cd7, 158 lines - as is
MaxDispatchesPerReservation. So had this PR routed gfx125+ to the shader, it
would have carried exactly the code that hung. It does not route it there.
The open question is why the blit queue's read index reads 0 while 5 completed
packets have been written. That is where the root cause is, it is not found
yet, and it is tracked independently of this PR.
WHAT IS NOT VALIDATED
KNOWN REVIEW POINT
Five of the sixteen tests cannot detect the feature being absent. This is
demonstrated rather than argued: they pass against a base runtime that has no
swap support at all. TC_SWAP_009_SameBuffer branches on the returned status and
accepts either outcome, so a runtime that refuses the op cannot fail it - the
success branch does still verify the buffer is unchanged. Effective coverage is
11 of 16. The gap is at the assertion end, not in the wrapper's argument
handling.
🤖 Generated with Claude Code