Skip to content

feat(rocke): gfx1250 fp8 GEMM bring-up and K-loop optimization (AICK-2347) - #14707

Merged
jefyang1 merged 47 commits into
developfrom
users/jefyang/rocke/aick-2347
Nov 7, 2026
Merged

jefyang1 merged 47 commits into
developfrom
users/jefyang/rocke/aick-2347

Conversation

@jefyang1

@jefyang1 jefyang1 commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Bring up the universal GEMM for 8-bit floats on gfx1250 in both engines and make it fast, fed by the Tensor Data Mover (TDM). Supported inputs are fp8 x fp8 or bf8 x bf8 (A and B the same type), with a bf16 or fp16 C. fp8 runs on the 16x16x64 and 16x16x128 WMMA atoms; bf8 on the 16x16x64 atom only. The K-loop is then optimised with three opt-in traits. The work comes with a reproducible case study and the measurement tools behind it, so every result can be regenerated from the repository.

JIRA ID: AICK-2347

Technical Details

Every new trait defaults off, and the Python and C++ engines emit byte-identical LLVM IR throughout.

fp8 GEMM on gfx1250

  • fp8e4m3/bf8e5m2 A/B with a 16-bit C (no fp8 accumulator). _validate_storage splits the A/B dtype from the C dtype; every other spec stays homogeneous.
  • The K=64 fp8 atom gets lane maps, and so does the K=128 one, develop's dedicated wmma_gfx1250_f32_16x16x128_fp8_fp8 instruction (feat(rocke): backend completeness for every production target and dense MMA instruction #14328 lists it without A/B maps). Both maps are verified on silicon. The K=128 row counts its fragments in the 16 dword slots of its <16 x i32> carrier, as the K=64 rows do; the other fp8/bf8 pairs at K=128 keep develop's unmapped rows.
  • HIP-source lowering for the K=128 fp8 instruction, in both engines.
  • Global A/B loads use the operand's real element size, so 1-byte operands get full 16-byte loads. TraitSpec.ab_load_elem_bytes overrides the width so the two can be compared in one binary. f16/bf16 kernels are unchanged.
  • The cshuffle epilogue works on the WMMA path. The staging tile is typed by the C dtype, not the A/B dtype.
  • The bf8 K=64 atom is gated, and the bf8 verify lane encodes e5m2 inputs.
  • Kernel names carry the C dtype when it differs from A's (c<dtype>, the fp8/bf8 path only), so fp8 with a bf16 C and with an fp16 C are two symbols; every other kernel keeps its name.

TDM in the universal GEMM

  • A and B are loaded through core's tdm_descriptor_2d, which now also takes an i64 global address. Core's encode_tdm_padding (new, both engines) turns lds_k_pad into the descriptor's pad fields, and the winograd GEMM's row skew now goes through it too.
  • tile.global_addr_of: a global address as an i64, without an inbounds GEP, so a tile origin past the end of the buffer stays defined.
  • TraitSpec.tdm / tdm_depth: 1 is unpipelined, 2 ping-pong, 3 or 4 a modular ring. This ports the TDM subset of feat(rocke): gfx1250 BF16 universal GEMM load-path, epilogue, and persistent-kernel levers #12658.

K-loop traits (opt-in, depth-2 TDM ping-pong)

  • split_barrier: the per-tile barrier is split into signal and wait, so WMMAs overlap the wait. The ring (depth ≥ 3) takes the same split.
  • tdm_alternate_issuers: each LDS half gets its own issuer wave pair, which takes the refill off the waiting waves' critical path.
  • tdm_early_refill: refill a half as soon as every wave has read it.
  • The TDM K-loops share one K-tile count, ⌈(K − k_lo) / tile_k⌉, so every loop processes a partial last K tile; TDM clips it to K. Kernel names carry the options that are on (tdm{depth}, spb, alti, erf), and the runbook Knob Catalog lists them.

Measured on gfx1250 at 4096x4096x65536 (fp8e4m3 A/B, bf16 C) on the 16x16x128 atom with fp8_trait_ab.py at the commits that added each trait (then the f8f6f4 form of the instruction; see Test Result for the rerun on the dedicated one). Each build is verified on the device before any timing (exact on integer inputs, within the bf16 rounding bound on random inputs), then the builds are timed round-robin in one process on random inputs (each sample: 25 ms idle, then 1 warmup and 3 timed launches; median of 15 samples). Ratios are medians against the depth-2 TDM ping-pong without these traits, on this branch's own kernels:

added on top of the previous ratio
split_barrier 1.012x
+ tdm_alternate_issuers 1.041x
+ tdm_early_refill 1.052x

The A/B is cumulative: each row adds one trait to the row above, so tdm_alternate_issuers was not measured on its own. The tdm_early_refill gain depends on K: at K = 8200 it falls below tdm_alternate_issuers (below), so it is meant for long K.

These are results at this shape and geometry, not a general claim; the case study gives the full method and caveats.

Correctness fixes along the way

  • LDS vector alignment is bounded by the row stride (gcd with the innermost dimension). The 8-bit floats count as 1 byte in the LDS element map. Before, fp8 LDS reads claimed align 32 for 16-byte accesses. This also lowers the align of 8-bit LDS loads in eight existing configs, moe_fused_mega_fp8[0..5] and gfx950_attention_tiled_2d[27], [31]; their ISA is unchanged. The differential golden is re-recorded for those two families only (32 hashes across llvm20/22/23/24).
  • The LDS budget charges lds_k_pad on every row. On gfx1250 it also accounts for the cshuffle C tile being aliased onto the A/B pool (max, not sum); gfx942/gfx950 keep develop's sum, since no CDNA shape the max admits has run on silicon.
  • The TDM ring slot parity is restored on the WMMA read path.
  • TDM fills are issued by the waves the kernel launches (block_size / wave_size). Counting warp_m * warp_n * warp_k gave a valid warp_k = 2 spec's B fill to a wave that is never launched, so B was never loaded. IR is unchanged for every warp_k = 1 spec.
  • split_k, emit_sched_hints and cshuffle_no_alias now reach the C++ engine through backend="cpp". They were dropped by the spec dict and by the binding, so the C++ engine validated and planned with their defaults; split_barrier's split_k rule and the gfx1250 cshuffle LDS budget depend on two of them.
  • Four is_valid_spec gaps against feat(rocke): gfx1250 BF16 universal GEMM load-path, epilogue, and persistent-kernel levers #12658 are closed; for example, lds_swizzle is now rejected on every WMMA target: the WMMA read path does not undo the swizzle, and gfx1151 returns wrong outputs with it as gfx1250 does.
  • Validation edges: ab_load_elem_bytes must be 1, 2, 4 or 8 and is rejected under tdm; the C++ engine rejects pipeline wmma_v1, as on develop, since it does not emit that schedule (Python keeps it); with K = 0 the TDM ring's fills start at the first K element, not one tile before it; the TDM tile origin, including a batched kernel's batch offset, is computed in i64, so an operand past 2^31 elements does not wrap.

Sweep harness and case study (python/rocke/examples/gfx1250/gemm_perf_fp8/)

  • A staged geometry and trait sweep:
    • one traits config section with inheritable configs;
    • a margin-based finalist cut;
    • machine state recorded per run;
    • ELF metadata (registers, AGPRs, spills, LDS) read on any arch.
  • sweep.py derives the arch from the ISA, and the HSACO cache key includes it.
  • build_all_instances takes an opt-in stall watchdog (stall_timeout, default off), so a build worker that dies cannot hang a stage; the fp8 sweep turns it on. Existing callers are unchanged.
  • The sweep and fp8_trait_ab.py share one exact input set (fp8_data.int_operands) and one GPU-contention check (gpu_contention.py).
  • Measurement tools, all reproducible from the repository:
    • fp8_trait_ab.py: interleaved, verified A/B of the K-loop traits;
    • wmma_roofline_probe.py: a data-matched WMMA ceiling;
    • memory_probe.py: bandwidth and the TDM feed floor;
    • clock_probe.py: in-kernel clock stamps and a device health check.
  • gemm_perf_fp8_case_study.md and the README record the method, the results and the conclusions it retracted.

Integration with develop

C ABI: ROCKE_ABI_VERSION 12 → 13

Not in this PR

  • Mixed fp8 x bf8 inputs, and bf8 on the 16x16x128 atom. Both engines' validators reject both: the catalog lists the instructions (at K=64 and K=128), but their K=128 rows have no verified lane maps yet, and an atom without A/B maps is rejected with one reason ("no verified A/B lane map for …") instead of failing at build time. They can come in a separate change with their own maps and tests.
  • A dispatch route. fp8 GEMM is opt-in: no KernelCandidate selects it, and dispatch/gemm/support.py rejects specs whose A/B and C dtypes differ. The measured kernel is the hand-built champion_spec in fp8_trait_ab.py.

Test Plan

  • Byte-identity gate (tools/check_byte_identity.py, llvm23) on the merged tree. Parity coverage added for the new paths:
    • gemm_emit configs 13–32: fp8, the load-width override, the LDS pad alignment, TDM depths 1–4, cshuffle on WMMA, the dense K=128 atom, the K-loop traits (also batched) and bf8;
    • three TDM descriptor configs in gfx1250_lowering_emit, built by core's tdm_descriptor_2d from an i64 address.
  • Differential golden: gemm_emit configs 13–32 are blessed, so CI checks their IR at llvm20/22/23/24 and in canonical mode.
  • C++ host unit tests: ctest -L unit, including rocke_dense_fp8_k128_hip (new), rocke_gemm_universal_spec_guards (new: the validator rules this PR adds, with their reason strings), rocke_opcode_values and rocke_mma_catalog_lookup.
  • rocke_arch_table_engine_parity (new CTest): the two engines' arch tables agree, run from the source tree.
  • rocke_gemm_spec_dict_binding_parity (new CTest): the keys the universal, batched, grouped and FlatMM spec dicts write are the keys fill_universal_spec and the family readers read, run from the source tree without the rocke_engine build.
  • Python host tests for the touched modules:
    • TDM, the K=128 fp8 atom, the gfx1250 foundation, the manifest runner and the arch catalog;
    • the trait A/B, memory probe and clock probe tools.
  • On a gfx1250 device:
    • fp8 GEMM verified bit-exact against an exact reference on integer inputs, at ten shapes including ragged ones and 4096x4096x8192;
    • every K-loop trait arm verified exact on integer inputs and within tolerance on random inputs, on several launches each, before any timing.

Test Result

  • Differential golden (run_diff.py --check-golden): passes for every family in ll mode at llvm20/22/23/24 and in canonical mode. Blessing gemm_emit configs 13–32 adds 69 hashes; after the first develop merge, the 7 configs on the K=128 atom change 21 hashes, only in the WMMA call. Deciding the TDM batch offset once (one i64 per operand) changes the .ll and canonical IR of configs 16–19 and 22–29 (36 hashes at llvm23/llvm24 and canonical), re-blessed for those configs only; their ISA is unchanged.

  • Byte-identity gate: GREEN at llvm20, llvm22, llvm23 and llvm24 (99, 99, 98 and 98 families), with no mismatches.

  • Arch domain: test_arch_domain_artifact.py passes at llvm20/22/23/24 with no key unmeasured; check_arch_domain.py is GREEN.

  • After the latest develop merge, ctest -L unit: 58 of 58 passed, including rocke_dense_fp8_k128_hip, rocke_gemm_universal_spec_guards (27 cases) and rocke_opcode_values. The full ctest: 62 of 62, one skipped (rocke_arch_domain_reproducibility), including rocke_arch_table_engine_parity and rocke_gemm_spec_dict_binding_parity.

  • Python host tests (the 33 test files this PR adds or changes, with the rocke_engine extension built): 531 passed, 1 skipped. At ROCKE_LLVM_FLAVOR=llvm22: 521 passed, 11 skipped (also the TDM cases).

  • On device: every fp8 shape and every trait arm verified, with 0 mismatches on integer inputs. The performance ratios are in Technical Details.

  • On device, rerun at 5d010905603 (later commits add tests, docs, refactors with no IR change, a fix that changes only batched TDM kernels and validation, and the review fixes; of those, only deciding the TDM batch offset once changes the measured kernels' IR, and it leaves their machine code identical: the four measured arms and the batched variant disassemble to the same instructions before and after; the toolchain, driver and SHA are recorded with the results, outside the repository): fp8_trait_ab with all four arms, then again at a ragged K. No other GPU process at the start or end of either run. Every arm verified before timing: exact on integer inputs (5 launches), max relative error 3.7e-3 (K = 65536) and 3.1e-3 (K = 8200) on random inputs (bound 7.8e-3). K = 8200 is not a multiple of the K tile, so it exercises the partial last tile in every K-loop, early refill's included.

    vs tdm_d2, random inputs K = 65536 K = 8200
    split_barrier 1.023x 1.035x
    + tdm_alternate_issuers 1.053x 1.038x
    + tdm_early_refill 1.068x 1.031x

    At K = 65536 the ratios match the table in Technical Details within noise. At K = 8200 the early refill falls below tdm_alternate_issuers, as the case study's K dependence predicts.

  • On device, after merging develop (the GEMM now issues the dedicated K=128 instruction): fp8_trait_ab with all four arms at K = 65536 and K = 8200. No other GPU process at the start or end. Every arm verified before timing: exact on integer inputs (5 launches), max relative error 3.7e-3 and 3.1e-3 on random inputs. The trait ratios match the runs above within run-to-run noise, and wmma_roofline_probe puts the dedicated instruction at 1.009x the f8f6f4 form on the same random data.

    vs tdm_d2, random inputs K = 65536 K = 8200
    split_barrier 1.014x 1.038x
    + tdm_alternate_issuers 1.039x 1.049x
    + tdm_early_refill 1.053x 1.037x
  • On a gfx1250 device after the review fixes (e357d26381d): universal_gemm_verify bit-exact for fp8 on the K=128 and K=64 atoms, at a ragged 520x264x8200, for bf8, with an fp16 C and with the cshuffle epilogue; fp8_trait_ab verifies all four trait arms (exact on integer inputs, 5 launches; within the bf16 rounding bound on random inputs). No other GPU process at the start or end.

Submission Checklist

@therock-pr-bot

therock-pr-bot Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot

Copy link
Copy Markdown

Pre-commit check failed

⛔ pre-commit failed

Please run locally:

  • python -m pip install pre-commit
  • pre-commit install
  • pre-commit run --all-files --show-diff-on-failure

This repo uses .pre-commit-config.yaml.

@therock-pr-bot

therock-pr-bot Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unsafe inbounds address calculations, an unversioned public ABI change, and verification that accepts NaN outputs leave critical correctness and compatibility defects unresolved.

14 open issues
What changed in this PR

Extends rocKE’s dual Python/C++ GEMM infrastructure for gfx1250 fp8/bf8 operands, adding dense WMMA support, TDM trait plumbing, and verification and tuning coverage.

Changes:

  • Adds the dense K=128 FP8 WMMA atom, lane maps, catalog entries, and lowering support.
  • Updates mixed-width manifests, vector loads, LDS alignment, and TDM/K-loop interfaces.
  • Adds host regressions, engine-parity checks, sweep configurations, and reproducible input data.

Review validation included source inspection and a dependency-free NaN-check reproduction; project builds and tests were not run.

File Description
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​test_sweep_machine_state_flush.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​test_sweep_elf_meta.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​test_sweep_build_pool.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​test_memory_probe.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​test_manifest_runner_registry.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​test_fp8_trait_ab.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​test_clock_probe.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​instances/​test_universal_gemm_verify.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​instances/​test_rocke_multiarch.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​instances/​test_gemm_tdm_traits_cpp_engine.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​instances/​test_gemm_tdm_ring_parity.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​instances/​test_gemm_tdm_early_refill.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​instances/​test_gemm_tdm_alternate_issuers.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​instances/​test_gemm_split_barrier.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​instances/​test_gemm_lds_budget_pad.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​instances/​parity/​gfx1250_lowering_emit.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​instances/​parity/​gfx1250_lowering_emit.c Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​core/​test_tensor_view_load_vec_dtypes.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​core/​test_tdm.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​core/​test_mma_catalog_lookup.cpp Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​core/​test_arch_table_engine_parity.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​core/​test_arch_mma_ssot.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​CMakeLists.txt Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​run_manifest.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​instances/​common/​manifest_runner/​gemm.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​helpers/​tensor_view.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​helpers/​manifest.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​examples/​gfx1250/​gemm_perf_fp8/​fp8_data.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​examples/​gfx1250/​gemm_perf_fp8/​configs/​sq4k.json Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​examples/​gfx1250/​gemm_perf_fp8/​configs/​skinny.json Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​examples/​gfx1250/​gemm_perf_fp8/​configs/​_base.json Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​examples/​gfx1250/​gemm_perf_fp8/​__init__.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​examples/​common/​universal_gemm_verify.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​core/​lower_llvm.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​core/​lower_hip.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​core/​isa/​backend.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​core/​ir.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​core/​backend.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​core/​arch/​target.py Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​core/​arch/​data/​arch_specs.json Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​instances/​common/​moe_gemm_fused.cpp Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​instances/​common/​gemm_mma.cpp Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​instances/​common/​conv_implicit_gemm_conv_load_phase.cpp Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​include/​rocke/​tdm.h Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​include/​rocke/​isa_backend.h Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​include/​rocke/​ir.h Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​include/​rocke/​ir_internal.h Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​include/​rocke/​instance_gemm_universal.h Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​include/​rocke/​instance_gemm_internal.h Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​include/​rocke/​helper_rocke.helpers.spec.h Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​helpers/​tensor_view.cpp Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​core/​lower_llvm/​mma.cpp Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​core/​lower_llvm/​mem.cpp Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​core/​lower_llvm/​data.cpp Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​core/​lower_hip/​lower_hip_mem.cpp Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​core/​isa/​backend.cpp Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​core/​ir/​ir_tile.cpp Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​core/​ir/​ir_flow.cpp Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​core/​ir/​core_types.cpp Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​core/​ir/​core_builder.cpp Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​core/​arch/​data.cpp Updated as part of this pull request.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​cpp/​bindings/​rocke_engine.cpp Updated as part of this pull request.

Review effort: Max

@jefyang1

Copy link
Copy Markdown
Contributor Author

Addressed all 14 review items in 9d779dc, 3426581, dd02210 and 9a1e498 (replies on each thread). Byte-identity gate GREEN (88 families), ctest -L unit 34/34, and the touched test files pass standalone. One item is resolved by narrowing scope rather than as suggested: fp8/bf8 GEMM is now gfx1250-only (#4).

@cgmillette

Copy link
Copy Markdown
Contributor

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max

PR #14707 — gfx1250 fp8 GEMM bring-up and K-loop optimization (jefyang1, AICK-2347) — 🔴 First-round: blocking — the LDS-alignment fix changes 8 develop fp8 configs without a re-bless, so CI's differential golden will fail at all four flavors (N1), and the case study puts device clock values into public history (N2); all 14 Copilot threads are addressed

Prior item Status
Copilot (10-29) ×2: the inbounds GEP of tile.global_addr_of is poison when a clipped TDM fill points past the allocation (lower_llvm.py:5340, mem.cpp:923) 🟢 FIXED 34265817 — both LLVM lowerings emit a plain GEP plus ptrtoint (lower_llvm.py:5333, mem.cpp:919), and both HIP lowerings form the address in integers (lower_hip.py:1136, lower_hip_mem.cpp:438). test_global_addr_of.py passes and byte identity is GREEN (ran)
Copilot (10-29): the trait and GEMM spec structs change without an ABI bump (instance_gemm_universal.h:107) 🟢 FIXED 34265817 — ROCKE_ABI_VERSION and BINARY_ABI go 4 → 5, with a history line naming all seven new fields (abi.h:88) (read). The change is not append-only, so the bump is required, and it collides with #14616's 5 (Our checks)
Copilot (10-29): the verify lane always uses the OCP fp8 encoder, which gfx942's FNUZ MFMA misreads (gemm.py:296) 🟢 ADDRESSED 9d779dcc by narrowing scope, one of the two options Copilot gave: both engines refuse fp8/bf8 A/B off gfx1250 with the same message (gemm_universal.py:668, gemm_spec.cpp:713), and build_universal_gemm runs that gate, so the verify CLI cannot build a gfx942 fp8 kernel. Parity config 31 (fp8 on gfx950) is in the gemm family, which is byte-identical at llvm20/22/23, and TestFp8IsGfx1250Only passes (ran)
Copilot (10-29): a NaN output is not counted as a mismatch (gemm.py:316) 🟢 FIXED 9d779dcc — bad = ~(out_f32 == ref_f32), and a NaN error reports as inf (gemm.py:343); test_nan_output_is_a_mismatch passes (ran). Develop's f16/bf16 and batched checks (:94, :409) still threshold the error, so a NaN passes there; that predates this PR
Copilot (10-29) ×8, tests: positional parser vs designated initializers; source-only parity test in the installed cohort; missing arch; three bare sibling imports; the fp8×fp8 catalog now has K=64 and K=128; sweep module loaded by source path 🟢 FIXED dd022108 — the parser reads each .field = by name (test_arch_table_engine_parity.py:55), and the file is excluded from the installed cohort (platform/CMakeLists.txt:446), so no CI lane runs it now; arch="gfx1250" at both calls; sys.path.insert in all three files; fp8×fp8 expects both shapes (test_rocke_multiarch.py:68); both sweep tests import by package name. Each of these files passes in our run, apart from the flavor failures in N3 (ran)
Copilot (10-29): the dense K=128 atom has no C++ HIP lowering (lower_hip.py:919) 🟢 FIXED 9a1e498c — h_emit_gfx1250_dense_f8f6f4 (lower_hip_mma.cpp:144) emits the same 6-operand builtin as Python (lower_hip.py:915); the new rocke_dense_f8f6f4_hip ctest and test_dense_f8f6f4_hip.py pass (ran).
🟡 DEFERRED, the C++ HIP gap for the K=64 fp8/bf8 atoms: "predates this PR … separate change". True: develop's Python HIP lowers those atoms (lower_hip.py:903), and its C++ never did. Acceptable, since the C++ engine refuses them (ROCKE_ERR_NOTIMPL) rather than emitting wrong code (read)
jefyang1 (body 10-29, summary 10-30): all 14 items addressed; byte identity GREEN on 88 families; ctest -L unit 34/34; the touched test files pass standalone; the golden anchor is not re-blessed because it "predates many unrelated develop changes" 🟡 Partly. Byte identity holds: every family at llvm22 and the touched families at llvm20/22/23 are GREEN, and our ctest is 35/35 (ran). The TDM test files pass only at an LLVM flavor of llvm23 or later (N3). The anchor reason is out of date: #13974 (10-26) repaired this golden and made it a CI check. Develop passes it for the affected families, and the only mismatches on the merge and at head are this PR's 8 configs (N1)
Bots / CI 🟡
• Policy bot pass (10-29); the first head's pre-commit failure (dfd02ac4) was fixed by 3c987f41. At head 9a1e498c: pre-commit, clang-tidy, Component CI, both hipDNN superbuilds, the Multi-Arch ASAN summary and base freshness pass.
• TheRock Multi-Arch (run of 00:08Z): both compiler-runtime stages and the gfx125X and gfx950 math-libs builds pass. The gfx94X and Windows gfx110X builds were still running at 01:37Z. No hipkernelprovider test job has run at any head, because later pushes cancelled the runs at dfd02ac4 and 3c987f41 before their tests. When the shards run, rocke_differential_golden_ll will fail (N1).
• The red emulation stage is not this PR's. Its build and build tests finished; then artifact_manager.py push segfaulted (exit 139) while uploading the emulator artifacts. The stage passed at 3c987f41 and on other PRs' runs that finished between 00:50Z and 01:01Z, and this PR touches nothing it builds → re-run the job (read: CI logs)
Our checks 🔴
• Merge (ran): clean with develop df91d568 (14 behind, 34 ahead; merge 1a83c010). Native build OK, ctest 35/35 (1 skip), including the new rocke_test_tdm and rocke_dense_f8f6f4_hip. Byte identity is GREEN for the touched families at llvm20/22/23 (including gemm's 32 configs and gfx1250_lowering's 116) and for every family at llvm22. Representative golden PASS ×3. Differential golden: canonical OK; ll fails at llvm20/22/23/24 (N1). ROCm clang 24 -O3 compiles 104 configs of conv_implicit_gemm, gemm, gfx1250_lowering and moe_gemm_fused: 0 failures, 0 new rejections, 0 spills.
• Defaults off (ran): apart from N1's 8 configs, no develop config's IR changes at any flavor, and the canonical IR is unchanged, so the new traits and the load-width change leave existing kernels alone. NEW is 206 at llvm23/24, against develop's usual 180: these are the PR's new parity configs, which the gate reports but does not fail on.
• Tests (ran): the PR's 27 test files on CPU, with our harness's ROCKE_LLVM_FLAVOR=llvm22: 334 passed and 8 skipped. 28 tests in 7 files fail or error, all because "tensor_load_to_lds requires LLVM flavor llvm23" (N3). With the flavor left to COMGR (LLVM 24 here, as with CI's toolchain), test_gemm_split_barrier.py (9) and test_sweep_config.py (30) pass, and the TDM specs behind the other failures lower cleanly. None of them needs a GPU; the three sweep tests need COMGR to build HSACO.
• ABI (read; ran: merge-tree): not append-only. tdm and tdm_depth are inserted mid-struct, before lds_swizzle (instance_gemm_universal.h:106–107), and the trait struct sits mid-struct in rocke_gemm_universal_spec_t and eight other GEMM and MoE specs. No positional initializer or ctypes mirror uses it. #14616 also claims 5: merging its head into this merge conflicts only in abi.h, while abi.py's identical BINARY_ABI = 5 line merges silently, so whichever PR lands second must bump both to the next free number.
• Rules (ran): no tracker IDs, PR numbers or links in the added code or docs. The only environment reads are tool paths in the example scripts. The rule scan's five "perf" hits are the timing protocol's idle gap and a synthetic test value, not results. The body and the 33 commit messages carry only ratios and percentages of a measured ceiling. The case study's clock values are N2
# Sev file:line Finding
N1 🔴 core/lower_llvm.py:1271, :1286; cpp/core/lower_llvm/mem.cpp:35; differential/golden/llvm_gfx_all.json Problem: the LDS-alignment fix (0435380e: the 8-bit floats count as 1 byte, and align is bounded by gcd(vec, innermost dim)) also lowers the align of 8-bit LDS loads in 8 develop configs: moe_fused_mega_fp8[0..5] and gfx950_attention_tiled_2d[27], [31] (<16 x i8> from 32 to 16, <8 x i8> from 16 to 8). Nothing else in their IR changes, and their gfx950 -O3 code is identical before and after (ran: ROCm clang 24 on 5 of the 8). The golden was not re-blessed. On develop these families pass; on the merge and at head exactly these 8 configs mismatch, at every flavor (ran).
Consequence: rocke_differential_golden_ll, a quick-tier CTest that every hipkernelprovider CI shard runs, fails at this head with "GOLDEN MISMATCH" (ran: the CI command), and after merge it would fail on every PR.
Fix: from platform/tests/instances/differential, run python run_diff.py --mode ll --python-only --record-golden --flavor llvm20,llvm22,llvm23,llvm24 --only moe_fused_mega_fp8,gfx950_attention_tiled_2d. On a scratch copy of the merge's golden, this changes exactly those 24 hashes (llvm24 aliases llvm23), plus a harmless re-sort of gemm's keys 11–12; the check then passes at all four flavors (ran)
N2 🔴 examples/gfx1250/gemm_perf_fp8/gemm_perf_fp8_case_study.md:74, :76 Problem: §2(c) gives the device's memory-clock DPM level and its shader clock as absolute MHz values. The repo's review rules allow no absolute performance numbers in docs; the rest of the case study and the body correctly use ratios and percentages. The lines arrive in 4bce34ba and stay in every later commit.
Consequence: merging as is puts these values into develop's public history, where they remain after any later edit.
Fix: describe the episode without the values (for example "a drop between two memory-clock levels"; "the higher figure was the shader clock"), then squash on merge, or rewrite the branch, so that no commit keeps them
N3 🟡 instances/test_gemm_split_barrier.py:98, test_gemm_tdm_ring_parity.py:92, test_gemm_validity_gaps.py:87, tests/test_clock_probe.py:28; gemm_perf_fp8/fp8_gemm_sweep.py:625 Problem: the new TDM tests lower tensor_load_to_lds kernels at whatever LLVM flavor the environment gives. Under ROCKE_LLVM_FLAVOR=llvm22, 28 tests in 7 files fail or error ("requires LLVM flavor llvm23"). warn_if_sweep_is_inert turns the same error into an empty warning list (:625), so its three tests fail on [] rather than on the cause. Develop's TDM tests pin the flavor (core/test_gfx1250_tdm.py:48).
Consequence: CI should pass them, since nothing there pins the flavor and TheRock's compiler is LLVM 24 (ran: both files pass here at COMGR's flavor). But a run pinned to llvm20/22, or a host with an older COMGR, reports 28 failures that look like regressions.
Fix: pass llvm_flavor="llvm23" in these _lower helpers, and pin or skip the HSACO-based inertness tests the same way

Utility: 🟢 Positive — gfx1250 fp8 GEMM with TDM in both engines; re-bless, drop clock values.


Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted

First-round review: a local qwen3.6 model made the independent first pass, and the orchestrator (Claude Opus 5.5) verified it. On a clean merge with develop, it ran the native build and ctest, byte identity, both golden gates, ROCm clang 24 compiles of four families and the PR's 27 test files. It re-ran the differential golden for the two failing families on develop, the merge and the head, diffed their IR and gfx950 ISA, and tested the re-bless on a scratch copy of the golden. It traced the 28 test failures to the pinned LLVM flavor with single-config lowerings and two single-file runs at COMGR's flavor. It checked each Copilot fix at head, compared the public headers with develop, simulated a merge with #14616's head, scanned the added docs and all commit messages, and read the CI jobs and the emulation log 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.

@cgmillette

Copy link
Copy Markdown
Contributor

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max

PR #14707 — gfx1250 fp8 GEMM bring-up and K-loop optimization (jefyang1, AICK-2347) — 🔴 Follow-up: the re-bless fixes our N1, and CI's differential golden now passes at this head; but 33 earlier commits still carry the device clock values, so this must land by squash or after a branch rewrite (N2); N3 is down to one llvm22-only test

Prior item Status
Copilot (10-29) ×14: inbounds GEP ×2; ABI bump; gfx942 FNUZ encoder; NaN not counted as a mismatch; 8 test fixes; no C++ HIP lowering for the dense K=128 atom 🟢 Unchanged since our first round, where we checked each item at 9a1e498c: 13 are FIXED, and the FNUZ encoder is ADDRESSED by making fp8/bf8 GEMM gfx1250-only. The C++ HIP gap for the older K=64 atoms stays DEFERRED; that is acceptable, because the C++ engine refuses those atoms rather than emitting wrong code. aa422f14 changes none of the code behind these items. All 14 threads are resolved, and no new thread has been opened (read: git diff, gh)
Ours (10-30) N1 🔴: the LDS-alignment fix changed 8 develop fp8 configs without a differential re-bless 🟢 FIXED aa422f14, exactly as proposed.
• The re-bless (ran: JSON diff): 24 hashes change: moe_fused_mega_fp8[0..5] and gfx950_attention_tiled_2d[27], [31], each at llvm20, llvm22 and llvm23 (llvm24 aliases llvm23). No key is added or removed, and the only other change re-sorts gemm's keys 11–12. The result matches our first-round scratch re-bless leaf for leaf.
• Gate (ran): on the merge, the differential golden is OK in ll mode at llvm20/22/23/24 and in canonical mode.
• CI (read: logs): at this head, both gfx94X hipkernelprovider shards passed rocke_differential_golden_ll (GOLDEN OK at all four flavors) and rocke_differential_golden_canonical. The body now describes the re-bless. There is no reply to our comment
Ours (10-30) N2 🔴: the case study put absolute device clock values (MHz) into the PR and its public history 🔴 OPEN, history only: the files are clean, but the history is not.
• Files (ran): aa422f14 rewords §2(c) without the values (gemm_perf_fp8_case_study.md:74, :76) and also drops the §5 device temperatures (:136). No clock or temperature value is left in any file the PR changes; the only MHz hits are unit labels in clock_probe.py. The body and all 35 commit messages contain no such values.
• History (ran: git log -p over the PR range): the branch was not rewritten; aa422f14 is a new commit on top of 9a1e498c. The values arrive in 4bce34ba and stay in the tree of all 33 commits from 4bce34ba to 9a1e498c, and so do the §5 temperatures.
• At merge (read: gh): develop's ruleset allows only squash and rebase merges. A squash merge lands a single new commit, titled with the PR title and with the body as its message, so develop's history would not contain the values. A rebase merge would replay 4bce34ba and the commits after it onto develop, values included. Auto-merge is off, so the merge method is not set yet.
→ land it with squash only, or rewrite the branch (for example, squash it) so that no commit keeps the values. Only a rewrite also removes them from this PR's own commits
Ours (10-30) N3 🟡: the TDM tests don't pin llvm_flavor="llvm23"; 28 failures under ROCKE_LLVM_FLAVOR=llvm22 🟡 Mostly FIXED aa422f14. The four IR helpers now pin llvm23 (test_gemm_split_barrier.py:101, test_gemm_tdm_ring_parity.py:94, test_gemm_validity_gaps.py:90, test_clock_probe.py:31), and the HSACO inertness tests skip below llvm23 (test_sweep_config.py:206).
• Left (ran: single-file pytest at llvm22): 5 failures, down from 28, all in one test. test_both_engines_emit_identical_kernels (test_gemm_tdm_traits_cpp_engine.py:92) fails once per arm: tdm_d2, split_barrier, alternate_issuers, split_alternate and early_refill. Both engines report "tensor_load_to_lds requires LLVM flavor llvm23, got llvm22", because lower_universal_gemm takes no flavor and uses the environment's.
• CI is not affected (read: logs): this class needs the rocke_engine binding, which CI never builds, so CI skips it. In both gfx94X shards, every other TDM test file passes, including test_sweep_config.py.
→ skip this class below llvm23, as test_sweep_config.py:206 does
Ours (10-30) ABI note (not marked abi-ok): not append-only; the 4 → 5 bump collides with #14616's 5 🟡 OPEN, land order.
• Unchanged (ran: git diff): abi.h:92 and abi.py:78 are still 5 (develop has 4), and aa422f14 touches neither file nor the struct.
• #14616 (head 13619c6c, unchanged since 10-29) still sets 5 in both files. Merging its head into this merge now conflicts in abi.h and also in the differential golden: in gfx950_attention_tiled_2d at llvm20, llvm22 and llvm23, a family both PRs re-bless. abi.py's identical BINARY_ABI = 5 line still merges silently (ran: merge-tree).
→ whichever PR lands second must bump both constants to the next free number and re-record gfx950_attention_tiled_2d at all four flavors
Bots / CI 🟡
• At head aa422f14 (CI started 02:46Z): the policy bot, pre-commit, clang-tidy, Component CI, the Multi-Arch ASAN summary, base freshness and both hipDNN superbuilds (Linux, Windows) pass. The one cancelled check is a duplicate Libraries PR Bot run; its twin passed.
• TheRock Multi-Arch CI was still running at 04:18Z. Compiler-runtime, emulation and the math-libs stages for gfx94X, gfx950, gfx125X and Windows gfx110X pass. Both gfx94X hipkernelprovider shards passed 49/49 CTests, including rocke_differential_golden_ll, rocke_differential_golden_canonical, rocke_golden_static and rocke_pytest. The two Windows gfx110X hipkernelprovider shards are queued. gfx950 and gfx125X run no test jobs on PR CI.
• GitHub: MERGEABLE but BLOCKED (review required from rocKE-platform; Multi-Arch CI still running); auto-merge is off. There have been no reviews, threads or replies since our first round (read: gh)
Our checks 🟢
• The push (ran: git): one commit, aa422f14, on top of 9a1e498c (no rebase). It changes 7 files: the golden, the case study, four test helpers and test_sweep_config.py.
• Merge ac79abfe (head + develop 44502aed; 16 behind, 35 ahead, 0 conflicts):
– native build OK and ctest 35/35 (1 skip);
– byte identity GREEN ×3 for the touched families, and at llvm22 for every family;
– representative golden PASS ×3;
– differential golden OK (ll at llvm20–24 and canonical; 0 changed, 0 missing). NEW is 126 at llvm20/22 and 206 at llvm23/24 and canonical, against develop's 115 and 180. The difference is the PR's own new parity configs, which the gate reports but does not fail on;
– compile checks of conv_implicit_gemm, gemm, gfx1250_lowering and moe_gemm_fused: 0 failures, 0 new rejections, 0 spills;
– the PR's 27 test files on CPU at llvm22: 356 passed, 12 skipped, 5 failed (N3) (ran).
• Develop since our first round: 2 commits. Only #14710 (GDN lane no longer registered on Windows) touches rocke/, and it touches none of this PR's files.
• Rules 1–3 (ran): the added lines have no tracker IDs. The "perf" hits are the timing protocol's default idle gap (--gap-ms, in 4 files) and a synthetic offset in a test fixture (test_clock_probe.py:104); neither is a result. The only environment reads are tool and output paths in the example tools

Net-new: none.

Utility: 🟢 Positive — gfx1250 fp8 GEMM with TDM in both engines; land it by squash.


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. The orchestrator compared the new commit with our first-round head file by file. It diffed the differential golden key by key, against both the previous golden and our first-round scratch re-bless. It searched every commit, commit message and changed file in the PR range for the removed device values. On a clean merge with develop, it ran the native build and ctest, byte identity, both golden gates, compile checks of four families and the PR's 27 test files, then re-ran the failing test file on its own. It simulated a merge of #14616's head with git merge-tree. It read CI at head, including the logs of both finished hipkernelprovider shards, along with the repository's merge settings and the GitHub activity since our first round. The orchestrator owns this merge-worthiness call. This review flags whether a closer look is needed; it is not an approval or a rejection.

@cgmillette

Copy link
Copy Markdown
Contributor

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max

PR #14707 — gfx1250 fp8 GEMM bring-up and K-loop optimization (jefyang1, AICK-2347) — 🔴 Follow-up (CI update): both Windows shards fail the PR's new test_sweep_build_pool.py (Windows has no fork start method), so the required Multi-Arch CI Summary is red (N4); our follow-up above still stands

Prior item Status
Ours (10-30, the follow-up above): N2 🔴 device clock values in 33 earlier commits; N3 🟡 one llvm22-only test; ABI note 🟡 Unchanged: no push since aa422f14
Bots / CI 🔴 At aa422f14, both Windows gfx110X hipkernelprovider shards fail rocke_pytest (103 of 104 CTests pass), and the required Multi-Arch CI Summary fails. On both shards the only failures are the three TestSweepBuildPool tests. Both Linux gfx94X shards pass (read: logs)
# Sev file:line Finding
N4 🔴 platform/tests/test_sweep_build_pool.py:56 The new test (42173df4) asks for mp.get_context("fork"), which Windows does not have: all three tests stop with ValueError: cannot find context for 'fork' on both gfx110X shards (read: logs). The test needs fork by design: the pool's workers must inherit the patched _build_one, and _fake_build kills its worker with signal.SIGKILL, which Windows lacks too. The sweep itself is unchanged in this respect: develop's sweep.py already uses a fork context (read) → skip the class where fork is unavailable, e.g. @unittest.skipUnless("fork" in mp.get_all_start_methods(), "needs the fork start method")

Utility: 🟢 Positive — unchanged; land it by squash once N2 and N4 are done.


Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted

CI update to the follow-up above: once the Windows shards finished, the orchestrator (Claude Opus 5.5) read both gfx110X hipkernelprovider shard logs at aa422f14, and the new test and sweep.py at head 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.

@CongMa13

Copy link
Copy Markdown
Contributor

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.

@cgmillette

Copy link
Copy Markdown
Contributor

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max

PR #14707 — gfx1250 fp8 GEMM bring-up and K-loop optimization (jefyang1, AICK-2347) — 🟡 Follow-up: squash fixes N2–N4; CI running

Prior item Status
Copilot (10-29) ×14: GEP ×2; ABI; FNUZ; NaN; 8 tests; C++ HIP 🟢 FIXED, unchanged
• squash 5cc3f5e4 keeps every fix; 14/14 threads resolved (read)
• FNUZ: fp8/bf8 gfx1250-only; K=64 C++ HIP gap deferred, acceptable
Our 10-30 N1: LDS-align change needed a differential re-bless 🟢 FIXED aa422f14
• re-bless kept; differential OK ×4 flavors + canonical (ran)
Our 10-30 N2: device clock values in earlier commits 🟢 FIXED 5cc3f5e4
• one commit now; tree and message carry no values (ran)
• squash or rebase merge both safe
Our 10-30 N3: TDM tests fail at llvm22 🟢 FIXED 5cc3f5e4
• last class skips below llvm23 (test_gemm_tdm_traits_cpp_engine.py:94)
• llvm22: skips; llvm23: 3 ✓ (ran)
Our 10-30 ABI note: 4 → 5 collides with other PRs 🟡 OPEN, land order
• develop is 4; takes develop + 1 when it lands (order: rk abi)
• abi.h conflicts with #14706, #14616; #14616 also in the golden (ran: merge-tree)
• whichever lands second: re-bump both files, re-record gfx950_attention_tiled_2d
Our 10-30 N4: build-pool test needs fork; Windows red 🟢 FIXED 5cc3f5e4
• no fork: 3 skipped (ran: simulated); Linux: 3 ✓ (ran)
• Windows gfx110X test shards not run yet
CongMa13 (10-30): no author reply on our 3 posts 🟡 OPEN
• fixes are in; no reply yet (read)
• jefyang1: reply on each post
Approvals 🟢 None yet
• rocKE-platform review requested (read)
Bots / CI 🟡 running
• 32 ✓ · 3 running · 0 ✗; 1 cancelled duplicate PR Bot run
• running: gfx94X hipkernelprovider shards 1–2; Windows gfx110X math-libs, then its test shards
• merge: BLOCKED, review required; auto-merge off
Our checks 🟢 Clean
• merge vs 64c2954a (2 behind): clean; ctest 35/35, 1 skip (ran)
• moved ctests follow #14494's layout: test_cmake_test_registration.py 5 ✓ (ran)
• byte identity GREEN: 24 families ×3 flavors; all 88 at llvm22 (ran)
• goldens PASS ×3; differential OK ×4 + canonical; 11/26 new configs unblessed, as before (ran)
• 27 PR test files at llvm22: 355 ✓ / 13 skip; compile 113 configs clean; rules 1–3 clean (ran)

Net-new: none.

Utility: 🟢 Positive — gfx1250 fp8 GEMM with TDM and K-loop traits in both engines.


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: PR patch vs our last head, merge with develop, build + ctest, byte identity, goldens, 27 test files, compile, lane merge-tree, CI at head. Flags whether a closer look is needed; not an approval or rejection.

@jefyang1

Copy link
Copy Markdown
Contributor Author

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max

PR #14707 — gfx1250 fp8 GEMM bring-up and K-loop optimization (jefyang1, AICK-2347) — 🔴 First-round: blocking

Thanks — all addressed, now in the squashed commit 5cc3f5e:

N1: the differential golden is re-recorded for moe_fused_mega_fp8[0..5] and gfx950_attention_tiled_2d[27], [31] only; --check-golden is OK in ll mode at llvm20/22/23/24 and in canonical mode.
N2: the device values are removed from the case study, and the branch is squashed to one commit, so no commit in the PR carries them.
N3: the TDM tests pin llvm_flavor="llvm23", and the HSACO and engine-parity tests skip below llvm23.
ABI: agreed. Whichever of this PR and #14616 / #14706 lands second will bump ROCKE_ABI_VERSION and BINARY_ABI to the next free number and re-record gfx950_attention_tiled_2d.

@jefyang1

Copy link
Copy Markdown
Contributor Author

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max

PR #14707 — gfx1250 fp8 GEMM bring-up and K-loop optimization (jefyang1, AICK-2347) — 🔴 Follow-up: the re-bless fixes our N1, and CI's differential golden now passes at this head; but 33 earlier commits still carry the device clock values, so this must land by squash or after a branch rewrite (N2); N3 is down to one llvm22-only test

Both remaining items are done in 5cc3f5e:

N2: the branch is squashed to a single commit on top of develop, so the history no longer contains the values, whichever merge method is used.
N3: test_both_engines_emit_identical_kernels now skips below llvm23. At llvm22 it skips; at llvm23/24 it passes.

@jefyang1

Copy link
Copy Markdown
Contributor Author

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max

PR #14707 — gfx1250 fp8 GEMM bring-up and K-loop optimization (jefyang1, AICK-2347) — 🔴 Follow-up (CI update): both Windows shards fail the PR's new test_sweep_build_pool.py (Windows has no fork start method), so the required Multi-Arch CI Summary is red (N4); our follow-up above still stands

N4 is fixed in 5cc3f5e: TestSweepBuildPool is skipped unless fork is in mp.get_all_start_methods(), so the Windows shards skip it, and Linux still runs all 3 tests.

@jefyang1

Copy link
Copy Markdown
Contributor Author

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.

@CongMa13 replied on all three; the fixes are in 5cc3f5e.

@jefyang1

Copy link
Copy Markdown
Contributor Author

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max

PR #14707 — gfx1250 fp8 GEMM bring-up and K-loop optimization (jefyang1, AICK-2347) — 🟡 Follow-up: squash fixes N2–N4; CI running

Thanks for the re-check on 5cc3f5e. I've replied on the three earlier reviews. The ABI merge order stays as noted: whichever of this PR and #14616 / #14706 lands second will bump ROCKE_ABI_VERSION and BINARY_ABI to the next free number and re-record gfx950_attention_tiled_2d.

illsilin added a commit that referenced this pull request Oct 30, 2026
…a split barrier (#14466)

JIRA ID : AICK-2347

## Summary

A fast fp8 GEMM for gfx1250 in CK Tile. This PR adds `comp_tdm_v3`, an
opt-in GEMM pipeline: a 16-wave tile fed by the Tensor Data Mover (TDM),
with the per-tile workgroup barrier split into arrive and wait so the
WMMAs overlap the wait. At the lead shape, 4096 x 4096 x 65536 (RCR, fp8
e4m3 in, fp32 accumulate, bf16 out), it matches the fastest known
gfx1250 fp8 GEMM. Everything is opt-in: existing kernels compile
byte-for-byte the same.

## What's new

- **`comp_tdm_v3` pipeline** (gemm): 16 waves with a waves-aware
register budget; one split barrier per K tile, with B held in registers
and A streamed per row slice; the A row windows built outside the K
loop; TENSORcnt drained on exit. The joint barrier stays available
(`kUseSplitBarrier = false`), and an early-refill policy
(`RefillPoint::AfterArrive`) is available opt-in; it brings no gain at
the lead shape, so it is off by default.
- **Split workgroup barrier** (core): fenced, pinned
`workgroup_barrier_arrive` / `_wait`.
- **tile_engine**: `comp_tdm_v3` registered for `gemm_universal`
(`COMPUTE_TDM_V3`, `[4,4,1]` and CShuffle for this pipeline), the
fp8/bf8 32x32x128 warp tile on gfx1250, opt-in bf16 C for fp8/bf8,
`-init 3` (normal) and `-init 4` (integers), and `-verify_exact`. Its
targets build with the post-RA scheduler.
- **Example** `53_gemm_comp_tdm_v3_fp8`: a timed benchmark and verifier
for the lead shape.

## Performance

Ratio to the fastest known gfx1250 fp8 GEMM at the lead shape (the rocKE
kernel of rocKE PR #14707, with the split barrier and alternating TDM
issuers), random data, builds interleaved sample by sample in one
session on one gfx1250 host:

| | Peak | Sustained |
|---|---|---|
| `comp_tdm_v3`, default policy | 1.008x | 1.001x |

Differences of about 1% or less are within session noise, so this is
parity. Every timed build was bit-exact at 2048³ and passed integer and
random checks at the lead shape first.

## Scope of `comp_tdm_v3`

Supported and tested:
- fp8 or bf8 A and B, including mixed fp8 x bf8; bf16 C;
- A row-major, B column-major, C row-major (RCR);
- 16 waves (256x256x256) and 4 waves (128x128x256), warp tile 32x32x128;
- the plain GEMM kernel with the CShuffle epilogue;
- `k_batch = 1`.

Rejected:
- **Split-K.** `IsSupportedArgument` rejects `k_batch != 1`.
- **Non-RCR layouts.** The pipeline has a `static_assert`, and
tile_engine rejects them. The other A/B layouts spill VGPRs in the K
loop at the 16-wave tile.
- **Other kernels.** The multi-D, grouped and batched-contraction
kernels have a `static_assert` against the pipeline's new
`kPlainGemmOnly` marker.
- **tile_engine.** It offers the pipeline for fp8/bf8 and
`gemm_universal` only.

Not in scope: bf16/fp16 A/B (they compile but are not tested), LDS
padding, other C types, MX GEMM and group quant. `comp_tdm_v2` remains
the TDM path for MX GEMM and group quant.

## Default behaviour changes

- The new pipeline and every tile_engine rule are opt-in. No default or
CI config lists `comp_tdm_v3`.
- Existing kernels are byte-identical before and after this PR (gfx1250,
plus gfx942 and gfx950 cases), on TheRock 10.1.0 and 10.2.0.
- **One host-side change.** `UniversalGemmKernel::IsSupportedArgument`
used to return `true` for every TDM pipeline right after the general
k_batch checks. It now returns `IsTdmArgumentSupported(...)`, the check
the multi-D, grouped, batched-contraction and MX kernels already apply:
`k_batch == 1`, `K > 0`, element-aligned A/B/E.
- For comp_tdm / comp_tdm_v2 / comp_tdm_pc in the plain GEMM kernel,
split-K was rejected only for fp16/bf16 C. With fp32 C, k_batch > 1
passed the generic split-K check, although the TDM epilogue's tensor
store overwrites the output tile; it is now rejected. A/B/E pointers
must also be element-aligned, which TDM addressing requires. No test
used k_batch != 1 or misaligned pointers with a TDM pipeline.
- Device code is unchanged: on ROCm 10.1.0, the device ISA is identical
for 235 kernels (the TDM tests, compv3 and example 53).

## Testing

On TheRock 10.1.0 (CK CI compiler) and 10.2.0, gfx1250 under the
rocJITsu emulator; no hardware CI exists for gfx1250 yet:
- gtests for the split barrier and `comp_tdm_v3` (both barrier forms,
4x4 and 2x2 wave maps, alternating and fixed issuers, long K),
bit-exact.
- The example at 256 x 256 x {1024, 65536} and odd K-tile counts:
bit-exact on integers, within tolerance on random data.
- ISA checks: 0 in-loop scratch, VGPRs <= 256, the expected barrier and
wait placement; 13 of 13 planted sync bugs caught.
- Other GPU targets still parse the headers (gfx942, gfx950, gfx1201).

On gfx1250 hardware, at the head of this PR:
- `comp_tdm_v3` gtests: 55 of 55 short and 16 of 16 long-K passed,
including bf8, mixed fp8/bf8 and the split-K rejection check.
- The scratch-use check passed: 9 kernels, no scratch, no VGPR spills.

Compile-only checks (ROCm 10.1.0, gfx1250):
- `comp_tdm_v3` with RRR, CRR, CCR and RCC each fails on the layout
`static_assert`; RCR compiles.
- `comp_tdm_v3` in the multi-D, grouped and batched-contraction kernels
each fails on the `kPlainGemmOnly` `static_assert`; the plain GEMM
kernel compiles.
- The existing multi-D, grouped and batched-contraction TDM gtests still
compile, and the `comp_tdm_v3` gtests build with `-Werror -Weverything`.

## Notes

- **Merge.** Squash and merge, per rocm-libraries convention. The
commits form a reviewable stack; each message has a `Gate:` paragraph
with its evidence.
- **Not in this PR.** Dispatcher registration of `comp_tdm_v3` (and bf16
C for fp8 there) conflicts with open #13975; it follows after #13975
lands.
- **Credit.** The split-barrier K-loop, the B-resident / A-streamed
order and the post-RA scheduler finding are Jeffrey Yang's, from the
rocKE gfx1250 fp8 GEMM (rocKE PR #14707). He co-authored several commits
and authored the scope gates.

## Submission Checklist

- [x] Look over the contributing guidelines at
https://github.com/ROCm/TheRock/blob/main/GOVERNANCE.md#pull-requests.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-authored-by: Jeffrey Yang <jeffreyj.yang@amd.com>
Co-authored-by: Illia Silin <Illia.Silin@amd.com>
Co-authored-by: Illia Silin <98187287+illsilin@users.noreply.github.com>
…2347)

Bring up the universal GEMM for 8-bit floats on gfx1250 in both engines, fed by
the Tensor Data Mover (TDM), and optimise its K-loop with three opt-in traits.
Inputs are fp8 x fp8 or bf8 x bf8 with a bf16 or fp16 C; fp8 runs on the
16x16x64 and the dense 16x16x128 f8f6f4 WMMA atoms, bf8 on the 16x16x64 atom.
Every new trait defaults off, and the Python and C++ engines emit
byte-identical LLVM IR throughout.

fp8 GEMM on gfx1250
- fp8e4m3/bf8e5m2 A/B with a 16-bit C; _validate_storage splits the A/B dtype
  from the C dtype. fp8/bf8 A/B is accepted on gfx1250 only.
- Lane maps for the K=64 fp8 atom; the dense 16x16x128 f8f6f4 atom, with both
  matrix formats pinned to FP8, lowered to LLVM in both engines and to HIP.
- Global A/B loads use the operand's element size, so 1-byte operands get full
  16-byte loads; TraitSpec.ab_load_elem_bytes overrides it for A/B comparison.
- The cshuffle epilogue on the WMMA path, its staging tile typed by the C dtype.
- bf8 verification encodes e5m2 inputs; the manifest runner verifies fp16 C and
  counts NaN outputs as mismatches.

TDM in the universal GEMM
- A 2D TDM descriptor builder in both engines (core/tdm.py, cpp/core/tdm.cpp,
  C ABI rocke/tdm.h), cross-checked field by field against the hardware layout.
- tile.global_addr_of: a global address as an i64, without an inbounds GEP, so
  it stays defined past the end of the buffer. Appended to rocke_opcode_t.
- TraitSpec.tdm / tdm_depth: 1 unpipelined, 2 ping-pong, 3+ a modular ring.

K-loop traits (depth-2 TDM ping-pong)
- split_barrier: the per-tile barrier split into signal and wait; the ring
  takes the same split.
- tdm_alternate_issuers: each LDS half gets its own issuer wave pair.
- tdm_early_refill: refill a half as soon as every wave has read it.

Correctness fixes
- LDS vector alignment is bounded by the row stride; 8-bit floats count as one
  byte in the LDS element map. This lowers the align of 8-bit LDS loads in
  moe_fused_mega_fp8[0..5] and gfx950_attention_tiled_2d[27], [31] (ISA
  unchanged); the differential golden is re-recorded for those families.
- The LDS budget charges lds_k_pad on every row and aliases the cshuffle C
  tile onto the A/B pool.
- The TDM ring slot parity is restored on the WMMA read path.
- Four is_valid_spec gaps against upstream are closed, e.g. lds_swizzle is
  rejected on the WMMA path.

C ABI: ROCKE_ABI_VERSION and BINARY_ABI 5 -> 6
- rocke_gemm_trait_spec_t gains tdm, tdm_depth, ab_load_elem_bytes and its
  _set flag, split_barrier, tdm_alternate_issuers and tdm_early_refill, which
  changes the layout of every GEMM spec that embeds it.

Sweep harness and case study (python/rocke/examples/gfx1250/gemm_perf_fp8/)
- A staged geometry and trait sweep: one inheritable traits config section, a
  margin-based finalist cut, machine state per run, ELF metadata on any arch;
  a build worker that dies no longer hangs the pool.
- fp8_trait_ab.py (interleaved, verified trait A/B), wmma_roofline_probe.py
  (data-matched WMMA ceiling), memory_probe.py (bandwidth and TDM feed) and
  clock_probe.py (in-kernel clock and device health check).
- gemm_perf_fp8_case_study.md and the README record the method and results.

Tests
- gemm_emit parity configs 13-31 and three TDM descriptor configs in
  gfx1250_lowering_emit; C++ unit tests rocke_test_tdm and
  rocke_dense_f8f6f4_hip; opcode and arch-catalog pins updated.
- Python tests for TDM, the f8f6f4 atom, the traits, the probes and the sweep.
  TDM tests pin or require llvm23+; fork-dependent tests skip without fork.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jefyang1
jefyang1 force-pushed the users/jefyang/rocke/aick-2347 branch from 5cc3f5e to d10c730 Compare October 30, 2026 22:19
@CongMa13

CongMa13 commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

I reviewed this PR again. The following review threads are still unresolved, or comments have no author reply:

For reassessment, the PR author should reply to top-level feedback or state that thread feedback is addressed. Before approval, top-level comments need explicit resolution from their author or that author's approval of the PR.

jefyang1 and others added 2 commits November 6, 2026 16:27
Conflicts: the opcode enum and the ABI version. develop's #14448 appended
tile.gpu_check_failed as opcode 230 and took binary ABI 10. Both stay as
develop has them; this branch's tile.global_addr_of follows as opcode 231
(enum, the two opcode tables and test_opcode_values), and its
rocke_gemm_trait_spec_t change becomes ABI 11 (ROCKE_ABI_VERSION and
BINARY_ABI together).
…ments

The parity test compared the binding's reads with the dict's keys, but the
dict's top-level keys (name, wave_size, block_size, dtype, batch_size ...)
are written by hand, so a new top-level spec field would be missing from
both sides of the comparison. It now also checks that each dict carries
every field of its spec, nested and top-level. It also strips C/C++
comments from the binding before looking for reads, so a commented-out
read is reported as missing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jefyang1

jefyang1 commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max

PR #14707 — gfx1250 fp8 GEMM bring-up and K-loop optimization (jefyang1, AICK-2347) — 🟡 Follow-up: N9, N10 fixed; 14464 overlap

N11: agreed. This PR keeps the split_k, emit_sched_hints and cshuffle_no_alias reads in fill_universal_spec, and test_gemm_spec_dict_binding_parity checks them. If #14464 lands after this PR, it drops its copies and reads its other trait keys in fill_universal_spec; that test then reports any key it adds to the dict without a read.

The two code-quality notes on the parity test (357a71f565e): it now also checks that each dict carries every field of its spec, nested and top-level, so a new top-level field that is not written by hand fails it; and it strips C/C++ comments from the binding first, so a commented-out read is reported. Dropping a top-level key from a builder, adding a top-level field the dict does not write, or commenting out a read (// or /* */) each fails it.

Merged develop (0434e3c852f). #14448 conflicted in the opcode enum and the ABI version. develop's tile.gpu_check_failed keeps opcode 230 and ABI 10. This PR's tile.global_addr_of follows as opcode 231 (enum, both opcode tables, test_opcode_values), and its rocke_gemm_trait_spec_t change becomes ABI 11 (ROCKE_ABI_VERSION and BINARY_ABI together). All three opcode tables index by enum name.

Checks on the merge: build + ctest 61/61 (1 skip, arch-domain reproducibility), -L unit 57/57; byte identity GREEN ×4; golden OK ×4 (ll); the 33 changed test files: 532 passed, 1 skipped (llvm22: 522 passed, 11 skipped).

@jefyang1

jefyang1 commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

I reviewed this PR again. The following review threads are still unresolved, or comments have no author reply:

For reassessment, the PR author should reply to top-level feedback or state that thread feedback is addressed. Before approval, top-level comments need explicit resolution from their author or that author's approval of the PR.

Thank you, replied.

@cgmillette

Copy link
Copy Markdown
Contributor

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: high

PR #14707 — gfx1250 fp8 GEMM bring-up and K-loop optimization (jefyang1, AICK-2347) — 🟠 Follow-up: rocke_pytest at its time limit

Prior item Status
Copilot (10-29) ×14: GEP ×2; ABI; FNUZ; NaN; 8 tests; C++ HIP 🟢 FIXED d10c7309, unchanged
• the delta touches none of them (read)
Our 10-30 N1–N4, 10-31 N1–N3 🟢 FIXED, unchanged
• differential 0 changed ×4 + canonical (ran)
Our 10-30 ABI note: bump collides with other PRs 🟡 OPEN, land order
• develop is 10; takes develop + 1 when it lands (ran)
• abi.h and BINARY_ABI both 11 at head (ran)
• other open PRs claim 11 too: the number moves at landing
Merge 0434e3c8: 14448's opcode and ABI conflicts 🟢 RESOLVED
• develop's tile.gpu_check_failed keeps 230; tile.global_addr_of is 231 (read)
• enum, name and purity tables, pinned test agree; opcode test ✓ (ran)
• abi.h keeps develop's 10 line, then 11 (read)
CongMa13 (10-30, 11-02): replies; partial last K tile 🟢 FIXED a624052f, unchanged
• the delta touches none of it (read)
Our 11-02 N1–N7 🟢 FIXED 7ea9e665, 86f52810, unchanged
• the delta touches none of them (read)
amd-khushbu (11-03) items 1–13, docs, minor 🟢 FIXED, unchanged
• minor srem still left (read)
Our 11-03 N1, 🟡 ×3: lds_swizzle; batched offset; CI tests; README 🟢 FIXED e4433941, unchanged
• the delta touches none of them (read)
Our 11-03 N2 and quality: batched i64 test; abeb test base 🟢 FIXED 4ee1e7fe, unchanged
• the delta touches none of them (read)
AnaghaRaoAMD (11-03) ×8: goldens; waves; C++ guards; parity CTest; sweep; agprs; watchdog; fields 🟢 FIXED 344060ae…afe4f1da, unchanged
• the PR's CTests pass in both superbuilds at head (read)
AnaghaRaoAMD (11-03) ×2: K-loop and arch-literal refactor; sweep tools 🟡 DEFERRED, unchanged
• AICK-2979 and AICK-2980; reasonable as follow-ups (read)
Our 11-04 N3–N7: figures; bf8 K=128; bf16 codec; batch width; rounder 🟢 FIXED 9d20ae2b, 43395b27, unchanged
• the delta touches none of them (read)
Our 11-04 quality: lane-map evaluator; trait keys 4× 🟢 FIXED 43395b27, unchanged
Our 11-05 N8 and quality: docstring; zero sign; NaN element; tests 🟢 FIXED 41580037, unchanged
Our 11-06 N9: two binding reads the body claims untested 🟢 FIXED 67f5b45d, unchanged
• parity CTest ✓ in both hipDNN superbuilds at head (read)
Our 11-06 N10: three local IR def-map parsers 🟢 FIXED e2fe0507, unchanged
Our 11-06 quality: top-level keys; commented-out reads 🟢 FIXED 357a71f5
• a new top-level spec field now fails it; old test passed (ran)
• a read under // or /* */ fails it; old test passed (ran)
Our 11-06 N11: 14464 adds the same three binding reads 🟢 AGREED
• this PR keeps them; 14464 drops its copies (read)
• 14464 still carries them until it rebases (read)
CongMa13 (11-06): asks the author to answer our post 🟢 ANSWERED
• author replied 16:38Z and 16:39Z (read)
Approvals 🟢 None yet
• review required; code-owner approvals pending (read)
Bots / CI 🟠 1 failing
• fails: gfx94X rocKE shard 1/2 — rocke_pytest hit its CTest limit: N12
• running: Windows gfx110X sanity; Windows rocKE shards not started
• 39 ✓; both hipDNN superbuilds ✓; merge: clean (read)
Our checks 🟢 Clean
• merge vs 6c156add: clean; no rocKE change since 07d49a25 (ran)
• build + ctest 60 ✓, 1 skip; byte identity GREEN ×4, every family (ran)
• golden PASS; differential 0 changed ×4 + canonical (ran)
• changed tests 637 ✓ / 30 skip; installed 368 ✓ (ran)
• compile: 0 spills, 0 new rejections; ABI 10 → 11 (ran)
Code quality 🟢 Good
• one spec table now drives both parity checks (read)
• dropped keys, new fields, commented reads all fail it (ran)
• merge appends the opcode in order in all four places (read)
# Sev file:line Finding
N12 🟠 platform/tests/test_sweep_build_pool.py:72 rocke_pytest times out with this PR's sweep tests
• impact: shard 1/2 stopped at 98% on its CTest time limit; shard 2/2 passed near it
• the PR's two sweep test files use about a tenth of it (read)
• fix: shorten the watchdog waits and inert-sweep compiles, or a CTest entry of their own in a CI tier
N13 🟡 platform/tests/core/test_opcode_values.cpp:266 Open PR 14833 also takes opcode 231
• impact: the two conflict in ir.h, core_types.cpp, this test and abi.h
• fix: in the planned order 14833 lands first; this PR then moves to 232

Utility: 🟢 Positive — parity test closes both gaps; merge renumbers opcode and ABI cleanly


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: the parity test against a dropped top-level key, a new spec field and // / /* */ reads, the merge's opcode and ABI resolution, opcode claims in the open rocKE PRs, a trial merge with 14833, the gfx94X shard logs, the superbuild CTests, the merge with today's develop, build + ctest, byte identity ×4, goldens, differential, installed tree, compile, ABI, code quality. Flags whether a closer look is needed; not an approval or rejection.

rocke_pytest reached its CTest time limit on a CI shard. The two
dead-worker tests each waited out a 5 s watchdog on the same scenario;
they are one test now, with a 2 s watchdog (about a hundred times a fake
build). The slow build sleeps 0.8 s under a 0.2 s watchdog instead of
1.5 s under 0.3 s. The inert-sweep tests compile two values of the trait
they check instead of the config's four or five: waves_per_eu null and 4
(the pair that compiles to the same code on the tile), lds_k_pad 16 and
48. The two files take about a third of their previous time.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jefyang1

jefyang1 commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: high

PR #14707 — gfx1250 fp8 GEMM bring-up and K-loop optimization (jefyang1, AICK-2347) — 🟠 Follow-up: rocke_pytest at its time limit

N12 (a3e38f9d0a1): the two sweep test files take about a third of their previous time.

  • test_sweep_build_pool.py: the two dead-worker tests waited out a 5 s watchdog on the same scenario; they are one test now, with a 2 s watchdog (about a hundred times a fake build), still checking the record count, the failure message and the order. The slow-build test sleeps 0.8 s under a 0.2 s watchdog instead of 1.5 s under 0.3 s.
  • test_sweep_config.py: the inert-sweep tests compile two values of the trait they check instead of the config's four or five: waves_per_eu null and 4, the pair that compiles to the same code on the sampled tile (1 and 2 change the register budget), and lds_k_pad 16 and 48.
  • Both files pass at the default flavor and llvm24; the build-pool file passed 5 of 5 repeated runs.
  • For the CI owners: rocke_pytest runs every installed test under one TIMEOUT 300, and develop keeps adding to it (240 → 278 platform test_*.py files since this PR's base on 10-30), so the shard can reach its limit with or without this PR. Raising the limit or splitting the entry would keep it from failing whichever PR comes next.

N13: noted. Whichever of this PR and #14833 lands second takes the next opcode (and ABI version) and renumbers in ir.h, both opcode tables, test_opcode_values.cpp and abi.h, as in the #14448 merge.

@cgmillette

Copy link
Copy Markdown
Contributor

Automated review: Model - GLM-5.3-Flash, effort: high

PR #14707 — gfx1250 fp8 GEMM bring-up and K-loop optimization (jefyang1, AICK-2347) — 🟡 Follow-up: sweep tests shortened

Prior item Status
Copilot (10-29) ×14: GEP ×2; ABI; FNUZ; NaN; 8 tests; C++ HIP 🟢 FIXED d10c7309, unchanged
• the delta touches none of them (read)
Our 10-30 N1–N4, 10-31 N1–N3 🟢 FIXED, unchanged
• differential 0 changed ×4 + canonical (ran)
Our 10-30 ABI note: bump collides with other PRs 🟡 OPEN, land order
• develop is 10; takes develop + 1 when it lands (ran)
• other open PRs claim 11 too: the number moves at landing
Merge 0434e3c8: 14448's opcode and ABI conflicts 🟢 RESOLVED, unchanged
CongMa13 (10-30, 11-02): replies; partial last K tile 🟢 FIXED a624052f, unchanged
Our 11-02 N1–N7 🟢 FIXED 7ea9e665, 86f52810, unchanged
amd-khushbu (11-03) items 1–13, docs, minor 🟢 FIXED, unchanged
• minor srem still left (read)
Our 11-03 N1, 🟡 ×3: lds_swizzle; batched offset; CI tests; README 🟢 FIXED e4433941, unchanged
Our 11-03 N2 and quality: batched i64 test; abeb test base 🟢 FIXED 4ee1e7fe, unchanged
AnaghaRaoAMD (11-03) ×8: goldens; waves; C++ guards; parity CTest; sweep; agprs; watchdog; fields 🟢 FIXED 344060ae…afe4f1da, unchanged
• the delta changes the watchdog's tests, not its code (read)
AnaghaRaoAMD (11-03) ×2: K-loop and arch-literal refactor; sweep tools 🟡 DEFERRED, unchanged
• AICK-2979 and AICK-2980; reasonable as follow-ups (read)
Our 11-04 N3–N7: figures; bf8 K=128; bf16 codec; batch width; rounder 🟢 FIXED 9d20ae2b, 43395b27, unchanged
Our 11-04 quality: lane-map evaluator; trait keys 4× 🟢 FIXED 43395b27, unchanged
Our 11-05 N8 and quality: docstring; zero sign; NaN element; tests 🟢 FIXED 41580037, unchanged
Our 11-06 N9, N10 and quality: binding reads; IR parsers; parity keys 🟢 FIXED 67f5b45d, e2fe0507, 357a71f5, unchanged
Our 11-06 N11: 14464 adds the same three binding reads 🟢 AGREED, unchanged
• this PR keeps them; 14464 drops its copies (read)
Our 11-06 N12: rocke_pytest at its CTest time limit 🟢 FIXED a3e38f9d
• both sweep files about a third of their old time, timed side by side (ran)
• covers shard 1/2's shortfall at the old head, with little margin (read)
• the gfx94X shards have not run at this head yet (read)
Our 11-06 N13: 14833 also takes opcode 231 🟡 OPEN, land order
• author agrees: whichever lands second renumbers (read)
• both heads still pin 231 in test_opcode_values.cpp (read)
CongMa13 (11-06): asks the author to answer our post 🟢 ANSWERED, unchanged
Approvals 🟢 None yet
• review required; code-owner approvals pending (read)
Bots / CI 🟡 Running
• running: Linux math-libs ×3; Windows tests queued; no rocKE shard yet
• 26 ✓, no failures; both hipDNN superbuilds ✓; merge: clean (read)
• CI capacity: one CTest time limit covers every installed test in rocke_pytest, and develop keeps adding files to it (read)
Our checks 🟢 Clean
• merge vs 58f13225: clean (ran)
• build + ctest 62, 1 skip; byte identity GREEN ×4 (ran)
• golden PASS; differential 0 changed ×4 + canonical (ran)
• changed tests 636 ✓ / 30 skip; installed 367 ✓ (ran)
• both sweep files ✓ at llvm24, which runs the inert tests (ran)
• compile: 0 spills, 0 new rejections; ABI 10 → 11 (ran)
Code quality 🟡 One gap
• merged dead-worker test keeps every old assert (read)
• order, dead-record and no-watchdog mutants caught as before (ran)
• shorter watchdog waits: 16 of 16 runs pass with the CPU oversubscribed (ran)
• the two-value inert test drops one guard: N14
# Sev file:line Finding
N14 🟡 platform/tests/test_sweep_config.py:260 Two waves_per_eu values no longer test partial duplicates
• impact: a detector that warns only when every value matches now passes; the old four-value test failed it (ran)
• fix: sweep [null,1,4]: one more compile, and it fails that mutant (ran)

Utility: 🟢 Positive — timing fix confirmed; one lost test guard found by mutation


Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted

Follow-up review: first pass by GLM-5.3-Flash (gateway), verified by the orchestrator (Claude Opus 5.5). Checks: the two sweep test files timed side by side at both heads, mutants of the build pool and the inert-sweep detector against the old and new tests, the build pool under CPU oversubscription, the previous head's gfx94X and Windows shard logs, opcode pins in both PRs, the merge with today's develop, build + ctest, byte identity ×4, goldens, differential, installed tree, compile, ABI, code quality. Flags whether a closer look is needed; not an approval or rejection.

With two waves_per_eu values that compile to the same code, a detector
that warns only when every value matches passed the test. It now sweeps
null, 1 and 4 (1 changes the register budget) and checks that the
warning names null and 4 without 1.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jefyang1

jefyang1 commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Automated review: Model - GLM-5.3-Flash, effort: high

PR #14707 — gfx1250 fp8 GEMM bring-up and K-loop optimization (jefyang1, AICK-2347) — 🟡 Follow-up: sweep tests shortened

N14 (2812c338e60): the waves_per_eu test sweeps null, 1 and 4. Null and 4 compile to the same code on the sampled tile and 1 does not, and the test now also checks that the warning names null and 4 without 1. A detector that warns only when every value matches, rather than when some do, fails it; the unmutated test passes. It costs one more compile; the file passes at the default flavor and llvm24.

@cgmillette
cgmillette enabled auto-merge (squash) November 6, 2026 23:20

@cgmillette cgmillette left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Conflict: abi.h. develop's #14907 took binary ABI 10 and renumbered
#14448's rocke_param_opts_t entry to 11; this branch's
rocke_gemm_trait_spec_t change becomes ABI 12 (ROCKE_ABI_VERSION and
BINARY_ABI together).
@jefyang1

jefyang1 commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Merged develop (9f23cdc5da2). The only conflict was abi.h: #14907 took binary ABI 10 and renumbered #14448's rocke_param_opts_t entry to 11, so this PR's rocke_gemm_trait_spec_t change is now ABI 12 (ROCKE_ABI_VERSION and BINARY_ABI together). The opcode stays 231. Checks on the merge: build + ctest 62/62 (1 skip, arch-domain reproducibility), -L unit 58/58; byte identity GREEN ×4; golden OK ×4; the 33 changed test files 531 passed, 1 skipped (llvm22: 521 passed, 11 skipped); pre-commit clean over the PR range.

@cgmillette

Copy link
Copy Markdown
Contributor

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: high

PR #14707 — gfx1250 fp8 GEMM bring-up and K-loop optimization (jefyang1, AICK-2347) — 🟡 Follow-up: armed message out of date

Prior item Status
Copilot (10-29) ×14: GEP ×2; ABI; FNUZ; NaN; 8 tests; C++ HIP 🟢 FIXED d10c7309, unchanged
• the delta touches none of them (read)
Our 10-30 N1–N4, 10-31 N1–N3 🟢 FIXED, unchanged
• differential 0 changed ×4 + canonical (ran)
Our 10-30 ABI note: bump collides with other PRs 🟡 OPEN, land order
• develop is 11; takes develop + 1 when it lands (ran)
• another open PR also claims 12: the number moves at landing (read)
Merge 0434e3c8: 14448's opcode and ABI conflicts 🟢 RESOLVED, unchanged
Merge 9f23cdc5: develop's ABI 10 and 11 🟢 RESOLVED
• abi.h history and both counters at 12 (read)
• body's version list matches abi.h (read)
CongMa13 (10-30, 11-02): replies; partial last K tile 🟢 FIXED a624052f, unchanged
Our 11-02 N1–N7 🟢 FIXED 7ea9e665, 86f52810, unchanged
amd-khushbu (11-03) items 1–13, docs, minor 🟢 FIXED, unchanged
• minor srem still left (read)
Our 11-03 N1, 🟡 ×3: lds_swizzle; batched offset; CI tests; README 🟢 FIXED e4433941, unchanged
Our 11-03 N2 and quality: batched i64 test; abeb test base 🟢 FIXED 4ee1e7fe, unchanged
AnaghaRaoAMD (11-03) ×8: goldens; waves; C++ guards; parity CTest; sweep; agprs; watchdog; fields 🟢 FIXED 344060ae…afe4f1da, unchanged
AnaghaRaoAMD (11-03) ×2: K-loop and arch-literal refactor; sweep tools 🟡 DEFERRED, unchanged
• AICK-2979 and AICK-2980; reasonable as follow-ups (read)
Our 11-04 N3–N7: figures; bf8 K=128; bf16 codec; batch width; rounder 🟢 FIXED 9d20ae2b, 43395b27, unchanged
Our 11-04 quality: lane-map evaluator; trait keys 4× 🟢 FIXED 43395b27, unchanged
Our 11-05 N8 and quality: docstring; zero sign; NaN element; tests 🟢 FIXED 41580037, unchanged
Our 11-06 N9, N10 and quality: binding reads; IR parsers; parity keys 🟢 FIXED 67f5b45d, e2fe0507, 357a71f5, unchanged
Our 11-06 N11: 14464 adds the same three binding reads 🟢 AGREED, unchanged
• this PR keeps them; 14464 drops its copies (read)
Our 11-06 N12: rocke_pytest at its CTest time limit 🟢 FIXED a3e38f9d
• sweep files timed side by side last round (ran)
• no rocKE shard has finished since; CI still owes it (read)
Our 11-06 N13: 14833 also takes opcode 231 🟡 OPEN, land order
• both heads still pin 231 in test_opcode_values.cpp (read)
• whichever lands second renumbers, as agreed (read)
Our 11-06 N14: two waves_per_eu values miss partial duplicates 🟢 FIXED 2812c338
• "warn only if all match" mutant now fails the test (ran)
• a warning naming every value fails it too (ran)
CongMa13 (11-06): asks the author to answer our post 🟢 ANSWERED, unchanged
Approvals 🟢 APPROVED 2812c338 (cgmillette)
• older head; still counts on develop
• predates the develop merge and its ABI renumber (read)
• code-owner review still required (read)
Bots / CI 🟡 Running
• running: math-libs ×4 (Linux ×3, Windows); no rocKE shard yet
• 24 ✓, no failures; both hipDNN superbuilds ✓ (read)
• no rocKE shard finished since 357a71f5: each push cancelled them (read)
Our checks 🟢 Clean
• merge vs 886472ee: clean, 3 behind (ran)
• build + ctest 62, 1 skip; byte identity GREEN ×4 (ran)
• golden PASS; differential 0 changed ×4 + canonical (ran)
• changed tests 636 ✓ / 30 skip; installed 367 ✓ (ran)
• both sweep files ✓ at llvm24, no skips (ran)
• compile: 0 spills, 0 new rejections; ABI develop 11, PR 12 (ran)
Code quality 🟢 Clean
• inert-sweep test guards the partial-duplicate case again: N14 (ran)
• develop merge renumbers the abi.h history only (read)
# Sev file:line Finding
N15 🟡 auto-merge (squash message) Armed squash message still says ABI 10 → 11
• impact: the squash commits that stored text, not the live body's 11 → 12 (read)
• fix: re-arm auto-merge after the last body edit

Utility: 🟢 Positive — lost test guard restored, confirmed by mutation; squash message needs re-arming


Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted

Follow-up review: first pass by qwen3.6 (local), verified by the orchestrator (Claude Opus 5.5). Checks: inert-sweep mutants against the old and new tests, both sweep files at llvm24, the abi.h merge resolution, ABI and opcode pins in the sibling PRs, the armed auto-merge message, CI at the last three heads, the merge with today's develop, build + ctest, byte identity ×4, goldens, differential, installed tree, compile, ABI, code quality. Flags whether a closer look is needed; not an approval or rejection.

@jefyang1
jefyang1 disabled auto-merge November 7, 2026 02:06
@jefyang1
jefyang1 enabled auto-merge (squash) November 7, 2026 02:06
@jefyang1

jefyang1 commented Nov 7, 2026

Copy link
Copy Markdown
Contributor Author

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: high

PR #14707 — gfx1250 fp8 GEMM bring-up and K-loop optimization (jefyang1, AICK-2347) — 🟡 Follow-up: armed message out of date

Re-armed auto-merge after the last description edit; the squash message now says ABI 11 → 12.

Conflict: dispatch_tests/core/test_binding.py. develop's #15004 runs the
binding tests on every covered arch; this branch moved the binding to the
shared rocke.numeric.bf16 codec and dropped its private
_bf16_from_f32/_f32_from_bf16. The file keeps develop's arch
parametrization and this branch's codec: the reference output encodes
with f32_to_bf16, and the bf16 class checks the binding uses the shared
codec. The round-trip and nearest-even tests it replaced are in
tests/numeric/test_bf16_codec.py.
@jefyang1

jefyang1 commented Nov 7, 2026

Copy link
Copy Markdown
Contributor Author

Merged develop (921b134). The only conflict was tests/dispatch/dispatch_tests/core/test_binding.py: #15004 runs the binding tests on every covered arch, and this PR moved the binding to the shared rocke.numeric.bf16 codec, dropping its private _bf16_from_f32/_f32_from_bf16. The resolved file keeps #15004's arch parametrization and this PR's codec: _write_reference_output encodes with f32_to_bf16, and TestBf16Encoding checks that the binding uses the shared codec and that NaN stays NaN. The round-trip and nearest-even tests it replaces are in tests/numeric/test_bf16_codec.py. ABI (12) and opcode (231) are unchanged.

Checks on the merge: build + ctest 62/62 (1 skip), -L unit 58/58; byte identity GREEN ×4; golden OK ×4; the binding tests on every covered arch; the 33 changed test files 531 passed, 1 skipped (llvm22: 521 passed, 11 skipped); pre-commit clean over the PR range.

develop's #11642 replaced the flat MMA operand fields and role enum with
indexed sources (srcs[0..2]) and an independent destination in both
engines, and took binary ABI 12. Conflicts:

- cpp/core/arch/data.cpp, core/arch/target.py: the gfx1250 fp8/bf8 WMMA
  rows take #11642's indexed form; the four K=64 rows and the K=128
  fp8 x fp8 row keep this branch's A/B lane maps and carrier fragment
  lengths (8 and 16) as srcs[0]/srcs[1]. The accumulator map is
  #11642's _wmma_gfx12_row_col_16x16 / lm_wmma_gfx12_src2, _dst.
- core/arch/target.py: has_ab_layouts() reads srcs[0]/srcs[1].layout.
- core/lower_hip.py: #11642's validate_unscaled_wmma with
  source_count=k // 8, so the K=128 path validates its i32x16 operands.
- tests/core/gfx1250_wmma_ops.cpp: this branch's fragment and layout
  checks on the srcs[] fields.
- core/ir.py: this branch's TDM pad encoder beside #11642's
  _mma_dst_frag_len.
- abi.h: #11642 keeps ABI 12; this branch's rocke_gemm_trait_spec_t
  change becomes ABI 13 (ROCKE_ABI_VERSION and BINARY_ABI together).

This branch's code that merged without a conflict moves to #11642's
names too: the gfx1250 fp8 lane maps and the K=128 HIP lowering use
ROCKE_MMA_ROLE_SRC0/SRC1, the GEMM validator's lane-map check reads
srcs[0]/srcs[1].layout, and the fp8 fragment-layout test reads
_FragInfo.srcs[i].fn / .frag_len.
@jefyang1
jefyang1 disabled auto-merge November 7, 2026 04:13
@jefyang1
jefyang1 enabled auto-merge (squash) November 7, 2026 04:13
@jefyang1

jefyang1 commented Nov 7, 2026

Copy link
Copy Markdown
Contributor Author

Merged develop (5442420). #11642 (indexed MMA operands) conflicted in both engines' MMA catalogs, the HIP lowering, ir.py, the gfx1250 WMMA ops test and abi.h. The gfx1250 fp8 rows take #11642's srcs[]/dst form, with this PR's A/B lane maps as srcs[0]/srcs[1] (K=64 rows and K=128 fp8 x fp8); the K=128 HIP path uses validate_unscaled_wmma sized by K; this PR's other code moved to the new names (ROCKE_MMA_ROLE_SRC0/SRC1, srcs[i].layout, _FragInfo.srcs[i]). #11642 keeps ABI 12, so this PR is now ABI 13. Checks on the merge: build + ctest 64/64 (1 skip), -L unit 60/60; byte identity GREEN ×4; golden OK ×4; MMA/arch tests 324 passed; the 33 changed test files 531 passed, 1 skipped (llvm22: 521, 11 skipped); pre-commit clean over the PR range.

@jefyang1
jefyang1 merged commit e2f7acf into develop Nov 7, 2026
99 of 100 checks passed
@jefyang1
jefyang1 deleted the users/jefyang/rocke/aick-2347 branch November 7, 2026 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants