Repository navigation
feat(rocke): enable mixed scaled GEMM on shared operand storage - #12279
Conversation
github-actions
Bot
added
documentation
project: hip-kernel-provider
rocke: platform
963beb5 to
054d1b3
Compare
Report complete quant type aliases while preserving scalar quantization restrictions.
Keep empty descriptors valid while rejecting fragment loads before IR emission in both engines. Cover empty shapes and nonempty controls in Python and native tests.
Validate complete static fragment displacements before emitting i32 offsets in both engines. Resolve CTest readiness from registered commands and cover multi-config execution and failure propagation.
Size temporary load-pointer storage by addressable units so padded slots cannot overrun the array. Preserve IR order and add mirrored transport coverage for a layout requiring more loads than logical elements. Simplify scale-width queries while retaining the shared packing adapter, and clarify the contiguous-K matrix layout contract.
|
🎉 All checks passed! This PR is ready for review. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
Mixed-format support is consistently wired across catalogs, lowering, examples, and extensive tests, with only a minor stale test comment noted.
Review effort: Balanced
Findings: 1
What changed in this PR
Enables mixed A/B low-bit formats for gfx1250 SCALE/SCALE16 GEMM while preserving independent operand storage contracts.
Changes:
- Adds 40 mixed-format catalog entries across Python and C++.
- Removes the homogeneous-only validation restriction and adds a mixed-format CLI example.
- Adds parity, golden, host, and GPU numerical coverage.
| File | Description |
|---|---|
platform/tests/instances/test_gfx1250_mixed_scaled_gemm_numeric.py |
Adds GPU numerical coverage. |
platform/tests/instances/parity/gfx1250_lowering_emit.py |
Adds Python parity configurations. |
platform/tests/instances/parity/gfx1250_lowering_emit.c |
Mirrors mixed configurations natively. |
platform/tests/core/test_mma_catalog_lookup.cpp |
Updates catalog-count expectations. |
platform/tests/core/test_gfx1250_mixed_scaled_wmma.py |
Tests storage, lowering, and goldens. |
platform/tests/core/test_gfx1250_mixed_example.py |
Tests mixed-example CLI behavior. |
platform/tests/core/test_gfx1250_fp6_scaled_wmma.py |
Updates mixed-format validity tests. |
platform/tests/core/test_gfx1250_fp4_scaled_wmma.py |
Replaces a newly valid rejection case. |
platform/tests/core/test_arch_mma_ssot.py |
Validates expanded catalog identity. |
platform/tests/core/gfx1250_mixed_scaled_wmma_llvm23.json |
Records LLVM 23 golden hashes. |
platform/python/rocke/instances/SUPPORT_MATRIX.md |
Documents mixed-format support. |
platform/python/rocke/instances/gfx1250/block_scaled_gemm.py |
Allows mixed native scaled GEMM specs. |
platform/python/rocke/examples/gfx1250/gemm/README.md |
Documents the mixed example. |
platform/python/rocke/examples/gfx1250/gemm/mixed_scaled_gemm.py |
Adds the mixed-format CLI. |
platform/python/rocke/examples/gfx1250/gemm/block_scaled_gemm_verify.py |
Reports both operand formats. |
platform/python/rocke/core/arch/target.py |
Adds mixed fragment metadata. |
platform/python/rocke/core/arch/data/arch_specs.json |
Adds Python catalog entries. |
platform/cpp/core/arch/data.cpp |
Adds matching native catalog data. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: tenpercent <4273004+tenpercent@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The large dual-backend GPU catalog and ISA-contract expansion warrants final human validation despite extensive automated coverage.
Review effort: Balanced
Findings: None
Resolved since last review (1)
cgmillette
left a comment
cgmillette
left a comment
There was a problem hiding this comment.
PR #12279 — mixed scaled GEMM on shared operand storage (tenpercent, AICK-2285) — 🔴 REJECT: 40 stale llvm23 goldens fail at head; regenerate, then approve
| Prior item | Status |
|---|---|
Copilot (1 inline + 2 overviews): wmma_scaled fixture header still FP8-only (gfx1250_lowering_emit.c:121); 2nd overview "needs human look", no findings |
🟢 FIXED 3f7eff28: header now documents independent FP8/BF8/FP6/BF6/FP4 A/B; thread resolved |
| Bots: therock-pr-bot policy, base-freshness, Math CI | 🟢 Policy pass (AICK-2285). Math CI SKIPPED: no tests ran on head |
| # | Sev | file:line | Net-new finding |
|---|---|---|---|
| N1 | 🔴 | tests/core/gfx1250_mixed_scaled_wmma_llvm23.json (all 40) |
Stale goldens. Head merge 64c7c18d pulled in develop #12672 (llvm23 datalayout gains p10–p15) without regenerating them. test_mixed_llvm23_golden fails 40/40 at head; every SHA matches only with p10–p15 stripped. Collected by quick-tier rocke_pytest, so develop goes red. Body's validation (72246e6f, develop e01aac56) predates this merge. Regenerate and re-run on head. |
| N2 | 🟡 | block_scaled_gemm.py:239 |
is_valid_spec OK reason names only A (fp8×fp4 reports "native wmma_scale fp8 GEMM"); test_gfx1250_mixed_scaled_wmma.py:43 pins it. Report {a}x{b}. |
Utility: 🟢 Positive. Completes AICK-2285 as a data-only extension: 40 catalog rows with Python/C++ parity (all 50 scaled rows field-checked), catalog-driven gate, per-operand selectors in both engines, unique kernel names, homogeneous goldens untouched. Approve once N1 is regenerated.
Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good
There was a problem hiding this comment.
🔵 Needs a closer look
Hardware-specific catalog and lowering changes lack current-head GPU validation after the latest merge.
0 open findings
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
CongMa13
left a comment
CongMa13
left a comment
There was a problem hiding this comment.
No new actionable findings on a5fd7b410757bc326e428977bb09fa2fbe3da4d6.
The previous fixture-header, stale-golden, and A/B diagnostic observations are addressed by the current head as documented in the existing review history. The PR is good to go for approval.
cgmillette
left a comment
cgmillette
left a comment
There was a problem hiding this comment.
All green :)
PR #12279 — mixed scaled GEMM on shared operand storage (tenpercent, AICK-2285) — 🟢 ACCEPT: all items fixed
| Prior item | Status |
|---|---|
| Ours N1: 40 stale llvm23 goldens | 🟢 FIXED a5fd7b41 — 40/40 pass at head and merged with current develop (ran) |
| Ours N2: OK reason names only A | 🟢 FIXED — now {a}x{b}; only consumer is the pinning test |
| Copilot: fixture header FP8-only | 🟢 FIXED — gfx1250_lowering_emit.c:121 |
| Copilot (head): no current-head GPU run | 🟢 Info — IR unchanged except the p10–p15 datalayout |
| CongMa13 (10-09): approve | 🟢 Agree |
Net-new: none.
Utility: 🟢 Positive — data-only AICK-2285 completion with dual-engine parity; approve.
Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good
TheRock Submodule Bump ActivityNewest first
|
Enable different A/B matrix formats in gfx1250 SCALE/SCALE16 GEMM with E8M0 scales. The mixed example defaults to FP8 E4M3 x FP4 E2M1;
--dtype-aand--dtype-bindependently select FP8, BF8, FP6, BF6, or FP4. Equal normalized formats direct users to the homogeneous examples.--output-dtypeselects BF16 or FP16 through the shared verifier.Adds 40 mixed catalog entries (20 ordered pairs in each scale mode) in Python and C++, removes the homogeneous-only builder restriction, and adds the mixed example and focused coverage. Existing atom-selected storage, fragment loading, scale layouts, and lowering are reused. FP6 #12140, common storage #12457, and FP4 #11960 are merged; this branch incorporates develop at
eb209ed0b414de157635b0b3737d57df85a6986f. Alternative scale formats remain outside this PR.The review fixes regenerate all 40 mixed LLVM 23 golden hashes after the develop merge introduced
p10–p15in the emitted datalayout. Removing only those six entries from every emitted datalayout recovers all 40 previous hashes. The current datalayout is retained. Native validation success messages now name both canonical operand formats in A/B order, for examplewmma_scale fp8xfp4 GEMM.Validation for
a5fd7b410757bc326e428977bb09fa2fbe3da4d6:Historical GPU evidence, retained for its recorded source revisions:
a34575fa3e514887443365f1b5f65bc3adbcede2, archive SHA-256ca63cb2be5b50293fbd699ed6f593c08d5242638faa905bac3c3e602910a5061, ROCm 10.1.0a20260820 / LLVM23, required gfx1250 device, strict dual backend: 147 GPU tests and 1,557 successful comparisons plus 25 host tests, zero failures/errors/skips. The final merge72246e6f59741917060e289344c0112e5c3f6073differed only by verified AST-equivalent host-test formatting.c0227dfc6b9ec212c456c643da3e3da5bf2ba17a, archive SHA-2569d8ac059b24950458c291b2cc0aa52743cd348ac698a9cda0c31f3c7cb51c661, same ROCm/LLVM and required-device strict dual-backend setup: 88 mixed GPU tests (80 BF16 + 8 FP16), 1,408 successful comparisons, zero failures/errors/skips.These GPU results predate the latest develop merge and are not current-head GPU validation. The review fixes change a success diagnostic and hash expectations; GPU tests were not rerun. The complete
run_all.pyrunner and full differential sweep were not rerun for this follow-up. The earlier broad sweep retained an inherited gfx942 attention configuration-3 mismatch reproduced against its exact develop parent. Fixture-based GPU results do not establish arbitrary-input rounding or special-value semantics.ISSUE ID : AICK-2285