Repository navigation
feat(hip-kernel-provider): port the rocKE gfx942 tiled-2D mask-limit, sliced-K ring, phase split and HBM-direct K/V to C++ - #14526
Conversation
CongMa13
left a comment
There was a problem hiding this comment.
Two actionable findings are left inline.
| int shp[3] = {K_BUFS, T, K_SLICE_HD}; | ||
| ctx->K_lds = rocke_b_smem_alloc(b, K_LDS_DTYPE, shp, 3, "KldsS"); | ||
| } | ||
| else if(spec->use_k_hbm_direct) |
There was a problem hiding this comment.
[P2] K-direct mode aliases Q into the one-element K LDS stub
With use_k_hbm_direct=true and use_q_direct_global=false, Q_ALIAS_K can still become true because its size check uses the full logical K buffer size. This branch allocates K_lds as [K_BUFS, 1, 1], so the Q gather then indexes the one-element stub as [BLOCK_M, HD]. The added parity configurations 29 and 30 both exercise this combination. Please keep Q from aliasing the K stub, either by forcing direct Q loads or allocating a full Q LDS buffer.
| rocke_value_t* in_values[3] = {ctx->kv_tile_iv, lin, ctx->kv_head_idx}; | ||
| rocke_value_t* vo = NULL; | ||
| rocke_value_t* vl = NULL; | ||
| if(!rocke_transforms_descriptor_offset(b, ctx->kv_desc, in_names, in_values, 3, &vo, &vl)) |
There was a problem hiding this comment.
[P2] Direct HBM readers do not honor i64 KV addressing
This direct-K path obtains one descriptor offset and passes it to buffer_load_vN, but does not use the i64 split-offset path. The direct-V helper does the same before its global_load. A spec can currently combine either direct-HBM flag with use_i64_kv_addr=true; for caches beyond the i32 offset range these loads can address the wrong pages. Please reject those combinations until both readers handle split 64-bit offsets.
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14526 — port the rocKE gfx942 tiled-2D mask-limit, sliced-K ring, phase split and HBM-direct K/V to C++ (amd-khushbu, AICK-2712) — 🟠 First-round: not merge-ready — rebase needed — conflicts with develop (its own golden hunk is a plain union); the khbm Q-alias bug is ported from Python
Net-new: none. Utility: 🟢 Positive — C++ now builds gfx942's production flash prefill specs; untested in CI. 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. The PR conflicts with develop, so there is no merged tree yet; the build, byte identity, goldens and tests wait for the rebase. The orchestrator simulated the merge of every stack PR with |
… sliced-K ring, phase split and HBM-direct K/V to C++ JIRA ID : AICK-2712 ## Summary This is PR 5 of a stack of 7 that brings the rocKE C++ unified-attention engine to parity with Python (see the stack below). It sits on PR 4. The gfx942 fp16/bf16 flash prefill specs Python selects use the transposed-32x32 VALU stack (mask-limit, invariant hoist) and the sliced-K ring. The C++ builder rejected both, after PR 1 made it stop silently ignoring them. This PR ports them, plus the causal mask phase split and HBM-direct K/V, so C++ builds the production gfx942 flash specs byte-identically. ## Changes All paths are under `dnn-providers/hip-kernel-provider/rocke/platform/cpp` unless noted. - **Mask-limit and invariant hoist.** - The per-lane query-row invariants (and the ALiBi slope) are hoisted before the KV loop. - The mask-limit threshold compare honours `skip_mask`. - The ALiBi slope is picked as Python picks it; plain transposed + ALiBi previously failed to build. - The first parity configs to combine the production flash stack exposed other gaps on this path, fixed here: sinks seeding under transposed scalar state, the K single-buffer and split-cfvst branches, and C argument-order drift at four call sites. - **Sliced-K ring (depth 2 or 3).** - The transposed QK streams K in `k_slice_hd`-wide slices through `ring_depth` LDS slots, with `ring_depth - 1` slices in flight behind a partial `vmcnt` fence. - When a prefetch targets the slot the previous slice was read from, the slot is drained before reuse. The depth-3 ring uses the CK LdsSeq slot maps. - Slice geometries Python rejects are rejected the same way. - **Causal mask phase split, V/K from HBM.** `use_causal_mask_phase_split`, `use_v_hbm_direct` and `use_k_hbm_direct` are added to the shared spec, defaulted off; gfx950 ignores them. Their name tokens (`cmps`, `vhbm`, `khbm`) match Python. - **Gate:** the unified tiled-2D gate no longer rejects the gfx942 flash mask-limit and sliced-K ring cohorts. - **Stopgap: 64-bit KV offsets with the sliced-K ring.** - Develop (#14349) ported the gfx942 2D per-page i64 K/V loads. The ring's K slice loader still builds one i32 offset, so the C++ IR would differ from Python's and overflow at run time. - Until that is ported, the builder rejects `use_i64_kv_addr` with an active ring (NOTIMPL), and the 2D gate rejects those problems as "not ported to C++ yet". The gap is recorded in `known_gaps.md` §9. - A sweep of every i64 problem Python's 2D gate admits, run at the top of the stack: 810 specs, of which 558 are identical and the 252 ring specs are rejected. Co-Authored-By: Claude <noreply@anthropic.com>
fb4e246 to
f87527f
Compare
30d8f00 to
8c8e442
Compare
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14526 — port the rocKE gfx942 tiled-2D mask-limit, sliced-K ring, phase split and HBM-direct K/V to C++ (amd-khushbu, AICK-2712) — 🟠 Follow-up: rebase fixed the conflicts; the khbm Q-alias is still open
Net-new: none. Utility: 🟢 Positive — C++ builds gfx942's production flash specs byte-identically; untested in CI. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted Follow-up review: a local qwen3.6 model made the independent first pass, and the orchestrator (Claude Opus 5.5) verified it. The checks started with a range-diff of the rebased stack against the head we reviewed. On the merge with develop |
f87527f
into
users/khuagarw/rocke/aick-2712-cpp-parity-1-stopgaps
|
This stack is now combined into one PR, #14522, so it can be reviewed and merged as one change. GitHub shows this PR as merged because its commit is now on #14522's branch; nothing has gone into develop. This PR's change is one commit there, unchanged. Open review comments from here are listed in #14522's description and will be answered there. |
JIRA ID : AICK-2712
Summary
This is PR 5 of a stack of 9 that brings the rocKE C++ unified-attention engine to parity with Python (see the stack below). It sits on PR 4.
The gfx942 fp16/bf16 flash prefill specs Python selects use the transposed-32x32 VALU stack (mask-limit, invariant hoist) and the sliced-K ring. The C++ builder rejected both, after PR 1 made it stop silently ignoring them. This PR ports them, plus the causal mask phase split and HBM-direct K/V, so C++ builds the production gfx942 flash specs byte-identically.
Changes
All paths are under
dnn-providers/hip-kernel-provider/rocke/platform/cppunless noted.skip_mask.k_slice_hd-wide slices throughring_depthLDS slots, withring_depth - 1slices in flight behind a partialvmcntfence.use_causal_mask_phase_split,use_v_hbm_directanduse_k_hbm_directare added to the shared spec, defaulted off; gfx950 ignores them. Their name tokens (cmps,vhbm,khbm) match Python.use_i64_kv_addrwith an active ring (NOTIMPL), and the 2D gate rejects those problems as "not ported to C++ yet". The gap is recorded inknown_gaps.md§9.Stack
Each PR is based on the one before it. Review from 1 upward; each diff shows only its own slice.
Test plan
test_attention_routing_cpp_parity.pyandattention_unified_routing_test.cpp: the newly admitted cohorts, and the i64 + ring reject reasontest_attention_binding_spec_fields.py: the new fields are read rather than rejectedgfx942_attention_tiled_2d11–30 blessed; no other attention entry moves🤖 Generated with Claude Code