[ROCm][Bugfix][Perf] Tune multi-stream shared experts use; wvSplitKrc fixes - #56098
Conversation
The deterministic split-K readback stages one fp32 partial per (k-shard, N-tile) in LDS before summing them. That footprint is 256 * nt * k_rnd float4 and grows with the shard count, but the LDS allocation is fixed, so once it exceeds LDS the global_load_lds writes are dropped and the reduction returns a truncated K-sum -- silently, and only for the N-tiles whose staging fell off the end. On gfx950 this is reachable straight from the dispatch path (10 <= n <= 128, k > 512, m % 16 == 0): n >= 65 breaks from k = 5248 and n = 32 from k = 10368. Every guard passes -- fits_wvsplitkrc bounds the global workspace and CuNeeded bounds the grid, neither bounds LDS -- and the test grid stops at k = 3080, so nothing caught it. The measured error is 2-3x the output standard deviation, confined to exactly the output rows owned by the overflowing N-tile. Stage the readback in batches that fit. The main-loop staging areas are separated from the readback by a __syncthreads(), so a union lets the readback address all of LDS; the first batch is peeled and is the only batch for every shape that fits in one pass, which keeps the batch loop off the common path. Also correct the workspace predicate: the shard count is ceil(k * CHUNKK / 512), not ceil(k / 512), so it was 2x optimistic whenever the kernel halved the K-shard. The host wrapper now checks it rather than overrunning the workspace. Verified on gfx950 (MI355X). The new test fails 8/8 before the kernel change and passes 8/8 after; a 580-configuration sweep over n/k/m/dtype/bias reports no mismatches. Shapes that already worked are within ~1.3% of the previous kernel and remain 1.3-2x faster than hipBLASLt. Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CHUNKK=2 halves the K-shard and doubles the CUs used, but also doubles the
split-K shard count. The readback of those partials is serial in the shard
count, so the added parallelism stops paying once the per-block main loop no
longer dominates. Cap CHUNKK=2 at 11 shards.
Measured on MI355X (bf16, CUDA-graph replay to keep launch overhead from
swamping these kernels, min of 11 timings of 100 replays), sweeping 78 shapes
over N in {32,64,128}, M in {128..1024} and 4-24 shards, each built twice and
forced to each CHUNKK:
shards mean CHUNKK=2 vs CHUNKK=1 worst
4 -9.6% -14.0%
6 -7.9% -11.2%
8 -2.8% +3.4%
10 -2.3% +2.5%
12 0.0% +3.4%
16 +6.0% +10.1%
20 +8.9% +12.1%
24 +13.3% +18.2%
The crossover is not a constant: it falls as M grows, from ~16 shards at M=128
to ~4 at M=1024, since more output tiles read back at once. A flat cap is
therefore a compromise. Scoring each cap against an oracle that picks per
shape, caps 10 through 13 are indistinguishable (0.21-0.22% off oracle) and
form the optimum; cap 4 costs 1.80% and cap 24 costs 3.71%. This picks the low
end of that plateau, since overshooting the crossover costs about twice what
undershooting it does.
N_p2 == 16 is folded into the CHUNKK expression rather than corrected
afterwards; the dispatch below already hardcodes CHUNKK=1 for that case, so
this is equivalent and keeps the host rule in one place. The Python mirror
moves into wvsplitkrc_dispatch() so the dispatch rule has a single Python
definition that tests can consume.
No new shape is admitted to the kernel: CHUNKK only ever goes 2 -> 1, which
halves k_rnd and can only loosen the workspace guard.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
wvSplitKrc keeps its split-K partials and reduction counters in function-local static tensors, so one pair serves the whole process. They are recycled by protocol rather than re-zeroed -- the last workgroup to arrive resets them -- which is safe only while every caller is serialised on one stream. Two launches on different streams each satisfy the other's completion test and read the other's partials: no error, no crash, silently wrong results. vLLM's shared-experts overlap does exactly that, running the router gate and the shared expert's gate_up_proj concurrently with both admitted to the kernel. Allocate one pre-partitioned pool per device and hand each stream a slot on first use. Allocating per stream on demand would also fix it, but moves the allocation to an uncontrolled moment: the first qualifying GEMM on a new stream can land inside a cudagraph capture, where the zero-fill becomes a replayed graph node. Every kernel access is relative to the base pointers, so a slot is a pointer offset and the kernel is unchanged. This removes the reason for the ROCm multi-stream safety gate, which attributed the corruption to shared and routed experts aliasing their inputs. They do not alias -- the storage pointers differ at that call site on every launch, and a copy inserted there does not help. Quantized routed experts appeared safe because they take a different kernel, not because a copy broke an alias. gsm8k on DeepSeek-V4-Pro with the overlap enabled: 0.92 with this change against 0.001 without it, in the same batch. Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
… config The overlap was restricted to DP-only deployments on ROCm because it did not pay elsewhere. That finding predates two changes. The runtime now gives a graph's cross-stream dependency a device-resident value word, which removes most of the multi-stream penalty that made the overlap unattractive; and until the preceding commit the overlap produced wrong answers on ROCm, so any timing taken with it enabled was measuring corruption rather than the overlap. Neither reason is specific to a parallel configuration, so the gate goes and ROCm takes the same path as CUDA. The token threshold and the aux-stream availability check still apply to both. Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
ffe1c08 to
390812e
Compare
|
/ci run |
|
✅ Triggered Buildkite CI #87959 for commit |
| @@ -112,19 +107,11 @@ def _determine_shared_experts_order( | |||
| if self._mk_can_overlap_shared_experts(): | |||
| return SharedExpertsOrder.MK_INTERNAL_OVERLAPPED | |||
|
|
|||
| # On ROCm, empirically only DP-only deployments benefit from the overlap. | |||
| overlap_is_beneficial = not current_platform.is_rocm() or ( | |||
There was a problem hiding this comment.
Suggestion: Should we re-run the bench serve experiments from #52033 (or similar) and validate that it's in fact beneficial now also for non-DPA cases?
There was a problem hiding this comment.
I did a few sweeps over DSV4 and Qwen-3.5-35B at various concurrencies, TP8. And also did a sweep over DSV2-Lite with synthetic shared-expert sizes and various parallelism configs. It's beneficial now in essentially all cases I tested. Updated the PR body with some numbers.
| @@ -283,24 +283,11 @@ def __init__( | |||
| self._shared_experts: SharedExperts | None = None | |||
| if shared_experts is not None: | |||
| can_overlap = lambda: self._quant_method.mk_can_overlap_shared_experts | |||
| # When unquantized, shared expert inputs alias the hidden states, | |||
There was a problem hiding this comment.
Suggestion: Can we run gsm8k on gfx950 with the following cfg and check everything is still ok?
vllm serve Qwen/Qwen3.5-35B-A3B \
--max-model-len 4096 \
--data-parallel-size 2 \
--enable-expert-parallel \
--trust-remote-code \
--disable-uvicorn-access-log
(this will anyhow be run in nightly ci)
There was a problem hiding this comment.
Yep gsm8k checks out at 0.958-0.970 on Qwen3.5-35B, on all the parallelism configs (TP8, TP1+DP8 [with and without EP], TP2+DP4 [with and without EP] I tested. I also tested DeepSeek V4 Pro on TP8 and it comes out at 0.952-0.962.
|
/ci run |
|
❌ @mawong-amd, A reviewer with write access must run |
Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
bd9ecf9 to
1b2fad6
Compare
|
/ci retry |
|
✅ Queued 5 failed job(s) for retry in Buildkite CI #88008. |
…time Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
…/wvsplitkrc-improvements
|
Great work @mawong-amd ! A quick question do we have any kernels available from AITER directly? |
…V cache sometimes Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
|
/ci run |
|
✅ Triggered Buildkite CI #88183 for commit |
|
The CI failures are pre-existing in |
… fixes (vllm-project#56098) Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
After #55099, multi-stream performance in ROCm has markedly improved and allows new use-cases. In particular, the regime in which multi-stream shared-experts decode (implemented in #52033) brings performance advantages has shifted.
Purpose
This PR expands the use of multi-stream shared-experts decode on ROCm by loosening the gate that enables it and tuning it to where it's measured to improve performance. Furthermore, the correctness issue that led to the original implementation's (#48223) reversion has been identified as a bug in a gfx950-specific skinny GEMM (
wvSplitKrc).wvSplitKrcwas not previously multi-stream safe because it uses a statically allocated buffer which 2 or more streams can access at the same time. This has been fixed by enlarging the allocation 8x (7.5 MiB per buffer, so 60 MiB total which is still small) and giving distinct streams distinct slots into the workspace. If more distinct streams exist than can be accommodated, we now fall back to an on-demand allocation, which is safe albeit adding overhead. This also accounts for cases in whichwvSplitKrcis graph-captured on stream A but replayed on a different stream B.In addition, a latent bug in
wvSplitKrcthat causes it to truncate its readback on certain GEMM shapes (leading to silent incorrectness) is fixed.wvSplitKrc'sCHUNKKparameter is also retuned for a bit of performance.The different memory allocation patterns caused by multi-stream also revealed a latent bug in
vllm/v1/sample/ops/topk_topp_triton.pywhere the buffer cache teardown was not reliably done and so different allocation patterns (e.g. from multi-stream use) could lead to the buffer cache being allocated in the same segment as the KV cache, thus pinning them from being torn down. This has been fixed by doing the buffer cache reset during clean up.Performance numbers
Output token throughput, mean of 2 runs. gfx950, TP8/DP1, real weights, ISL 1024 / OSL 256.
Output token throughput. DeepSeek-V2-Lite (hidden 2048, 256 routed experts, top-k 8, 40 layers) with shared-expert size overridden for testing and random weights, single-GPU. Concurrency 1, mean of 2 runs.
In particular the numbers at shared-expert size = 64 are quite representative of Qwen3.5-35B at TP8.
Test Plan
2 new tests have been added for
wvSplitKrcthat exercise shapes it was previously failing on before and that exercise it in a multi-stream concurrent setting. The shared-experts multi-stream correctness (and a further test onwvSplitKrc) are exercised by theLM Eval Qwen3-5 Modelstest group which was failing earlier on the multi-stream shared experts path.Test Result
The tests pass, and there are no new test regressions.
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.