feat(rocm): wvSplitK skinny GEMM for decode M<=4 — the #487 decode GEMM lever - #506
feat(rocm): wvSplitK skinny GEMM for decode M<=4 — the #487 decode GEMM lever#506VikashLoomba wants to merge 2 commits into
Conversation
|
Reviewed as part of a sweep over the open external PRs. The port itself is careful — the upstream anchor is cited correctly and I verified Flagging up front: this is the same shape as findings on #523 and #509, so it is worth treating as one lesson rather than three separate reviews. In each case the calculation was ported faithfully and the guards vLLM performs before choosing that calculation were not. 1. Out-of-bounds device write on an odd output dimension. Upstream launches The store at if (threadIdx.x == (kThrds - 1)) {
for (int n = 0; n < N; n++)
for (int y = 0; y < kYtile; y++) C[m + y + n * M] = __float2bfloat16(sum[n][y]);
}
2. No arch guard, and wave32 is hardcoded while the path defaults ON. Upstream guards twice — The port has neither guard and takes only the wave32 arm, while 3. The 4. The upstream test exists and was not ported — and it contains exactly these cases.
Two smaller things while you are in there. The shipped test bounds aggregate NMSE where upstream bounds elementwise ( On the merge conflict: only We have no AMD hardware here, so none of your measurements could be reproduced and I am not disputing them — findings 1–3 are read from the source against the donor at the pin, and are independent of any run. Happy to look again once the guards are back. |
…udler#506 review rework Review sweep findings (localai-bot, 2026-08-13), all accepted and verified against the donor at pin 55596792: 1. OOB device write on odd output dim: the donor launches wvSplitK_hf_sml_ only under (Kbp*N <= max_lds) && (M_in % YTILE == 0) (skinny_gemms.cu:1217) and the port dropped the %YTILE half -- the unguarded y=1 store lands past the output on odd N. Restored as N % 2 == 0 (our YTILE=2). 2. No arch guard: the port carries only the wave32 reduction arm (__shfl_xor(x,16)); the donor branches to ROW_BCAST15/31 on gfx9 (skinny_gemms.cu:489-496) and double-guards dispatch (on_gfx9()/on_gfx1x() + compile-time ifdef). The gfx9 arm is NOT ported, so dispatch now refuses non-wave32 arches via CapabilityFromGcnArch( DeviceArchName()) instead of compiling and silently mis-reducing. 3. The m > 8 lower bound (utils.py:181, the feature-dim bound) restored as N > 8 -- at N==1 the first wave already wrote OOB. 4. The upstream test is now ported for real: the applicable NKM_FACTORS_WVSPLITK list (tokens 1-4 = our template arms), xavier on/off, and the ELEMENTWISE tolerance (atol = eps_bf16*sqrt(K), rtol = 1e-2 -- torch assert_close semantics) replacing the aggregate NMSE that ~10 wrong elements would have passed. Added boundary shapes: features<=8 and odd features must route BLAS and stay correct; K%8!=0 declines; K%512!=0 exercises the K-tail the old test never reached. Outputs write into a 0xDEAD-sentinel guard band so any residual OOB store fails outright. Mutation-proven: with the N%2 guard removed the sentinel band is corrupted and the case fails; restored, green. Deferred with reason (recorded in .agents/specs/rocm-skinny-gemm.md): fp16 (port is bf16-only), bias (the vt::MatmulBT seam has no bias operand), padded strides (the dispatch precondition is contiguous rows), the fp8/rc variants. The gfx9 wave64 arm is owed future work, guarded out loudly for now. Allowlist: main's re-sorted file taken wholesale + VT_ROCM_SKINNY inserted once in sorted position (per the review's merge note). Gates (gfx1100, flock): test_backend_cross_device 20/20 incl. the ported sweep; the 35B Q4_K_M decode e2e was re-measured for mudler#523's stack, unchanged by this guard-only dispatch change (the skinny path fires identically at the production shapes). FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: pi:kimi-k3 [pi]
…udler#506 review rework Review sweep findings (localai-bot, 2026-08-13), all accepted and verified against the donor at pin 55596792: 1. OOB device write on odd output dim: the donor launches wvSplitK_hf_sml_ only under (Kbp*N <= max_lds) && (M_in % YTILE == 0) (skinny_gemms.cu:1217) and the port dropped the %YTILE half -- the unguarded y=1 store lands past the output on odd N. Restored as N % 2 == 0 (our YTILE=2). 2. No arch guard: the port carries only the wave32 reduction arm (__shfl_xor(x,16)); the donor branches to ROW_BCAST15/31 on gfx9 (skinny_gemms.cu:489-496) and double-guards dispatch (on_gfx9()/on_gfx1x() + compile-time ifdef). The gfx9 arm is NOT ported, so dispatch now refuses non-wave32 arches via CapabilityFromGcnArch( DeviceArchName()) instead of compiling and silently mis-reducing. 3. The m > 8 lower bound (utils.py:181, the feature-dim bound) restored as N > 8 -- at N==1 the first wave already wrote OOB. 4. The upstream test is now ported for real: the applicable NKM_FACTORS_WVSPLITK list (tokens 1-4 = our template arms), xavier on/off, and the ELEMENTWISE tolerance (atol = eps_bf16*sqrt(K), rtol = 1e-2 -- torch assert_close semantics) replacing the aggregate NMSE that ~10 wrong elements would have passed. Added boundary shapes: features<=8 and odd features must route BLAS and stay correct; K%8!=0 declines; K%512!=0 exercises the K-tail the old test never reached. Outputs write into a 0xDEAD-sentinel guard band so any residual OOB store fails outright. Mutation-proven: with the N%2 guard removed the sentinel band is corrupted and the case fails; restored, green. Deferred with reason (recorded in .agents/specs/rocm-skinny-gemm.md): fp16 (port is bf16-only), bias (the vt::MatmulBT seam has no bias operand), padded strides (the dispatch precondition is contiguous rows), the fp8/rc variants. The gfx9 wave64 arm is owed future work, guarded out loudly for now. Allowlist: main's re-sorted file taken wholesale + VT_ROCM_SKINNY inserted once in sorted position (per the review's merge note). Gates (gfx1100, flock): test_backend_cross_device 20/20 incl. the ported sweep; the 35B Q4_K_M decode e2e was re-measured for mudler#523's stack, unchanged by this guard-only dispatch change (the skinny path fires identically at the production shapes). FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: pi:kimi-k3 [pi]
4e11213 to
96764fe
Compare
|
All four findings accepted and reworked (commit 96764fe, rebased onto current main):
Mutation-proven: with the Allowlist: main's re-sorted file taken wholesale, The one place I checked rather than accepted: fp16/bias/padded-stride arms are deferred with the reasons recorded in |
…udler#506 review rework Review sweep findings (localai-bot, 2026-08-13), all accepted and verified against the donor at pin 55596792: 1. OOB device write on odd output dim: the donor launches wvSplitK_hf_sml_ only under (Kbp*N <= max_lds) && (M_in % YTILE == 0) (skinny_gemms.cu:1217) and the port dropped the %YTILE half -- the unguarded y=1 store lands past the output on odd N. Restored as N % 2 == 0 (our YTILE=2). 2. No arch guard: the port carries only the wave32 reduction arm (__shfl_xor(x,16)); the donor branches to ROW_BCAST15/31 on gfx9 (skinny_gemms.cu:489-496) and double-guards dispatch (on_gfx9()/on_gfx1x() + compile-time ifdef). The gfx9 arm is NOT ported, so dispatch now refuses non-wave32 arches via CapabilityFromGcnArch( DeviceArchName()) instead of compiling and silently mis-reducing. 3. The m > 8 lower bound (utils.py:181, the feature-dim bound) restored as N > 8 -- at N==1 the first wave already wrote OOB. 4. The upstream test is now ported for real: the applicable NKM_FACTORS_WVSPLITK list (tokens 1-4 = our template arms), xavier on/off, and the ELEMENTWISE tolerance (atol = eps_bf16*sqrt(K), rtol = 1e-2 -- torch assert_close semantics) replacing the aggregate NMSE that ~10 wrong elements would have passed. Added boundary shapes: features<=8 and odd features must route BLAS and stay correct; K%8!=0 declines; K%512!=0 exercises the K-tail the old test never reached. Outputs write into a 0xDEAD-sentinel guard band so any residual OOB store fails outright. Mutation-proven: with the N%2 guard removed the sentinel band is corrupted and the case fails; restored, green. Deferred with reason (recorded in .agents/specs/rocm-skinny-gemm.md): fp16 (port is bf16-only), bias (the vt::MatmulBT seam has no bias operand), padded strides (the dispatch precondition is contiguous rows), the fp8/rc variants. The gfx9 wave64 arm is owed future work, guarded out loudly for now. Allowlist: main's re-sorted file taken wholesale + VT_ROCM_SKINNY inserted once in sorted position (per the review's merge note). Gates (gfx1100, flock): test_backend_cross_device 20/20 incl. the ported sweep; the 35B Q4_K_M decode e2e was re-measured for mudler#523's stack, unchanged by this guard-only dispatch change (the skinny path fires identically at the production shapes). FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: pi:kimi-k3 [pi]
96764fe to
5d8bba9
Compare
…routing (mudler#487) Ports vLLM's wvSplitK_hf_sml_ (csrc/rocm/skinny_gemms.cu:351-573) — the split-K, LDS-staged, CU-count-aware skinny GEMM that wins M<=4 shapes — and routes decode-skinny MatmulBT (M 1..4, bf16, K%8==0, activation fits the 64KB LDS stage) to it instead of the 128x128-macro-tile rocBLAS GEMM. gfx1100 (GFX1X/wave32), bf16. VT_ROCM_SKINNY=0 restores the BLAS path for A/B. Measured on 4x RX 7900 XTX (gfx1100), ROCm 7.14, Release: - kernel-level vs the current rocBLAS tile path, same buffers back-to-back: qkv(5120x1024) 2.52x, o_proj(1024x2048) 3.47x, mlp_gateup(3072x1024) 1.78x, lm_head(151936x1024) 3.64x (the issue's worst single case) - in-engine decode, Qwen3-0.6B 128-token steady state: 88.1 vs 70.4 tok/s (+25%) with VT_ROCM_SKINNY on vs off; 0.8B GDN model output unchanged - cross-device: new decode-skinny MatmulBT case green (8/8, NMSE vs CPU) FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: pi:kimi-k3 [pi]
…udler#506 review rework Review sweep findings (localai-bot, 2026-08-13), all accepted and verified against the donor at pin 55596792: 1. OOB device write on odd output dim: the donor launches wvSplitK_hf_sml_ only under (Kbp*N <= max_lds) && (M_in % YTILE == 0) (skinny_gemms.cu:1217) and the port dropped the %YTILE half -- the unguarded y=1 store lands past the output on odd N. Restored as N % 2 == 0 (our YTILE=2). 2. No arch guard: the port carries only the wave32 reduction arm (__shfl_xor(x,16)); the donor branches to ROW_BCAST15/31 on gfx9 (skinny_gemms.cu:489-496) and double-guards dispatch (on_gfx9()/on_gfx1x() + compile-time ifdef). The gfx9 arm is NOT ported, so dispatch now refuses non-wave32 arches via CapabilityFromGcnArch( DeviceArchName()) instead of compiling and silently mis-reducing. 3. The m > 8 lower bound (utils.py:181, the feature-dim bound) restored as N > 8 -- at N==1 the first wave already wrote OOB. 4. The upstream test is now ported for real: the applicable NKM_FACTORS_WVSPLITK list (tokens 1-4 = our template arms), xavier on/off, and the ELEMENTWISE tolerance (atol = eps_bf16*sqrt(K), rtol = 1e-2 -- torch assert_close semantics) replacing the aggregate NMSE that ~10 wrong elements would have passed. Added boundary shapes: features<=8 and odd features must route BLAS and stay correct; K%8!=0 declines; K%512!=0 exercises the K-tail the old test never reached. Outputs write into a 0xDEAD-sentinel guard band so any residual OOB store fails outright. Mutation-proven: with the N%2 guard removed the sentinel band is corrupted and the case fails; restored, green. Deferred with reason (recorded in .agents/specs/rocm-skinny-gemm.md): fp16 (port is bf16-only), bias (the vt::MatmulBT seam has no bias operand), padded strides (the dispatch precondition is contiguous rows), the fp8/rc variants. The gfx9 wave64 arm is owed future work, guarded out loudly for now. Allowlist: main's re-sorted file taken wholesale + VT_ROCM_SKINNY inserted once in sorted position (per the review's merge note). Gates (gfx1100, flock): test_backend_cross_device 20/20 incl. the ported sweep; the 35B Q4_K_M decode e2e was re-measured for mudler#523's stack, unchanged by this guard-only dispatch change (the skinny path fires identically at the production shapes). FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: pi:kimi-k3 [pi]
5d8bba9 to
1756daf
Compare
… validates an outside contributor's trailers (#773) (#782) Closes #773. `check-pr-size.py` and `check-commit-trailers.py` both required the base revision to be an ANCESTOR of head. CI passes `github.event.pull_request.base.sha`, the TIP of the base branch, which stops being an ancestor the moment main advances after the branch was cut -- continuously, on this repo. Measured on three open PRs before changing anything. Base is not an ancestor in any of them, and a merge base exists in all three: #506 ancestor=NO merge-base=e1087a881 #523 ancestor=NO merge-base=fdd452637 #559 ancestor=NO merge-base=fafa16f0f The consequence was not a noisy red check. Both checkers aborted BEFORE examining anything, so CI had never validated commit trailers on an external contribution: the gate enforcing FOLLOWING_AGENTS_PROTOCOL and Assisted-by: exited before reading a single commit. Across the external PRs reviewed this week, hand-checking by a reviewer was the only verification those trailers received. pr-size aborted identically, so path classification and the checker-evidence contract went unenforced on forks too. THE FIX. Diff from the merge base, which is what a pull request IS: `git diff A...B` is defined as `git diff $(git merge-base A B) B` and is what GitHub shows. Two-dot diffing against a moved main is not merely stricter, it is WRONG -- main's own commits render as reversions inside the contributor's diff, so paths they never touched get classified and charged to them. The new pr-size test asserts both halves: the PR's file present, main's absent. `executable_evidence` gets the same treatment, since the BASE version of a checker for the red-before half is the one at the merge base. WHAT DELIBERATELY DOES NOT MOVE. The old rule conflated two situations: ordinary divergence (merge base exists) now examines merge_base..head; unrelated histories (no merge base) STILL RAISES. Absence of information must never look like absence of work -- the script's own require_origin_main() docstring already states that principle for the other input. test_missing_and_nonancestor_objects_fail_closed uses an ORPHAN branch, so it still raises; only its regex changed, because the message now names what is actually wrong. Its assertRaises(ValueError) is untouched. The trailers case was SPLIT, not deleted: its divergent-branch half built two branches off a common root -- which share a merge base and are the ordinary shape of every PR -- so that half now asserts it validates, with the genuinely-unrelated case asserted separately. Nothing that used to fail closed stopped failing closed. Range changed, contract unchanged: test_a_bad_trailer_in_the_merge_base_range_is_still_reported puts a trailerless commit inside the new range and requires it still be reported. RED before on the unmodified checkers, GREEN after (74 passed, 148 subtests). Stop conditions checked individually rather than inferred. Full tests/scripts: 9 failed / 1359 passed, all nine pre-existing and reproduced on main. CI: agent-record and pr-size both SUCCESS on this PR -- the two checkers it repairs passing on a live PR. Remaining red is baseline only: windows-msvc-* are the PR-only arm (#584), and sanitize-cpu is red on main itself for #775 (test_nemotron_h_scaffold, nemotron_h_registry.cpp:112 downcasting a doctest StubModel to NemotronHLoadedModel). This PR touches no C++.
|
Hey @VikashLoomba — coordinating from the ROCm/gfx1201 (RDNA4, wave32) side so we help here rather than collide. Heads-up on a real conflict. We have an approved fix in flight (#837) that replaces the single-TLS The logic is complementary, though: your skinny-GEMM eligible shapes (M=1..4, N>8/even, K%8, LDS-bound, wave32) return before The bigger offer: your dispatch admits capability major 11 or 12, but the evidence here is gfx1100-only and the notes say the gfx1200 path wasn't ported. We run dual Radeon AI PRO R9700 (gfx1201) and can provide the missing RDNA4 validation — positive candidate witness/dispatch breadcrumb, M={1,4}, real decode N/K, odd-N and N≤8 fallback-guard, K-tail, candidate-vs-BLAS correctness, and a forward/reverse perf A/B. That either earns gfx1201 coverage or tells us to narrow the arch gate to gfx11. Want us to run it once there's an integration head that preserves the dual-slot handle? |
Row
BACKEND-ROCM— M5-adjacent decode perf, the RDNA3 skinny-GEMM path. Issue #487 (decode M=1 GEMMs on 128-tile rocBLAS), coordinating with @joral (gfx1200 generic line). This board is gfx1100 (RDNA3) — the only arch that can test the RDNA3-specific config.What changed
Ports vLLM's
wvSplitK_hf_sml_(csrc/rocm/skinny_gemms.cu, de-torched) into NEWsrc/vt/rocm/rocm_skinny_gemm.hipand routes decode-skinnyMatmulBT(M in 1..4, bf16, K%8==0, activation fits the 64KB LDS stage) to it inrocm_matmul_hipblaslt.hip, ahead of the default-off naive GEMV and the rocBLAS tile path.VT_ROCM_SKINNY=0rolls back to BLAS for A/B. New cross-device case (decode-skinny MatmulBT, bf16, M∈{1,4}) gates it vs the CPU oracle.Evidence (4× RX 7900 XTX gfx1100, ROCm 7.14, Release)
Kernel-level A/B vs the current rocBLAS path (same buffers, back-to-back, many iters):
In-engine decode (Qwen3-0.6B, 128-token steady state): 88.1 vs 70.4 tok/s (+25%) with VT_ROCM_SKINNY on vs off. Qwen3.5-0.8B GDN model output unchanged.
Correctness: ported kernel standalone-validated vs CPU reference across real decode shapes (NMSE ~3e-6, zero bad outputs); new in-tree cross-device case 8/8 green; full ctest zero-delta vs base (same 7 pre-existing host/lane failures).
Speed claims
$GPU_LOCK; recorded here with the repro recipe. (Kernel bench + in-engine A/B, same binary.)Honest gaps