build(BACKEND-ROCM): route -Wall -Wextra -Werror to the HIP compile language and clear the 19 diagnostics it raises - #2846
Merged
Merged
Conversation
…before the repair #2713 asks for the number first, because a large one makes the flag a build break rather than a detector. Two arms on strix:gpu0 give 3 diagnostics under clang defaults and 19 under -Wall -Wextra, all 19 in project code and none in a ROCm header. The spec records the arms, the per-class disposition, and the evidence that the ten unused-function reports are real dead code rather than a HIP host-pass artefact. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5-1m [claude-code]
…P compile language enable_language(HIP) makes .hip a distinct COMPILE_LANGUAGE, and the 23 HIP sources land on the same `vllm` target the CXX genex covers. The function had no HIP branch, so those translation units were the only project code whose diagnostics could not fail a build. This commit adds the flag and nothing else, so the 19 diagnostics it raises are a red result that a reader can reproduce at this exact commit. The next commit clears them. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5-1m [claude-code]
…r arm raises The previous commit armed -Wall -Wextra -Werror on the HIP compile language. This commit clears what it found, so the ROCm backend builds green under the flag that now guards it. Sixteen of the nineteen are dead code and are deleted: the __half Ld/St overload pair in each of the three GDN files, which no kernel instantiates while the float and __hip_bfloat16 overloads beside them do; DotFp8Row, LoadRow8Bf16, StoreRow8Bf16 and one GridFor, each mentioned in its file exactly once at its own definition; the kBlock and kDecEpl constants in the paged-attention unit; and the `tokens` parameter that RopeFromCacheK never reads, because the kernel derives its own token index from the flat index. The other three change no computation. graph_dedup_runtime.h now casts to void the three hipError_t results the runtime discards on purpose, which is the exact class #2713 was filed about. The decode GQA4 predicate gains the parentheses C++ already applied, since && binds tighter than ||. The skinny-GEMM warp guard casts its int bound to unsigned, matching the two casts below it; mindiv returns a positive count by construction. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5-1m [claude-code]
…e control The spec stated the gate; this records what it returned. RED fails on 15 errors, GREEN builds all 23 objects clean, and the mutation the issue names fails on this branch while the identical mutation on the base tree still only warns. The control arm is the part worth keeping: without it, a red MUT proves only that the build broke, not that the new flag is what broke it. RED reports 15 rather than 19 because the four end-of-translation-unit diagnostics live in the one unit that hard-fails earlier, and clang stops before emitting them. The note records that, and records that the job's class-tally line printed nothing because its regex does not match the -Werror spelling. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5-1m [claude-code]
… branch main moved 900-odd commits since this branch was cut, and the HIP warning arm has to hold against what landed there rather than against the tree it was measured on. The merge message is authored because a --no-edit merge lands trailer-less and reds the trailer walk. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5-1m [claude-code]
mudler
added a commit
that referenced
this pull request
Sep 3, 2026
… on HIP #2846 landed, so main carries the HIP -Wall -Wextra -Werror genex. This branch rewrites LtWorkspace, and that rewrite has never been compiled under the flag. Merging first turns a pairwise risk no gate here can see (#2848) into a sequential one the ordinary ROCm build covers. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5-1m [claude-code]
5 tasks
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.
Row
BACKEND-ROCM— one row per PR. Spec:.agents/specs/rocm-hip-werror.md.Before starting
gh pr listshows no open pull request touchingcmake/CompilerWarnings.cmakeor the HIP warning arm. The five open ROCm pull requests (perf(GFX1100-TG200): T2b flips ROCm support_static_graph_mode #2777, perf(ROCm): parallel random sample with shared primitives #2778, Port async device-mirror combine/scatter kernels to ROCm #2779, feat(KERNEL-QUANT-CIQ-GEMM-ROCM): land the W1 keep-quant providers on kROCM #2782, docs(BACKEND-ROCM): reconcile merged Q8_K records #2625) are kernel and record work and touch none of these lines.6f2ec89f5), implementation after.BACKEND-ROCM, alreadyACTIVE. No lifecycle change, so nothing is owed to a public document.cmake/CompilerWarnings.cmake:54-66(the non-MSVC branch, no HIP arm),CMakeLists.txt:404enable_language(HIP),:1484vllm_cpp_set_warnings(vllm),:1711-1734the 23.hipsources on that same target,src/vt/rocm/rocm_backend.hip:390-404(the comment that recorded this finding and never got an issue).What changed
vllm_cpp_set_warningsgains a$<$<COMPILE_LANGUAGE:HIP>:-Wall -Wextra ${_vllm_cpp_werror}>arm beside theCXX,OBJCXXandCUDAarms it already had, and the 19 diagnostics that arm raises are cleared at their source. Sixteen are dead code and are deleted; the other three are a(void)on threehipError_tresults the runtime discards on purpose, the parentheses C++ already applies to one decode predicate, and one explicit unsigned cast. No kernel computes anything different.${_vllm_cpp_werror}rather than the literal-WerrortheOBJCXXline uses, so a HIP build under a sanitizer lane keeps the diagnostics visible and non-fatal for the same reason theCXXarm does.The count, which the issue asks for first
Measured on
strix:gpu0(gfx1151, ROCm/HIP 7.2.53211),rcjob60f7861d, two arms, each a fresh configure into a fresh build directory, each compiling all 23 HIP objects:file:line:col:flag-Wall -Wextra, no-WerrorZero of the 19 are in a ROCm or system header. Asked of the build system rather than the cmake source: on arm A a HIP compile line carries
-Wall0 times and-Werror0 times; on arm B,-Wallonce and-Werrorstill 0 times. By class:-Wunused-function10,-Wunused-value3,-Wunused-const-variable2,-Wlogical-op-parentheses2,-Wunused-parameter1,-Wsign-compare1.19 is small, so this pull request adds the flag and clears them rather than reporting the size and stopping.
The ten unused-function reports are real dead code, not a HIP host-pass artefact
__device__functions are not emitted in the host pass, so "clang reports a used__device__helper as unused" is the obvious hypothesis. The line numbers refute it. Each ofrocm_gdn_conv.hip,rocm_gdn_fused.hipandrocm_gdn_postconv.hipdeclares threeLdand threeStoverloads on consecutive lines —float,__half,__hip_bfloat16— and in all three files exactly oneLdline and oneStline is reported, and it is always the__halfone. The neighbours are silent because kernels instantiate them. A pass artefact would report all six. After deletion,grep -c __halfreturns 0 in all three files. The other four warned symbols appear in their file exactly once, at their own definition.Evidence
rcjobc74c0cdf-804f-4666-a155-00fa0f59b963onstrix:gpu0. Four arms, each a fresh configure into a fresh build directory,ninja -k 0over the same 23 HIP objects.-Werroron a HIP compile linehipError_thipError_tThe mutation is the exact construct #2713 is about, applied identically to both trees. On this branch it reads
error: ignoring return value of type 'hipError_t' declared with 'nodiscard' attribute [-Werror,-Wunused-value]. On the base tree the same line readswarning:and the build goes green. The control is the part that matters: without it a red MUT proves only that the build broke, not that the new flag broke it.RED reports 15 rather than 19 because the four missing ones —
rocm_paged_attn.hip:30,:335,:448,:451— are end-of-translation-unit diagnostics in the single unit that hard-fails earlier on-Wlogical-op-parentheses, and clang stops before emitting them. GREEN is what shows all 19 were cleared.The arms were archived from commits later reworded to repair an
Assisted-byvalue. The trees are byte-identical (da7b2417…RED,38d96fce…GREEN), verified withgit rev-parse <sha>^{tree}on both the pre- and post-reword commits, so the result binds to the commits on this branch.After merging origin/main
mainmoved under this branch, so the measurement above was re-checked rather than assumed still valid.git diff --name-only ca07f6e94 origin/mainshows main touched none of the eleven files this branch edits. The only headers it changed areinclude/vllm/multimodal/{audio_processor,dots3_note_processor}.h, andgrep -rl "vllm/multimodal" src/vt/returns nothing — novttranslation unit reaches them — so the preprocessed input to all 23 HIP units is byte-unchanged by the merge. Re-read on the merged tree: 23 HIP translation units, the HIP genex present once,enable_language(HIP)still atCMakeLists.txt:404,vllm_cpp_set_warnings(vllm)still at:1484.Two rebuilds are queued on
strix:gpu0behind other sessions' leases, and neither result is in this body:merged — this branch's GREEN arm on the merged tree, so the count is re-measured rather than argued forward.
cross — this branch merged together with
row/BACKEND-ROCM-SCRATCH-SYNC(Two ROCm scratch allocators are unsynchronised: EnsureQuantScratch locks the map not the entry, and LtWorkspace frees with stream work possibly in flight #2712, PR fix(BACKEND-ROCM): hold one lock across the scratch entry, and retire the block instead of freeing it #2847). That branch rewritesLtWorkspacein a file this branch also edits.git merge-tree --write-treereports a clean merge with no conflict, but a clean merge is textual: after both land, that new code is compiled under the-Werrorthis branch arms, which it has never been compiled under. Neither pull request's own gate covers that combination, which is why the arm exists. Both arms require-Werror-on-HIP == 1as well asBUILD_OK, so an arm that silently lost the flag reads FAIL rather than PASS.scripts/agent-preflight.shpassestests that cover this change: the build itself, on the only toolchain that can run it. There is no unit test to name, and adding a checker that asserts the genex exists would be a transcription of the line it guards.
public docs changed only when this PR changes their owned user-facing facts: none changed. This edits
cmake/,src/and one spec.Speed claims
${GPU_LOCK}.Honest gaps
## Owed; not repaired, because the derived checker that would catch it needs an exception list forCandASM, and a list is the transcription this file's own rules warn about.gfx1151and ROCm 7.2.5 only.-Wall -Wextraoutput is target-independent for these six classes, but that is reasoning, not a measurement, and no secondgfxtarget was built.mindivreturns positive by construction — not on a numeric gate.[-Wflag]and the-Werrorspelling is[-Werror,-Wflag]. The per-error list above it is complete and is what the table reports; the tally was not relied on.Closes #2713
FOLLOWING_AGENTS_PROTOCOL
Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: AGENT:claude-opus-5-1m [claude-code]