[OPUS] add ut test for sparse mla kernel - #5456
minmengdie wants to merge 2 commits into
Conversation
🏷️ CI GuideRuns automatically on every PR:
Extended tests (opt-in via labels):
PR title tags & labels: |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate correctness and reliability issues remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds GPU shape-sweep benchmarks and optional accuracy checks for MLA GQA, OPUS D192/V128, and dense MHA D256 kernels.
Changes:
- Adds configurable MLA decode and context-parallel sweeps.
- Adds OPUS and D256 prefill sweeps with sampled references.
- Reports performance metrics and optional correctness results.
File summaries
| File | Review findings |
|---|---|
op_tests/test_mla_gqa_logits.py |
Moderate (2 votes): FP8 LSE is compared against the BF16 reference instead of _lse_ref_fp8. |
op_tests/test_mha_opus_d192_v128_logits.py |
Moderate (1 vote): Causal FLOPs are incorrectly halved for rectangular bottom-right masks. |
op_tests/test_mha_d256_logits.py |
Critical (3 votes): Default failing IAE shapes are not excluded or handled. Moderate (2 votes): Missing LSE is silently accepted. Moderate (1 vote): Rectangular causal FLOPs are incorrectly halved. |
Review details
Suppressed comments (2)
op_tests/test_mha_d256_logits.py:204
- [verified] The same causal-halving error is present here: the public wrapper documents a bottom-right mask, so for rectangular inputs the visible-token count is not
seqlen_q * seqlen_k / 2. For defaultsq=1024, sk=65664, this makes the table underreport TFLOPS by nearly 2x. Author must derive causal FLOPs from the number of visible key tokens rather than unconditionally dividing the full-matrix count by two.
"ref_rows": ref_n,
"fwd us": us,
op_tests/test_mha_opus_d192_v128_logits.py:190
- [verified] With
seqlen_k > seqlen_q,flash_attn_funcuses a bottom-right causal mask, so rowiseesseqlen_k - seqlen_q + i + 1keys; only square shapes are approximately half of the full QK/PV work. Dividing by two here underreports TFLOPS for rectangular defaults such assq=1024, sk=65664by almost 2x, making the reported metric misleading. Author must compute the visible-token count from(seqlen_q, seqlen_k)before deriving causal FLOPs.
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if batch_size >= 16 and nheads >= 64 and seqlen_k >= 131072: | ||
| return False |
| lse_got = softmax_lse[:, :, rows] if softmax_lse is not None else None | ||
| if lse_got is not None: |
| err = checkAllclose( | ||
| lse_ref, | ||
| attn_lse.reshape(total_q, nhead), |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues affect correctness, benchmark accuracy, and default sweep reliability.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (7)
Previously missed (3) — in code that hasn't changed since the last review.
op_tests/test_mha_d256_logits.py:193
- [verified] For bottom-right causal attention with
seqlen_k >= seqlen_q, the matrix is not half full: the visible pair count isseqlen_q * seqlen_k - seqlen_q * (seqlen_q - 1) // 2. Dividing by two therefore under-reports TFLOPS by almost 2x for shapes such assq=1024, sk=65664, so this sweep's reported performance is inaccurate. Author must compute FLOPs from the actual causal visible-pair count.
op_tests/test_mha_opus_d192_v128_logits.py:173 - [verified] For bottom-right causal attention with
seqlen_k >= seqlen_q, the matrix is not half full: the visible pair count isseqlen_q * seqlen_k - seqlen_q * (seqlen_q - 1) // 2. Dividing by two therefore under-reports TFLOPS by almost 2x for shapes such assq=1024, sk=65664, making the M×N performance table inaccurate. Author must compute FLOPs from the actual causal visible-pair count.
op_tests/test_mla_gqa_logits.py:294 - [verified] Despite the PR title saying this adds a sparse MLA UT, the MLA metadata is requested with
is_sparse=Falsehere (and again in the normal decode path), while the reference consumes the full page table; this exercises dense persistent GQA, not the sparse kernel/metadata path. The existing sparse test usesis_sparse=True. Author must either add sparse top-k inputs and a matching reference or retitle/describe this as a dense GQA sweep.
op_tests/test_mha_d256_logits.py:301
- [verified] The loop only filters
seqlen_k < seqlen_q; it has no_shape_okfor the known HIP illegal-access cases. The documented default therefore still launchesb=16, h=64, sq>=4096cases, and theexceptbelow re-raises anything that is not OOM, so the default sweep aborts at the first IAE instead of producing the claimed 45-row table; custom large-batch/large-K cases are also unguarded. Author must add the documented shape filter before allocation/launch rather than catching a poisoned HIP IAE and continuing.
if seqlen_k < seqlen_q:
continue
op_tests/test_mha_d256_logits.py:98
- [verified]
run_flashpasses0.0assoftmax_scale, butaiter.flash_attn_functreats onlyNoneas the default and forwards an explicit zero to the kernel (aiter/ops/mha.py:2619-2620). The benchmark therefore computes unscaled, uniform attention while the reference uses1/sqrt(256), so--refis validating a different operation. Author must passNoneor `HEAD_DIM**-0.5 here.
0.0,
op_tests/test_mha_d256_logits.py:166
- [verified] With
--ref, requestingreturn_lse=Trueis not enough to test the LSE contract because aNoneresult simply skips this check. A dispatch regression that stops returning LSE would still report a successful output comparison, contrary to the stated LSE coverage. Author must assert thatsoftmax_lseis present before slicing and comparing it.
lse_got = softmax_lse[:, :, rows] if softmax_lse is not None else None
if lse_got is not None:
op_tests/test_mla_gqa_logits.py:928
- [verified] The no-argument sweep defaults to
nhead=96and128, but the PR notes that these LEGACY page-size-1 cases have no a16w16 PS code object and abort when swept.check_supportonly filters dtype/arch, so the advertised default command still reaches those unsupported heads instead of producing a complete table. Author must remove unsupported heads from the defaults or add an explicit capability gate that skips them.
default=[32, 64, 96, 128],
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
| if seqlen_k < seqlen_q: | ||
| continue |
Summary
Adds three serving-shape sweep UTs, each with one markdown table per kernel path:
op_tests/test_mla_gqa_logits.py— persistent LEGACY MLA decode (and a round-robin CP phase) across GQA head countsop_tests/test_mha_opus_d192_v128_logits.py— gfx950 OPUS dense MHA prefill, Q/K=192 V=128op_tests/test_mha_d256_logits.py—aiter.flash_attn_funcdense MHA prefill, BSHD D=256Motivation
The serving axes that pick a kernel config are M (query rows / decode batch × MTP) and N (KV length), plus head count. In-tree coverage for these three paths is either a single-shape correctness UT or a bench that reports no accuracy:
op_tests/test_mla_persistent.py/test_mla_persistent_round_robin.py, which do not sweep GQAnhead ∈ {32,64,96,128}against the ctx/batch/MTP grid a model actually serves.test_mha.py(test_flash_attn_func_opus_d192_v128).flash_attn_funcwithout a dedicated M×N table.There was no single place to answer "what does this op do across the shapes a model actually serves, and is it still correct there".
Changes
Three new files. Each path is a different kernel with a different calling convention, so each gets its own
@benchmarkfunction and table:test_mla_gqa_logits.pydecodeaiter.mla_decode_fwd(persistent LEGACY,nhead_kv=1)[B, decode_qlen, H, D], fragmentedrandpermpage tablestest_mla_gqa_logits.pycpg(j)=j*W+r,-cpwranks, skipped whenctx < Wtest_mha_opus_d192_v128_logits.pyfmha_fwd_bf16_opus_fwdtest_mha_d256_logits.pyaiter.flash_attn_funcMLA decode reproduces the production paged-KV convention: LEGACY layout, page size 1, each sequence owns its pages, and a randomized (fragmented) page table rather than a sequential one a real KV cache never hands the kernel. Prefill, non-persistent decode, 3BUFFER and DS32_OPUS are out of scope. KV length is uniform across the batch.
Two things worth flagging for review:
--refis off by default. The default product is a kernel-timing sweep. Pass--refto compare againsttorch_mla_extend/ CP merge / sampledattention_ref. The tables below were taken with--no-ref, so theerrcolumns are empty on purpose — they are not a claim that every default shape was golden-checked.[B, H, Sq, Sk]fp32 score tensor does not fit at the large shapes (same issue as [Test] Sweep the MQA logits indexer's M and N across prefill and decode #5434). Opus samples query rows (ref_rows); d256 does the same when--refis on. LSE is compared separately (opus_ref_lse/ flash LSE).TB/sis compulsory traffic only (Q+K+V+O bytes / us). Prefill re-reads KV per query row, so that column is a lower bound there and TFLOPS is the metric to read; MLA decode at large ctx is closer to bandwidth-bound and TB/s is the one that matters.Default products after trimming shapes that OOM'd or illegal-memory-access'd on MI355X:
nhead={32,64,96,128},ctx={4096,16384,65536,131072},batch={16,32,64,128},mtp={1,2,4,8},dtype=kvtype ∈ {bf16,fp8}(Q dtype must equal KV dtype),cpw={2,3,4,7,8}. The numbers below only cover nhead 32/64 — 96/128 have no a16w16 LEGACY PS.coon this tree and abort if swept.batch={1,16,64,128},nheads={32,64},seqlen_q={1024…16384},seqlen_k={4096,16384,65664}, skipsk < sq. Droppedsk=131072andbatch=256from defaults after OOM.batch={1,16},nheads={32,64}, same q/k lists._shape_okalso skipsb≥64andb≥16 & h≥64 & sk≥131072.How to run it
Every flag is a swept list (
-n,-c/-k,-b,-mtp,-d,-kvd,-cpw,-p,-q), so a different model's shapes go in without touching the file. Prefill skipsseqlen_k < seqlen_q.--perfon the opus file is an alias for--no-ref.Performance
Environment: MI355X (gfx950, 256 CU, 288 GB HBM), idle GPUs, this tree,
--no-ref. Tables list every timed shape (decode 256, opus 99, d256 45). CP timings omitted. Emptyerr/ always-true columns (gfx=gfx950, opuscausal,ref_rows=0, kvtype==dtype) are dropped so the comment fits GitHub's 65535-byte limit.usis kernel time.MLA decode (256/256, nhead 32/64 × bf16+fp8): peak 1829 TFLOPS at fp8 nhead=64 mtp=8 batch=16 ctx=131072, and peak 6.01 TB/s at bf16 nhead=32 mtp=2 batch=128 ctx=131072. fp8 is the TFLOPS story; bf16 at large batch/ctx is the bandwidth story.
Opus D=192/V=128 (99/99 current defaults): peak 1269 TFLOPS at b=1 h=32 sq=sk=16384. Peak 1.49 TB/s at b=1 h=64 sq=1024 sk=4096 — short-M is more traffic-heavy; long-M is the TFLOPS column. An earlier untrimmed run (180 calling shapes) hit 41 OOMs at sk=131072 / b=256; those are no longer in the default product.
BSHD D=256 (45 timed of 52 defaults): peak 512 TFLOPS at b=1 h=32 sq=sk=4096, peak 0.71 TB/s at b=1 h=64 sq=1024 sk=4096. Seven
b=16 h=64shapes withsq>=4096never timed (HIP illegal memory access on nearby large-skcases).MLA decode —
mla_decode_fwdpersistent LEGACY, 256 shapesOpus prefill —
fmha_fwd_bf16_opus_fwd, 99 shapes (current defaults)BSHD D=256 —
flash_attn_func, 45 timed of 52 default shapesTesting
--ref) on GPU, small shapes only:err=0vstorch_mla_extendfailed!(max abs ~0.07–0.08) — expected quantization, not a kernel bug; vs fp8 golden:warning!,cal_diffdid not abort--refb=1 h∈{32,64} q=1024 k=4096: output pass, LSE max diff ~2e-6--refsame small shapes: output pass; LSE pass with derived atol=0.125--no-refsweeps: MLA decode 256/256, opus 99/99 (after OOM trim), d256 45 timed then IAE on largeb/h/skSUPPORTED_GFXgates it; MLA also skipsbf16 + nhead=32on gfx942. Opus is gfx950-only.nhead={96,128}MLA not in the perf tables: no a16w16 LEGACY PS.coon this treeFollow-ups (not in this PR)
cprr) heuristic / kernel — currently decode-only for fp8.b=16 h=64andsk=131072(also a fewsq>=4096at that batch/head that never timed). Either fix the kernel path or keep those shapes out of defaults after a clean repro.sk=131072andbatch=256on MI355X — left out of defaults rather than shipped unrun.--refon the default product. Golden is sampled and expensive; the published tables are kernel-only.