Record why ragged prefill is ungated, and what would have to be measured to gate it - #372
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
Reconcile the benchmark-scope claims before approval.
Pull request overview
This documentation-only PR explains why ragged prefill remains ungated and records its measured performance across architectures.
Changes:
- Documents AITER performance results and the gfx950 exception.
- Clarifies that a dedicated crossover sweep is needed to establish a threshold.
File summaries
| File | Summary |
|---|---|
docs/rocm/backends.md |
Documents ragged-prefill performance and gating rationale; benchmark-scope claims still need reconciliation. |
Review details
Suppressed comments (1)
docs/rocm/backends.md:514
- The PR description says
BatchPrefillWithRaggedKVCacheWrapperwas measured at 9 query lengths × 5 shapes × 2 architectures (and #367's test plan likewise says both routes were swept at 9 query lengths), but this text says the crossover sweep is missing and presents q=16 as the available evidence. Please reconcile these claims: if the 9-point ragged data exists, explain why it is insufficient to site a per-architecture threshold; otherwise correct the benchmark description to say only q=16 was measured.
It is ungated for want of a crossover sweep, not for want of a penalty: at 16
query tokens AITER is slower in every measured shape on gfx942 (1.07–4.42×)
and in four of five on gfx950 (1.10–5.09×), but wins 1.7× at bs 32 / kv 2048
there. Siting a threshold needs its own per-arch sweep, as the paged one did.
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4dec408 to
0323b31
Compare
|
Suppressed comment on |
There was a problem hiding this comment.
🟡 Changes recommended
Qualify the “lose” wording to accurately reflect shape-dependent crossovers.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
0323b31 to
dc91aec
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Correct the documented crossover range to match the benchmark sweep.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
dc91aec to
d1ca157
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The gfx942 crossover description must match the benchmark data by shape.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
…vers it
The short-query section gave an argument -- ragged runs on already-contiguous
KV, so there is no gather to amortise -- which reads as "no penalty". The
measurement does not say that, and the sweep that sited the paged gate covers
ragged as well. Re-measured on merged main, 9 query lengths x 5 shapes x 2
architectures, refcheck clean, `auto` resolved to aiter in all 90 rows:
gfx942 q16 q24 q32 q48 q64 q96 q128 q192 q256
bs1/kv1024 1.18 1.24 1.24 1.16 1.08 1.07 1.01 0.80 0.69
bs1/kv8192 4.74 2.88 2.88 2.33 1.81 1.50 1.09 0.81 0.58
bs8/kv1024 1.28 0.99 0.97 0.82 0.65 0.43 0.43 0.33 0.31
bs8/kv8192 1.24 0.63 0.61 0.48 0.33 0.25 0.23 0.20 0.19
bs32/kv2048 1.22 0.65 0.65 0.39 0.38 0.29 0.23 0.27 0.22
gfx950 q16 q24 q32 q48 q64 q96 q128 q192 q256
bs1/kv1024 1.29 1.47 1.26 1.14 1.05 1.02 0.90 0.60 0.63
bs1/kv8192 5.29 4.28 3.30 2.02 1.59 1.17 0.91 0.55 0.45
bs8/kv1024 1.11 0.81 0.71 0.43 0.38 0.28 0.25 0.29 0.15
bs8/kv8192 1.35 0.71 0.54 0.31 0.26 0.18 0.12 0.24 0.12
bs32/kv2048 0.58 0.34 0.34 0.37 0.21 0.22 0.17 0.22 0.16
Ratio is aiter/fa2 median time in one process, so >1 means AITER is slower.
First query length at which each shape favours AITER, in the row order above:
gfx942 192, 192, 24, 24, 24
gfx950 128, 128, 24, 24, 16
gfx950's bs32/kv2048 wins at every length measured, so no threshold serves
that shape and ragged cannot be gated there. gfx942 has no such shape -- all
five lose at 16 -- so a gfx942-only gate is supportable and simply unclaimed.
That, not "ragged is cheap", is the reason, and it is worth recording because
the argument is what invites the change.
The doc carries the two conclusions; these tables are the evidence behind them,
which is why the paragraph points here rather than growing a third table.
Co-Authored-By: Claude <noreply@anthropic.com>
d1ca157 to
625c868
Compare
There was a problem hiding this comment.
🟢 Approval recommended
Focused, benchmark-backed documentation update with no unresolved issues.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
Summary
The short-query section explains why ragged prefill is not gated with an argument — it runs on already-contiguous KV, so there is no gather to amortise — which reads as "no penalty". The measurement does not say that, and the sweep that sited the paged gate covers ragged too. The real reason is narrower and more useful: gfx942 could be gated at ≤16 and is not, while gfx950 cannot be gated at all, because one shape favours AITER from the shortest query measured onward.
Follow-up to #366 and #362, and the only thing worth keeping from #367, now closed as superseded.
What changed
docs/rocm/backends.md— six lines in the "deliberately not gated" paragraph: what the sweep shows per architecture, and which of the two gates is merely unclaimed rather than unsafe.Benchmark results
Re-measured on merged main (
fa37f6408) throughBatchPrefillWithRaggedKVCacheWrapper, 9 query lengths × 5 shapes × 2 architectures,--refcheckclean,autoresolved toaiterin all 90 rows. Ratio isaiter / fa2median time back-to-back in one process, so >1 means AITER is slower.gfx942 — loses in every shape at 16, first wins at 24
gfx950 — bs32/kv2048 favours AITER at every query length
A gfx942-only gate at ≤16 would gain 1.18–4.74×. It is not claimed here — this PR documents the state; taking the win is a separate change with its own test surface.
Test plan
pre-commit run -agh api /markdownrender check on the edited paragraph/code-review medium— the levelreview-level.sh --explaincomputes for a docs-only changelistNo test changes: this documents a decision already implemented and covered by
test_batch_prefill_kernels.py's short-query tests.Note on two earlier drafts
Worth recording, since the diff is small and the reasoning changed twice. The first draft quoted only the one cell where the sign flips and argued "a shared threshold would regress CDNA4" — which the per-arch table one screen above disproves. The second claimed ragged was ungated "for want of a crossover sweep", which Copilot correctly caught as contradicting this PR's own description: the sweep exists. Both are replaced by what the data actually shows.