Porting existing FMHA infra from users/shumway/ck/exp-kpack to develop - #7891
Conversation
There was a problem hiding this comment.
Pull request overview
This PR ports the FMHA backward (BWD) infrastructure from an experimental branch (users/shumway/ck/exp-kpack) into develop. It adds three kernel families — OGradDotO, DqDkDv, and ConvertDQ — exposed through a layered spec/api/dev header convention (consteval factories, host-only helpers, device-only CK Tile bridges), a host-side variant registry, comprehensive GoogleTest unit/death tests, and CMake-driven compile-fail tests for the consteval validators.
Changes:
- New
rocm_ck/ops/fmha_bwd/*header trio per kernel family plus sharedcommon.hpp/grid_dim.hpp/ck_type_map.hpp, with constevalmakeSpec()validators and named tensor/scalar slot constants. - New variant registry header and
findVariant()/ name-lookup helpers for programmatic kernel selection. - New unit, consteval, death, compatibility, and compile-fail tests wired into the existing CMake test targets.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| include/rocm_ck/grid_dim.hpp | HIP-free GridDim type. |
| include/rocm_ck/ck_type_map.hpp | Device-only DataType → CK Tile type map. |
| include/rocm_ck/ops/fmha_bwd/common.hpp | Shared FmhaMode / FmhaBiasType enums. |
| include/rocm_ck/ops/fmha_bwd/ograd_dot_o_{spec,api,dev}.hpp | OGradDotO kernel spec, host helpers, and CK Tile bridge. |
| include/rocm_ck/ops/fmha_bwd/dqdkdv_{spec,api,dev}.hpp | DqDkDv kernel spec, host helpers/validator, and CK Tile bridge. |
| include/rocm_ck/ops/fmha_bwd/convert_dq_{spec,api,dev}.hpp | ConvertDQ kernel spec, host helpers, and CK Tile bridge. |
| tests/unit/fmha_bwd_registry.hpp | Variant registry + consteval name lookup. |
| tests/unit/unit_fmha_bwd_common.cpp | Tests for shared enums and GridDim. |
| tests/unit/unit_fmha_bwd_compat.cpp | Frozen-baseline + registry tests across all three families. |
| tests/unit/unit_fmha_bwd_consteval.cpp | SWA/CMaskBR consteval spec/slot tests. |
| tests/unit/unit_fmha_bwd_convert_dq.cpp | ConvertDQ spec/slot/grid tests. |
| tests/unit/unit_fmha_bwd_dqdkdv.cpp | DqDkDv spec/slot/grid tests. |
| tests/unit/unit_fmha_bwd_ograd_dot_o.cpp | OGradDotO spec/slot/grid tests. |
| tests/unit/unit_fmha_bwd_validate_args.cpp | Death tests for the debug-mode DqDkDv validateArgs. |
| tests/compile_fail/*.cpp + CMakeLists.txt | Compile-fail coverage for consteval validators. |
| tests/CMakeLists.txt | Adds new unit sources to the unit-test target. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bidlekm
left a comment
There was a problem hiding this comment.
LGTM! The FMHA BWD has a lot of pr-s merged, do you think it would be better to push this to production like it is now, or to push it with the merges?
|
No additional code changes needed on the productionize branch. The only new exp-kpack content since our port (96bd1a114bd) that touches headers/non-compat-tests was #7534 , and it's already present. Specifically the new PRs to port (post 96bd1a1, touching FMHA headers/tests):
|
There was a problem hiding this comment.
Thanks for getting this onto develop! I went through it carefully and diffed it against exp-kpack — overall this is a clean, well-structured port, and the ops/fmha_bwd/ layout + test wiring fit develop's conventions nicely. No blocking correctness issues; a few things worth a look before merge, plus inline notes on specific lines.
On "does this carry everything from exp-kpack?" — I verified the headers are a faithful snapshot of exp-kpack @ #7534 (2026-05-28): the _dev/_api headers are byte-identical, and _spec/common.hpp differ only by the datatype_utils.hpp→datatype.hpp include renames. So nothing was dropped — it's just that three FMHA commits landed upstream after the snapshot and aren't here yet:
#7538— consteval tile-config table (06-01)#7274—FmhaMaskTypeenum (06-03)#7928— CK-Tile Arg-struct sync (06-03)
Could we make the snapshot scope explicit in the title/description and track those three as catch-up? One call worth making now: #7274 replaces the public has_mask bool with a FmhaMaskType enum and it merged today — re-cutting to include it would avoid a breaking field change on develop later.
Docs — the PR adds a whole new ops/ subtree, but README.md is unchanged: its Directory-layout tree and Status blurb are still flat/GEMM-only, and it documents a single unified Signature model that FMHA intentionally breaks. Worth updating; and a short ops/fmha_bwd/README.md carrying the exp INSIGHTS.md rationale (per-kernel Signature, PhysicalTensor-rejected, pad_hdim=8, TrLoad/IsWG32 future) would stop a future maintainer from "fixing" intentional choices.
Device-bridge catch-up (heads-up, not blocking) — the _dev.hpp headers are dormant on develop (no .hip instantiations ship), but they're one CK-Tile-sync generation behind tip (pre-#7928/#6152/#5504), so their Kargs aggregate-inits would mis-map against current CK Tile. They'll need a rebase onto exp-tip before any kernel instantiation lands — the __AMDGCN_WAVEFRONT_SIZE assert (inline note) is the first thing that'll trip on clang ≥ 23.
Test note — the mask int → ck_tile::GenericAttentionMaskEnum contract is live at runtime (read from the MASK_TYPE scalar slot) but nothing pins the canonical integer values the host packer must write; a tiny mapping test would lock it down.
A handful of smaller Suggestions (unused <cstdint>, grid_dim.hpp Role: header, include sorting, slot-count canaries, asymmetric-hdim handling, etc.) I've kept out of the thread to reduce noise — happy to share the full list. Nice work overall; the structure is solid. 🙏
Signed-off-by: Chris Tsiaousis <chris.tsiaousis@streamhpc.com>
The global link_libraries(hip::device) in CK's top-level CMakeLists.txt injects -x hip via INTERFACE_COMPILE_OPTIONS, forcing amdclang++ to run a device compilation pass on every TU — including the pure-host rocm_ck unit tests. The device pass defines __HIP_DEVICE_COMPILE__, which triggers the #error guards in host-only _api.hpp headers. Add --offload-host-only to the rocm_ck_unit target (guarded by if(TARGET hip::device)) so the device pass is skipped for these host-only tests. Signed-off-by: Chris Tsiaousis <chris.tsiaousis@streamhpc.com>
Signed-off-by: Chris Tsiaousis <chris.tsiaousis@streamhpc.com>
225b42b to
7e21ee1
Compare
Signed-off-by: Chris Tsiaousis <chris.tsiaousis@streamhpc.com>
f1255bb to
a596aa4
Compare
Second-round /ck-code-review fixes for the FMHA BWD port: - convert_dq_api validateArgs: fix out-of-bounds read of tensor_names[] (6 entries) for GROUP mode, which needs 8 (requiredTensors==8). Use a per-mode, correctly-sized name table; also fixes the wrong BATCH-mode slot-2 name (was "SEQSTART_Q", is NSPLITS). - dqdkdv_dev: drop two phantom positional initializers (stride_dq_acc, nhead_stride_dq_acc) from the FmhaBwdCommonKargs aggregate-init. The bundled CK Tile struct has 30 fields, not 32 -- dq_acc strides are derived in-kernel. 32-initializer list was ill-formed once instantiated. - ograd_dot_o_dev: reorder the FmhaBwdOGradDotOCommonKargs aggregate-init to the current (post-#5504) layout -- lse/sink/d_sink pointers belong after d_ptr, and nhead before stride_do. The old layout placed p_undrop (float) into the lse_ptr (const void*) slot -> ill-formed init. - Cardinal-rule (ASCII-only) fixes: replace em-dashes with -- in ck_type_map.hpp, dqdkdv_spec.hpp, and two FMHA unit tests. - ck_type_map.hpp: use <ck_tile/core.hpp> angle include (W9 straggler). Verified: rocm_ck host suite (401 unit + 58 compile-fail) passes under -Werror (Clang 23); device bridges compile clean against bundled CK Tile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Porting existing FMHA infra from users/shumway/ck/exp-kpack to develop (#7891) Signed-off-by: Chris Tsiaousis <chris.tsiaousis@streamhpc.com> Co-authored-by: Adam Osewski <Adam.Osewski@amd.com>
#7891) Signed-off-by: Chris Tsiaousis <chris.tsiaousis@streamhpc.com> Co-authored-by: Adam Osewski <Adam.Osewski@amd.com>
Porting existing FMHA infra from users/shumway/ck/exp-kpack to develop Signed-off-by: Chris Tsiaousis <chris.tsiaousis@streamhpc.com> Co-authored-by: Adam Osewski <Adam.Osewski@amd.com>
No description provided.