Repository navigation
feat(rocke): row-load-order knob for gfx950 GDN/KDA decode - #14720
AviralGoelAMD merged 1 commit into
Conversation
fec1a71 to
d8e76e8
Compare
d9e5b7a to
3142f9c
Compare
d8e76e8 to
3f5cf3a
Compare
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14720 — row-load-order knob for gfx950 GDN/KDA decode (AviralGoelAMD, AICK-2556) — 🔴 First-round: blocking — the new shared HSACO helper turns on CI's GDN spill checks, and a stale 0 B expectation now fails two subtests of
|
| Prior item | Status |
|---|---|
| Reviewers | 🟢 None yet: no reviews, threads or conversation comments |
AviralGoelAMD (body, 10-30): known limitation, the rocke.dispatch.tuning request cache is keyed before knob typing; "it never returns a different kernel"; a type-aware key is a follow-up |
🟡 DEFERRED, acceptable. Real, and on develop too: resolve is an lru_cache on the request (tuning/candidate.py:194), and 4 == 4.0. The body understates it, though: a refusal is cached too. So in one process, a well-formed pin that follows an equal-valued malformed one is refused with the malformed one's reason (develop: waves_per_eu 4.0, then 4; merge: a str-subclass 'first', then 'first') (ran). No wrong kernel either way. The follow-up should key the cache on the typed knobs, or cache only successes |
| Bots / CI | 🔴 • At head 3f5cf3aa: Gate, clang-tidy, Component CI, both hipDNN superbuilds and the Multi-Arch ASAN summary pass, and so do the Linux build stages (compiler-runtime, emulation, and the gfx94X, gfx950 and gfx125X math-libs). • gfx94X hipkernelprovider fails on both shards (1 of 49 CTests): only rocke_library_gdn_pytest fails, on the two subtests in N1. The lane ran 296 spill checks that it skipped at #14632's head 12 minutes earlier (read: CI logs). • The gfx950 and gfx125X test stages are skipped at this head, so the knob's gfx950 GPU numerics ( test_gdn_decode_knobs_gfx950_numeric.py, in rocke_library_gpu_pytest) are the author's results, not CI's; on CPU, 35 knob tests skip without torch. The Windows gfx110X stage was still building at 03:00Z |
| Our checks | 🟡 • Merge (ran): clean with develop 587f9942 (22 behind, 2 ahead: #14632's 3142f9c4 plus this commit; merge d0133d91). Develop has not touched a GDN or tuning-kit file since the base. Native build OK, ctest 33/33 (1 skip). Library C++/Python parity (attention) passes at llvm20/22/23. Representative golden PASS ×3. Differential golden OK at llvm20/22/23/24 and canonical (0 changed, 0 missing; GDN decode is not in that harness). Rules 1–3 clean: the one environment read is SWEEP_JOBS in a CASE_STUDY.md script example, not a library knob, and §8 holds spill counts, no timings. • Tests (ran): the stack's 17 changed test files on CPU at the merge: 757 passed, 51 skipped, 2 failed. The two failures are N1's subtests (60 B each here). They fail on develop here too, because this host has llvm-objdump on PATH. • Knob (ran): on four single-wave fuse_conv tiles (KDA f32 at WTK 8 without conv_once, with it, and with it at waves_per_eu 4; GDN bf16 at WTK 16), the forced order that equals auto's keeps auto's name and byte-identical IR. The other order adds rlf/rll and changes the IR. On a 4-wave tile and a tile without fuse_conv, both orders leave the name and the IR unchanged. Seven malformed values ('LATE', '', 1, True, None, a str subclass, …) are refused, each with a name token of its own. The 22 new auto golden entries hash identically on develop at llvm20 and llvm22, so the 48 new cases per flavor are 26 forced-order kernels plus 22 kernels develop already emits. For a KDA f32 fused request, the registry lists 2,496 forced-order rows at production and 11,232 at full, none of them auto's order, with every kernel name unique. GDN decode has no C++ engine mirror. • Tuning kit (ran): only GDN sets outer_dependent_knobs. Attention listings (dense, unified and WMMA spaces; production and sampled full; gfx942, gfx950, gfx1151, gfx1201; 14,356 rows) are identical on develop and the merge. _typed had no other user, so exporting it as typed_knobs changes nothing else. • Keys (ran): every probed GDN config_key changes (3 of 3; the kernel names do not), and an id recorded on develop is refused as stale on the merge, not re-mapped. Nothing in the tree stores a GDN id (the tests use synthetic ones), AUTO_DEFAULTS stores knobs, and no AOT manifest or ingestor pack names GDN decode, so nothing needs regenerating here. • Land order (ran: merge-tree): after #14632, then retarget to develop. #14710 (auto-merge on) conflicts with #14632 in one comment-only hunk of platform/CMakeLists.txt (the GDN lane's comment), so whichever lands second needs a trivial rebase |
| # | Sev | file:line | Finding |
|---|---|---|---|
| N1 | 🔴 | library/tests/_gdn_decode_compile.py:29–35; test_gdn_decode_fused.py:203, :255, :257 |
Problem: the new shared helper looks for llvm-objdump on PATH, then under $ROCM_PATH/llvm/bin and /opt/rocm*. CI sets ROCM_PATH to its ROCm tree (it shows in rocke_pytest's PATH), so the GDN lane now measures scratch. At #14632's head (this PR's base), the lane skipped all 296 spill checks ("missing tool: llvm-objdump"). At this head it runs them, and 294 pass, including the knob's own no-new-spill test. Two fail: test_lowers_and_compiles_without_scratch expects 0 B for the single-wave (1, 16, 1) tile with an f32 state, fuse_conv and fuse_out_norm ((1, 16) is in _NO_SPILL_TILES), and CI's compiler spills 52 B (GDN) and 60 B (KDA) there. Our toolchain (AMD clang 24) spills 60 B in both, on develop too (ran). Consequence: rocke_library_gdn_pytest fails on both gfx94X hipkernelprovider shards at this head, and after merge it would fail for every PR. Fix: keep the helper, since CI should measure these spills, and correct the expectation instead. Take the f32-state conv + norm case at (1, 16) out of the 0 B set, or give it a spill budget as test_registered_tiles_compile does for its may_spill tiles (test_gdn_decode_spec.py:373). Adjust the held count (:257) and re-run the lane |
Utility: 🟢 Positive — sweepable row-load order with byte-exact auto; one stale spill check blocks.
Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted
First-round review: a local qwen3.6 model made the independent first pass, and the orchestrator (Claude Opus 5.5) verified it. On a clean merge with develop, it ran the native build and ctest, the attention C++/Python parity, both golden gates and the stack's 17 changed test files, and re-ran the failing fused tests on the merge. It lowered four single-wave tiles in all three orders, hashed the new auto golden cases on develop, walked the GDN registry at both sweep levels, compared attention listings on develop and the merge, replayed develop tuning ids on the merge, and probed the request cache in both orders. It read the gfx94X hipkernelprovider logs at this head, at #14632's head and on develop, and simulated merges with #14710. The orchestrator owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection.
|
I reviewed this PR again. The following review threads are still unresolved, or comments have no author reply: If you have already fixed these, please reply on each comment to say so and resolve the threads. |
3142f9c to
7436d04
Compare
3f5cf3a to
a4c6acc
Compare
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14720 — row-load-order knob for gfx950 GDN/KDA decode (AviralGoelAMD, AICK-2556) — 🔴 Follow-up: N1 unchanged, no reply
Net-new: none. Utility: 🟢 Positive — sweepable row-load order with byte-identical auto; N1's stale spill expectation still blocks. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted Follow-up review: first pass by a local qwen3.6 model, verified by the orchestrator (Claude Opus 5.5). Checks: range-diff vs our reviewed head, merge with develop, build + ctest, parity, goldens, the stack's tests, a re-run of the failing GDN test, CI at head. Flags whether a closer look is needed; not an approval or rejection. |
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14720 — row-load-order knob for gfx950 GDN/KDA decode (AviralGoelAMD, AICK-2556) — 🔴 Follow-up: CI confirms N1
Utility: 🟢 Positive — unchanged; N1's stale spill expectation blocks the merge. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted CI update to the follow-up above: the orchestrator (Claude Opus 5.5) read both gfx94X hipkernelprovider shard logs at |
A single-wave fuse_conv decode kernel loads its rows' late inputs (each
row's v conv inputs, each state chunk, the per-lane out_gate /
norm_weight) either first, ahead of the math, or where each is first
used. The emitter picks one order by a fixed rule. No order is known to
win on every shape: loading first shortens the wait before the math but
usually keeps more registers live, and only timing says which wins. This
makes the order a registry knob, so a sweep can pick it per shape for
pinned and tuned configurations. auto and the shipped defaults select
the same kernels; their ids and hashes change (see Registry).
- GdnDecodeSpec.row_loads ("auto" | "first" | "late", default "auto").
"auto" is the existing rule, now auto_late_row_inputs;
late_row_inputs is the order the emitter uses (the forced one where
row_loads_applies: one wave with fuse_conv). row_loads_overrides says
where a forced order is not auto's own for that spec (its knobs and
waves_per_eu included); only there does it change code and add its
name token (rlf / rll). A forced order equal to auto's, or on any
other spec, emits auto's kernel under auto's name. A value outside the
three strings gets a name token of its own, so it never reaches a
launcher cached under auto's name. One helper decides that, for
kernel_name and the validator alike.
- is_valid_spec refuses a row_loads that is not one of the three
strings (a non-str included), on every spec. It refuses no forced
order: a compile sweep of every single-wave fuse_conv spec family in
both orders found no build or compile failure, and where a forced
order spills more than auto's is for the sweep to rank, not a
legality rule. auto_late_row_inputs is chosen by a spill rule, not by
timing; widening it is a timing decision for the sweep tool.
- Registry: a row_loads axis (axes.py), in scope on single-wave
fuse_conv tiles (rules.py). A forced order that restates auto's is
dropped like an out-of-scope knob (GdnDecodeSpace.inert), so at each
waves_per_eu value each knob set lists one forced order, the one auto
does not choose there, and the walk never lists auto's kernel twice.
A stored tuning_id whose row_loads restates auto on another problem is
refused naming it (scope_check passes waves_per_eu to out_of_scope,
which judges the restated order only on a spec the kernel accepts, so
an out-of-range waves_per_eu is refused under its own name). The
restated order is judged on the knobs as the shared kit types them
(conv_once: 1 reads as True). The dispatch explanation names every
dropped knob, including a restating row_loads. Malformed values are
refused, in scope or not. AUTO_DEFAULTS and auto dispatch select the
same kernels as before. Adding the field changes every config_key
(the key fingerprints the knob defaults), so a tuning_id recorded
before it is refused and must be re-swept. It also changes the
spec_hash, compile_key and selection_key of every result, auto's
included (kernel name and IR are unchanged), so a cached build under
the old key is rebuilt once.
- Shared tuning kit (rocke.dispatch.tuning.space): the drop rules
(inert, base_value) may now read the outer knob. A knob set that holds
a knob listed in the new outer_dependent_knobs hook is canonicalized
at every outer value, drop rules included (per_outer), and the default
is_valid keeps a prefix that only the base's outer value drops. auto's
row_loads order depends on waves_per_eu, so without this the walk
dropped a forced order at waves_per_eu 0 and never listed it under a
floor. For knob sets without such a knob, and for drop rules that do
not read the outer knob (attention's), every listing and id is
unchanged. The kit's knob typing is exported as typed_knobs so the GDN
scope check reuses it.
Tests: validator matrix and names apart from every legal order;
row_loads_applies / overrides against the kernel name and IR in both
directions; name injectivity; the load order each forced order emits
(structural, every predicate case); registry scope, drop, refusal, the
one-forced-order walk at production and full levels, stored-id travel
(an aliased conv_once included), the dropped-knob explanation, an
out-of-range waves_per_eu on a stored id, the tuning contract kit on a
single-wave fused tile, and the per-tile configuration count;
decode_table_sweep --list-only lists both orders on a single-wave fused
tile and one elsewhere; the kit lists a knob that one outer value drops
at the other values. The three compile-to-scratch spill checks (spec, fused, KDA
resources) share one test helper (_gdn_decode_compile.py): it uses the
llvm-objdump on PATH (as analyze_hsaco does by default), and a skip
names the tool failure. A host with no usable llvm-objdump on PATH runs
no spill check.
Golden: every existing hash unchanged at llvm20/22/23; 48 new cases per
flavor (each forced order next to the auto kernel it replaces, GDN and
KDA, bf16 and f32 state, conv only, under a waves_per_eu floor, and per
gate kind at DK = DV = 64 and 256 and with an f16 state). On gfx950,
every forced order on every single-wave BPV=1 fused tile, and on a set
of them at DK = DV = 64 and 256 and with an f16 state, matches the fp32
reference and is bit-identical to the auto order's output, state and
conv state. With an f16 state the output and conv state are
bit-identical. With the per-lane norm (fuse_out_norm) the state is within
one f16 ulp: the compiler folds the final f32 -> f16 rounding of a state
element into its FMA (v_fma_mix) for a different set of elements in each
order (seen in the ISA with the ROCm 7.1.0 comgr). Without the norm an
f16 state has no v_fma_mix in either order and the same number of
v_pk_fma_f32 and v_cvt_pk_f16_f32 (the state-rounding instructions), so
the state is compared bit for bit. The numeric test checks this on gfx950. All eleven
knobs run at once; a mistyped row_loads after an auto launch in the same
process is refused.
Docs: ALGORITHM.md section 4.3 (the exception and the knob), 4.6 (scope,
listing, configuration count and the id/hash change), 4.10 (knob row,
names, the spill rule, numerics), 7 and a follow-up in 8; CASE_STUDY.md
section 8 (the spill snapshot of both orders per distinct kernel, with
its toolchain and the command that regenerates it); README and
dsl_docs/instances/gdn.md knob lists; the shared kit's outer-knob
contract in dispatch ARCHITECTURE.md section 14, the dispatch README and
the dispatcher workflow.
a4c6acc to
7e11e25
Compare
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14720 — row-load-order knob for gfx950 GDN/KDA decode (AviralGoelAMD, AICK-2556) — 🟡 Follow-up: N1 fixed, CI pending
Net-new: none. Utility: 🟢 Positive — sweepable row-load order with byte-identical auto; spill checks still skip in CI. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted Follow-up review: first pass by a local qwen3.6 model, verified by the orchestrator (Claude Opus 5.5). Checks: range-diff vs our reviewed head, merge with develop, build + ctest, parity, goldens, the stack's tests, the fused spill tests with and without |
f652bb8
into
users/avirgoel/rocke/gdn-decode-tools
…ode (#14632) ISSUE ID : AICK-2556 Follows #14353 (measured per-mode `auto` defaults), which has merged. This PR is based on `develop` and also carries the row-load-order knob (#14720), which was merged into this branch, not into `develop`; see "The row-load knob" below. `develop` has since been merged in, and the branch records the golden hashes at every flavor `develop` records. ## What this adds The gfx950 GDN/KDA decode `auto` table (`AUTO_DEFAULTS`) was chosen with a sweep that ran outside the repo, so nobody could repeat it after a kernel or knob change. This PR brings the method in tree: a sweep tool, a report tool, one shared cold-memory timing helper and one fp32 gate. It removes `tune.py`. No `auto` selection changes, and no existing golden hash changes. ## What changed | Area / files | Crux of change | |---|---| | `benchmarks/gfx950/gdn/decode_table_sweep.py` | Per decode shape: lists registered configurations, checks each against the fp32 reference, times the ones that pass in cold memory (stage 1 alone, stage 2 interleaved with `auto`), writes JSON rows. Shards long runs. Fails closed. | | `benchmarks/gfx950/gdn/decode_defaults_report.py` | Reads the rows, shows how far `auto` is from the fastest, proposes a default by the `ALGORITHM.md` §4.6 rule. Advisory; runs on CPU. | | `builders/gfx950/gdn/gdn_decode.py` | One cold-memory timing helper (`--rotate-mb`) and one NaN-safe fp32 gate, used by the sweep, both benchmarks, `--bench` and the GPU numeric tests. The tensor code stays here; the gate's pass test and the buffer-set arithmetic are imported from `dispatch/gdn/gate_rules.py`. | | `dispatch/gdn/` | `dispatch_gdn_decode_all` / `gdn_sweep_space` take `sweep_level`, `tuning_sample`, `seed`, as attention does. The argument decides; they no longer follow an ambient level. New stdlib-only `cli_args.py` holds the shared flag parsers; new stdlib-only `gate_rules.py` holds the fp32 pass test (`Gate`, `TOL`) and the cold-copy-count arithmetic, so they can be tested without torch. | | tests, docs, CMake, pyproject | New CPU tests for both tools and the helpers; `tune.py` and its test removed; two console scripts; docs name the new regen path. | - Coverage is bounded by `--sweep-level`: `production` lists `waves_per_eu` 0 and 4 only; `full` needs `--tuning-sample N` and times a random sample per tile. - The tools regenerate the evidence for `AUTO_DEFAULTS` from rocKE's own registry. They do not reproduce any external comparison. ## Why it works The sweep gates every configuration on disjoint read/write slots before timing it, so a wrong or NaN result is never ranked. Timing rotates input buffers so one cycle touches at least 1 GiB; a smaller rotation is labelled partly warm and the report withholds a proposal from it. Every row records the walk, toolchain and source sha, so a proposal can be traced to the run that produced it. ## How we validated | Gate | Result | |---|---| | GPU (gfx950, ROCm 7.13, Python lowerer) | At the last head with a code change (the newest commit only adds golden entries, checked on CPU): GDN, fused, KDA and knobs numeric suites, timing, sweep, report, KDA benchmark, golden, load-order, `dispatch/gdn` (including the new torch-free gate tests) and the tuning template pass with 0 failures. The shipped tools run end to end with exit code 0 and every row `ok`: sweep stage 1 and stage 2 on a plain and a fused shape, the report on those rows, both benchmarks (one with `--sweep-tiles`) and `--bench`. A gate forced to pass makes the verdict tests fail, and a gate forced to fail makes the sweep and the benchmark exit non-zero with nothing timed. An earlier head also passed a `--bases-only` run, a production `--shard` and a `full` sample on `kda fused f32 32/32`. | | CPU | GDN/KDA dispatch, sweep, report, timing, tuning-template and layering suites pass at this head, with and without torch installed. The pass test, the rotation sizing and what the sweep and both benchmarks do with a failing verdict are tested without torch, so they run in CI; the tensor reductions inside `gate()` run only where torch is installed (no CI lane has it). The earlier Windows-lane fix stands (a usage error prints the file path as given, not its `repr`; the tests use a backslash path so they also fail on Linux with the old text). | | IR / ABI | No default change: with the knob at its default the kernel name and IR are unchanged, and no existing golden hash changes. The golden file gains entries only: the 48 `knob_row_loads_*` cases of the row-load knob, recorded at `llvm20`, `llvm22` and `llvm23`, are also recorded at `llvm24`, the flavor `develop` records. The golden test and the C++ parity test pass with them. | | Review | rocKE checklist graded on the review-response diff (every section, A to J, every item); the sections that cover the final delta of tests and comments (C, E, G) were re-graded on it. No BLOCKER or HIGH open. Open MEDIUM and LOW items are recorded in the internal review log. | ## The row-load knob The branch also contains the `row_loads` spec field from #14720 (`auto`, `first` or `late`). It adds no kernel by default: only a forced order changes the kernel name (a tag) and its IR. Against `develop` this adds 48 golden cases per flavor (the forced orders, plus their `auto` twins, whose IR matches the pre-knob emitter), the knob's axis in the GDN tuning space, and the matching support in the shared tuning template. ## Notes - Not changed or not measured here: the in-tree sweep has not been run end to end for every `AUTO_DEFAULTS` case, and gfx942 is out of scope. - Measured results stay in the internal performance record; this PR makes no performance claim. - The comparison harness for the external-baseline table in #14353 is still out of tree.
ISSUE ID : AICK-2556
Stacked on #14632 (in-tree sweep and defaults report). Review only the top commit. The base is
users/avirgoel/rocke/gdn-decode-tools; it moves todeveloponce #14632 merges.What was wrong
A single-wave
fuse_convGDN/KDA decode kernel loads its late inputs (each row'svconv inputs, each state chunk, the per-lane norm inputs) either first, ahead of the math, or where each is first used. The emitter picks one order by a fixed rule, chosen to avoid register spills. Neither order is best on every tile, and only a sweep can tell which one is. The registry could not express the choice, so the sweep from #14632 could not rank it.What changed
kernels/gfx950/gdn_decode.pyrow_loads(auto,first,late).autois the old rule, nowauto_late_row_inputs. A forced order changes code and the kernel name (rlf/rll) only where it differs from auto's. A value outside the three strings is refused.dispatch/gdn/row_loadsis a registry axis on single-wavefuse_convtiles. A forced order that restates auto's is dropped, so each knob set lists one forced order perwaves_per_euvalue. Stored ids that no longer apply are refused by name.platform/python/rocke/dispatch/tuning/space.pyouter_dependent_knobs). Attention listings and ids are unchanged.typed_knobsis now exported.CASE_STUDY.mdsection 8, doc updates.autoandAUTO_DEFAULTSselect the same kernels as before. Adding the field changes everyconfig_key,spec_hashandcompile_key, so stored tuning ids must be re-swept and cached builds recompile once.Why it works
The knob only moves loads that already exist; the addresses, guards and operations are the same in both orders. The spill rule stays the default, so nothing changes unless a sweep or a pin asks for the other order. The registry lists exactly the order that auto does not choose, so the walk never lists the same kernel twice.
How we validated
BPV=1fused tile, and a set atDK = DV64 and 256 and with an f16 state, matches the fp32 reference. Output and conv state are bit-identical to auto's. The f16 state differs by at most one f16 ulp, and only with the per-lane norm. GDN, KDA, fused, knob, load-order, golden, table-sweep and dispatch suites pass. Mutants of the knob, name token, validator and typed restate check each fail a named test. A short sweep and report run lists both orders and passes the gate.ROCKE_BACKEND=both) andcheck_byte_identity.pyare green.Notes
autois not widened here. Widening it is a follow-up for the sweep tool.test_gdn_decode_fused.py,test_gdn_decode_spec.py,test_kda_decode_resources.py, through one shared helper) need anllvm-objdumponPATHthat can read the AMDGPU code object. The CI containers have none, so these checks are skipped there, as on the base (feat(rocke): in-tree sweep and defaults report for gfx950 GDN/KDA decode #14632); a skip names the tool failure. They were run here with ROCm'sllvm-objdumpfirst onPATH: they pass, and an injected fault in the auto load-order rule makes the spill test fail. A spill reported by one CI compiler on one tile family is being investigated separately; this change does not resolve it.rocke.dispatch.tuningis keyed on request equality before knob typing, so in one process an equal-valued request of another type (for example4.0after4, or astrsubclass after'late') can be served from the cache instead of refused. It never returns a different kernel and exists on the base forwaves_per_euand the state hints. In that case the explanation can also omit a droppedrow_loadsline. A type-aware cache key is a follow-up.