feat(rocm): implement the vt::Backend graph-capture seam on hipGraph (W1, #332) - #473
Conversation
…(W1, mudler#332) The six capture virtuals mirrored from cuda_backend.cu call for call, plus the RED-first test that makes the mirror checkable. SupportsGraphCapture() goes true for kROCM; the model-level path stays gated on Platform's support_static_graph_mode(), which is W2. RED first, for the intended reason: before this change the case failed on CHECK(rocm.SupportsGraphCapture()) == false and BeginCapture threw "vt: graph capture unsupported on this backend" from src/vt/backend.cpp -- the base impl, confirming kROCM inherited the throwing default rather than overriding it. Green after: 10/10 assertions across the stored-graph and handle paths. Both mutations from the spec's §6 were run in a scratch copy and both turn the test red: * ReplayGraph made a no-op -> 3 failures, dst never updates. * EndCaptureGraph executing once and freezing the result (the "replays a snapshot" defect) -> step 4 PASSES, step 5 FAILS (CHECK(17 == 34): dst holds pattern A where it must hold B). The second is why step 5 exists. A graph that replays baked values instead of re-executing over persistent buffers passes a naive test and then silently serves stale inputs on every decode step after the first. rocm_backend.hip was restored byte-for-byte afterwards (sha256 4a6abb18... before and after). D1 VERIFIED rather than trusted, and it is worse than the spec assumed. A cold captured GEMM fails with hipMalloc AND hipFree "operation not permitted when stream is capturing" AND hipblasCreate INTERNAL_ERROR -- there are TWO lazy initialisations in that path, not just LtWorkspace's growth. All fail loudly; none corrupts. Pre-warming the identical GEMM clears both and the captured graph replays numerically correct. The new test pins the MITIGATION, not the hazard: asserting that the cold path throws would forbid a future capture-safe allocator. The spec records the consequence for W2 -- the pre-warm must reach handle creation, not only workspace growth. DEVIATION from the CUDA source: hipGraphExecDestroy and hipGraphDestroy are [[nodiscard]] where their cudaGraph* counterparts are not, so the mirrored bare calls do not compile. They are Check()ed, matching Free/FreePinned in the same file. VT_BENCH_PROFILE_CONTROL is deliberately not ported (spec §2). Gates: (1) HIP gfx1200 -Werror 0 warnings; non-HIP build object-compiles the test file clean via vllm_rocm_platform_syntax_check; CPU ctest 380/382, the two failures pre-existing and structurally unrelated (rocm_backend.hip is compiled 0 times in a CPU build, and test_safetensors links nothing this touches) -- test_safetensors is an mmap-residency measurement, test_serve_low_tools wants shellcheck. (2) ctest -R 'rocm|cross_device' 3/3. (7) check-agent-record, check-public-doc-tables, check-test-registration, check-device-leakage all OK; DSR holds at 32, no new device predicate. Issue: mudler#332 Spec: .agents/specs/rocm-decode-graph.md FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
Finding: LOW-1 in review-rocm-decode-graph-w1.md claimed the comment above
DestroyGraph in rocm_backend.hip is "factually wrong" because a bare
hipGraphExecDestroy call compiled with 0 warnings, and asked for the
compile-failure rationale to be deleted.
That remediation is itself wrong and was NOT applied — checked and confirmed:
$ hipcc -std=c++20 -Werror -Wall -Wextra --offload-arch=gfx1200 -c probe.hip
error: ignoring return value of type 'hipError_t' declared with 'nodiscard'
attribute [-Werror,-Wunused-value]
Under this project's actual build flags (-Werror; the review's probe most
likely omitted it, or compiled below C++17 where __HIP_NODISCARD expands to
nothing) a bare call to any HIP function returning hipError_t is a hard error.
What WAS wrong is the attribution, not the claim of a compile failure.
Verified directly against $ROCM_PATH/include/hip/hip_runtime_api.h (ROCm
7.2.3, /nix/store/8nqihd79gkvmjpc3i9icz6y7pc6rf4ma-clr-7.2.3):
line 293: #define __HIP_NODISCARD [[nodiscard]] (guarded __cplusplus >= 201703L)
line 305: typedef enum __HIP_NODISCARD hipError_t { ... } hipError_t;
line 8321: hipError_t hipGraphExecDestroy(hipGraphExec_t graphExec); -- no attribute of its own
[[nodiscard]] sits on the hipError_t return TYPE, not on hipGraphExecDestroy's
declaration — so it applies to every HIP API returning hipError_t under
C++17+, not to hipGraphExecDestroy specifically. CUDA's cudaError_t carries no
such attribute, which is why the mirrored bare calls compile on the CUDA leg
and not here.
The same imprecision (source-attributed to hipGraphExecDestroy/hipGraphDestroy
specifically, rather than to hipError_t generally) is present in 150b8f4's
commit message. 150b8f4 is the reviewed, immutable head and is not amended;
this commit is the correction of record.
Remediation applied: rewrote the comment to attribute [[nodiscard]] correctly
to hipError_t via __HIP_NODISCARD, name the -Werror consequence, and contrast
with cudaError_t — while keeping the Free/FreePinned consistency rationale for
Check() unweakened, since that design-choice justification was never disputed.
Issue: mudler#332
Spec: .agents/specs/rocm-decode-graph.md
FOLLOWING_AGENTS_PROTOCOL
Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: AGENT:claude-sonnet-5 [claude-code]
…ymmetry (INFO-1) Finding: INFO-1 in review-rocm-decode-graph-w1.md — DestroyGraph and the prior-exec_ destroy inside EndCapture (src/vt/rocm/rocm_backend.hip) both Check() hipGraphExecDestroy's return and throw on failure, where the CUDA leg silently ignores it. A decode-graph teardown or column-change recapture (W2+) that destroys/recaptures an exec still in flight would throw on HIP where CUDA succeeds silently. Not a W1 defect: the model-level path is not engaged (support_static_graph_mode() stays false until W2), and W1's tests always Synchronize before destroying or recapturing, so the asymmetry is inert here. The reviewer's own remaining_concern section made the same point about the in-EndCapture destroy. Evidence: reviewer's static read of rocm_backend.hip's Check() calls on hipGraphExecDestroy in both DestroyGraph and EndCapture, contrasted against cuda_backend.cu's bare (unchecked) calls at the same call sites. Remediation: recorded as D6 in .agents/specs/rocm-decode-graph.md §8, matching D1's "Consequence for W2" shape — the decode-graph class must synchronize before destroying or recapturing an exec on HIP. Issue: mudler#332 Spec: .agents/specs/rocm-decode-graph.md FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-sonnet-5 [claude-code]
… path (INFO-2)
Finding: INFO-2 / mutation 8 in review-rocm-decode-graph-w1.md — EndCaptureGraph
returning the stale member exec_ instead of the freshly captured local exec
went GREEN, 10/10, in "ROCm backend: graph capture/replay re-executes captured
ops" (tests/vt/test_rocm_backend.cpp). Both the stored-graph path (EndCapture,
Copy(dst, src)) and the handle-path (EndCaptureGraph) captured the identical
operation, so the test could not tell a correct EndCaptureGraph from one that
silently handed back the previous graph.
Remediation: the handle-path section now allocates its own destination buffer
(dst2) and captures Copy(dst2, src) instead of Copy(dst, src), then reads back
from dst2. A stale exec_ (still bound to the stored-graph path's Copy(dst,
src)) now writes the wrong buffer, leaving dst2 at its pre-capture zero and
failing the post-replay CHECKs.
IMP-TEST-FIRST / IMP-MUTATE evidence — mutation 8 reapplied to a scratch edit
of EndCaptureGraph's return (`return reinterpret_cast<void*>(exec_);` in place
of the local `exec`), rebuilt, and rerun in isolation:
before fix: GREEN, 10/10 (reviewer's original finding)
after fix: RED, 7/10, 3 failed — CHECK(back.front() == 0x11) at line
366: values 0 == 17; CHECK(back.front() == 0x22) at line 373:
0 == 34; CHECK(back.back() == 0x22) at line 374: 0 == 34
(dst2 never written by the stale exec_, which still targets dst)
Restored byte-for-byte, sha256 confirmed equal before and after the mutation
for both src/vt/rocm/rocm_backend.hip and tests/vt/test_rocm_backend.cpp:
rocm_backend.hip: 7421f4d78c8f42d16c4d66c954ad5e3aac90bddb3daf1aa493f481736a9f1b5e
test_rocm_backend.cpp: 27dc8fc115cccd8da85849e2ac2d799a1e0b91739c7a946bee03b8e3628beec4
Post-fix, unmutated: capture case 10/10 (unchanged assertion count), gate 2
(ctest -R 'rocm|cross_device') 3/3.
Reviewer mutations 9 (leaked hipGraph_t) and 10 (skipped prior-exec_ destroy)
are explicitly out of scope per the finding — leak-only, not visible to a unit
test, not built here.
Issue: mudler#332
Spec: .agents/specs/rocm-decode-graph.md
FOLLOWING_AGENTS_PROTOCOL
Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: AGENT:claude-sonnet-5 [claude-code]
…h comment (LOW-1, take 2) e2160d8 fixed the [[nodiscard]] ATTRIBUTION (hipError_t via __HIP_NODISCARD, not hipGraphExecDestroy specifically) but kept an overstated consequence: "under this project's -Werror it is a hard error." That claim came from a standalone `hipcc -Werror -Wall -Wextra ...` probe, which does NOT reflect this project's actual HIP build flags. Caught by the reviewer's own corrected revision of the review (see previous commit) and independently reconfirmed here two ways: 1. Static: cmake/CompilerWarnings.cmake's vllm_cpp_set_warnings gates -Wall -Wextra -Werror on $<COMPILE_LANGUAGE:CXX/OBJCXX/CUDA> only -- no HIP branch. build-hip/compile_commands.json's actual entry for rocm_backend.hip.o carries no -Wall/-Wextra/-Werror at all: clang++ -DVLLM_CPP_HIP ... -O3 -DNDEBUG -std=c++20 --offload-arch=gfx1200 -fPIC -ffp-contract=off -x hip -c rocm_backend.hip 2. Dynamic: applied a scratch mutation (bare hipGraphExecDestroy call, no Check()) to DestroyGraph and rebuilt through this project's own `vllm` cmake target (not a standalone hipcc invocation): rocm_backend.hip:298:7: warning: ignoring return value of type 'hipError_t' declared with 'nodiscard' attribute [-Wunused-value] [100%] Built target vllm <- exit 0, 0 errors, build succeeds So in this project's actual build, a bare call WARNS and does not fail the build. It is a hard error only under an explicitly-added -Werror, such as the standalone hipcc probe e2160d8 relied on -- which is not what this project's CMake applies to .hip translation units. Remediation: rewrote the comment to state the warning as fact, name the exact gate that would need to change for it to become a build failure (vllm_cpp_set_warnings has no HIP branch), and keep the Check() rationale on its own merits (Free/FreePinned consistency, leak-vs-swallow) rather than on an overstated compile-failure claim. Restored byte-for-byte after the scratch mutation: sha256 7421f4d78c8f42d16c4d66c954ad5e3aac90bddb3daf1aa493f481736a9f1b5e before and after (matches the hash recorded in 93ad9ccc, confirming no drift from the mutation probe). Issue: mudler#332 Spec: .agents/specs/rocm-decode-graph.md FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-sonnet-5 [claude-code]
…udler#444) Since 9302732, rocm_paged_attn.hip includes <rocwmma/rocwmma.hpp> whenever the target is gfx1200/gfx1201. The guard is on ARCH, not on availability, so targeting a gfx12 board fires the include whether or not rocWMMA exists. clr — the store path this shell uses as ROCM_PATH — does not ship it, so `nix develop .#rocm-shell` could not build main at all on a gfx12 board: src/vt/rocm/rocm_paged_attn.hip:8:10: fatal error: 'rocwmma/rocwmma.hpp' file not found Reproduced on pristine upstream/main at 0f2b12e in a clean worktree with no other commits applied, so it is not a local-branch artifact. rocwmma is header-only, so it rides the existing overlay: rocmOverlayInputs symlinks each input's include/ into $ROCM_OVERLAY/include, which is already on CPATH. Adding it to the shell's packages list too keeps the two lists reading the same. Note the overlay is cached behind a .complete sentinel, so an existing shell needs $ROCM_OVERLAY removed once to pick this up. This is the NARROW half of mudler#444 and does not close it. It fixes this shell only; every other rocWMMA-free environment targeting gfx12 still fails the same way. The issue asks for CMake-level detection that fails configure with a message naming the package, which is the real fix and stays open. Deliberately NOT done here: gating the include on __has_include. It works (verified under hipcc, which correctly reports the header absent), but VT_ROCWMMA_OK also selects the kernel BODY — PagedAttnPrefillWmmaWave at rocm_paged_attn.hip:900 casts every parameter to (void) and returns when the macro is undefined. Deciding that macro by availability rather than arch would compile a paged-attention kernel that silently writes nothing on a gfx12 board with no rocWMMA. A build failure is the correct outcome there; this commit supplies the missing dependency instead of hiding the symptom. Verified: with this change `nix develop .#rocm-shell` builds the HIP target on gfx1200 with 0 warnings, and `ctest -R 'rocm|cross_device'` is 4/4. Issue: mudler#444 FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
|
@mudler This implementation is technically well evidenced, but it needs a maintainer call before merge: the follow-on measurements refute the throughput rationale and no production path consumes the seam yet. I also recommend splitting the unrelated |
|
Reviewed this as part of a sweep over the open external-contributor PRs. Short version: the code is in good shape and I could not fault the isolation — what is holding it is one records item and one decision that is not mine to make. What I verified. Isolation checks out three independent ways: every decode-graph call site ANDs The new bit-rot test has real teeth. I changed the Worth saying plainly: reporting the W3 result the way you did — measuring instead of assuming, then retracting the earlier prediction against your own interest — is exactly the behaviour this protocol is trying to produce. That is not a mark against the PR. What needs to change before it can merge. The W3 refutation lives only in the PR description. You have already written the analysis; it just needs to live in the file. A Two smaller things while you are in there:
The open decision. Because the row's own performance rationale is refuted, whether to carry the capability at all is a product call for @mudler rather than something a reviewer should settle. I have put the case to him with a recommendation to take it — the argument being that the runtime cost today is zero (nothing engages it) and that the One thing to be aware of either way: #523's per-call Not asking for a rebase — the merge is clean and nothing you touch has moved. |
…udler#444 localai-org-maint-bot review on PR mudler#473 recommended this PR stay scoped to the graph-capture seam; the rocwmma dev-shell fix (46d2f4c) is unrelated scope for mudler#444 and belongs in its own PR. Reverting it here; it will land via a fresh branch targeting mudler#444 instead. This reverts commit 46d2f4c. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-sonnet-5 [claude-code]
…s refuted (mudler#332) localai-bot's review on PR mudler#473 flagged that the W3 refutation (capture moved throughput 0-2%, not the predicted ~1.36x convergence) lived only in the PR description while this spec still opened with the live 2.99x/1.90x/1.46x table and an unretracted 'the premise survived' argument. A future agent opening this file to pick up W2/W3 would re-derive a result already closed. Adds D7 (§8) with the A/B table, the 126-replays-over-128-steps proof capture engaged, the user/sys/wall split showing capture removes no host CPU work, and the ordered next hypotheses (same-tool rocprof trace; where the host CPU time goes). Marks §1's fit paragraph and §7 gate 5's prediction table SUPERSEDED rather than rewriting them -- a falsified pre-registered prediction that gets quietly reworded is worthless. The 0.6B figure uses the review-corrected 0-2% (not the original run's overstated +3.2%, later traced to one low outlier in the OFF arm and confirmed independently) -- see the row's W2/W3 review findings. Also two records fixes from the same review: - src/vllm/platforms/rocm.cpp:67-69 still said hipGraph capture 'is not implemented' after this PR implemented it (the twin comment in rocm_backend.hip:22-28 was updated, this one was not). - Issue mudler#444 (main does not build for gfx1200/gfx1201) is carried by this row's flake.nix commit but was absent from the roadmap issue table. Issue: mudler#332 Spec: .agents/specs/rocm-decode-graph.md FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-sonnet-5 [claude-code]
|
Thanks for the thorough review — addressed all items:
|
Implements W1 of #332, the
first work item under the spec that landed in #390. 8 commits, 5 files,
+375/-14.
Updated 2026-08-13 in response to review: the
flake.nixrocwmma fix(previously bundled here) is now split into
#638 against #444, and the W3
throughput refutation described below is now also recorded in the spec itself
(
.agents/specs/rocm-decode-graph.md, D7) rather than living only in thisdescription — see
localai-bot's reviewcomment
for the full ask.
Row
BACKEND-ROCMBefore starting
#332, whose spec
(
.agents/specs/rocm-decode-graph.md) landed onmainvia spec(BACKEND-ROCM): port the vt::Backend graph-capture seam to hipGraph (#332) #390. W1 is its§9 second item. Distinct from rocm_matmul_hipblaslt.hip:384:13: error: no matching function for call to 'hipblasGemmEx' #201 and ROCm: -O0 RmsNorm triggers a CLR HostcallListener teardown deadlock #132, and from the in-flight gfx1100
GDN kernel work — different ops, different board.
#444 was opened/used here
originally but its fix has since moved to
#638 (see Honest gaps).
BACKEND-ROCMstaysACTIVE; this moves nolifecycle state, so no
STATUS/BENCHMARKS/NOWobligation and no## Nowsection under NOW.md is still a surface every PR must write: the doc-checkpoint gate marches them into it #374.
--- CUDA-graph capture/replayblock insrc/vt/cuda/cuda_backend.cu(the mirror source) and its"NO cudaMalloc/cudaFree inside the region"contract comment; the sixvirtuals
SupportsGraphCapturethroughDestroyGraphininclude/vt/backend.hwith their throwing base impls insrc/vt/backend.cpp;LtWorkspace()insrc/vt/rocm/rocm_matmul_hipblaslt.hip;support_static_graph_mode()inplatforms/interface.h,cuda.cpp,rocm.cpp; theCUDA backend: graph capture/replay re-executes captured opscase in
tests/vt/test_cuda_backend.cpp.What changed
The six
vt::Backendcapture virtuals implemented against hipGraph inrocm_backend.hip, mirroringcuda_backend.cucall for call, plus theRED-first test that makes the mirror checkable.
SupportsGraphCapture()becomestrue for
kROCM. No model-level behaviour changes —RocmPlatformstillinherits
support_static_graph_mode() == false, so no decode-graph classengages; flipping that is W2. This is the seam's second implementation;
Metal and Vulkan still carry the
stays FALSEnote.VT_BENCH_PROFILE_CONTROLis deliberately not ported (spec §2).Evidence
scripts/agent-preflight.sh— unticked deliberately. The declaredW1 gates (spec §7 gates 1, 2, 7) are green; 3 NixOS-local host failures
remain, all confirmed on an unmodified base:
test_release_archiveandtest_release_metadata(our ELF binaries carry/nix/store/...RPATHswhere the release-bundle validator wants bundle-relative), and
test_agent_onboard(fixture inherits hostinit.defaultBranch=mainagainst the test's hardcoded
master).tests/vt/test_rocm_backend.cpp—graph capture/replay re-executes captured opsanda pre-warmed GEMM captures and replays.moves and the row's lifecycle state is unchanged.
RED first, for the intended reason
Before implementation the case failed on
CHECK(rocm.SupportsGraphCapture()) == false, andBeginCapturethrewvt: graph capture unsupported on this backendfromsrc/vt/backend.cpp— thebase impl, confirming
kROCMinherited the throwing default rather thanoverriding it.
Gates, rerun on the rebased head
Gate 1's other half: a non-HIP build still object-compiles the test file clean
via
vllm_rocm_platform_syntax_check.Mutation evidence
Every guarantee the tests pin was deleted or inverted in a scratch copy and the
focused test rerun, then the tree restored byte-for-byte. The load-bearing one:
making
EndCaptureGraphexecute once and freeze its result — the "replays asnapshot" defect — leaves step 4 passing and step 5 failing
(
CHECK(17 == 34)). That is why step 5 exists: a graph that replays bakedvalues instead of re-executing over persistent buffers would pass a naive test
and then serve stale inputs on every decode step after the first.
An independent reviewer ran 10 further mutations against the same head and
returned PASS. Three findings came back and all are addressed in-branch: a
[[nodiscard]]attribution error in a code comment, theDestroyGraphasymmetry now recorded as D6, and a coverage gap where returning a stale
exec_fromEndCaptureGraphpassed because both paths captured an identicalcopy — the handle path now targets a distinct buffer, and re-running that
mutation turns it red. The full review write-up is available on request.
Speed claims
gate stays false. And the row's performance rationale has since been
refuted — see "What W3 measured" below. Read that before weighing this
change on throughput grounds.
${GPU_LOCK}— N/A, contributor's ownboard (RX 9060 XT, gfx1200).
What W3 measured — the rationale for this row does not hold
This PR is W1. W2 and W3 have since run on a follow-up branch, and the
pre-registered performance gate failed: capture engaged correctly
(confirmed by 126 replays over 128 decode steps and a host/user/sys split
showing it removes no host CPU work) but moved throughput only 0-2% across
three model sizes, not the predicted ~1.36x convergence — indistinguishable
from noise. Not a ceiling claim: the gap is unexplained, not irreducible.
This is now recorded as D7 in
.agents/specs/rocm-decode-graph.md, withthe full A/B table, the replay-count and CPU-split evidence, the correction
history (an initial +3.2% for Qwen3-0.6B was overstated — an independent
reviewer traced it to one low outlier and the honest figure is 0-2%), and the
ordered next hypotheses (same-tool
rocproftrace; where the host CPU timegoes). Reading it there instead of duplicating it here so this description
and the spec cannot drift apart.
What this PR still delivers, unaffected: the seam has a second
implementation, mirrored call-for-call, mutation-tested, and behaviour-neutral
across four models and a 6.7x parameter range (Qwen3-0.6B/1.7B/4B dense and
Qwen3.5-0.8B GDN hybrid, capture ON vs OFF byte-identical). D1 and D6 are
documented. What died is the reason it was built, not the code. If the project
would rather not carry an unused capability, that is a fair call to make on
this PR — say so and I will close it.
Honest gaps
(
grep -i 'hip\|rocm' .github/workflows/ci.ymlmatches only comment prose),so nothing here is machine-verified. The four ROCm (AMD GPU) backend #41 boards have not run it.
flake.nixno longer carries the rocwmma fix. It was here becauseadding
rocwmmato the shell is what made W1's gates runnable in the firstplace, but it was unrelated scope — now split into
#638 against ROCm: main does not build for gfx1200/gfx1201 — rocm_paged_attn.hip includes rocwmma unconditionally on arch, not availability #444. That fix
is still only the narrow half of ROCm: main does not build for gfx1200/gfx1201 — rocm_paged_attn.hip includes rocwmma unconditionally on arch, not availability #444: it fixes one shell, while every
other rocWMMA-free gfx12 environment still fails the same way.
GEMM fails on
hipMalloc,hipFreeandhipblasCreate— two lazyinitialisations, not one. All fail loudly; none corrupts. A pre-warm of the
identical GEMM clears both. Recorded in the spec, with the consequence for W2:
the pre-warm must reach handle creation, not just workspace growth.
Asserting that the cold path throws would forbid a future capture-safe
allocator.
DestroyGraph/EndCaptureCheck()the destroywhere CUDA's leg silently ignores it, so a teardown that destroys an exec
still in flight would throw on HIP and succeed on CUDA. Not reachable in W1 —
the model path is not engaged and the tests synchronise first — and recorded
in the spec as a W2 constraint.
support_static_graph_mode(), the capture path is reachable only from tests.