Repository navigation
docs(hip-kernel-provider): add GDN kernel family design doc - #11807
Conversation
Specifies the Gated DeltaNet operator, its relationship to KDA, and the two kernel pathways: the single-token decode kernel and the chunkwise prefill forward that ships as a mode of the KDA chunkwise kernel. Covers the gated delta rule, why a scalar forget gate makes GDN a special case of KDA's per-channel gate, the parallel decomposition and dataflow of each kernel, why decode cannot be prefill at chunk size 1, and the asymmetry that makes the reverse reuse a kernel change rather than a configuration.
✅ 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. |
There was a problem hiding this comment.
🟡 Changes recommended
The document describes implementations absent from the repository and contains inconsistent mathematical notation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a design specification for gfx950 GDN decode and prefill kernels.
Changes:
- Documents GDN/KDA relationships, kernel architecture, validation, and limitations.
- Describes decode and chunkwise prefill execution.
File summaries
| File | Description |
|---|---|
builders/gfx950/gdn/GDN_DESIGN.md |
Adds the GDN kernel-family design document. |
Review details
Suppressed comments (2)
dnn-providers/hip-kernel-provider/rocke/library/builders/gfx950/gdn/GDN_DESIGN.md:395
- These products are dimensionally inconsistent with the document's definition of
SasDV × DK: for example,Sᵀ GKᵀcannot be multiplied, andDiag(dec)(which isDK × DK) cannot left-multiply aDV × DKstate. The block also introduces undefinedEV. Keep the earlier GDN state orientation and put the transpose/decay on the appropriate operands.
Zᵀ = Sᵀ GKᵀ EV × C
Rᵀ = Vᵀ − Zᵀ EV × C (in register)
Ṽᵀ = Rᵀ Aᵀ EV × C
O = GQ S + Aqk Ṽ C × EV
Sᵀ ← Diag(dec) Sᵀ + Ṽᵀ Ktᵀ EV × DK
dnn-providers/hip-kernel-provider/rocke/library/builders/gfx950/gdn/GDN_DESIGN.md:356
Γwas defined above as the cumulative decay factor, so exponentiating differences ofΓdoes not reconstructΓ_i / Γ_j; that identity applies to cumulative log-decays. Introduce a separate log-domain symbol before this formula so the stated stabilization is mathematically consistent.
Akk = (K * e^(Γ_c − Γ_ref)) (K * e^(Γ_ref − Γ_c))ᵀ
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Correct claims a code-vs-doc audit flagged: scope the decode state-pool validation to the driver/prepare() path (dispatch selects specs, not tensors); fix the tile-selection trend to match the non-monotone banded table; correct the KDA-on-GDN reuse attribution (raw-prep and the value_splits knob are pre-existing KDA; the value_splits selection table is GDN-only); reconcile the state-scan EV x DK notation with the DV x DK state; correct the triangular-solve serial-work scaling to linear in solve_block; and tighten the softplus-overflow, fusion-flag, l2norm-flag, idle-wave-Kt, and tensor-contract wordings.
… the KDA algorithm Section 5 had silently adopted kda/ALGORITHM.md's transposed state convention while sections 0 and 4 kept the DV x DK orientation that both kernels actually allocate ([pool, HV, DV, DK] for decode, [BH, DV, DK] for the prefill scan). The scan equations therefore did not type-check, and the state update had Diag(dec) on the left, where a DK x DK diagonal cannot multiply an EV x DK state. Keep the DV x DK orientation everywhere, drop the stray transposes on S, and move Diag(dec) to the right. State the S_kda = S^T mapping once so the two docs can be read together. Also replace the near-verbatim restatement of the six-tile factorization and the midpoint-factored decay with a link to kda/ALGORITHM.md, keeping only what GDN changes: the scalar gate broadcasts across DK, so Gamma is channel-constant and dec is a DK vector with equal entries. Renumber the rest of section 5 and its cross-references and the contents block. Addresses review comments r3975876924, r3975879718, r3975887752.
…-domain mapping Section 1 named no target model, so every constant in the doc -- the tile bands, the value_splits bands, the (Hv, Hk) shapes in the validation section -- had no provenance. Add 1.4 with the published config.json geometry for the two deployments this family serves, following the builders/mla/DESIGN.md precedent: a per-model row plus a named out-of-scope entry with the config keys that exclude it. The table pins three things the rest of the doc assumed silently: DK == DV on every supported row, kv_group = 2 as the only shipping GDN grouping (kv_group = 4 in section 7 is a stress point), and where each model lands in the tuned tables -- at Hv = 32 the prefill value_splits bands put batch 1-2 on 8 splits and batch 5+ unsplit, with batches 2 and 4 exactly on band edges. Also correct the log-domain mapping added in the previous commit: kda/ALGORITHM.md reconstructs the decay ratio with exp(Gc - Gref), so its Gc is the natural log of Gamma, not log2. The base-2 exponent domain there is the hardware exp2 detail (the sum is pre-scaled by log2(e)), not the symbol's definition. Addresses review comment r3975904504.
Section 1.2 gives the gate as a decay multiplier in (0, 1); section 2.1 gives KDA's bound as (lower_bound, 0) without noting that the domain changed to log space in between, so the two ranges read as a contradiction. Name the domain and give the bridge (decay = exp(gate)). Also correct 'unbounded' to 'unbounded below': GDN's gate is -exp(A_log)*softplus(.), and softplus is positive, so the value is always <= 0 -- which is what makes section 8's decay-range limit a one-sided contract. The raw per-token inputs g and a are called out as distinct from the gate value so the sentence carries no second meaning for g. Addresses review comment r3975918258.
Every arch-scoped family directory under builders/gfx950 documents itself with an ALGORITHM.md + README.md pair (attention, kda; kda adds CASE_STUDY.md). The only DESIGN.md precedent, builders/mla, is an arch-less design spike with no kernel code, so GDN_DESIGN.md took its name from the one directory whose shape does not match this one. Rename to ALGORITHM.md and state the hierarchy in the header: this file is the family entry point, README.md is host-side tooling. The GDN decode walkthrough that the kernel PR adds under the same name is folded into this file there, so the directory ends with the standard pair rather than three overlapping documents. Addresses review comment r3975928486.
The blocked-solve entry said the scalar substitution shrinks as the square of the block size. That is the per-block cost; shrinking the block also multiplies the block count, so the total is ~C*b/2 and is linear in solve_block. Halving the block halves the scalar work rather than quartering it, which is how a reader sizes the knob. Markdown only, so this PR stays docs-only. Addresses review comment r3975891138.
Four review fixes, all in gdn/ALGORITHM.md: - §7 now names the prefill head shapes it validates -- (4,4), (8,4) and (32,8) -- so §1.4's pointer at the kv_group=4 stress point resolves. Shapes match the parametrize list in test_kda_gdn_gfx950_numeric.py. - §5.5 uses EV for DV/value_splits, the name already defined in §0 and used in §5.3. ev_slice was prose-only and is gone. - §4.6 prints the batch-banded tile table instead of describing it. The non-monotone num_warps claim (4 -> 2 -> 1 -> 8) is now checkable from the rows. Values are _TUNED_TILES in dispatch/gdn/gfx950.py. The band edges are also marked as interpolated between the four measured batch anchors, since the table now prints them as concrete numbers. - §5.2 cites the code site enforcing the wave-0-only solve (the tid < 64 scf_if and _emit_idle_during_solve), and notes that §8 follow-up 4 would have to change that constraint.
TheRock Submodule Bump ActivityNewest first
|
#12070) ISSUE ID : AICK-1513 ## What this is PR 1 of a 4-PR stack that splits the GDN gfx950 work (originally one 51-file PR) into reviewable layers. This is the foundation layer: it adds a new crosslane hardware primitive and a launch-path speedup, with **no GDN code** and no dependency on the other three PRs. Stack (bottom → top): **quad_perm (this)** → dispatch.core hoist → GDN decode → GDN prefill. ## What changed **1. `quad_perm` — an intra-quad DPP permute intrinsic, added to both engines.** `quad_perm(data, [p0,p1,p2,p3])` lets lane `4q+i` read `data` from lane `4q+perm[i]` on the VALU (via `v_mov_b32_dpp` / `llvm.amdgcn.update.dpp.i32`), with no LDS crossbar and no `lgkmcnt` wait. Also adds `warp_shuffle_xor_quad`, a fast path for the two xor-masks that stay inside a quad: masks 1 and 2 lower to `quad_perm`, and **any other mask is rejected** — callers needing a wider mask use `warp_shuffle_xor`, which goes through `ds_swizzle`. - Python engine: `core/ir.py` (builder + validation), `core/lower_hip.py`, `core/lower_llvm.py`. - C++ engine (live): `core/ir/ir_flow.cpp`, `core/ir/core_types.cpp` (opcode + purity), `core/lower_hip/lower_hip_mma.cpp`, `core/lower_llvm/crosslane.cpp`, `include/rocke/ir.h`. - Coverage: Python `tests/test_rocke.py`, C++ `tests/core/future_intrinsic_lowering.cpp`, and cross-engine parity `tests/instances/parity/target_intrinsics_emit.{c,py}` (identical IR from both lowerers). **2. Kernarg-packer precompile — a launch-path speedup for every kernel.** `runtime/packing.py` gains `compile_packer(signature)`; `runtime/launcher.py` compiles the packer once at module-load and calls it per launch instead of re-deriving the layout each time. Byte-identical to `pack_args`. What it removes is **kernarg layout reconstruction** — the offset/alignment walk, the per-argument type dispatch, and the format-string assembly. It is *not* a saving on format compilation: CPython's `struct` module already caches recently used format strings, so re-packing the same format is a cache lookup, not a recompile. ## Review round 1 — what changed since the first push Four items were raised by @yraparti and @tenpercent. All four are addressed. **1. Lowerers accepted out-of-range `ctrl` (`bffb5fd911f`).** All four `quad_perm` lowering sites masked the control word with `0xFF` instead of validating it. `ctrl` packs four two-bit lane selectors (`p0 | p1<<2 | p2<<4 | p3<<6`), so `0..255` is the whole legal range — and masking silently rewrote malformed IR into a *different, valid* permute: `256` became `0` (`[0,0,0,0]`, a lane-0 broadcast) and `-1` became `255` (`[3,3,3,3]`). A wrong reduction then computed wrong numbers instead of failing. The builders already validate selectors, but IR reaching a lowerer by another route (deserialized, rewritten by a pass, hand-built) skips them. Now rejected in the Python lowerers (`ValueError`) and the C++ ones (`ROCKE_ERR_VALUE`), with the mask dropped so the check cannot be bypassed. Tests on both sides are **mutation-verified**: with the mask restored, the Python subtests and the four C++ assertions fail. **2. Wave-size semantics undocumented (`cfeec0a1315`, `8cea213942f`).** `quad_perm` makes no wave-size assumption, and both engines now say so: the control word applies within every four-lane group, and four divides both 32 and 64, so a lane never addresses outside its own quad. Wave size changes only the *number* of quads (8 in wave32, 16 in wave64). Deliberately scoped — wave-size-independent is **not** architecture-independent. The op still needs DPP-capable hardware; the useful point is that base-DPP `quad_perm` is available on CDNA where `dpp_xor`'s RDNA-only `row_xmask` is not. Also recorded: the op has no lane targeting (the control is broadcast to every quad, row/bank masks fixed at `15, 15`), so selecting a subset of quads is the caller's job. A first draft of this docstring cited `dpp_xor` as an op whose partner lane can leave the wave; `8cea213942f` corrects that. `dpp_xor` caps `xor_mask` at `1..15` and its partner stays inside a 16-lane row, which also divides both wave sizes. The accurate counterexample is `warp_shuffle_xor` at `lane_xor = 32`. **3. "The dominant Python cost" was unbacked (`786f6c827b0`).** Correct: a packing-only microbenchmark cannot establish a share of launcher overhead. Both sites now describe the mechanism instead of a magnitude -- the packer precomputes the fixed argument layout once per launcher, so a launch does not rebuild the offset table, re-dispatch on argument types, or re-assemble the format string. `struct` already caches recently used format strings, so the saving is that surrounding work rather than the format compile itself. The profiler written to measure the share now lives in its own PR (#12168, draft), and performance claims are deferred until its gate is fixed: its chunk-size-sensitivity check is a ratio, so a chunk size large enough to saturate both comparison windows accepts a back-pressured run. **4. PR description misdescribed `warp_shuffle_xor_quad`.** It said the helper "leaves wider masks on `ds_swizzle`", implying a dispatcher. It is a specialist: masks other than 1 and 2 raise. Corrected in the What-changed section above. ## Scope note (disclosed) Item 2 is functionally independent of item 1. It is kept here rather than split for one concrete reason: its byte-identity test (`test_compile_packer_matches_pack_args_byte_for_byte`) lives in `tests/test_rocke.py`, the same file that holds the `quad_perm` tests — splitting would put two PRs on one test file. Reviewers should weigh the packer's **global blast radius** (it changes the launch path for every kernel), not only its decode benefit. ## Why it's safe - No kernel IR changes — this only adds an opcode and a runtime fast path. - `quad_perm` is validated at the builder (selectors ∈ 0..3, i32-only) **and now at every lowerer** (`ctrl` ∈ 0..255), and is marked pure in both engines. - `llvm.amdgcn.update.dpp.i32` is an already-shipped overload (used by `mov_dpp8`/`ds_swizzle`), not a new intrinsic surface. - The packer is asserted byte-identical to the existing `pack_args`. ## Why it's first in the stack GDN decode's in-quad xor-butterfly reduction lowers to `quad_perm` (3 call sites), so this must land before the decode PR. It touches only `platform/`, so it reviews without any GDN context. ## Verification (run, not planned) Both engines built and exercised locally on this diff: - Python, with the C++ extension importable: `pytest tests/test_rocke.py -k "quad_perm or warp_shuffle_xor_quad or compile_packer"` → **8 passed, 0 skipped** (the cross-engine assertions no longer skip). Full file: **291 passed, 14 skipped, 43 subtests**. - C++ engine: `rocke_future_intrinsic_lowering` → **31 case(s) OK**. - Cross-engine byte-identity: `tools/check_byte_identity.py --only target_intrinsics` → **GREEN, configs=8, bad=0** — Python and C++ emit identical `.ll`. - `test_rocke_ci_static.py` **5 passed**. - Formatters: `black` clean; `clang-format` applied (local binary is v20; repo pre-commit pins v18.1.4, so CI may adjust whitespace). GPU numerical execution of `quad_perm` itself and the full all-family byte-identity gate were **not** run here. ## Stacking / base #11807 has merged, and this PR is now based directly on `develop`.
ISSUE ID : AICK-1513
What was wrong
The GDN (Gated DeltaNet) kernel family has two very different kernels and a non-obvious
relationship to the existing KDA family — GDN prefill is not a new engine, it is a mode of
the KDA chunkwise kernel — but none of that was written down. A reviewer or a future
maintainer had to reconstruct the design from the emitters: why there are two kernels rather
than one, why one of them reuses KDA wholesale and the other cannot, and which parts of the
structure are load-bearing versus tuning. That reconstruction cost is paid again by every
person who touches this family.
What we changed
Adds a single design document,
GDN_DESIGN.md, next to the GDN builders — following theexisting
builders/mla/DESIGN.mdprecedent. Documentation only: no code, no build files,no test changes. It covers:
value heads outnumber key heads because the recurrent state is per value head;
scalar per token/head versus a per-channel vector) — and what GDN mode nevertheless has to
add on top;
cannot;
Why it works
The document is written against the emitters rather than from memory, so it stays true to the
code: the tile formulas, the solve structure, the scan recurrence, the grid formulas and the
spec-validator rules are all taken from the kernels themselves. It is deliberately a
specification — it states what each kernel computes and which structural choices are
load-bearing, and it avoids recording tuning history, which would go stale.
The two claims most likely to be misread are called out explicitly rather than left implicit:
that a scalar gate is a legal value of a per-channel gate (which is what makes the prefill
reuse sound, not merely convenient), and that this relationship is asymmetric — the
general kernel can compute the special case, but a scalar-only kernel has nowhere to put a
per-channel decay, so the reverse direction is a kernel change and not a configuration.
How we validated
gdn_decode.py,kda_chunkwise.py, the GDN dispatch modules) rather than against prior notes.pre-commitpasses on the added file.no golden to re-bless; the diff is one new Markdown file.
Notes
builders/mla/DESIGN.mdconvention: the design doc lives withthe family it describes.
as a follow-up.
review history, since it is an input contract callers need to know about.
lives in the internal perf repository and the team vault, per repository compliance.