Repository navigation
perf(rocke): depthwise 7x7 and grouped cpg4/8 wgrad on batched mfma 4x4x4 - #14157
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. |
ac4d72b to
58f09db
Compare
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14157 — depthwise 7x7 and grouped cpg4/8 wgrad on batched mfma 4x4x4 — 🟠 First-round: closer look recommended — the new kernels are byte-identical in both engines and build for gfx950; Copilot's two items are still open
No net-new findings beyond Copilot's. Utility: 🟢 Positive — adds depthwise 7×7 and 4 / 8-channel grouped wgrad on gfx950 through batched 4×4×4 MFMA, byte-identical in both engines; the description includes no measurements, so the speedup is not verified here. 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 with byte identity for the conv family, gfx950 builds of every new config through both engines, both Copilot claims reproduced on the merge, the golden gate, the CPU tests and the CI at head, and owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection. |
…4x4x4 atoms
The depthwise and grouped batched-4x4x4 wgrad validators accepted a
target_blocks of 0 or -1, which n_z() and the emitted smax then turned into
one z partition. Both engines now reject it as the other direct-wgrad
validators do; parity configs 159..162 pin the rejection in both engines.
The C++ HIP lowerer had no handler for mfma_f32_4x4x4_{f16,bf16}: tile.mma
fell through to the legacy dispatch and failed with "no HIP lowering". Emit
the same builtin calls as the Python handlers, with a both-engines HIP test.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
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. |
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.
jakpiase
left a comment
There was a problem hiding this comment.
One blocking item, see the inline comment: the new mfma.f32.4x4x4bf16.1k intrinsic is missing from the arch-domain tables, so tests/core/test_arch_domain_artifact.py fails in the source-tree gate.
…ect-f-opt-v2 Both sides added the mfma_f32_4x4x4_bf16 atom with identical code; the conflicts were comment wording only and take develop's text. conv_direct_grouped_emit: this branch's direct-wgrad configs run 114..162, so develop's bf16 DirectConv4cSpec configs move from 148..150 to 163..165 in both the Python and C emitters. The representative IR golden is re-blessed (no case hash changed; summary counts only). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The bf16 4x4x4 atom added the decl key mfma.f32.4x4x4bf16.1k but the
llvm20 arch-domain column had no row for it (nor for the
amdgcn.fdot2.{f16,bf16} keys from develop), so
test_every_decl_key_is_covered_on_every_wired_arch failed and the
arch-domain check stayed silent when the atom was used on RDNA.
Regenerated with tools/gen_arch_domain.py against ROCm 7.1.1's LLVM 20,
the toolchain the column was measured with. Additions only: the bf16 atom
is ok on gfx90a/gfx942/gfx950 and arch_absent on RDNA, matching its f16
twin. The llvm22 column already has these rows from develop.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Same gap as the llvm20 column: no rows for mfma.f32.4x4x4bf16.1k or
amdgcn.fdot2.{f16,bf16}. Regenerated with tools/gen_arch_domain.py
against clang 23 at 8f497e0992fb (the toolchain the column was measured
with, from the rocm/pytorch rocm10.0 image, CPU only). The bf16 atom is
ok on gfx90a/gfx942/gfx950 and arch_absent on RDNA and gfx1250, like its
f16 twin. Four existing rows change evidence wording only (this llvm-mc
names the target in its message); their status is unchanged.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@bartekxk -- develop currently fails the source-tree arch-domain checks in |
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14157 — depthwise 7x7 and grouped cpg4/8 wgrad on batched mfma 4x4x4 — 🟠 Follow-up: rebase needed — both of our items are fixed and the arch-domain rows clear develop's break, but develop's #13794 now conflicts in
|
| Prior item | Status |
|---|---|
Ours (10-22) / Copilot Rule 4: target_blocks ≤ 0 accepted |
🟢 FIXED in d3d9ee71: both validators now return (False, 'target_blocks >= 1') for 0 and −1 (ran) |
| Ours (10-22) / Copilot Rule 5: no C++ HIP lowering for the 4x4x4 atoms | 🟢 FIXED in d3d9ee71: at the head, the C++ engine lowers both tile.mfma_f32_4x4x4_f16 and _bf16 to HIP; the f16 gap predated this PR (ran) |
Develop break (cgmillette, 10-23): #13075 declared amdgcn.fdot2.bf16, amdgcn.fdot2.f16 and mfma.f32.4x4x4bf16.1k with only an llvm22 column |
🟢 FIXED in 2760d87f and becfb94a: all three keys now have llvm20 / 22 / 23 answers. On develop, test_arch_domain_artifact.py reports "no answer for 3 decl key(s)" for llvm20 and llvm23; at the head those cases pass (ran) |
jakpiase: CHANGES_REQUESTED (10-23), then APPROVED at head becfb94a; CongMa13 APPROVED (10-22) at c5d2c07e |
🟢 / 🟡 jakpiase's approval is at the head; CongMa13's predates the arch-domain commits. All three review threads are resolved |
| Bots / CI | 🟠 At the head: Test hipkernelprovider on gfx110X passes (both shards), and the gfx94X shards are still running. GitHub now reports the PR as conflicting |
| Our checks | 🟢 At the head: native build and 29 ctests pass, and byte identity is GREEN for conv_direct_grouped (166 configs) at llvm20 / 22 / 23 (ran). In a scratch merge with today's develop, resolving N1 as described builds, passes 30 ctests, lowers both atoms through C++ HIP, keeps conv_direct_grouped GREEN and passes the representative golden (ran) |
| # | Sev | file:line | Finding |
|---|---|---|---|
| N1 | 🟠 | platform/cpp/core/lower_hip/lower_hip_mma.cpp |
#13794 (merged 10-23) added the f64 4x4x4 / 16x16x4 HIP handlers at the same spot as this PR's 4x4x4 f16 / bf16 handler → rebase and keep both. Close the 4x4x4 block (…operands[2])); return ROCKE_OK; }) before develop's f64 blocks: a plain concatenation of the two sides does not compile (ran). |
Utility: 🟢 Positive — adds the depthwise 7×7 and 4 / 8-channel grouped wgrad, and the llvm20 / llvm23 arch-domain rows develop is currently missing; once rebased it is approved at the head with every thread resolved, so it could land quickly to clear the develop break.
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 with a native build and checks at the head, both Copilot fixes, the arch-domain tests on develop and the head, a scratch merge with today's develop resolved and built, and the CI state, and owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection.
…arkocot/direct-f-opt-v2 # Conflicts: # dnn-providers/hip-kernel-provider/rocke/library/tests/parity/conv_direct_grouped_emit.c # dnn-providers/hip-kernel-provider/rocke/library/tests/parity/conv_direct_grouped_emit.py # dnn-providers/hip-kernel-provider/rocke/platform/cpp/core/lower_hip/lower_hip_mma.cpp # dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/instances/SUPPORT_MATRIX.md # dnn-providers/hip-kernel-provider/rocke/platform/tests/golden/rocke_representative_ir_sha256.json Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…arkocot/direct-f-opt-v2
The develop merge left conv_direct_dgrad_wmma and conv_direct_dw_wgrad_mfma both imported as dwm in select_direct_kernel_builder, so the second shadowed the first and every direct conv build raised AttributeError on build_direct_dgrad_wmma. Import the MFMA dw wgrad module as dwmf. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…arkocot/direct-f-opt-v2 # Conflicts: # dnn-providers/hip-kernel-provider/rocke/platform/tests/golden/rocke_representative_ir_sha256.json
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>
TheRock Submodule Bump ActivityNewest first
|
…ke/gfx1250-fwd-direct-wmma Conflicts with the grouped/depthwise wgrad MFMA work (#14157): - library/kernels/conv/direct_variants.py: keep both builder imports (conv_direct_dw_wgrad_mfma and conv_direct_fwd_wmma). - library/tests/parity/conv_direct_grouped_emit.{py,c}: take develop's shifted bases (bf16 4c from 163, wave32 depthwise forward from 166) and keep this branch's channel-packed cpl > 1 configs after them, now at 171..178; headers updated in both emitters. - platform/tests/golden/rocke_representative_ir_sha256.json: re-blessed with rocke_ir_parity_harness.py --write on the merge result; only this branch's cases differ from develop's golden. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>


Motivation
Improve conv backward weight performance for depthwise and grouped edge case
Technical Details
Add new direct conv specs with depthwise 7x7 and grouped cpg4/8 using mfma 4x4x4
Test Plan
test_conv_*_correctness.py
Test Results
Passed
Submission Checklist
JIRA ID: AICK-2328