Repository navigation
feat(rocke): add chunkwise KDA prefill kernels for gfx950 - #11223
Merged
Merged
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
AnaghaRaoAMD
left a comment
Contributor
There was a problem hiding this comment.
DoD audit against rocke/KERNEL_AUTHORING.md — five boxes unticked for a new kernel family, most importantly the golden IR SHA256 fixture.
Core DoD (every change, KERNEL_AUTHORING.md L95-113)
| Item | Status | Evidence |
|---|---|---|
| On-GPU numeric parity test, not spec-only geometry | ✅ | library/tests/test_kda_chunkwise_gfx950_numeric.py — 14 tests against a token-serial float64 oracle that shares no algebra with the kernel. Genuinely strong. |
Coverage in both directions: spec → IR and problem → spec |
❌ | Neither. No IR-level test at all, and no dispatcher/selection test (nothing selects KDA — see below). |
| Golden: "a new family/case adds an entry" | ❌ | library/tests/golden/ still contains exactly one file, attention_dense_gfx942_ir_sha256.json. No KDA entry. |
run_checks.py green |
Stages are guard → gate → parity → pytest → numeric. Every KDA test is pytest.mark.gpu (numeric L62; spec tests likewise), so on any machine without gfx950 this PR contributes zero executed coverage. |
|
| Docs | CASE_STUDY.md is present and good. No README.md harness/results doc, no ALGORITHM.md. |
§A — New kernel family (L120-140)
| Requirement | Status | Evidence |
|---|---|---|
Spec-driven build_*(), reuse helpers/atoms |
✅ | KdaTileSpec/KdaChunkFusedSpec/KdaChunkScanSpec + build_kda_chunk_{prep,split,fused}. Clean. |
| C engine mirror in the same change | ➖ exempt | library/kernels/ has no C++ mirror by design. |
.py/.c parity pair under tests/parity/ |
➖ exempt | Follows from the above. library/tests/parity/ holds 20 pairs, all platform-mirrored families; no kda_*_emit.* is owed. |
| Dispatcher wiring — "A family not in a registry is unreachable." | ❌ | library/dispatch/ has no KDA reference (grep -ril kda across the tree, excluding the 8 PR files, returns only an incidental substring in kernels/common/fmha_appendkv.py). Arguably KDA shouldn't go in ATTENTION_REGISTRY — it selects from an SDPA UnifiedAttentionProblem and linear attention has no such request shape. But the dense precedent registered opt-in rather than skipping (library/dispatch/AGENTS.md L35-37: attention_gfx942_dense, attention_gfx950_dense, both "(opt-in only)"). Either wire it or state the decision in the PR body — right now the family is reachable only by running a builder by hand. |
| Golden created for the new family | ❌ | See below. |
End-user visibility — SUPPORT_MATRIX.md |
❌ | platform/python/rocke/instances/SUPPORT_MATRIX.md "## Attention / FMHA" table (L73, ~14 rows) has no KDA row. |
Docs discoverability — instances/index.md row + per-family page |
❌ | platform/dsl_docs/instances/index.md "## Attention Family" table (L65) has no KDA row; no per-family page. |
| A benchmark scenario | ❌ | library/benchmarks/ has 12 drivers, none for KDA. The bench loop inside kda_chunk_fused.py main() is a builder-local sweep, not a registered scenario. |
Appendix §A step 8 (L219-257)
| Requirement | Status |
|---|---|
ALGORITHM.md "if the math is new" |
❌ — the math is new here (chunkwise factorization of the gated delta rule); library/builders/gfx950/attention/ carries both this and a README, library/builders/gfx950/kda/ carries neither |
README.md harness/results doc |
❌ |
| If any of these are not to be addressed in the PR, we need follow up tickets created |
Adds a spec-driven family for chunkwise gated delta-rule linear attention (KDA) prefill: the per-chunk tile builder, a standalone state scan over materialized tiles, and a fused variant that keeps the tiles in LDS. The per-chunk body factors into six state-independent tiles plus a serial state recurrence, and the two halves have opposite parallelism profiles. Both compositions therefore ship: the split path (tiles to HBM, one workgroup per chunk, then one per batch-head for the scan) is the default, and the fused path is retained for the bitwise cross-check and for future arch budgets. The fused path is the design that looks strictly better on paper -- it never writes the tiles out -- and it loses. Holding the tile builder's staging tiles and the scan's operands live at once puts a workgroup over half the LDS budget, so only one is resident per CU, and the scan is a latency-bound chain of small matmuls with nothing left to cover it. That trade is encoded as a rejection rule: is_valid_scan_spec checks LDS against LDS_LIMIT // min_occupancy rather than the full budget, so a spec that fits the hardware but not two workgroups per CU is refused. Correctness rests on two independent float64 oracles: a tile oracle built from pairwise exponent differences with no midpoint factoring, and a token-serial walk of the recurrence that is not chunked at all. The split and fused paths share one emitted scan body and are checked for bitwise equality, not merely agreement within tolerance. Methodology, levers and the attribution technique behind them are in library/builders/gfx950/kda/CASE_STUDY.md. Measurements are not recorded here; they live on the protected internal page. Co-authored-by: Cursor <cursoragent@cursor.com>
yraparti
force-pushed
the
users/yraparti/rocke/gfx950_kda
branch
from
September 3, 2026 05:45
1b050eb to
6c6186d
Compare
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
Co-authored-by: Cursor <cursoragent@cursor.com>
AnaghaRaoAMD
approved these changes
Sep 3, 2026
Co-authored-by: Cursor <cursoragent@cursor.com>
yraparti
enabled auto-merge (squash)
September 3, 2026 21:27
Contributor
TheRock Submodule Bump ActivityNewest first
|
AnaghaRaoAMD
added a commit
that referenced
this pull request
Sep 10, 2026
…ntract test_every_spec_class_is_listed asserts REQUIRED_FIELDS covers exactly the spec classes reachable from library builders, so it is only correct at the moment it merges. The guard landed in #11237 (Aug 27); #11226 (gfx942 KDA, Sep 2) and #11223 (gfx950 KDA, Sep 3) were branched before it and never re-ran it against their own specs. Both merged clean and develop broke with 8 unlisted specs. All 8 are fully defaulted -- no field, problem shape included, is mandatory -- so each records an empty required set and the frozen-fields guard stays honest.
AnaghaRaoAMD
added a commit
that referenced
this pull request
Sep 10, 2026
…ntract (#11842) ## Problem `library/tests/test_builder_signature_contract.py::test_every_spec_class_is_listed` fails on `develop`: ``` AssertionError: REQUIRED_FIELDS is out of sync with the specs reachable from builders. not listed: ['kernels.gfx942.kda_chunkwise.KdaChunkFusedSpec', 'kernels.gfx942.kda_chunkwise.KdaChunkPrepSpec', 'kernels.gfx942.kda_chunkwise.KdaChunkScanSpec', 'kernels.gfx942.kda_chunkwise.KdaTileSpec', 'kernels.gfx950.kda_chunkwise.KdaChunkFusedSpec', 'kernels.gfx950.kda_chunkwise.KdaChunkPrepSpec', 'kernels.gfx950.kda_chunkwise.KdaChunkScanSpec', 'kernels.gfx950.kda_chunkwise.KdaTileSpec'] listed but gone: [] ``` The guard asserts `REQUIRED_FIELDS` covers *exactly* the spec classes reachable from `library/kernels/` builders, so any kernel family added after it must add its own entry. Two didn't. Running the guard at each commit: it passes at `287838b7bb8` (#11237, guard lands), fails at `b0473e8b842` (#11226, gfx942 KDA) with the 4 gfx942 specs unlisted, and fails at `2c41636142a` (#11223, gfx950 KDA) with all 8. Both merge-result trees were already red at merge time — the guard was an ancestor of both, it just never ran: no workflow executes the rocke `library/tests` lane. ## Fix List the 8 KDA specs. Every field on all of them — problem shape included — already carries a default, so each records an empty required set. `KdaTileSpec` is not a builder argument; it is reached through the `tile` field of the other three, which is why the annotation walk finds it. No kernel, builder, or spec code changes: `REQUIRED_FIELDS` is a record of what the specs already are, not a constraint being loosened. `test_spec_required_fields_are_frozen` now covers these 8 too, so a future KDA field added without a default fails at authoring time. ## Verification ``` pytest library/tests/test_builder_signature_contract.py 9 passed (was 8 passed, 1 failed) pytest library/tests 1206 passed, 149 skipped, 343 subtests ruff check / black --check clean ``` Two failures in `library/tests/dispatch/test_grouped_conv_wgrad_dispatch.py` (`TypeError: ConvGroupedRequest.__init__() got an unexpected keyword argument 'force_deterministic'`) reproduce on unmodified `develop`, are unrelated to this diff, and are left for a separate change. Same failure mode as this one — an exact-set invariant with no CI lane behind it. JIRA ID : AICK-2224
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ISSUE ID : AICK-2051
Adds a spec-driven kernel family for chunkwise gated delta-rule linear attention (KDA) prefill on gfx950, under
library/kernels/gfx950/kda_chunkwise.pywith host builders underlibrary/builders/gfx950/kda/.The per-chunk body factors into six state-independent tiles (
A,GK,GQ,Aqk,Kt,dec) plus a serial state recurrence. The two halves have opposite parallelism profiles — the tiles are fully parallel over the sequence, the recurrence is serial per (batch, head) — so both compositions ship:build_kda_chunk_prep+build_kda_chunk_scan), defaultbuild_kda_chunk_fused)Three specs, each with its own
is_valid_*_specandbuild_*, following the platform instance conventions.The interesting result is a negative one
The fused path is the design that looks strictly better on paper — tiles never leave LDS, so it is strictly less memory traffic — and it loses.
Holding the tile builder's staging tiles and the scan's operands live simultaneously puts a workgroup over half the LDS budget, so only one is resident per CU. The scan is a latency-bound chain of small matmuls with a serial dependency between chunks; at one workgroup per CU there is no second workgroup to cover that latency, and it bleeds into the tile phase as well. Paying for a tile round trip through HBM to keep two workgroups resident is the cheaper side of the trade.
This was isolated, not guessed: ablating the scan showed the fused kernel's tile phase alone costs far more than the standalone tile kernel doing identical work; a looped tile-only kernel writing to HBM ruled out the loop structure and the destination switch; and forcing the standalone kernel down to one workgroup per CU with unused LDS ballast reproduced the fused cost.
The conclusion is encoded in the spec, not left as a comment.
is_valid_scan_speccheckslds_bytes()againstLDS_LIMIT // min_occupancyrather than the full budget, so a scan spec that would fit the hardware but not two-per-CU is rejected — it would not be worth running.Levers
ds_read_b128/ds_write_b128. The pad doubles as bank-conflict padding: at a row stride ofDK, an unpadded 128-element bf16 row puts every lane in the same 16 banks.GK,GQand the three MFMA operands all read the sameq/k/gate values; merging them cuts both LDS traffic and the exponential count, since eachexp2result feeds several consumers.solve_blocktrades scalar work for one more block step.A B^Tform. The recurrence is written transposed throughout, putting the contraction on the fastest axis of both operands for all five of its matrix products. No operand ever needs an LDS transpose.vchannels, soVis subtracted straight into the accumulator andOgoes straight to HBM already coalesced.Two bugs worth recording, both now spec rejection rules or explicit barriers: a
C x Cbf16 tile at a pitch that is a multiple of 4 but not 8 puts odd rows off a 16-byte boundary and silently violates theds_read_b128alignment contract (hencepad_cb); and the blocked solve's cross-laneds_write->ds_readneeds an explicits_waitcnt lgkmcnt(0), which presented as a failure only at the smallest block sizes.Correctness
Two float64 oracles, deliberately independent of the kernels and of each other:
The gate range is swept to
-5(the referencegate_lower_bound), where a 32-token chunk accumulates up to 160 nats — the only regime that exercises the factoring and the clamps.The split and fused paths share one emitted scan body and are checked for bitwise equality, not merely agreement within tolerance.
Test plan
tests/test_kda_chunkwise_spec.py— spec admission and rejection rules, plus comgr compile of all three builders (no GPU). 20 tests.tests/test_kda_chunkwise_gfx950_numeric.py— both oracles across the gate range, plus split-vs-fused bitwise equality. 14 tests,@pytest.mark.gpu, gated ongcnArchName.tools/check_byte_identity.py).library/tests/run_all.pyCPU lane — no new failures. (Three pre-existing failures in gfx942 attention code are untouched by this change.)Notes for reviewers
library/builders/gfx950/kda/CASE_STUDY.md. PerAGENTS.md§Compliance, measured numbers are not in this repo or this PR — they are recorded on the protected internal page.BH, so below roughly the CU count it is parallelism-starved and an implementation that splits thevdimension to manufacture more workgroups will do better. Fixing it needs a second partitioning rule for the same accumulator, which invites cross-wave reductions; it is documented rather than implemented.