Repository navigation
fix(rocke): f64 follow-ups: fabs for abs, 8-byte scalar alignment, mean as a division - #13887
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
There was a problem hiding this comment.
🟡 Changes recommended
The new C++ shared-memory test emits a store whose claimed alignment exceeds its address alignment.
1 open issue
What changed in this PR
Refines rocKE’s FP64 support with matching Python and C++ changes.
Changes:
- Uses native f64 absolute value and direct division for means.
- Adds 8-byte scalar memory alignment.
- Strengthens regression tests and updates support documentation.
Paths below are relative to dnn-providers/hip-kernel-provider/rocke/.
| File | Description |
|---|---|
platform/tests/test_rocke.py |
Tests f64 arithmetic, alignment, and rejection reasons. |
platform/tests/instances/parity/elementwise_emit.py |
Adds Python f64 abs parity case. |
platform/tests/instances/parity/elementwise_emit.c |
Adds matching C parity case. |
platform/tests/core/test_smem_liveness_packing_extra.py |
Tests f64 shared-memory alignment. |
platform/tests/core/test_f64_lowering.cpp |
Adds C++ f64 regression coverage. |
platform/tests/core/test_amdgpu_lowering_ops.py |
Tests fabs lowering and exact rejections. |
platform/python/rocke/instances/common/reduce.py |
Divides f64 means and aligns output stores. |
platform/python/rocke/instances/common/elementwise.py |
Uses fabs for f64 abs. |
platform/python/rocke/helpers/tensor_view.py |
Aligns scalar f64 accesses. |
platform/python/rocke/core/lower_llvm.py |
Supports f64 fabs; corrects alignment docstring. |
platform/python/rocke/core/lower_hip.py |
Emits double fabs. |
platform/python/rocke/core/ir.py |
Adds typed f64 load helper. |
platform/python/rocke/core/arch/data/intrinsic_arch_domain.llvm23.json |
Records LLVM 23 f64 fabs support. |
platform/python/rocke/core/arch/data/intrinsic_arch_domain.llvm22.json |
Records LLVM 22 f64 fabs support. |
platform/python/rocke/core/arch/data/intrinsic_arch_domain.llvm20.json |
Records LLVM 20 f64 fabs support. |
platform/dsl_docs/reference/op_vocabulary.md |
Updates supported operations and load helpers. |
platform/dsl_docs/reference/dtype_support.md |
Corrects f64 support details. |
platform/dsl_docs/ir_lowering/ir_model.md |
Documents f64 lowering and loads. |
platform/cpp/instances/common/reduce.cpp |
Mirrors mean and store-alignment changes. |
platform/cpp/instances/common/elementwise.cpp |
Mirrors f64 abs selection. |
platform/cpp/include/rocke/ir.h |
Declares typed f64 load helper. |
platform/cpp/helpers/tensor_view.cpp |
Mirrors scalar f64 alignment. |
platform/cpp/core/lower_llvm/data.cpp |
Declares f64 fabs intrinsic. |
platform/cpp/core/lower_llvm/arith.cpp |
Enables f64 fabs lowering. |
platform/cpp/core/lower_hip/lower_hip_arith.cpp |
Emits double fabs in C++. |
platform/cpp/core/ir/ir_mem.cpp |
Implements typed f64 load helper. |
Review effort: Balanced
|
🎉 All checks passed! This PR is ready for review. |
75cfbbd to
5ddca72
Compare
The fabs builder op now lowers f64 in both engines: the LLVM lowerer emits llvm.fabs.f64 and the HIP lowerer emits the double fabs overload, so the value stays in double instead of going through the f32 helper. Other dtypes and the remaining f64 rejections are unchanged. The elementwise abs op uses fabs for f64, a single sign-bit clear, instead of max(x, -x), whose maxnum needs several instructions under IEEE mode. f32, f16 and bf16 keep max(x, -x), so their IR is unchanged. The llvm20, llvm22 and llvm23 intrinsic arch-domain columns gain the measured fabs.f64 row. An f64 abs elementwise parity config covers the new emission in the byte-identity gate. Tests check both lowerings of f64 fabs in Python and C++, and that the f64 abs kernel calls llvm.fabs.f64 with no maxnum. The op vocabulary, IR model and dtype support docs list fabs as accepting f64. Co-Authored-By: Claude <noreply@anthropic.com>
Scalar f64 global loads and stores claimed align 1, so their good codegen relied on the target's unaligned-access mode. They now claim the 8-byte element alignment in both engines. The IR builder gains global_load_f64 (default align 8), next to the other typed global loads. TensorView.load_scalar uses it for f64, and TensorView.store_scalar defaults f64 stores to align 8; other dtypes keep their existing alignment. This covers the f64 elementwise scalar tail. The reduce2d f64 output store passes align 8 explicitly. Tests check the new typed load's alignment and that every scalar f64 global access of the elementwise tail and the reduce output store claims align 8, in Python and C++. The IR model and op vocabulary docs list global_load_f64. Co-Authored-By: Claude <noreply@anthropic.com>
The reduce2d f64 mean was computed as sum * (1/N), which rounds twice. It now divides the sum by N, a single rounding, in both engines. f16 and bf16 keep sum * (1/N) in f32: the extra f32 rounding is far below the precision of their narrow output, and an f32 division would expand into the full division sequence. Their IR is unchanged. Tests check, in Python and C++, that the f64 mean with N=3072 (no exact reciprocal) emits one fdiv double of the sum by N and no fmul double. Co-Authored-By: Claude <noreply@anthropic.com>
test_f64_path_builds only checked that "double" appears in the reduce IR, which the f64 input load alone satisfies, so an f32 accumulator would pass. It now checks, for sum, max and mean, that the combine step is a double op (fadd double, llvm.maxnum.f64), that no float combine appears, that the LDS pool holds one 8-byte partial per thread, and that every LDS store writes a double. Co-Authored-By: Claude <noreply@anthropic.com>
The f64 rejection tests checked only the exception type, or discarded the is_valid_spec reason, so a rejection raised for an unrelated reason would pass. They now pin the whole message. - TestHipF64Rejection.test_unsupported_ops_raise lists each op's exact HIP and LLVM lowering message and matches both in full. - The elementwise and reduce f64 is_valid_spec tests assert the reason string for each rejected op and for vec=4. Co-Authored-By: Claude <noreply@anthropic.com>
… docstring The f64 rejection lists in op_vocabulary.md, ir_model.md and dtype_support.md said tanh and exp2_fast reject f64 in both lowerings. tanh accepts only f32 and is rejected by the builder before any lowering; exp2_fast rejects f64 in the LLVM lowering and has no HIP lowering for any dtype. The lists now say so. The _compute_smem_layout docstring described the old alignment rule (16 bytes for byte-element types, 4 otherwise); it now also states the 8-byte alignment of f64 allocations. New tests pin that rule in both engines: a live 2-byte f16 allocation followed by an f64 allocation places the f64 buffer at byte 8, not at byte 4 (Python packer test and a C++ ctest that checks the emitted LDS pool offset). Co-Authored-By: Claude <noreply@anthropic.com>
The C++ test of the 8-byte f64 LDS alignment stored a two-double vector with a 16-byte alignment claim at byte offset 8, which the allocation does not guarantee. It now stores one double loaded with its 8-byte alignment, and checks that the LDS store claims 8 bytes. Co-Authored-By: Claude <noreply@anthropic.com>
5ddca72 to
41b79ae
Compare
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #13887 — f64 follow-ups: fabs for abs, 8-byte scalar alignment, mean as a division — 🟢 First-round: no closer look needed — engines byte-identical; f64 abs bit-exact on gfx1151
Net-new: none. Utility: 🟢 Positive — tidy both-engine f64 follow-ups: one sign-bit clear instead of three max ops, truthful 8-byte alignment claims, a single-rounding mean, sharper tests and corrected docs. 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 on hardware — merge with develop, native build + ctest, the byte-identity gate, the f64 tests, and f64 abs on a gfx1151 GPU — and owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection. |
CongMa13
left a comment
There was a problem hiding this comment.
No new issues found. I agree with the following existing comments:
- Test uses misaligned two-double store at offset 8: @copilot-pull-request-reviewer[bot] (#13887 (comment))
CongMa13
left a comment
There was a problem hiding this comment.
No new P0 or P1 issues were identified during this review. The changes are approved.
TheRock Submodule Bump ActivityNewest first
|
The differential golden had gone stale, because nothing ran the check: 675 of its 1116 LLVM IR entries (compared at llvm20) and 608 of its 1089 canonical IR entries no longer match the tree. Recorded afresh in the new format, with the Python reference emitters, from a tree whose two-engine gate is green at llvm20, llvm22 and llvm23: - ll at llvm20 and llvm22: 82 families, 1474 configs per flavor. At llvm23: 83 families, 1578 configs, where gfx1250_lowering has configs that need LLVM 23 and are refused below it, and every conv_direct_dgrad_wmma config needs LLVM 23. - ir_canonical: 82 families, 1561 configs (conv_abi's emitter has no IR mode). Canonical IR was checked to be identical at llvm20 and llvm23. What the re-bless absorbs: every mismatch that could be traced is an intended code generation change that did not re-bless this file -- the shared LDS pool (#8844, about 425 configs), the ds.swizzle declaration (#9583, about 177), renamed values in the attention transpose reads (#9133, 57), the smaller LDS pool (#10112, about 39), the flat work group size of one-wave wave32 kernels (#12710, about 39), #9220 (6) and the conv changes of #13109 (6). 441 entries were already stale at the start of the history available here and cannot be attributed; no unexplained change was found after it. The two repeated keys (conv_direct_grouped, conv_wgrad_workspace_reduce), which hid their newer copies, are gone. It also takes in the code generation changes merged since then, each traced by recording the file at the commit and at its parent; the count is per section unless a flavor is named: - the magic division sum emitted without signed-overflow poison (#13802): 215 configs in 19 families -- attention_unified 6, attention_unified_stress 32, conv_direct_grouped 16, conv_implicit_gemm 26, conv_implicit_gemm_dgrad 31, conv_implicit_gemm_wgrad 42, deep_fused_conv_pool 2, gfx1201_deep_fused_conv_pool 5, gfx950_deep_fused_conv_pool 5, dynamic_helpers 2, fmha_splitkv_decode 6, img2col 5, moe_fused_mega 5, moe_gemm_fused 6, moe_sorting 6, pooling 4, sage_attention 4, sparse_attention 6, transpose 6; - attention decoding on gfx942 and gfx950 (#13492): 23 changed and 25 new configs in attention_strided_kv, gfx942_attention_tiled_3d and gfx950_attention_tiled_3d; - the f64 follow-ups (#13887): 3 changed and 1 new elementwise config, 5 changed reduce configs; - MX scale and fp6 decoding (#13885): 6 mx_gemm configs; - the gfx950 conv dgrad speed-up (#13075): 36 new conv_direct_dgrad_ext configs; 22 changed, 15 new and 2 removed conv_implicit_gemm_dgrad configs; 2 changed and 3 new conv_direct_grouped configs; 2 new dynamic_helpers and 3 new target_intrinsics configs; - removing the ds.read.tr16.b128 declaration (#14054): gfx1250_lowering, 100 changed and 2 removed configs at llvm23 and in canonical IR, 5 changed, 2 new and 4 removed at llvm20 and llvm22; - optional LSE output on unified split-KV attention (#13955): 18 gfx942_attention_tiled_3d and 16 gfx950_attention_tiled_3d configs per LLVM flavor, canonical IR unchanged; - the running softmax max in 3D split-KV (#14276): 14 attention_strided_kv, 18 gfx942_attention_tiled_3d and 16 gfx950_attention_tiled_3d configs; - depthwise 7x7 and grouped wgrad on batched MFMA (#14157): 7 changed, 10 new and 1 removed conv_direct_grouped configs; - the retirement of wmma_fmha_fwd (#13897): the gfx1151_wmma_fmha_fwd (157 configs) and gfx1201_wmma_fmha_fwd (12) families are gone; - new configs, every existing hash unchanged: the barrier_fence family (#13575, 10), one conv_implicit_gemm_dgrad config (#13782), 14 conv_direct_grouped configs for the split-C and grouped LDS-halo kernels (#13993), one config each in fmha_splitkv_decode, sage_attention and sparse_attention (#13876), 23 conv_direct_grouped configs for the direct wgrad kernels (#13990), 2 conv_implicit_gemm_dgrad configs for patch-embedding dgrad (#13988), 6 f64_mma configs (#13794), 5 conv_direct_grouped and 7 conv_implicit_gemm configs for depthwise and grouped conv forward (#13987), and the conv_direct_dgrad_wmma family (7 configs, llvm23 and canonical IR only, #14156). #13885, #13955 and #14276 updated the old file's entries for their changes; the new llvm20 and canonical IR sections reproduce every one of those hashes. Co-Authored-By: Claude <noreply@anthropic.com>
The differential golden had gone stale, because nothing ran the check: 675 of its 1116 LLVM IR entries (compared at llvm20) and 608 of its 1089 canonical IR entries no longer match the tree. Recorded afresh in the new format, with the Python reference emitters, from a tree whose two-engine gate is green at llvm20, llvm22 and llvm23: - ll at llvm20 and llvm22: 82 families, 1474 configs per flavor. At llvm23: 83 families, 1578 configs, where gfx1250_lowering has configs that need LLVM 23 and are refused below it, and every conv_direct_dgrad_wmma config needs LLVM 23. - ir_canonical: 82 families, 1561 configs (conv_abi's emitter has no IR mode). Canonical IR was checked to be identical at llvm20 and llvm23. What the re-bless absorbs: every mismatch that could be traced is an intended code generation change that did not re-bless this file -- the shared LDS pool (#8844, about 425 configs), the ds.swizzle declaration (#9583, about 177), renamed values in the attention transpose reads (#9133, 57), the smaller LDS pool (#10112, about 39), the flat work group size of one-wave wave32 kernels (#12710, about 39), #9220 (6) and the conv changes of #13109 (6). 441 entries were already stale at the start of the history available here and cannot be attributed; no unexplained change was found after it. The two repeated keys (conv_direct_grouped, conv_wgrad_workspace_reduce), which hid their newer copies, are gone. It also takes in the code generation changes merged since then, each traced by recording the file at the commit and at its parent; the count is per section unless a flavor is named: - the magic division sum emitted without signed-overflow poison (#13802): 215 configs in 19 families -- attention_unified 6, attention_unified_stress 32, conv_direct_grouped 16, conv_implicit_gemm 26, conv_implicit_gemm_dgrad 31, conv_implicit_gemm_wgrad 42, deep_fused_conv_pool 2, gfx1201_deep_fused_conv_pool 5, gfx950_deep_fused_conv_pool 5, dynamic_helpers 2, fmha_splitkv_decode 6, img2col 5, moe_fused_mega 5, moe_gemm_fused 6, moe_sorting 6, pooling 4, sage_attention 4, sparse_attention 6, transpose 6; - attention decoding on gfx942 and gfx950 (#13492): 23 changed and 25 new configs in attention_strided_kv, gfx942_attention_tiled_3d and gfx950_attention_tiled_3d; - the f64 follow-ups (#13887): 3 changed and 1 new elementwise config, 5 changed reduce configs; - MX scale and fp6 decoding (#13885): 6 mx_gemm configs; - the gfx950 conv dgrad speed-up (#13075): 36 new conv_direct_dgrad_ext configs; 22 changed, 15 new and 2 removed conv_implicit_gemm_dgrad configs; 2 changed and 3 new conv_direct_grouped configs; 2 new dynamic_helpers and 3 new target_intrinsics configs; - removing the ds.read.tr16.b128 declaration (#14054): gfx1250_lowering, 100 changed and 2 removed configs at llvm23 and in canonical IR, 5 changed, 2 new and 4 removed at llvm20 and llvm22; - optional LSE output on unified split-KV attention (#13955): 18 gfx942_attention_tiled_3d and 16 gfx950_attention_tiled_3d configs per LLVM flavor, canonical IR unchanged; - the running softmax max in 3D split-KV (#14276): 14 attention_strided_kv, 18 gfx942_attention_tiled_3d and 16 gfx950_attention_tiled_3d configs; - depthwise 7x7 and grouped wgrad on batched MFMA (#14157): 7 changed, 10 new and 1 removed conv_direct_grouped configs; - the retirement of wmma_fmha_fwd (#13897): the gfx1151_wmma_fmha_fwd (157 configs) and gfx1201_wmma_fmha_fwd (12) families are gone; - new configs, every existing hash unchanged: the barrier_fence family (#13575, 10), one conv_implicit_gemm_dgrad config (#13782), 14 conv_direct_grouped configs for the split-C and grouped LDS-halo kernels (#13993), one config each in fmha_splitkv_decode, sage_attention and sparse_attention (#13876), 23 conv_direct_grouped configs for the direct wgrad kernels (#13990), 2 conv_implicit_gemm_dgrad configs for patch-embedding dgrad (#13988), 6 f64_mma configs (#13794), 5 conv_direct_grouped and 7 conv_implicit_gemm configs for depthwise and grouped conv forward (#13987), and the conv_direct_dgrad_wmma family (7 configs, llvm23 and canonical IR only, #14156). #13885, #13955 and #14276 updated the old file's entries for their changes; the new llvm20 and canonical IR sections reproduce every one of those hashes. Co-Authored-By: Claude <noreply@anthropic.com>
ISSUE ID : AICK-2640 ## Motivation Enable fp64 GEMM on gfx942 in rocKE using the f64 16x16x4 MFMA atom (#13794). ## Technical Details - Universal, batched and grouped GEMM accept fp64 (A/B/C fp64, fp64 accumulation, 16x16x4 warp tile) on gfx942 in both the Python and C++ engines, with byte-identical LLVM IR. - Supported: `mem`, `compv3` and `compv4` pipelines; `default` and `cshuffle` epilogues; padding; `chiplet_swizzle`, `lds_k_pad`, `active_tile_skip`, `preshuffle_b`, `lds_swizzle`, `direct_to_lds` and `dtl_prefetch`. - Rejected for fp64 by `is_valid_spec`: `split_k > 1`, fused epilogues, `direct_to_lds` with `pad_k` (the DirectToLDS path does not mask the K tail), and targets other than gfx942. - `direct_to_lds`: when a lane's chunk is smaller than one element (f64 on gfx942, 1 dword per lane), the global source is addressed in bytes. 16-bit dtypes keep the element-based path and their IR. - `lds_swizzle`: the swizzle granule is at least the LDS store vector width. This fixes wrong results for f64 and for f16/bf16 with the 16x16x16 atom and the gfx942 32x32x8 atom, and changes the IR of those `lds_swizzle` kernels only. When the granule fills the whole K row (one slot, e.g. 32x32x8 with `tile_k=8`), the swizzle is the identity instead of a fixed fallback that addressed past the row. - `mfma_cycle`: f64 16x16x4 is 32, from the gfx940/gfx942 scheduling model in LLVM `SISchedule.td` (`V_MFMA_.64_16X16X*` is `Write8PassDGEMM`, 8 passes). - `_emit_smem_load` (and its C++ twin `rocke_gemm_emit_smem_load`): the f64 16x16x4 atom takes one f64 per lane for A and B, so the fragment is one element wide, so a one-element fragment is returned as a scalar (`vec_extract(v, 0)`) matching the atom's `double` operand. No other GEMM atom has a one-element fragment, so other dtypes' IR is unchanged. - `GroupedGemmSingleLaunchRunner` takes an `arch` argument (default gfx950), so it can be built for fp64 on gfx942. - Not in this PR: fp64 GEMM candidates in the dispatcher (`rocke.dispatch.gemm`); fp64 kernels are built from an explicit spec for now. ## Test Plan - `test_gfx942_f64_gemm_numeric.py`: numeric check against numpy on both engines for each supported option, a batched `active_tile_skip` case, and rejection cases. - Byte-identity gate (llvm20/22/23), representative IR golden (two f64 cases added), L5 golden, parity suite, differential numeric, ctest. - L5 golden re-blessed for `gemm` 11/12 (f64 scalar load alignment, #13887) and `gemm_stress[23]` / `batched_gemm_stress[23]` (f16 `lds_swizzle` granule). - Full pytest compared against develop. ## Test Result - Byte-identity gate green on all three flavors; representative golden, parity suite, differential numeric (39/39) and ctest (29/29) pass. - f64 GEMM numeric tests pass on both engines. - Full pytest (`platform/tests library/tests`), branch vs. develop `85cf2ff700`: 180 failed on each side with byte-identical failing sets (all pre-existing, e.g. gfx1250/gfx950 scale-format and MXFP tests, strided KV tests); 67488 vs 67469 passed (the extra 19 are the new f64 tests). ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/GOVERNANCE.md#pull-requests.

Motivation
Follow-ups to the base f64 support (#13141), agreed in its review: f64
abstook three max instructions where one sign-bit clear does, f64 scalar loads and stores claimed 1-byte alignment, the f64meanrounded twice, two f64 tests could not fail for the reason they named, and the docs' f64 support lists and the LDS pool alignment docstring were out of date.ISSUE ID : AICK-2638
Technical Details
All changes are in both engines with byte-identical IR.
fabsbuilder op lowers f64 (llvm.fabs.f64; HIP's doublefabs). The elementwiseabsuses it for f64 instead ofmax(x, -x). f32, f16 and bf16 are unchanged. Thefabs.f64key is measured into the llvm20, llvm22 and llvm23 arch-domain artifacts. A new f64 abs elementwise parity configuration covers the new emission in the byte-identity gate.global_load_f64(default align 8);TensorView.load_scalaruses it for f64 andstore_scalardefaults f64 stores to align 8 (the elementwise scalar tail); the reduce2d f64 output store passes align 8. Other dtypes are unchanged.doubleappears; the f64 rejection tests pin the full messages and theis_valid_specreasons.op_vocabulary.md,ir_model.md,dtype_support.md) now say tanh is rejected by the builder and exp2_fast has no HIP lowering for any dtype; the LDS pool docstring states the 8-byte f64 alignment, pinned by new tests in both engines.Test Plan
llvm.fabs.f64with nomaxnum.fdiv doubleand nofmul double.Test Result
Submission Checklist
🤖 Generated with Claude Code