Skip to content

[FlyDSL] Read (B, next_n) context lengths in the paged MQA logits kernel - #6450

Open
amd-sriram wants to merge 6 commits into
ROCm:mainfrom
amd-sriram:rocm/flydsl-paged-mqa-logits-2d-context-lens
Open

amd-sriram wants to merge 6 commits into
ROCm:mainfrom
amd-sriram:rocm/flydsl-paged-mqa-logits-2d-context-lens

Conversation

@amd-sriram

@amd-sriram amd-sriram commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Currently, vLLM passes context_lens to the paged MQA logits kernels as a 2D (B, next_n) table, the same layout DeepGEMM uses. The FlyDSL kernel only reads a 1D [B] vector, so with MTP on, flydsl_fp8_paged_mqa_logits fails with a reshape error:

RuntimeError: shape '[4]' is invalid for input of size 12

This PR lets the FlyDSL paged_mqa_logits kernel accept the 2D context_lens that vLLM sends with MTP, while staying backward compatible with the 1D format.

Split out of #6153 as requested in review (one backend per PR); the Triton and Gluon change is in #6451. The two PRs touch different files and can merge in either order.

Example: 4 requests, next_n=3, context lengths 8192 / 1500 / 3000 / 700, vLLM sends the following 2D array:

b j=0 j=1 j=2
0 8190 8191 8192
1 1498 1499 1500
2 2998 2999 3000
3 698 699 700

The fix uses the row length (next_n) as the stride and reads the last entry of each row, which gives [8192, 1500, 3000, 700].

What changes

  • The stride is a build-time constant. The kernel reads request b's length at b * ctx_stride + ctx_stride - 1; stride 1 builds the pre-PR kernel (same code and name), stride n builds a variant named ..._cs{n}, and _compile() caches one build per stride.
    • 1D [B]: stride is 1, so this is context_lens[b], the same load as today.
    • 2D (B, n): stride is n, so this is the last entry of row b, which is the full sequence length.
  • The wrapper picks the stride from the tensor's rank, and raises ValueError unless context_lens is a contiguous int32 [B] or (B, n) on the same device as q_fp8, with exactly B rows and n equal to 1 or next_n:
    • a non-contiguous input is rejected rather than copied, so there is no hidden allocation per call. Before, reshape silently copied some strided inputs and kept others as strided views that the kernel read as dense.
    • a (B, 0) table would have made the kernel read before the start of the tensor, and a (B, n) with n other than next_n read the wrong column as the length.
    • a wrong row count raised a RuntimeError from reshape before; it is now a ValueError with the expected shape.
    • the kernel reads context_lens as int32 words, so an int64 table gave wrong lengths before.

Backward compatibility

  • A contiguous [B] is unchanged, and (B, 1) behaves exactly like [B].
  • A contiguous (B, next_n) table is the only new input. vLLM already keeps its decode seq_lens contiguous for this.
  • A strided, non-int32 or off-device context_lens now raises ValueError.

Tests

In op_tests/test_flydsl_fp8_paged_mqa_logits.py:

  • The context_lens layout is a new axis of the existing sweep, -c/--ctx_lens (default 1d 2d). For 2d, FlyDSL gets the (B, next_n) table vLLM builds, is graded against the same fp32 reference as 1d, and is timed next to it in the summary table.
  • test_fp8_paged_mqa_logits_mixed_context_lens: B=5, next_n=3, distinct lengths and one empty (CUDA-graph padding) request, graded against the fp32 reference for 1d and 2d, with and without SplitKV. It fails if the kernel reads row 0's length for every request.
  • test_fp8_paged_mqa_logits_bad_context_lens: too few or too many rows, no columns, a column count other than next_n, an int64 table, a CPU table, a strided [B] and a non-contiguous (B, n) must raise ValueError.

Results

MI355X, Triton 3.8, FlyDSL 0.3.4.1, upstream main at 9fd025efd plus this change:

  • python op_tests/test_flydsl_fp8_paged_mqa_logits.py: all tables pass. In the default sweep (batch 1-8, next_n 1-8, 1k context) 2d has the same error against the reference as 1d in every case, and its kernel time is within +0.1% to +1.9% of 1d.
  • The new test fails on main without this change, with the reshape error above.
  • black and ruff (0.16.0) are clean.
  • Kernel time at the GLM-5 indexer shape with 8k context: 1D matches the kernel on main (e.g. B=16, next_n=3: 30.6 / 30.2 us vs 30.7 / 30.2 us over two runs), and 2D stays within about 1% of 1D.
B next_n main 1D µs 1D µs 2D µs
16 3 30.70 30.62 30.80
64 3 33.20 33.64 33.73
128 5 63.63 63.73 63.37

@github-actions

Copy link
Copy Markdown
Contributor

🏷️ CI Guide

Runs automatically on every PR:

  • ✅ Pre-checks (submodule verification, code formatting)
  • ✅ Aiter op tests (gfx942 + gfx950)
  • ✅ Triton tests on MI35X (only when aiter/ops/triton/** or related paths are changed)

Extended tests (opt-in via labels):

Label Tests
ci:gfx1250-ffm-triton Run the five-shard gfx1250 FFM Triton test suite
ci:triton-300x Run an additional Triton test job on MI300X in PRs (added automatically when gfx942 configs change); main branch always runs both MI35X and MI300X
ci:triton-355 Run the full Triton test suite on MI35X, not only the tests the change affects
ci:windows Windows JIT op smoke test on the gfx1201 ROCm runner
ci:multi-gpu Aiter multi-GPU tests on the 8-GPU runner
ci:sglang SGLang integration tests: DeepSeek-R1-MXFP4 accuracy, Qwen 3.5 accuracy
ci:atom ATOM benchmark: DeepSeek-R1-0528, GPT-OSS-120B
ci:atom_full ATOM accuracy suite for PR and main models from ATOM models_accuracy.json
ci:vllm vLLM benchmark: DeepSeek-V4-Pro, MiniMax-M3-MXFP4
ci:all All standard extended tests (excludes ci:atom_full)

Only add ci:atom_full for FlyDSL or Triton upgrades.
Add labels via the sidebar or gh pr edit 6450 --add-label <label>

One backend per PR:
A PR changes one kernel backend: [Triton/Gluon] (Triton and Gluon count as one), [HIP], [ASM], [CK], [OPUS] or [FlyDSL]. If the title ends up with two backend tags, split the PR -- as stacked pull requests when one part cannot merge without the other.

PR title tags & labels:
Component tags ([Triton/Gluon], [HIP], [CK], [ASM], ...) are added to the PR title and as PR labels automatically from the changed files and re-synced on every push — change-type tags like [fix]/[Perf], op tags like [MLA], and human labels (ci:*) are left untouched. Add the no-auto-title label to stop the title rewrites; labels stay in sync either way.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The wrapper accepts unsupported context-length dtypes and devices that the kernel may misinterpret.

2 open findings
What changed in this PR

Extends the gfx950 FlyDSL paged MQA logits path to consume 2D per-row context-length tables.

Changes:

  • Adds runtime row-stride indexing while preserving 1D behavior.
  • Validates context-length layout and adds 2D/rejection tests.
File Description
aiter/​ops/​flydsl/​kernels/​mqa_logits/​fp8_paged_mqa_logits_gfx950.py Adds 2D context-length handling.
op_tests/​test_flydsl_fp8_paged_mqa_logits.py Tests equivalent outputs and invalid layouts.

🧠 Review effort: Balanced


💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread aiter/ops/flydsl/kernels/mqa_logits/fp8_paged_mqa_logits_gfx950.py Outdated
Comment thread op_tests/test_flydsl_fp8_paged_mqa_logits.py Outdated
The kernel reads context_lens as int32 words, so an int64 table gave wrong
lengths. Compare the 2D and 1D logits with checkAllclose at zero tolerance.
Copilot AI balanced review requested due to automatic review settings October 12, 2026 21:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The runtime GPU kernel change needs head-matched gfx950 validation before approval.

0 open findings

2 resolved since last review

🧠 Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

GPU kernel correctness and performance require final human review because no head-matched deterministic validation report was available.

0 open findings

🧠 Review effort: Balanced

@samremes samremes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly minor nits on docs, validation and tests; no blockers from my side.

Comment thread aiter/ops/flydsl/kernels/mqa_logits/fp8_paged_mqa_logits_gfx950.py Outdated
Comment thread aiter/ops/flydsl/kernels/mqa_logits/fp8_paged_mqa_logits_gfx950.py Outdated
Comment thread aiter/ops/flydsl/kernels/mqa_logits/fp8_paged_mqa_logits_gfx950.py Outdated
Comment thread op_tests/test_flydsl_fp8_paged_mqa_logits.py Outdated
Comment thread op_tests/test_flydsl_fp8_paged_mqa_logits.py Outdated
…lumns

A (B, n) table with n other than next_n read the wrong column as the
length.
Copilot AI balanced review requested due to automatic review settings October 14, 2026 17:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Independent GPU correctness and performance validation was unavailable, so final human verification remains necessary.

1 open finding

🧠 Review effort: Balanced

Comment on lines +501 to +506
- ``[B]``: the full length ``L_b`` of request ``b``, including its ``next_n``
query tokens, or
- ``(B, next_n)``: row ``b`` is ``[L_b - next_n + 1, ..., L_b]``, the layout
vLLM builds. Only the last column is read: query row ``j``
(``0 <= j < next_n``) attends to KV positions ``[0, L_b - next_n + 1 + j)``,
so other row values are not supported.
… sweep

-c/--ctx_lens (default 1d 2d) runs the 2D table through the existing
sweep, graded against the fp32 reference and timed next to 1D. The
rejection checks stay as a small separate test.
Copilot AI balanced review requested due to automatic review settings October 14, 2026 17:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The GPU indexing change needs stronger regression coverage and independently confirmed target-hardware validation.

2 open findings

🧠 Review effort: Balanced

Comment thread op_tests/test_flydsl_fp8_paged_mqa_logits.py
Stride 1 builds the pre-2D kernel (same code and name); stride n builds a
variant named ..._cs{n}. _compile() caches one build per stride.
Copilot AI balanced review requested due to automatic review settings October 14, 2026 20:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Final human review should confirm gfx950 compilation, correctness, and performance evidence, which static inspection cannot establish.

2 open findings
Previously missed (1)

In code that hasn't changed since last review

Low severity Document causal bound using per-row live query count

aiter/​ops/​flydsl/​kernels/​mqa_logits/​fp8_paged_mqa_logits_gfx950.py:519

This causal bound uses the padded next_n, but the kernel uses nn = min(next_n_lens[b], next_n) when next_n_lens is supplied. For example, with next_n=8 and three live rows, row 0 includes position L_b-3, rather than stopping at L_b-8. Document the bound using the live row count so callers get the correct ragged-query semantics.

🧠 Review effort: Balanced

samremes
samremes previously approved these changes Oct 15, 2026

@samremes samremes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The contract, the column check, the compile-time stride, and the sweep axis match what we asked for.

Comment thread op_tests/test_flydsl_fp8_paged_mqa_logits.py
One case with B=5, next_n=3, distinct lengths and an empty (CUDA-graph
padding) request, graded against the fp32 reference for 1D and 2D, with
and without SplitKV. The 2D table clamps at 0 like vLLM.
Copilot AI balanced review requested due to automatic review settings October 15, 2026 20:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

No blocking defects were identified, but gfx950 correctness and performance results were not independently verified.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants