Repository navigation
fix(rocm): LLMM1 wave32 reduction + refuse wvSplitK on gfx10 - #54
Merged
Merged
Conversation
LLGemm1_kernel launches K/8 threads, so for K <= 8192 a wave32 block has up to 32 waves, but the second-stage reduction reads only red_smem[..][0..15]. For K > 4096 on gfx10/gfx11/gfx12 the partials of waves 16..31 were dropped and the result silently lost part of the dot product. Fold them into the first 16 lanes before the shuffle. wave64 (gfx9) has at most 16 waves in this range and is unchanged (compile-time WARP_SIZE check). Ported from FaisalBiyari/vLLM-ALU-RDNA2@a8f8c02 (csrc/rocm/skinny_gemms.cu).
gfx10 (RDNA2) is neither __HIP__GFX9__ nor __HIP__GFX1X__, so the wvSplitK kernels are the assert(false) stub there and any call faults the queue (HSA_STATUS_ERROR_EXCEPTION). The Python dispatcher already routes gfx10 decode to gemv_f16_rdna2; make direct callers fail with a clear error. Tests: skip the wvSplitK tests on gfx10, and add an LLMM1 case with K=4104 (17 waves on wave32) that exercises the second-stage partials 16..31. Adapted from the gfx1030 wvSplitK dispatcher guard in FaisalBiyari/vLLM-ALU-RDNA2@a8f8c02 (csrc/rocm/skinny_gemms.cu); this tree does not build the wave32 path for gfx10, so the guard is unconditional. Co-authored-by: FaisalBiyari <11506542+FaisalBiyari@users.noreply.github.com>
Refresh the branch onto the current rdna_extras tip (1136bb9) before re-evaluating the LLMM1 wave32 fix and the wvSplitK gfx10 refusal on gfx1030. Signed-off-by: BlivionIaG <kev29lt@gmail.com>
…n fix K=4104 (17 waves on wave32) reaches the second-stage partials 16..31, but partial 16 then holds only 8 of the 4104 products (0.2%), below the test's rtol of 1e-2, so that case also passes on the unfixed kernel. K=4352 fills the whole 17th wave (5.9% of the sum). Measured on a V620 (gfx1030, wave32): with the unfixed _rocm_C the K=4352 cases fail (8 of 8 rows_per_block x dtype) and K=4104 passes; with the fix all 80 LLMM1 cases pass. Signed-off-by: BlivionIaG <kev29lt@gmail.com>
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.
Ports the LLMM1 wave32 fix from FaisalBiyari/vLLM-ALU-RDNA2@a8f8c02 (
csrc/rocm/skinny_gemms.cu, author FaisalBiyari).What changed
LLGemm1_kernel: K/8 threads means up to 32 waves for K <= 8192 on wave32, but the second-stage reduction only read partials 0..15. For K > 4096 on gfx10/11/12 the dot product silently lost waves 16..31. The fix folds them into the first 16 lanes. wave64 (gfx9) is unchanged (compile-timeWARP_SIZE == 32). Commit authored by Faisal, verbatim.gemv_f16_rdna2comment, AGENTS.md). gfx10 decode stays ongemv_f16_rdna2; the user-visible effect is on gfx11/12 (LLMM1is reached for n=1, m%4==0, K<=8192).wvSplitK:TORCH_CHECK(!on_gfx10()). gfx10 is neither__HIP__GFX9__nor__HIP__GFX1X__, so the kernels are theassert(false)stub and a call faults the queue. With this change, direct callers get an error instead. The Python dispatcher already never routes gfx10 here. Adapted from Faisal's gfx1030 guard (Co-authored-by).(1, 4104, 1024), 17 waves on wave32, the smallest K that reaches the fixed partials.Tests run (no GPU)
ruff check/ruff format --checkon the test file: passclang-format21.1.2--dry-run --Werroronskinny_gemms.cu: passGPU validation to run
_rocm_C(gfx1030; gfx1100 too if available).pytest tests/kernels/quantization/test_rocm_skinny_gemms.py -k "llmm1 or wvsplitk"on gfx1030: before the fix, LLMM1 should fail at K in {4104, 6144, 8192}; after it, LLMM1 should pass and wvSplitK should be skipped. On gfx1100: LLMM1 should pass and wvSplitK should be unchanged.gemv_f16_rdna2. To confirm: Flash-Next W4A16 TP=4 F&P, 1k/512 and 16k/1k at c=1/c=8, MTP0, with prefill tok/s, decode tok/s/req, TTFT and ITL within noise ofrdna_extras.Re-evaluation 2026-11-11
Verdict: ready to merge. The fix is correct, cannot change wave64 code, and was exercised on a real wave32 GPU (V620, gfx1030) and by compiling
skinny_gemms.cufor six architectures. One test commit was added (below). Not merged; waiting for the merge decision.Branch state
rdna_extras1136bb952b8efea0ee37=a635342ab0+ merge ofrdna_extras(17527859a3, clean: upstream only renamed two include paths inskinny_gemms.cu) + one test commitcsrc/rocm/skinny_gemms.cu,tests/kernels/quantization/test_rocm_skinny_gemms.py)4d6b653133FaisalBiyari.a635342ab0and the two new commits are BlivionIaG (Co-authored-byFaisal ona635342ab0). No agent-authored commit, so no re-authoring is needed.New commit
8efea0ee37(test only). The PR's(1, 4104, 1024)case does not fail on the unfixed kernel: at K=4104 wave 16 holds 8 of the 4104 products (0.2 %), below the test'srtol=1e-2. It stays (it covers the 17-wave path) and(1, 4352, 1024)is added: the 17th wave is full (5.9 % of the sum), so the case fails without the fix. The existing(1, 6144, 1024)and(1, 8192, 2048)cases also fail without the fix on any wave32 device.The bug is still on
rdna_extrasLLGemm1_kernellaunchesNUM_THREADS = max(rows_per_block * 16, ceil32(K / 8))threads: 544 threads (17 waves) at K=4104 up to 1024 (32 waves) at K=8192 on wave32. The second stage readsred_smem[row][threadid % 16], so partials 16..31 are dropped. wave64 has at most 16 waves for K <= 8192 and is not affected. The same expression is on upstreamvllm-project/vllmmain (csrc/rocm/skinny_gemms.cuh,acc[qwarpid] = qthreadid < num_warps ? ...), so the fix could also go upstream. Upstream has since splitskinny_gemms.cuper kernel family (vllm-project#62718), so this hunk has to be re-applied when the fork next rebases onto upstream.Who reaches LLMM1 / wvSplitK
Dispatcher:
vllm/model_executor/layers/utils.pyrocm_unquantized_gemm_impl. The only other callers are the_custom_ops.pywrappers and tests; nothing intools/orbenchmarks/calls them.LLMM1via dispatcherwvSplitKvia dispatcherWARP_SIZE == 32is false at compile time, device code unchanged (table below)wvSplitKunchanged (on_gfx10()is false)wvSplitKcalls raise instead of hitting theassert(false)stubThe description above says m%4==0 for LLMM1; the dispatcher also requires
not use_wvsplitk, which leaves m in {4, 8} for n=1. On gfx10 the dispatcher routes fp16 n <= 8 togemv_f16_rdna2, n 9..32 tordna_gemm_smallm, everything else to torch/rocBLAS. gfx1250 does not build this file.Evidence on a V620 (gfx1030, wave32), HIP 0, ROCm 7.14, torch 2.12.0+rocm7.14.0
_rocm_Cbuilt for gfx1030 from the exact trees (rdna_extras1136bb952band this branch's merge17527859a3), same tree path and build dir so code objects are comparable. Kernel code is identical in8efea0ee37(test-only commit on top).rdna_extras_rocm_Cpytest test_rocm_skinny_gemms.py -k llmm1(80 cases)test_rocm_skinny_gemms.pytests/model_executor/layers/test_rocm_unquantized_gemm.pyrdna_extras: they assume a gfx9/gfx11 dispatcher and expectwvSplitK/LLMM1to be called, which gfx10 never does)rocm_unquantized_gemm_implcalls (fp16/bf16, n in {1..6, 8, 9, 16, 24, 32, 64, 128}, m in {1, 4, 8, 16, 128, 4096}, k in {64, 2048, 4096, 4104, 6144, 8192}, contiguous / row-stride-padded, with and without bias)gemv_f16_rdna2, 3156 torch;LLMM1/wvSplitK/wvSplitKrcreached 0 timesops.wvSplitK(...)RuntimeError: wvSplitK is not built for gfx10 (RDNA2); use gemv_f16_rdna2; queue still alive: anLLMM1call right after matches the reference (rel. error 4.5e-4)_rocm_CopsBinary:
_rocm_C.abi3.so(gfx1030 only) 91,695,992 -> 91,699,064 bytes (+3,072).Device code, six architectures (compile only; no RDNA3/4 or CDNA hardware on the box)
skinny_gemms.hip.oofrdna_extrasvs this PR, one build tree and build dir withgfx90a;gfx942;gfx950;gfx1030;gfx1100;gfx1200. Each arch's code object is disassembled and compared per function. Two layout artefacts are tolerated and nothing else: PC-relative literals afters_getpc_b64, and the__LINE__operand ofassert(false)stubs /__assert_fail, which has to have moved by +18 or +24 lines (what this PR inserts above them). Opcodes, registers, other immediates, branch offsets and instruction counts must match exactly.__LINE____assert_fail).text,.rodata,.note)LLGemm1_kernelLLGemm1_kernel__assert_fail)LLGemm1_kernelCDNA (wave64) device code is unchanged, and on every wave32 arch only the eight
LLGemm1_kernelinstantiations (4 rows_per_block x fp16/bf16) differ: 4900 -> 5549 instructions on gfx1030, 4981 -> 5641 on gfx1100, 5286 -> 6060 on gfx1200 (+13 to +15 % over the eight kernels). On gfx1030, a whole-library fingerprint (64 objects) shows 63 identical and onlyskinny_gemms.hip.ochanged.Platform risk
num_warps <= 16and the added branch is never taken. The source change is confined to the second-stage reduction tail, which runs once per block. Back-to-back LLMM1 launches on gfx1030 (m in {4, 8}, K 1024..8192, fp16/bf16,rows_per_block=4as the dispatcher uses) are launch-bound at 8.3-8.5 us per call and identical within +-1.6 % (mean +0.05 %). No gfx1100/gfx1200 GPU was available, so the RDNA3/4 part rests on the gfx1030 run (same wave32 path,WARP_SIZE == 32) and the compile diff above; kernel time on RDNA3/4 was not measured. The gfx1100 item under "GPU validation to run" in the description above is still open.wvSplitKrefuses.on_gfx10()cachesgetCurrentDeviceProperties()in a function-local static like the neighbouringon_gfx1x()helpers, so a mixed-architecture process whose first call is on a gfx10 device would cachetrue. Nothing in this fork callswvSplitKon gfx10, so this is theoretical.wvSplitKonly;wvSplitKrcandwvSplitKQstay Python-gated.Notes, not blocking
docs/rdna/agent-rules.md(L33-34, "assert on the device") and the comment atvllm/model_executor/layers/utils.py:361-362("numerically wrong on gfx1030") describe LLMM1 on gfx10 as asserting / wrong. After this PR LLMM1 is numerically right there; it is still not routed, so the rule "never route to LLMM1 on gfx10" is unchanged. I left the wording alone.