Skip to content

fix(rocke): refuse the fused fp8 scale-and-convert ops off gfx950, and catch them in the arch-domain table - #13954

Merged
cgmillette merged 3 commits into
developfrom
users/cgmillette/rocke/fp8cvt
Oct 22, 2026
Merged

cgmillette merged 3 commits into
developfrom
users/cgmillette/rocke/fp8cvt

Conversation

@cgmillette

@cgmillette cgmillette commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

cvt_scalef32_pk_f32_fp8x4 and cvt_scalef32_pk_f32_bf8x4 wrap v_cvt_scalef32_pk_f32_fp8 / _bf8, which only gfx950 has. Both engines lowered them for any target. For gfx942 and gfx90a, LLVM then selects the intrinsic and, through COMGR, builds a code object that contains the gfx950 encoding without reporting anything; only the text assembler rejects it. A kernel using these ops for the wrong target should fail at lowering with a message that says what to do instead.

The arch-domain table, which exists to catch intrinsics a target cannot run before link time, missed this case: its probe recorded a key as available whenever the probe linked. This PR closes that gap too.

The fp8 / bf8 conversion builders were also documented as saturating. Measured on gfx942 and gfx950, they do not.

ISSUE ID : AICK-2646

Technical Details

  • The ISA backend states the capability: has_fp8_scale_conversion, True only for gfx950 (Python ISABackend / Gfx950Backend; C++ a new last field of rocke_isa_backend_t, set on the gfx950 row). This is the same home as the existing per-target capability flags.
  • Both lowerers check it first in the two handlers and refuse with the same text, before anything is emitted, e.g.:
    arith.cvt_scalef32_pk_f32_fp8 needs v_cvt_scalef32_pk_f32_fp8, which gfx942 does not have (gfx950 does); convert with cvt_pk_f32_fp8x4 and multiply by the scale instead
  • The builder has no target, so lowering is the first point that can check. The builder doc says so.
  • The arch-domain generator (tools/gen_arch_domain.py) now also compiles each probe that links to assembly and assembles it for its target with the llvm-mc of the same install, which checks every instruction against the target. "instruction not supported on this GPU" records arch_absent with the rejected mnemonic as evidence; any other assembler failure is probe_error, never ok.
    • Re-assembling every ok cell of the llvm20 column found only these two keys on gfx90a and gfx942; the full llvm20 column still regenerates unchanged.
    • The four cells are corrected in the llvm20, llvm22 and llvm23 columns, each measured with its own LLVM.
  • The emitter's arch-domain check only warns, and only in the Python engine. So the backend capability flag stays as the refusal for these ops, in both engines; the corrected table now also warns for them.
  • Docs: the f32 -> fp8 / bf8 builders (scalar and packed) round to nearest even and do not saturate. A value past the largest finite value, and +-inf / NaN, become NaN; bf8 on gfx950 becomes +-inf. The docstrings, the C header and the quantisation notes now say so and point to clamp_f32.
  • gfx950 output is unchanged; no LLVM IR changes for any existing kernel.

Test Plan

  • New tests/core/test_fp8_scale_conversion_gate.py: refusal text pinned in both engines for every other wired target (read from wired_arches(): gfx90a, gfx942, gfx1151, gfx1201, gfx11-generic, gfx12-generic, gfx1250); gfx950 lowers both ops, and the two engines' IR is identical.
  • New native test rocke_fp8_scale_conversion_gate: the same text pinned in the C++ engine for the same targets, gfx950 still lowers. It is listed in the quick and ffm-quick categories of ROCKE_ENGINE_test_categories_external.yaml.
  • Generator: the assembler's evidence carries no scratch path; with a host toolchain, the probe is arch_absent on gfx942 and ok on gfx950; --check regenerates the llvm20 column unchanged.
  • Instruction availability for the fused conversion on gfx90a, gfx942, gfx950, gfx11, gfx12 and gfx1250, with llvm-mc from LLVM 20 and LLVM 23: only gfx950 accepts it.
  • Conversion behaviour on GPUs (gfx942 and gfx950): f32 values just above the largest finite value, 1e3 .. FLT_MAX, +-inf and NaN, through every fp8 / bf8 conversion builder.
  • Byte identity at llvm20, llvm22, llvm23; ctest; full platform suite against develop.

Test Result

  • New tests pass, including under ROCKE_BACKEND=both ROCKE_CPP_STRICT=1.
  • Byte identity green at llvm20, llvm22 and llvm23; ctest passes.
  • Full platform suite passes with no failures.
  • pre-commit passes.
  • gen_arch_domain.py --check at llvm20: every cell reproduces.
  • Measured conversion behaviour (none saturates):
Builder gfx942 (FNUZ) gfx950 (OCP)
f32 -> fp8, past the largest finite value (240 / 448) NaN NaN
f32 -> fp8, +-inf / NaN NaN NaN
f32 -> bf8, past 57344 NaN +-inf
f32 -> bf8, +-inf / NaN NaN +-inf / NaN

Submission Checklist

🤖 Generated with Claude Code

@therock-pr-bot

therock-pr-bot Bot commented Oct 19, 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

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

@cgmillette

Copy link
Copy Markdown
Contributor Author

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

PR #13954 — refuse the fused fp8 scale-and-convert ops off gfx950, and catch them in the arch-domain table — 🟠 First-round: not merge-ready — rebase needed (conflicts with #14040); change itself verified

Prior item Status
Reviewers 🟢 None yet — no reviews or threads
Bots / CI 🟢 Policy bot, pre-commit, clang-tidy pass; TheRock CI hipkernelprovider green on Linux gfx94X and Windows gfx110X — all on the pre-conflict head. GitHub now reports the PR dirty, so nothing has run against current develop
Our checks 🟢 Merged with current develop in a scratch tree (conflict resolved by keeping both new methods) and tested that merge: native build clean, new rocke_fp8_scale_conversion_gate ctest passes; gate + arch-domain tests with both engines: 60 passed, 4,937 subtests (the one skip is the regeneration check, which needs an llvm22 toolchain); test_arch_domain_lane.py 40 passed (ran). Motivation reproduced on LLVM 24 (the PR measured 20 / 23): clang -c -mcpu=gfx942 silently writes v_cvt_scalef32_pk_f32_fp8 into the object, llvm-mc -mcpu=gfx942 rejects it, gfx950 accepts both (ran). No kernel in the tree calls the refused ops, so no existing kernel changes (ran). rule_scan clean (ran)
# Sev file:line Finding
N1 🟠 python/rocke/core/lower_llvm.py:1936 Conflicts with develop: #14040 (merged 10-20) replaced _require_gfx1250_llvm23 with the fact-keyed _require_llvm23_target_fact exactly where this PR inserts _require_fp8_scale_conversion. The resolution is to keep both methods — done in a scratch merge, which builds and passes everything above → rebase. While rebasing, consider recording the fused conversion as a target fact the way #14040 does (_OpTargetFact / rocke_ll_target_fact_t), so both engines read a recorded capability and a future target needs a data row, not a new backend override.
N2 🟡 tests/CMakeLists.txt:40 The new native test rocke_fp8_scale_conversion_gate is not listed in ROCKE_ENGINE_test_categories_external.yaml, so it carries no tier label and drops out of TheRock's ctest -L quick / standard selection — it never runs in CI → add it to both categories (its CMake neighbours rocke_tf32 and rocke_storage have the same pre-existing gap).

Utility: 🟢 Positive — turns a silent wrong-encoding build (a gfx950-only instruction in a gfx942 code object) into an actionable lowering error in both engines, and closes the arch-domain table's blind spot with an assembler pass. The fp8 / bf8 no-saturation doc corrections rest on the author's gfx942 / gfx950 measurements (not re-run here).


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

First-round review: a local qwen3.6 model made the independent first pass; the orchestrator (Claude Opus 5.5) verified it on hardware — scratch merge with develop, native build + ctest, both-engine tests, and an LLVM 24 clang / llvm-mc reproduction — and owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection.

@CongMa13 CongMa13 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.

No new issues found. I agree with the following existing comments:

@CongMa13 CongMa13 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.

No new P0 or P1 issues were identified during this review. The changes are approved.

cgmillette and others added 3 commits October 21, 2026 14:50
cvt_scalef32_pk_f32_fp8x4 and cvt_scalef32_pk_f32_bf8x4 wrap
v_cvt_scalef32_pk_f32_fp8 / _bf8, which only gfx950 has. Both engines
lowered them for any target. For gfx942 and gfx90a LLVM then selects the
intrinsic and, on the direct object path, writes an encoding the GPU
does not have; only the text assembler reports it.

The ISA backend now states the capability (has_fp8_scale_conversion,
true only for gfx950) and both lowerers refuse the two ops for a target
without it, before emitting anything, with the same message:

  arith.cvt_scalef32_pk_f32_fp8 needs v_cvt_scalef32_pk_f32_fp8, which
  gfx942 does not have (gfx950 does); convert with cvt_pk_f32_fp8x4 and
  multiply by the scale instead

The builder has no target, so lowering is the first point that can
check. gfx950 output is unchanged.

Tests pin the refusal text in both engines for every other wired target
and check that gfx950 still lowers the ops identically in both.

The new native test, rocke_fp8_scale_conversion_gate, is listed in the
quick and ffm-quick CTest categories so the tiered test runs select it.

Co-Authored-By: Claude <noreply@anthropic.com>
The f32 to fp8 / bf8 builders (scalar and packed) were documented as
rounding and saturating. Measured on gfx942 and gfx950, they round to
nearest even and do not saturate: a value that rounds past the largest
finite fp8 value, and +-inf and NaN, become NaN; bf8 becomes NaN on gfx942
and +-inf on gfx950. The docstrings, the C header and the quantisation
notes now say so, name each target's encoding, and point to clamp_f32
for a saturating conversion.

Co-Authored-By: Claude <noreply@anthropic.com>
…ding ok

The arch-domain generator recorded a key as ok for a target when its probe
compiled and linked. A link is not proof: LLVM selects
llvm.amdgcn.cvt.scalef32.pk.f32.fp8 / .bf8 for gfx90a and gfx942 and writes
the gfx950-only instruction into the object without complaint, so the
committed columns said ok there and the emitter's arch-domain check stayed
silent.

A probe that links is now also compiled to assembly and assembled for its
target with the llvm-mc of the same install, which checks every instruction
against the target. "instruction not supported on this GPU" records
arch_absent with the rejected mnemonic as evidence (no scratch path); any
other assembler failure is probe_error, never ok. The generator refuses to
run without an llvm-mc beside clang.

Re-assembling every ok cell of the llvm20 column found only these two keys
on gfx90a and gfx942; the full llvm20 column still reproduces. The four
cells are corrected in the llvm20, llvm22 and llvm23 columns, each measured
with its own LLVM. Tests pin the path-free evidence and, where a toolchain
is present, that the probe is arch_absent on gfx942 and ok on gfx950.

Co-Authored-By: Claude <noreply@anthropic.com>
@cgmillette
cgmillette force-pushed the users/cgmillette/rocke/fp8cvt branch from 71f383e to 0d0b16c Compare October 22, 2026 15:10
@cgmillette
cgmillette requested a review from a team as a code owner October 22, 2026 15:10
@cgmillette

Copy link
Copy Markdown
Contributor Author

Thanks for the reviews. Rebased onto current develop; changes, one line per item:

  • N1 (conflict with refactor(rocke): read gfx1250-only checks from target facts in both engines #14040 in lower_llvm.py): rebased; the resolution keeps both refactor(rocke): read gfx1250-only checks from target facts in both engines #14040's _require_llvm23_target_fact and this PR's _require_fp8_scale_conversion unchanged (the range-diff against the old head shows context lines only). Moving the gfx950-only check to a recorded target fact is a good idea but not a drop-in: refactor(rocke): read gfx1250-only checks from target facts in both engines #14040's check also requires llvm23 (this op lowers at llvm20/22/23) and arch_specs.json has no field for a conversion capability, so it needs a schema choice in both engines. Tracked as AICK-2706; this PR keeps the backend flag, which matches the existing per-target backend flags (has_async_lds_counter etc.).
  • N2 (native test not in the CTest categories): fixed in the first commit; rocke_fp8_scale_conversion_gate is now in both quick and ffm-quick in ROCKE_ENGINE_test_categories_external.yaml. Small correction to the finding: the install tree already labels every installed native test quick, so it was selected by ctest -L quick; what it lacked were the yaml labels (pre-commit, smoke, ffm-full). Eleven other installed native tests on develop have the same gap; that is AICK-2707, together with a check that catches the next one.
  • Also fixed (found while checking): the refusal tests said they covered every wired target but skipped gfx11-generic and gfx12-generic. The Python test now reads the list from wired_arches(); the native test lists the same targets.
  • CongMa13: thanks for the approval; both items you agreed with are addressed above.

@cgmillette

Copy link
Copy Markdown
Contributor Author

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

PR #13954 — refuse the fused fp8 scale-and-convert ops off gfx950, and catch them in the arch-domain table — 🟢 Follow-up: no closer look needed — both our items fixed; the rebase keeps both methods and covers the new generic targets

Prior item Status
Ours (10-20) N1 🟠: conflict with develop #14040 in lower_llvm.py 🟢 FIXED — rebased; _require_fp8_scale_conversion now sits beside #14040's _require_llvm23_target_fact (read). Merges cleanly with today's develop (27 behind) (ran). Our side suggestion to record it as a #14040-style target fact was not taken. It stays a backend flag, has_fp8_scale_conversion, which both engines read and which defaults to refusing — fine (read)
Ours (10-20) N2 🟡: rocke_fp8_scale_conversion_gate missing from the tier YAML 🟢 FIXED — it is listed in quick and ffm-quick, next to develop's rocke_qk_scale_spec_validate (read)
Since our review 🟢 The refused set now also covers develop's gfx11-generic / gfx12-generic: the Python test reads it from wired_arches(), and the C++ test lists the generic targets too (read)
CongMa13 APPROVED (10-21) at 71f383e2 🟡 Given before the rebase
Bots / CI 🟠 Re-run at head 0d0b16cc (10-22) and still running: pre-commit, clang-tidy, the ASAN summary, Component CI and the hipDNN Windows superbuild pass; TheRock multi-arch (compiler-runtime and emulation stages) and the hipDNN Linux superbuild are running
Our checks 🟢 Native build and 28 ctests pass, including rocke_fp8_scale_conversion_gate (ran). The gate and arch-domain tests pass with both engines, 57 in all (ran). On the merge, all three arch-domain tables (llvm20 / 22 / 23) equal develop's except the two fused fp8 / bf8 converts, which are arch_absent on gfx90a / gfx942 and ok on gfx950 (ran). The representative golden passes with no drift; rule_scan is clean (ran). Merge order: this PR, #13886 and #13885 insert at the same tier-YAML line, so the later ones need a rebase that keeps both entries (ran)

Prior reviews resolved; no net-new findings.

Utility: 🟢 Positive — turns a silently wrong encoding (a gfx950-only instruction in a gfx942 code object) into a clear lowering error in both engines, and the arch-domain table now assembles its probes before recording a result.


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

Follow-up review: a local qwen3.6 model made the independent first pass. The orchestrator (Claude Opus 5.5) verified it with a range-diff against the reviewed head, the native build and both-engine tests on the merge, a table-by-table comparison of the arch-domain data with develop, the golden gate, and scratch merges with #13886 and #13885, and owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection.

@cgmillette
cgmillette enabled auto-merge (squash) October 22, 2026 15:39
@cgmillette
cgmillette merged commit 2cce3a6 into develop Oct 22, 2026
143 of 147 checks passed
@cgmillette
cgmillette deleted the users/cgmillette/rocke/fp8cvt branch October 22, 2026 19:36
@assistant-librarian

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

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.

2 participants