Repository navigation
feat(rocke): free-stride SDPA forward and varlen host adapters - #13953
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. |
There was a problem hiding this comment.
Review findings
Went through this independently and cross-checked against _fmha_warp_body.py, RFC 0014, and the repo history.
Agree with @AnaghaRaoAMD's review on all 5 points raised
- The fully-masked-row (
len_kv==0) claim in_sdpa_padding_mask.py— verified against_fmha_warp_body.py's epilogue: there's nol==0/row_has_massguard, so the row comes out asmean(V), notO=0. sdpa_cu_seqlensderiving offsets viacumsum(seq_lens)instead of the spec'sragged_offset— verified against RFC 0014 §4.5; this misses the "packed with trailing padding" case.- The int32 narrowing in
_validate_lengthshappening before the range checks — we found this independently too, with the same repro. - The int32 overflow in the
cu_seqlensprefix sum itself — reproduced. - The missing both-set-or-neither check on
seqlen_q/seqlen_kv— confirmed it currently passes only by accident.
Two additional findings:
1. Duplicate padding-mask/oracle logic (MEDIUM)
references.py's dense_attention_reference grows its own padding-mask implementation in the same module that already imports rocke.numeric.sdpa_reference.sdpa_reference, which already supports seq_len_q/seq_len_kv (dense) and ragged q_offsets/kv_offsets (exactly the prefix-sum format sdpa_cu_seqlens produces). The two oracles' padding semantics are already documented as deliberately diverging, and this PR widens that split rather than reusing the more complete implementation. Related to Anagha's suggestion to build one keep mask up front instead of rebuilding predicates post-hoc — same root cause: the masking logic here isn't centralized.
2. Internal ticket IDs embedded in shipped docstrings (MEDIUM)
_sdpa_padding_mask.py ("ticket 10", "ticket 8", "ST4") and _sdpa_seqlen.py ("ticket 10", "ticket 12", "SH-01", "ST4"). Tracker references belong in the commit message / PR description, not in code that ships and outlives the ticket.
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #13953 — host-side SDPA sequence-length handling (padding + ragged) — 🟠 First-round: closer look — every reviewer item is still open (no commits since 10-19) and each one checks out
Utility: 🟢 Positive in intent — useful host-side groundwork for SDPA padding and ragged inputs, but not mergeable until the reviewers' items land, the fully-masked-row spec and the ragged-offset contract above all. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted First-round review: a local qwen3.6 model made the independent first pass; the orchestrator (Claude Opus 5.5) verified it — reproduced each reviewer claim on the head, checked the kernel path and RFC they cite, ran the tests on a merge with develop, and checked CI enrollment — and owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection. |
3105f3c to
3a46d21
Compare
|
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. Please request my review again once all comments and threads are resolved. |
3a46d21 to
6fad3ac
Compare
All five shared points are fixed in the latest push: the empty-row spec now requires a row_has_mass guard and the test runs the real softmax, the adapter checks lengths as int64 and catches overflow, it enforces Q/KV both-set-or-neither, and the ragged adapter is scoped to packed-only with ragged_offset support deferred. The tracker labels are gone, and I put the oracle's masking behind a single keep mask, though moving it fully onto sdpa_reference is a bigger change I'd like to do as a follow-up. |
Every open item in your table is handled in the latest push, rebased onto current develop, and both new test files are now in the attention CPU CTest list so they run in CI. The two narrowed items are the packed-only ragged scope and the sdpa_reference consolidation, both called out as follow-ups. |
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #13953 — host-side SDPA sequence-length handling (padding + ragged) — 🟢 Follow-up: no closer look needed — every reviewer item is fixed or explicitly deferred, and the tests now run in CI; amd-khushbu's changes-requested review still needs their sign-off on the deferred oracle consolidation
No net-new findings. Utility: 🟢 Positive — host-side groundwork for SDPA padding and packed ragged inputs. The length validation is now overflow-safe, and the fully-masked-row contract is written down with a test that has teeth. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted Follow-up review: a local qwen3.6 model made the independent first pass. The orchestrator (Claude Opus 5.5) verified it. The checks were the first round's reproductions on the new head and the spec and emulator against the reviewers' items, with a negative control on the epilogue guard. They also covered the tests on a merge with today's develop and CI enrollment and failures from the logs. The orchestrator owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection. |
6fad3ac to
c2b299d
Compare
|
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. |
c2b299d to
edfd555
Compare
edfd555 to
1310155
Compare
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #13953 — free-stride SDPA forward and varlen host adapters (arai713, AICK-2518, AICK-2521, AICK-2618) — 🟢 Follow-up: no closer look needed — the four new free-stride commits leave the default kernel byte-identical and pass on gfx942 hardware in CI, but the merge stays blocked until AnaghaRaoAMD's five 10-28 threads are answered and resolved, and both approvals predate these commits
Net-new: none. Utility: 🟢 Positive — layout-general fmha_mfma plus varlen host adapters; default kernel unchanged, gfx942-verified, dispatch deferred. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted Follow-up review: a local qwen3.6 model made the independent first pass. The orchestrator (Claude Opus 5.5) verified it. The checks were an interdiff of the PR's own change since the head we reviewed ( |
1310155 to
c3bc864
Compare
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #13953 — free-stride SDPA forward and varlen host adapters (arai713, AICK-2518, AICK-2521, AICK-2618) — 🟢 Follow-up: no closer look needed — the code is unchanged by the rebase; the merge is still blocked on AnaghaRaoAMD's five 10-28 threads and a Windows CI re-run
Net-new: none. Utility: 🟢 Positive — layout-general fmha_mfma plus varlen host adapters; default kernel unchanged, gfx942-verified, dispatch deferred. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted Follow-up review: a local qwen3.6 model made the independent first pass. The orchestrator (Claude Opus 5.5) verified it. The checks were a range-diff of the rebase since the head we reviewed ( |
c3bc864 to
6a76d24
Compare
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #13953 — free-stride SDPA forward and varlen host adapters (arai713, AICK-2518) — 🟡 Follow-up: fixes in; rebase needed
Utility: 🟢 Positive — layout-general fmha_mfma plus varlen host adapters; default kernel byte-identical. 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 since |
…port - dense_attention_reference gains optional seqlen_q / seqlen_kv. None keeps the exact unpadded arithmetic. When set, keys past seqlen_kv are masked, a row with no valid key yields O = 0, and padded query rows are zeroed. - _sdpa_seqlen.py: host adapter validating hipDNN's per-sequence lengths at the boundary (integer dtype, per-batch count, 0 <= len <= S_max) and returning the flat per-batch int32 arrays the shell reads. - _sdpa_padding_mask.py: host specification of the kernel-side mask -- keep_key (the shell's extra_mask_predicate hook) and row_is_valid (the epilogue's padded-row zeroing), with the emitter form documented. The live IR emitter is deferred to the shell, where it carries its byte-identity C++ mirror. This module is the host reference it must match, cross-checked against dense_attention_reference. Kernel wiring and GPU validation wait on the shell (ST4) and the arbitrary-length buffer clipping (ticket 8).
Ticket 12 (varlen/THD forward) needs hipDNN's ragged per-sequence lengths converted into the cumulative cu_seqlens the varlen kernels (fmha_varlen) address with. Extend the shared SDPA length adapter with sdpa_cu_seqlens, which validates the per-sequence counts and returns (B+1,) int32 exclusive prefix sums (leading 0, last element = packed token total). The validator is refactored to make the upper bound optional, since ragged tokens are packed with no padding and therefore no per-sequence S_max. Host numpy only, no kernel emission. Covers the host/adapter half of ticket 12, the non-causal unified no-mask path and arbitrary-length tails are separate work.
Correct the fully-masked-row spec to require an explicit row_has_mass epilogue guard, and make the padding test run the real online-softmax recurrence with that guard plus len_kv==0 and causal cases. Scope the ragged adapter to packed-only, validate lengths on a widened int64 before narrowing, accumulate cu_seqlens in int64 with an int32 overflow check, and enforce the Q/KV both-set-or-neither pairing. Build one keep mask in the oracle, drop the internal tracker labels, and enroll both tests in the attention CPU CTest list.
Add fmha_sdpa_fwd, the net-new MFMA-body layout shell (hipDNN requirement 2), reusing mfma_attention_fwd_inner_body for the attention math and adding only the addressing layer. This scaffold covers the contiguous BHSD dense case: batch is threaded through the grid z-axis plus the inner body's K/V offset hooks, with Q/O batch folded into the tile base. BSHD, general strides, and packed QKV are declared on the spec but gated off in is_valid_spec as Phase 3. CPU-only build/smoke tests cover the validator gates, ABI signature, grid, and that a valid spec lowers to a KernelDef.
Make fmha_sdpa_fwd read Q/K/V/O from any memory layout by rebasing each tensor's base pointer to the CTA's batch slice (batch_idx * stride_*_batch) and letting the shared body address head/token through its runtime strides. Batch, head and token offsets are all runtime, so BSHD, BHSD, general B/H/S strides and packed QKV build the same kernel, with nothing baked and the contiguous Q/O fold removed. Add _sdpa_fwd_strides.contiguous_strides as the launcher's single source of truth for the (batch, head, token) element strides of the bhsd/bshd orders, plus host tests: the layout contract and the updated validator/build smoke tests.
Add an opt-in free_stride mode that rebases Q/K/V/O base pointers per batch and declares per-tensor batch strides, so one kernel reads any layout (BSHD/BHSD/general/packed) instead of the BSHD-contiguous seqlen fold.The free-stride path is Python-only for now. Retire the fmha_sdpa_fwd scaffold, add a CPU ABI test and a GPU verify harness.
Extract the verify harness core into a reusable run_verify() and add a CDNA-gated pytest (skips off gfx942/gfx950) that checks fmha_mfma(free_stride= True) against the numpy reference, confirming layout invariance of the per-batch pointer rebase. Four placements over one logical problem: contiguous BSHD and BHSD, a padded head dim ([B,H,S,D+8] sliced to D, so every stride carries the padding while the head dim keeps unit stride), and packed QKV ([B,S,3,H,D], the three operands aliasing one allocation at base offsets 0, H*D and 2*H*D). The padded and packed cases fail if fed contiguous strides or zeroed base offsets, so they test the kernel rather than numpy. All four agree bit-for-bit, which the suite asserts directly. The harness runs on the default backend rather than forcing the Python one, and bf16 is dropped from the dtype table since is_valid_spec rejects it. Verified: 11/11 on gfx942 and gfx950. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
6a76d24 to
c9121ae
Compare
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #13953 — free-stride SDPA forward and varlen host adapters (arai713, AICK-2518) — 🟡 Follow-up: clean rebase; CI running
Net-new: none. Utility: 🟢 Positive — layout-general fmha_mfma plus varlen host adapters; default kernel byte-identical. 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 and tree compare since |
…ke/dense-paged-kv Conflicts with #13205 (gfx942 bf16 cfvst, K LDS double buffering, V register prefetch) and the attention CPU test list (#13953). All hunks keep both sides: - gfx942 _async_load takes develop's kbuf slot offset and this branch's per-wave page lookup. The KDB/VPF pipeline requires persistent + cfvst, and paged K/V takes neither, so paged never reaches it. - Name-injectivity table: lds_num_buffers is buildable on develop and the paged fields are buildable here, so neither stays in the unbuildable set; both new bases (persistent KDB/VPF, paged) are kept. - The gfx942 numeric file keeps the paged class and develop's pipeline tests. - CMake: develop's SDPA host tests plus the non-Windows gfx942 dense block. No golden re-bless needed: the representative and differential goldens pass as merged.
TheRock Submodule Bump ActivityNewest first
|
JIRA ID : AICK-2518, AICK-2521, AICK-2618
Motivation
hipDNN needs attention forward to read Q/K/V from arbitrary memory layouts and to handle variable-length sequences. This PR lands the foundation for both on gfx942/gfx950.
Free strides (AICK-2618): the production forward kernel (fmha_mfma) only served the BSHD-contiguous case. It folded the batch axis in as batch_idx * seqlen_q rows, which is expressible only when the batch stride is a whole number of token strides. BHSD, general B/H/S strides and packed QKV could not be served without a host-side repack. This generalizes fmha_mfma so one kernel reads any layout.
Varlen (AICK-2518, AICK-2521): the host-side foundation for padding masks (ticket 10) and ragged / THD varlen lengths (ticket 12). The oracle can express padding, hipDNN's length tensors are validated and normalized into both the padded and the ragged forms the kernels consume, and the exact mask the kernel must emit is specified and cross-checked. The kernel-side masking lands in the free-stride path in a follow-up, together with the arbitrary-length buffer clipping it rides on (a separate ticket), so it becomes a mechanical drop-in.
Technical Details
Free-stride forward
free_stride: bool = Falsefield to FmhaMfmaSpec (defaulted, so existing descriptors / AOT packs hydrate to the current behavior).global_ptr_add(ptr, batch_idx * stride_*_batch * elem_bytes)). The shared body then addresses head and token through the runtime strides it already takes. Batch, head and token offsets are all runtime, so BSHD, BHSD, general strides and packed QKV all build the same kernel with nothing baked._fs._sdpa_fwd_strides.contiguous_stridessingle-sources the (batch, head, token) element strides the launcher passes for the bhsd/bshd orders. The earlier standalone free-stride scaffold (fmha_sdpa_fwd) is retired, now subsumed by fmha_mfma.Varlen host adapters
Test Plan
test_fmha_mfma_free_stride.py): default ABI unchanged (15 params, no batch strides), free_stride appends exactly the four batch strides (19), name tagging, builds for gfx942/gfx950 and GQA.test_sdpa_free_stride_layouts.py): every layout (BHSD/BSHD/padded/packed) recovers the same values and attention output via the kernel's scalar address formula, andcontiguous_stridesmatches numpy.test_fmha_mfma_free_stride_numeric.py, CDNA-gated): fmha_mfma(free_stride=True) matches the numpy reference across four physical placements, contiguous BHSD, contiguous BSHD, a padded head dim and packed QKV, and all four agree bit-for-bit on one logical problem.test_numeric_references.py).test_sdpa_seqlen.py).test_sdpa_padding_mask.py).tools/check_byte_identity.py).Test Result
Submission Checklist