Repository navigation
Conversation
…that have them The base intrinsic table is shared by every flavor. It held the gfx1250 WMMA declarations, so an LLVM 20 host emitted calls to intrinsics its toolchain lacks and `check_ir_validity.py` could not tell that case from an emitter defect. Move the `wmma.gfx1250.*` declarations to the LLVM 22 override table and the `wmma.scale.*` declarations to the LLVM 23 override table, in Python and in the C++ engine. Prune the arch-domain rows that no longer have a declaration key. Make `_Lowerer._need` refuse a key that the flavor cannot declare. A key that a newer flavor adds raises "requires llvmNN", which the validity gate already reports as UNVALIDATED. A key in no table raises KeyError. Without this check the call was emitted with no `declare` and `opt` rejected the module. ISSUE ID : AICK-2788 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The Python lowerer refuses a `_need` demand for a key that the flavor table cannot declare. The C++ lowerer recorded such a key with a NULL declaration and `finalize` skipped it. An LLVM 20 host then emitted a call with no `declare`. Make `rocke_ll_need` fail the same way. A key that the LLVM 22 or LLVM 23 table adds fails with "requires llvmNN", which the validity gate reports as UNVALIDATED. A key in no table fails with a value error. ISSUE ID : AICK-2788 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
✅ 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. |
…ke/gfx1250-decl-flavor-tables # Conflicts: # dnn-providers/hip-kernel-provider/rocke/platform/cpp/core/lower_llvm/core.cpp # dnn-providers/hip-kernel-provider/rocke/platform/cpp/core/lower_llvm/data.cpp # dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/core/lower_llvm.py
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14437 — declare gfx1250 WMMA intrinsics only on the LLVM flavors that have them (CongMa13, AICK-2788) — 🔴 First-round: blocking — golden gates red at head; llvm22+ WMMA declares reordered
Utility: 🟢 Positive — closes the call-without-declare gap in both engines; needs re-bless and order fix. 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 a clean merge with today's develop it ran the native build and ctest, engine-wide byte identity, the representative and differential goldens, the PR's test file and the arch-domain artifact test. It lowered every drifted gfx1250 case on the merge and on develop at three lanes, compared the C++ and Python refusals, and dry-ran the re-bless into scratch copies. It also checked develop's changes since the base and read the four hipkernelprovider CI logs at head. The orchestrator owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection. |
…ke/gfx1250-decl-flavor-tables
… flavor Keep the gfx1250 WMMA keys in the base declaration table and record the lowest flavor that declares each one. `_decls_for_flavor` and the C++ lookup drop a key below its minimum flavor, so a demand is still refused with "requires llvmNN". Declares on llvm22 and later keep their place in the emit order, so only llvm20 output changes. Re-bless the representative and differential goldens for llvm20. Compare `conv_winograd_fused_1d_wmma` in the llvm23 lane of the byte-identity gate. ISSUE ID : AICK-2788 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…l-flavor-tables' into users/congma13/rocke/gfx1250-decl-flavor-tables
|
Thanks for the review. Head N2 (declare order): fixed.
N1 (goldens): fixed.
Gate change. At llvm20 every config of Verification, each flavor on its own toolchain image.
N3: not done in this PR. The same minimum-flavor map can gate the other keys that their columns mark The |
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14437 — declare gfx1250 WMMA intrinsics only on the LLVM flavors that have them (CongMa13, AICK-2788) — 🟠 Follow-up: closer look — N1 and N2 are fixed; a new, unused llvm23 declaration fails the arch-domain test, and TheRock's test shards have not run at this head
Utility: 🟢 Positive — closes the call-without-declare gap in both engines; only llvm20 output changes. 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. It traced the branch since |
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14437 — declare gfx1250 WMMA intrinsics only on the LLVM flavors that have them (CongMa13, AICK-2788) — 🔴 Follow-up: CI update — TheRock's gfx94X shards ran at this head, and both fail on a conv dispatch test this change breaks (N3)
Utility: 🟢 Positive — closes the call-without-declare gap in both engines; the conv dispatcher must follow it. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted CI update to our follow-up at this head: the orchestrator (Claude Opus 5.5) read both red gfx94X hipkernelprovider shard logs and re-ran the failing test on a clean merge with develop and on develop. The orchestrator owns this merge-worthiness call. This review flags whether a closer look is needed; it is not an approval or a rejection. |
…ke/gfx1250-decl-flavor-tables Resolve conflicts in check_byte_identity.py (keep both family lists) and in the two IR goldens (re-blessed: no hash changes against develop).
The fused kernel issues the gfx1250 f16 WMMA, which llvm20 no longer declares. Declare its LLVM flavors so the dispatcher admits no plan there, and update test_unfused_plans_need_llvm23 to expect that on llvm20. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…cale decl key The llvm23 declaration table gained mfma.scale.f32.16x16x128.f8f6f4 without a row in intrinsic_arch_domain.llvm23.json, so test_every_decl_key_is_covered_on_every_wired_arch failed on llvm23. Regenerated with tools/gen_arch_domain.py on the llvm23 toolchain. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14437 — declare gfx1250 WMMA intrinsics only on the LLVM flavors that have them (CongMa13, AICK-2788) — 🟠 Follow-up: conflicts, test break, second gate
Utility: 🟢 Positive — still needed: develop declares WMMA intrinsics that LLVM 20 lacks. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted Follow-up review: first pass by a local qwen3.6 model, verified by the orchestrator (Claude Opus 5.5). Checks: merge-tree with develop and #14328, scratch merge (both goldens, arch-domain test per flavor, 27 develop test files), llvm20 lowering on develop vs the merge, CI at head, code quality. Flags whether a closer look is needed; not an approval or rejection. |
…ke/gfx1250-decl-flavor-tables
…e mfma.scale decl key" This reverts commit 1307948.
- Drop the llvm23 override for mfma.scale.f32.16x16x128.f8f6f4 in both engines. No kernel demands it, and the llvm24 column and C++ array did not hold it. Revert the artifact regeneration that added its row. - Compare conv_winograd_fused_1d_wmma in the byte-identity default lane from llvm22 on. Only flavors below llvm22 move it to the llvm23 lane. - Raise ROCKE_ERR_KEY for a key that no table holds, as Python raises KeyError. Add a C++ case that pins the llvm20 refusal text of the gfx1250 f16 WMMA and the llvm22 declare. - Skip the gfx1250 cases of the tiled-attention cache-key test on flavors below llvm22. - Take the sharded representative golden from develop. Re-bless the llvm20 sections of the conv, conv_dgrad, conv_wgrad, gemm and conv_winograd_fused_1d_wmma shards and the llvm20 differential golden. ISSUE ID : AICK-2788 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@cgmillette Head
Test results:
|
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14437 — declare gfx1250 WMMA intrinsics only on the LLVM flavors that have them (CongMa13, AICK-2788) — 🔴 Follow-up: merge breaks matrix-call tests
Utility: 🟢 Positive — still needed: develop lowers gfx1250 WMMA at llvm20, which LLVM 20 lacks. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted Follow-up review: first pass by a local qwen3.6 model, verified by the orchestrator (Claude Opus 5.5). Checks: merge-tree with develop, local merge (build, ctest, byte identity llvm20–24, both goldens, 33 test files vs develop), saved probes, CI at head, code quality. Flags whether a closer look is needed; not an approval or rejection. |
Replace the per-key minimum-flavor declaration table with a `GFX1250_WMMA` emission-profile capability in both engines. The hand-written gfx1250 WMMA handlers check it before they demand the intrinsic. The declaration tables are the same as on develop, so the emit order and the arch-domain columns do not change. Keep `_require_declared` (Python) and the matching check in `rocke_ll_need` (C++) for keys that no table declares. Re-bless the llvm20 sections of the gemm and conv_direct_dw_fwd_wmma shards and of the differential golden. Compare both families in the llvm23 lane of the byte-identity gate below llvm22. ISSUE ID : AICK-2788 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@cgmillette Head
One more change came from the merge. Test results on the merge:
Not run: the full Python suite on LLVM 20 after this merge, the IR validity gate, GPU and Windows. |
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14437 — refuse gfx1250 WMMA on LLVM flavors that lack the intrinsics (CongMa13, AICK-2788) — 🔴 Follow-up: merge fails llvm20 goldens
Utility: 🟢 Positive — LLVM 20 now refuses gfx1250 WMMA cleanly; re-record and fp8 test left Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted Follow-up review: first pass by a local qwen3.6 model, verified by the orchestrator (Claude Opus 5.5). Checks: merge with today's develop (build, ctest, byte identity and both goldens at 4 flavors, a re-record dry run), changed and develop's related tests (llvm24, llvm20), mutants of both engines' gate, saved probes, CI at head, code quality. Flags whether a closer look is needed; not an approval or rejection. |
…ke/gfx1250-decl-flavor-tables
- Skip the depthwise dgrad WMMA lowering test where the flavor lacks the gfx1250 WMMA intrinsics. - Re-record the llvm20 sections of the conv_direct_dgrad golden and the differential golden for conv_direct_dgrad_ext. - Pin the llvm20 refusal of the fp8 K64 WMMA in both engines. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@cgmillette Head
Test results after the merge:
Not run after this merge: |
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14437 — refuse gfx1250 WMMA on LLVM flavors that lack the intrinsics (CongMa13, AICK-2788) — 🔴 Follow-up: llvm20 goldens fail after 14832
Utility: 🟢 Positive — llvm20 refuses gfx1250 WMMA in both engines; rebase needs 14832's re-record Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted Follow-up review: first pass by a local qwen3.6 model, verified by the orchestrator (Claude Opus 5.5). Checks: merge with today's develop (conflict resolved, both goldens at 4 flavors before and after a re-record, byte identity of the family 14832 changed), 20 changed and related test files on the merge and develop (llvm24, llvm20), mutants of both engines' gate, saved probes, CI at head, code quality. Flags whether a closer look is needed; not an approval or rejection. |
|
This PR has gotten stuck in a review loop. Every time I resolve a conflict and push an update, a new conflict pops up. It's been going on for five days now. Until we come up with a better way to handle these recurring merge conflicts, I'm going to close this PR. |
perhaps running hotspot analysis could help factoring out things that change more frequently than they need |
ISSUE ID : AICK-2788
Summary — For human reviewers
The short version
The rocKE lowerer keeps one base table of intrinsic declarations for all LLVM flavors. LLVM 20 does not have the unscaled gfx1250 WMMA intrinsics. The lowerer wrote the call for them on LLVM 20. The module then called an undeclared function, and
optrejected it.This PR makes the gfx1250 WMMA handlers refuse LLVM 20 with a clear message. It also makes
_needrefuse a key that no table declares.How it works
GFX1250_WMMA, marks the flavors that have the intrinsics. LLVM 22, LLVM 23 and LLVM 24 have it. LLVM 20 does not. Python (_LlvmEmissionCapability) and C++ (ROCKE_LL_CAP_GFX1250_WMMA) hold the same capability.intrinsic '<key>' requires llvm22, got llvm20. The Python backend (emit_wmma) and the C++ lowerer (_emit_wmma) use the same text._needrefuses a key that no table declares. Python raisesKeyError. C++ returnsROCKE_ERR_KEY. A missing key is an emitter defect, becausefinalizewould write a call without adeclare.develop. The scaled WMMA path already refuses flavors below LLVM 23 through_supports_llvm23_target_ops.The refusal uses the existing wording.
tools/_flavor_gate.pyalready matches "requires llvmNN", so the validity gate reports such a case as UNVALIDATED.What changes
On LLVM 20, every kernel that uses an unscaled gfx1250 WMMA becomes an expected refusal. Those are the gfx1250
gemm,conv,conv_dgrad,conv_wgrad,conv_winograd_fused_1d_wmma,conv_direct_dw_fwd_wmmaand depthwise windowed dgrad cases. The LLVM 20 sections of both IR goldens record this. The LLVM 22, LLVM 23 and LLVM 24 sections equaldevelop.LLVM 22 and later emit the same IR as before. The fused 1-D Winograd plan declares LLVM 22 and later, so LLVM 20 admits no plan for it.
tools/check_byte_identity.pycomparesconv_winograd_fused_1d_wmmaandconv_direct_dw_fwd_wmmain the default lane from LLVM 22 on. A flavor below LLVM 22 compares them in the LLVM 23 lane only, because LLVM 20 refuses every config of those families.Known issue, not part of this PR
On an LLVM 23 toolchain, 7 gfx950 fp16 attention cases fail with
Operand has incorrect register class. They fail the same way ondevelop. The cause is a compiler defect.How this was tested
Runs used the merge of
developat031d56af855, unless the line names a later merge.ctest: 46 of 47 pass.rocke_optimization_barrier_hip_pairtimes out on a node without HIP headers.tools/check_byte_identity.py, full run: GREEN on LLVM 20, LLVM 22, LLVM 23 and LLVM 24.test_gfx1250_scaled_wmma.py,test_matrix_calls.py,test_flavor_decl_tables.py,test_arch_domain_artifact.py(without the regeneration test): 161 pass on each of the four flavors.gemmandconv_direct_dw_fwd_wmma. Both shards are re-blessed.conv_direct_dw_fwd_wmmaconfigs.developat240c8b0dc18, which adds the gfx1250 depthwise dgrad kernels (dw_dgrad_win_wmma_k5,dw_dgrad_win_wmma_k7):conv_direct_dgradonly. The shard is re-blessed. Thetest_rocke_ci_static.pygolden tests pass (8).conv_direct_dgrad_extLLVM 20 configs that lower WMMA are re-recorded. The check passes on LLVM 20, LLVM 22, LLVM 23 and LLVM 24.test_lowers_to_wmma_with_lds_stagingskips on a flavor without the capability.test_gfx1250_scaled_wmma.pyand one in the C++ testrocke_future_intrinsic_lowering(92 cases pass).pre-commitpasses on the changed files.What did not run: the full Python suite on LLVM 20 after the latest merge, the byte-identity gate after the latest merge, the IR validity gate, any GPU run and any Windows run.
Reviewing this
The change adds one profile capability, one check in each WMMA emit path, and one check in
_need. No lowering handler outside the gfx1250 WMMA path changes.Summary — For AI reviewers
Change surface
Properties to verify
_decls_for_flavoris the same as ondevelop. Falsify: any hash change outside the LLVM 20 section of either golden.emit_wmmaand_emit_wmma_fp8. The C++ path is_emit_wmmafor the shapesWMMA_SHAPE_GFX1250andWMMA_SHAPE_GFX1250_FP8. Falsify: an unscaledwmma.gfx1250.*call that reaches_needon LLVM 20 by another path.intrinsic '<key>' requires llvm22, got <flavor>. The C++ case infuture_intrinsic_lowering.cpppins the text._require_declaredaccepts every key thatfinalizedeclares._op_vector_smaxwritesself._decls[intrin]before_need. In C++,rocke_ll_need_dynamicregisters the key first. Falsify: a_needcall that precedes the registration of a dynamic key.MATRIX_CALLSentries derive their declarations per flavor and do not use the hand-written handlers.Known-weak points
name_absenton every arch stay declarable (52 on LLVM 20, 17 on LLVM 22). This PR does not move them. Follow-up AICK-3035 covers them.Scope calibration
A finding that this PR does not fix the 7 LLVM 23 attention failures is out of scope. A finding that this PR does not add a CI entry for the validity gate is also out of scope. That work was dropped on purpose.
Test plan
ctest: 46 of 47 (one HIP-header timeout)check_byte_identity.pyfull run: LLVM 20, 22, 23, 24 GREENcheck_byte_identity.pyafter the latest merge: not runcheck_ir_validity.py: not run after this change🤖 Generated with Claude Code