Skip to content

fix(rocke): gfx942 dense KDB/VPF LSE max, knob types, shared softmax/PV (child of ROCm#13205) - #4

Merged
Parthiv911 merged 2 commits into
Parthiv911:users/Parthiv911/rocke/gfx942-dense-cfvst-kdb-vpffrom
AviralGoelAMD:users/avirgoel/rocke/pr13205-fixes
Oct 12, 2026
Merged

Parthiv911 merged 2 commits into
Parthiv911:users/Parthiv911/rocke/gfx942-dense-cfvst-kdb-vpffrom
AviralGoelAMD:users/avirgoel/rocke/pr13205-fixes

Conversation

@AviralGoelAMD

Copy link
Copy Markdown

JIRA ID: AICK-1900

@Parthiv911, this is a child PR for ROCm#13205. It targets your PR branch, so merging it here updates ROCm#13205. Please review it before you take it in. I have already pushed one commit to your branch: a merge of develop (18f75d42f4a). That merge resolves the conflict in test_attention_dense_gfx942_numeric.py: both your strict test and develop's LSE tests are kept, each with its own GPU guard. The two commits in this PR come after that merge.

What was wrong

  • Wrong LSE after the develop merge. develop's feat(rocke): add optional LSE output to gfx950 and gfx942 dense attention ROCm/rocm-libraries#13140 added an optional LSE output. Its epilogue reads loop.results[0] as the final row max. The K-double-buffer pipeline adds its last KV tile after the loop, so with emit_lse=True, lds_num_buffers=2 the LSE paired a stale max with the final sum. On gfx942 silicon the LSE was off by 1.18 at S=512 and by 6.9e29 at n_up == 1, where the stale max is the −inf seed. The output O was still correct.
  • Knob values coerced instead of rejected. use_cfvst was read through bool(), so use_cfvst="false" built a CFVST kernel. The pad resolver used is True, so the pad and the CFVST decision could disagree.
  • Untested configs reachable. The bf16 CFVST gate opens many knob combinations that the tuning sweep generates, but only the default-knob, block_n=64 row had numeric tests.
  • Duplicated tile math. finish_pipeline_tile copied tile_body's softmax/PV block. That copy is how the LSE state diverged.

What changed

Area / files Crux of change
kernels/gfx942/attention_dense.py The epilogue reads the final row max on both paths. __post_init__ rejects a non-bool use_cfvst and a non-int lds_num_buffers. The pad branch uses resolved_use_cfvst(). A single softmax_pv helper serves tile_body and the pipeline, replacing the copy.
tests/test_attention_dense_gfx942_numeric.py test_cfvst_knob_matrix_numeric: 15 knob rows × lds_num_buffers 1/2. test_kdbvpf_pipeline_lse: zero-trip and multi-tile, on both grids.
tests/test_attention_dense_gfx942.py A CPU test that wrong-type knob values raise.
tests/test_attention_dense_gfx942_golden.py, golden/*.json 5 new IR golden cases: CFVST, the pipeline at block_n 64/32, the persistent pipeline, and LSE × pipeline. That is 15 entries across LLVM 20/22/23.
  • Every existing kernel emits byte-identical IR. Only the LSE × pipeline kernels change, and that change is the fix.
  • No default or tuning-policy changes.

Why it works

Both branches now leave the final running max in m_i, and lse_store reads m_i. On the single-buffer path that is still loop.results[0], so its IR does not change. Because both paths call the same softmax_pv, they cannot drift apart in mask, exp2, rescale or PV handling. Rejecting wrong types in the spec stops a mistyped value from silently selecting a different kernel.

How we validated

Gate Result
Correctness (MI300A, gfx942, 228 CU) 105 passed: knob matrix 30/30 and pipeline LSE 4/4, both at bf16 < 1e-2 against an fp32 SDPA reference with LSE < 1e-3; your strict test; develop's full LSE class. The LSE error at S=512 and at n_up == 1 dropped from 1.18 / 6.9e29 to 9.5e-7 / 4.8e-7.
Merge only (18f75d42f4a) 71/71 on the same node: your strict test and develop's LSE class.
Regression (host) 555 passed, 0 skipped: dense gfx942 spec/LDS/golden, LSE, tuning space, LSE dispatch, C++/Python IR parity (run with a built rocke_engine).
IR All pre-existing golden entries unchanged in every flavor. A base-vs-fixed A/B over 240 specs: 161 identical, 76 rejected by both, 3 changed (all LSE × pipeline). Removing the pipeline's WAR barrier makes exactly the 3 pipeline cases × 3 flavors drift.

Notes

  • Tolerance margin: bf16 O error is about 9.5e-3 against the 1e-2 bound. The knob matrix passed on MI300A but has not run on MI300X (304 CU). If it proves tight there, compare the pipeline output against the same config at lds_num_buffers=1 instead of loosening toward 4e-2.
  • Not in this PR (review follow-ups for perf(rocke): add opt-in gfx942 BF16 D128 K LDS double buffering and V register prefetch ROCm/rocm-libraries#13205): label the shortcut rejections NOT_YET_IMPLEMENTED, share the gate predicate between supports_attention_dense and dense_rules.py, fix the stale _use_v_swizzle docstrings and vswz1 tag, and update dispatch/AGENTS.md:186. Also note in the PR description that the production sweep grows: fp16 D128 goes from 98 to 182 specs.

- The K-double-buffer pipeline folds its last KV tile in after the loop, but the
  LSE epilogue (develop #13140) read the loop's stale row max. The epilogue now
  reads the final max on both paths; the single-buffer IR is unchanged.
- Spec rejects non-bool use_cfvst and non-int lds_num_buffers instead of
  coercing them; the V^T pad branch reads the resolved CFVST decision, so the
  pad and the CFVST choice can no longer disagree.
- One softmax_pv helper serves tile_body and the pipeline (byte-identical IR).
- Numeric coverage for every knob the bf16 CFVST gate opens, with and without
  the pipeline, plus an LSE x pipeline case (zero-trip and multi-tile, both
  grids); CPU test for the type rejections.
No default-knob case reaches the opt-in bf16 CFVST body or its K-double-buffer
+ V-prefetch pipeline, so their IR had no drift tripwire. Adds CFVST, pipeline
(block_n 64/32, persistent) and LSE x pipeline cases for every LLVM flavor.
Existing entries are unchanged.
@Parthiv911
Parthiv911 merged commit e151739 into Parthiv911:users/Parthiv911/rocke/gfx942-dense-cfvst-kdb-vpf Oct 12, 2026
23 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants