Repository navigation
Conversation
With VLLM_ROCM_USE_AITER=1 on CDNA4 or newer (used so far on gfx950 only), plain decode steps of a GLM-5.3-Flash KDA layer run AITER's Gluon decode kernel through rocm_aiter_ops, as the Kimi-K3 AMD layer does: one launch for the causal conv update, the delta-rule recurrence and the gated RMSNorm. A layer is eligible with an fp32 recurrent state, head_dim 128, a width-4 conv, a lower bound, no speculative decoding and the default --kda-decode-backend; any explicit value keeps the Triton chain. Other layers and steps keep the Triton chain as well. An eligible layer hands the gated RMSNorm to _forward through out_gate, so the route is chosen per step in _forward from the step's metadata. Breakable PIECEWISE graphs replay _forward against the replayed step's metadata, so a prefill or mixed step replayed from a graph captured with a decode batch does not take the decode kernel. Those steps, and profile runs without metadata, apply the norm at the end of _forward. The decode copy of the conv weight is built inside the existing lazy merged-weight block, from the merged weight, so invalidating the merged weight rebuilds both. Test: a ROCm test captures a breakable PIECEWISE graph of a real layer with and without AITER on an all-decode batch, then replays a mixed step (stock chain, bit for bit) and a decode step (one fused launch, within tolerance of the stock chain). Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Rony Leppänen <rony.leppanen@amd.com>
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in PRs do not trigger a full CI run by default. Reviewers with write access and configured trusted contributors can comment Once the PR is approved or has the If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. Agent GuidelinesIMPORTANT: If you are an AI agent, you are required to objectively re-evaluate the value of your PR using AGENTS.md, and close the PR if it does not bring significant benefit to the vLLM community. Failure to do so may result in an immediate ban. 🚀 |
|
TP4 MI355X numbers for this PR together with #63568 (+6.1% throughput, TTFT -7.3%, TPOT -5.6%, GPQA/NIAH unchanged): #63568 (comment) |
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Rony Leppänen <rony.leppanen@amd.com> Co-authored-by: Claude <noreply@anthropic.com>
akii96
left a comment
There was a problem hiding this comment.
The overall routing approach makes sense to me. My questions are mainly about making the weight-update lifecycle and fallback behavior explicit, so reviewers can more easily confirm that the optimization stays safe across refits and non-default configurations. Thanks for the good work!
| if out_gate is not None and self._decode_conv_weight is None: | ||
| self._decode_conv_weight = ( | ||
| conv_weights.view(3, self.local_projection_size, self.conv_size) | ||
| .transpose(1, 2) | ||
| .contiguous() | ||
| ) |
There was a problem hiding this comment.
Am I reading the _decode_conv_weight lifecycle correctly? It looks like the transposed copy is created on the first eligible forward and then reused. If conv1d.weight is updated later by a refit or reload, what refreshes this copy? Would extending tests/models/glm5next/test_kda_conv_weight_refit.py:87-133 to exercise the AITER path make this clearer to reviewers?
There was a problem hiding this comment.
Thanks for noticing this! I have changed as follows:
- The
[3, width, dim]copy is now a non-persistent buffer,decode_conv1d_weight, allocated once in__init__, as in Kimi-K3's ROCm KDA layer. - It is refreshed from
conv1d.weightin two places:- the layer's
process_weights_after_loading, whichBaseModelLoader.load_modelruns after loading and the layerwise reload runs at its end; - the conv weight loader, after each shard load, which covers refits that call
load_weightsdirectly.
- the layer's
test_aiter_decode_reads_refitted_fused_convadds the AITER path to the refit test. It checks the weight the AITER decode receives after a load, after a refit through the loader, and after a layerwise reload, each time at the
same address.
| # The fused kernel keeps outputs in fp32 where the stock chain rounds to bf16: | ||
| # 2 bf16 ulp of the largest value. State rtol is Kimi-K3's spec-decode one, as | ||
| # one of these 41.9M elements reaches 3.8e-3, past its packed-decode 2e-3. | ||
| fused_rtol = 2 * torch.finfo(torch.bfloat16).eps | ||
| fused_state_tol = {"atol": 2e-3, "rtol": 3e-2} | ||
| # (query lengths, expected fused launches (rows each), output rtol, state tol) | ||
| steps: list[tuple[list[int], list[int], float, dict[str, float]]] = [ | ||
| ([1, 1, 50], [], 0, {"atol": 0, "rtol": 0}), # Mixed: stock, bit for bit. | ||
| ([1, 1, 1], [3], fused_rtol, fused_state_tol), # Decode: one fused launch. | ||
| ] |
There was a problem hiding this comment.
The underlying packed-decode test already checks untouched slots exactly. Would it also be useful to check them here, so this GLM integration test confirms that its state-index mapping only updates slots 1–3?
There was a problem hiding this comment.
Thanks. Now every slot other than 1–3 of the fused layer's conv and recurrent state must equal the saved caches.
| additional_config = vllm_config.additional_config | ||
| decode_backend = ( | ||
| additional_config.get("kda_decode_backend", "auto") | ||
| if isinstance(additional_config, dict) | ||
| else "auto" | ||
| ) | ||
| # AITER's single-launch decode needs an fp32 state, head_dim 128 and a | ||
| # width-4 conv. Without a lower bound the stock kernel falls back to -5.0 | ||
| # while AITER switches to the unbounded gate, so that config keeps the | ||
| # stock chain, as do spec decode and any explicit --kda-decode-backend. | ||
| self._use_aiter_decode = ( | ||
| decode_backend == "auto" |
There was a problem hiding this comment.
Would it be worth validating kda_decode_backend here, similar to _resolve_kda_prefill_backend? The CLI already checks its choices, but an unknown value supplied directly through additional_config appears to silently disable the AITER path.
There was a problem hiding this comment.
An unknown kda_decode_backend now raises ValueError. The accepted values and the message are the
same as in Kimi-K3's resolve_kda_decode_backend.
There was a problem hiding this comment.
I like @akii96 's suggestion of adding resolve_kda_decode_backend, this is similar to NV
AITER's single-launch decode reads a [3, width, dim] copy of the q|k|v conv weight. That copy was built on the first eligible decode and never refreshed, so a refit or reload of conv1d.weight left the AITER decode on the old weights. The copy is now a non-persistent buffer, allocated once in __init__ so cudagraphs keep its address, as in Kimi-K3's ROCm KDA layer. It is refreshed from conv1d.weight by process_weights_after_loading, which BaseModelLoader runs after loading and a layerwise reload runs at its end, and by the conv weight loader after every shard load, for refits that call load_weights outside the reload lifecycle. Defining process_weights_after_loading also makes the layer a deferred attention layer: a layerwise reload finalizes it after the other layers, as it does Kimi-K3's ROCm layer. An unknown kda_decode_backend in additional_config now raises ValueError, as in Kimi-K3, instead of silently keeping the stock chain. Tests: test_aiter_decode_reads_refitted_fused_conv checks the weight the AITER decode receives after a load, a refit through the loader and a layerwise reload. The GLM route test fills the copy as vLLM's loaders do, and checks that cache slots outside the batch keep their values. Signed-off-by: Rony Leppänen <rony.leppanen@amd.com> Co-authored-by: Claude <noreply@anthropic.com>
akii96
left a comment
There was a problem hiding this comment.
Thanks for addressing everything! I am overall happy with how this looks now just one optional nit but overall LGTM 🚀
IMO this is ready to come out of draft so maintainers can take a look!
| def test_aiter_decode_route_follows_replayed_step(dist_init, monkeypatch): | ||
| """AITER's single-launch decode kernel runs on plain decode steps only. | ||
|
|
||
| Breakable PIECEWISE graphs replay ``_forward`` against each step's metadata, | ||
| so the route is chosen there. Capture on an all-decode batch, then replay a | ||
| mixed step (stock chain) and a decode step (one fused launch); a route fixed | ||
| in ``forward`` fails one of the two. The reference is a layer built without | ||
| AITER, with the same weights and caches. | ||
| """ |
There was a problem hiding this comment.
Super Optional nit -> this test only covers the default conv-state layout. Could you also run it with the other layout (_conv_state_dim_first=True), the same way test_prefill_checkpoint_resumes_suffix does? Then we'd notice if the AITER path ever stops working for it.
There was a problem hiding this comment.
Done in ba1ed2b: the route test now runs in both conv-state layouts, and both pass on MI355X.
test_aiter_decode_route_follows_replayed_step now also runs with the dim-first (DS) conv-state layout that VLLM_SSM_CONV_STATE_LAYOUT=DS selects. The AITER route runs in either layout and receives the conv state with different strides; the test covered only the default (SD). Signed-off-by: Rony Leppänen <rony.leppanen@amd.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main now type-checks tests/models/glm5next strictly (tools/pre_commit/mypy.py), which rejects GDNAttentionMetadata as a value of ForwardContext's AttentionMetadata dict in the route test. Signed-off-by: Rony Leppänen <rony.leppanen@amd.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Rony Leppänen <rony.leppanen@amd.com>
| self._copy_decode_conv1d_weight() | ||
|
|
||
| delattr(weight, "weight_loader") | ||
| set_weight_attrs(weight, {"weight_loader": weight_loader}) |
There was a problem hiding this comment.
decode_conv1d_weight is non-persistent, so it is not in state_dict(). Elastic EP scale-up builds each new rank with load_dummy_weights=True, so this hook copies the dummy conv1d.weight into the buffer. The rank then receives only model.state_dict() tensors from a peer and runs no post-load pass. conv1d.weight ends up correct but this copy keeps the dummy values, so on MI355X with AITER and --enable-eplb --enable-elastic-ep plain decode on scaled-up ranks runs the conv with dummy weights.
simondanielsson
left a comment
There was a problem hiding this comment.
Thanks for adding this!
Suggestions:
- can we run a vllm bench serve sweep also at 64k/1k and 128k/1k workloads? Good to validate long context decode is also sped up
- Can we run niah @ 128k as well?
| additional_config = vllm_config.additional_config | ||
| decode_backend = ( | ||
| additional_config.get("kda_decode_backend", "auto") | ||
| if isinstance(additional_config, dict) | ||
| else "auto" | ||
| ) | ||
| # AITER's single-launch decode needs an fp32 state, head_dim 128 and a | ||
| # width-4 conv. Without a lower bound the stock kernel falls back to -5.0 | ||
| # while AITER switches to the unbounded gate, so that config keeps the | ||
| # stock chain, as do spec decode and any explicit --kda-decode-backend. | ||
| self._use_aiter_decode = ( | ||
| decode_backend == "auto" |
There was a problem hiding this comment.
I like @akii96 's suggestion of adding resolve_kda_decode_backend, this is similar to NV
| ) | ||
| if decode_backend not in ("auto", "native", "flashinfer", "triton"): | ||
| raise ValueError(f"Unsupported KDA decode backend: {decode_backend}") | ||
| # AITER's single-launch decode needs an fp32 state, head_dim 128 and a |
There was a problem hiding this comment.
Nit: we can remove this comment as the code says the same thing 👍
| ) | ||
|
|
||
| @torch.no_grad() | ||
| def _copy_decode_conv1d_weight(self) -> None: |
There was a problem hiding this comment.
Question: can we check if this helper is strictly needed?
| # AITER's single-launch decode applies the gated RMSNorm in-kernel, so an | ||
| # eligible layer hands the norm to _forward. The route is chosen there | ||
| # per step: breakable PIECEWISE graphs replay _forward, not this function. | ||
| out_gate = g2 if self._use_aiter_decode else None |
There was a problem hiding this comment.
Nit: Is fuse_gated_norm a more clear name? Then
self._forward(
qkv_proj_states=qkv,
g1=g1,
beta=beta,
core_attn_out=core_attn_out,
g2=g2 if fuse_gated_norm else None,
)
if not fuse_gated_norm:
core_attn_out = self.o_norm(core_attn_out, g2)and we wouldn't need to extra docstring comment
"""With ``out_gate``, also apply the gated RMSNorm that ``forward`` skips."""
| ): | ||
| # One launch for the conv update, recurrence and gated RMSNorm | ||
| # (out_gate implies no spec decode). The [N, 1] indices: the | ||
| # block-table column can be strided, which the flat form rejects. |
There was a problem hiding this comment.
Nit: should we also assert we're using aiter decode here just to be on the safe side?
| 0, :num_actual_tokens | ||
| ] | ||
|
|
||
| if out_gate is not None: |
There was a problem hiding this comment.
Question: Can this happen?
| from vllm._aiter_ops import rocm_aiter_ops | ||
| from vllm.platforms.rocm import get_cdna_version | ||
|
|
||
| return get_cdna_version() >= 4 and rocm_aiter_ops._kda_gluon_decode_importable() |
There was a problem hiding this comment.
Suggestion: Should we re-use the util that the code is using?rocm_aiter_ops.is_kda_gluon_decode_available
| from vllm.v1.attention.backends.gdn_attn import GDNAttentionMetadata | ||
| from vllm.v1.attention.backends.utils import compute_causal_conv1d_metadata | ||
|
|
||
| torch.manual_seed(0) |
There was a problem hiding this comment.
Nit: We can use set_random_seed here 👍
| fused_rtol = 2 * torch.finfo(torch.bfloat16).eps | ||
| fused_state_tol = {"atol": 2e-3, "rtol": 3e-2} | ||
| # (query lengths, expected fused launches (rows each), output rtol, state tol) | ||
| steps: list[tuple[list[int], list[int], float, dict[str, float]]] = [ |
There was a problem hiding this comment.
Suggestion: Could this be a pytest parametrization?
Net diff of vllm-project#63612 at 749147c, which has these commits on main: - [ROCm][Perf] Use AITER single-launch KDA decode in GLM-5.3-Flash - [ROCm][GLM] Refresh the AITER KDA decode conv weight after weight writes - [ROCm][GLM] Test the AITER KDA decode route in both conv-state layouts - [ROCm][GLM] Satisfy mypy in the AITER KDA decode route test Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Rony Leppänen <rony.leppanen@amd.com>
Overview
On ROCm with AITER, a plain decode step of a GLM-5.3-Flash KDA layer runs the conv update, the recurrence and the gated RMSNorm as separate launches. This PR runs them as one launch of AITER's Gluon decode kernel through the existing
rocm_aiter_ops.kda_fused_recurrent_packed_decodewrapper, which the Kimi-K3 layer has used since #57392. Companion prefill change for the same layer: #63568.Claims
VLLM_ROCM_USE_AITER=1, CDNA4 or newer (gfx950 is the only architecture run): a plain decode step of a GLM-5.3-Flash KDA layer takes one AITER launch in place of three. No new flag or environment variable; any explicit--kda-decode-backendkeeps the stock chain.FULL_DECODE_ONLYgraphs: output throughput +5.63 % over concurrency 16–64 (SE 0.80 pp), +13.70 % at concurrency 1, every row positive;FULL_AND_PIECEWISEgraphs: +3.10 % over concurrency 16–64 (SE 1.13 pp), −1.75 % at concurrency 4 and −2.31 % on the long-prompt row;Validation
Serving.
vllm/vllm-openai-rocm:nightly-rocm72-c50aca14416ef29f6ed0ed256302f30646726dcd(vLLMc50aca144, AITER 0.1.26), whosekda.pyandvllm/_aiter_ops.pyequal this PR's base at the time of measurement (5f63ea461c). Baseline: that base'skda.py; patched: this PR's at the time. Since then the PR'skda.pyinlines the eligibility check, honours--kda-decode-backend, refreshes its decode conv-weight copy after weight writes, rejects an unknownkda_decode_backend, and was merged with main's fused q|k|vconv1dparameter ([Bugfix] Fuse GLM-5.3 KDA q|k|v convs into one conv1d parameter #60991); none of these changes what a decode step runs, in the stock chain or the AITER route.amd/GLM-5.3-Flash-Quark-MXFP4at revisionb5688f25491202978c19c4d036eef579f61bbe07.FULL_AND_PIECEWISEgraphs and theconv2dcustom op disabled. Without-conv2d, MIOpen solver searches for the vision encoder's convolution stalled both arms, so that run's throughput is not reported.GLM-5.3-Flash KDA decode: AITER single-launch kernel.once; no baseline log does. KV cache memory is equal across arms (172.7 GiB in (i), 160.54 GiB in (ii)).VLLM_GPU_SYNC_CHECK=erroron the patched breakable-graph server (without-conv2d), all rows: noGPU<->CPU sync detectedline.Unit tests.
test_aiter_decode_route_follows_replayed_step(tests/models/glm5next/test_kda_recurrent.py):Glm5NextLinearAttentionlayers sharing weights and caches, one withVLLM_ROCM_USE_AITER=0and one with=1. It calls theirprocess_weights_after_loading, as vLLM's loaders do.VLLM_SSM_CONV_STATE_LAYOUT=DSselects. The fused kernel receives the conv state with different strides in each.atol=2e-3, rtol=3e-2. Kimi-K3's check of this kernel usesrtol=2e-3on up to 128 tokens; one of this test's 41.9M state elements reaches 3.8e-3, sortolis that of Kimi-K3's spec-decode check.test_aiter_decode_reads_refitted_fused_conv(tests/models/glm5next/test_kda_conv_weight_refit.py):conv1d.weightin the[3, width, dim]layout, at an unchanged address.process_weights_after_loading.MI355X, image
vllm/vllm-openai-rocm:nightly-rocm72-3966924d566340e971a6da313e790d05492aa505(AITER 0.1.26) with this PR'svllm/files over the installed package,VLLM_ROCM_USE_AITER=1:test_kda_conv_weight_refit.py: 4 passed;test_kda_recurrent.py: 8 passed (the route test once per layout), 14 skipped (12 need FlashKDA, 2 are NVIDIA-only).Each check fails when what it covers is removed. These are mutations of this PR's
kda.py, run once each in the same setup:test_kda_conv_weight_refit.py:181);process_weights_after_loading: the refit test fails after the layerwise reload (:191), and in both layouts the route test's decode output differs from the stock layer's (test_kda_recurrent.py:480; 87 % of elements in the default layout, 83 % in the dim-first one);test_kda_recurrent.py:479);test_kda_recurrent.py:480, 78 % of elements); the default case passes.On the serving image (above), before the review changes:
kda.pyit failed the decode step:AssertionError: [1, 1, 1]: fused launches (rows each) [].MI355X, serving image, config (i) server with
--kda-decode-backend triton: starts and serves, and logs noAITER single-launch kernelline.CI: the H200
GLM5Nextstep and its MI355 mirror runpytest -v -s models/glm5next. The route test needs AITER's Gluon KDA kernel and is skipped without it, as on CPU.ruff check,ruff format --check(ruff 0.14.0),typos(1.51.1): clean.tools/pre_commit/mypy.py 3.11(mypy 1.20.2): no issues.Details
Glm5NextLinearAttention(vllm/models/glm5next/common/kda.py), the KDA layer ofGlm5NextForCausalLMandGlm5NextForConditionalGeneration. The MTP layer is MLA and unaffected. Onmainonlyvllm/models/kimi_k3/amd/kda.pycalls this kernel.__init__: fp32 recurrent state,head_dim128, width-4 conv, a lower bound, no speculative decoding,--kda-decode-backendatauto, androcm_aiter_ops.is_kda_gluon_decode_available(). An unknownkda_decode_backendraisesValueError, as in Kimi-K3;additional_configbypasses the CLI's choices. An eligible layer logsGLM-5.3-Flash KDA decode: AITER single-launch kernel.once._forwardtakes the fused launch when a step has no prefill and at least one decode. The kernel applies the gated RMSNorm, so an eligible layer passesout_gateto_forwardandforwardskipso_norm; other steps, and profile runs without metadata, apply the norm at the end of_forward. The route is chosen in_forwardbecause a breakable PIECEWISE graph recordsforwardonce on a decode-only batch and replays only the eager break against each step's metadata, so a route chosen inforwardwould be fixed at capture. FULL graphs capture_forwardinline, which is safe because GDN FULL graphs replay uniform decode batches only.[3, width, dim]tensor, whileconv1d.weightis[3·dim, 1, width].decode_conv1d_weight. It is allocated once in__init__, so graphs keep its address, as in Kimi-K3's ROCm layer.conv1d.weightin two places:process_weights_after_loading, whichBaseModelLoader.load_modelruns after loading and the layerwise reload runs at its end (reload_weightsand the weight-transfer engines that use it);load_weightsdirectly.[N, 1]view because the block-table column can be strided.o_norm. Decode outputs therefore agree to 2 bf16 ulp, not bit for bit.copy_of the layer output, and under breakable PIECEWISE graphs runso_normeagerly, the likely cause of the (ii) long-prompt result. Fusing only the conv update and the recurrence, keepingo_norminforward, would avoid it, at the cost of part of the decode gain and a new_aiter_opswrapper.nccl_m2n's direct resharding writes parameters withoutload_weightsor a reload, so the copy stays stale.--kda-decode-backend tritonkeeps the stock chain for both.load_model.conv1d.weightin place would remove the copy.conv1dparameter; this PR no longer interacts with it.This change was developed with AI assistance (Claude); I reviewed every line and ran the tests listed.
Pull Request Checklist
I used vLLM's
/pr-checklistskill. (Mandatory for agents, optional for humans).AI assistance was used during the creation of this PR.
Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.
Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.
Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.
Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.
Need help on this PR? Tag
@codesmith-botwith what you need.