Skip to content

fix(rocke): core dtype review follow-ups: uitofp_f32 source check and native test fixtures in every lane - #14050

Merged
cgmillette merged 3 commits into
developfrom
users/cgmillette/rocke/pr1-review-followups
Oct 22, 2026
Merged

cgmillette merged 3 commits into
developfrom
users/cgmillette/rocke/pr1-review-followups

Conversation

@cgmillette

@cgmillette cgmillette commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Follow-ups from the review of the core dtype PR.

  • arith.uitofp_f32 was checked only by its builder, which accepts only a u8 input. IR that did not come through the builder (assembled by hand, or deserialized and retyped u8 -> i8) passed through and lowered differently per backend: LLVM's uitofp i8 reads 0xff as 255.0, while the HIP lowering's int8_t cast reads it as -1.0.
  • The installed rocke_pytest CTest entry set only one of the native C++ parity fixtures that tests/run_all.py sets for a source build (ROCKE_UNSIGNED_INT_OPS_CLI), not ROCKE_STORAGE_TEST, ROCKE_IR_TYPES_TEST or ROCKE_NONTEMPORAL_HIP_TEST, so those native parity cases skipped in the installed test tree that CI runs. ROCKE_ARCH_TARGET_TEST_EXE was set by neither, so the C++/Python target identity comparison skipped everywhere.
  • The C++ engine keeps a hand-written copy of arch_specs.json's generic_targets, and that comparison fed it only a sample of target names, so a member missing from or misspelled in the C++ copy could go unnoticed.

ISSUE ID : AICK-2667

Technical Details

Three commits.

  1. uitofp_f32 source check (fix)
    • Every lowering of both engines refuses an arith.uitofp_f32 whose source is not a u8 scalar: the Python LLVM and HIP lowerers (shared helper _refuse_non_u8_source in core/ir.py) and the C++ LLVM and HIP lowerers (shared helper rocke_i_refuse_non_u8_source in cpp/core/ir/core_builder.cpp).
    • One message in both engines: arith.uitofp_f32 expects a u8 source, got <type>.
    • The check sits in the lowerers, next to the existing refusal of signed ops on unsigned data, because lowering does not run the IR verifier.
    • No change to the emitted IR for valid kernels.
  2. Installed pytest fixtures (fix)
    • tests/run_all.py: also resolves ROCKE_ARCH_TARGET_TEST_EXE from the rocke_arch_target_identity CTest entry; a comment beside its fixture list says the installed entry carries the same variables.
    • platform/CMakeLists.txt: the rocke_pytest entry points each of the five fixture variables at its installed binary when that binary is built and installed, relative to the CTestTestfile, in both the provider and the standalone layouts, from one list that replaces the entry's separate ROCKE_UNSIGNED_INT_OPS_CLI handling. ROCKE_REPLAY_CLI and ROCKE_PROVENANCE_MANIFEST are set as before.
    • tests/tools/test_run_all.py: covers the new variable.
  3. Generic target coverage (test)
    • test_cpp_target_identity_matches_python now also feeds every generic processor and every member read from arch_specs.json (in all its prefix, profile and feature forms), so each must resolve to the same catalog row and generic processor in both engines.

Test Plan

  • New tests/core/test_u8_arith.py::test_lowerings_refuse_uitofp_of_a_non_u8_source: a real u8 kernel, serialized, retyped to i8 and parsed back, is refused with the message by the LLVM and HIP lowerings of both engines.
  • Existing u8 tests (LLVM forms, HIP forms, GPU numerics on gfx90a) unchanged.
  • Misspelling one member in the C++ copy makes the new identity test fail (the previous input list did not see it); not committed.
  • Staged install in the provider layout and the standalone layout: run the installed rocke_pytest CTest entry with and without the new variables and compare per-case results.
  • Byte identity at llvm20, llvm22, llvm23; ctest; touched tests with both engines strict; full platform suite; pre-commit.

Test Result

  • The new test passes for both engines on both lowering paths.
  • Installed rocke_pytest: the storage, IR type, nontemporal HIP and target identity native parity cases went from skipped to passed, the same set in both layouts; no case changed state otherwise.
  • The target identity comparison now runs and passes in the source lane (run_all's fixture resolution) and in the installed rocke_pytest entry in both layouts.
  • Byte identity green at llvm20, llvm22 and llvm23; ctest passes; touched tests pass with ROCKE_BACKEND=both ROCKE_CPP_STRICT=1; full platform suite passes with no failures; pre-commit passes.

Submission Checklist

🤖 Generated with Claude Code

@cgmillette
cgmillette requested a review from a team as a code owner October 20, 2026 19:45
@therock-pr-bot

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

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

Copy link
Copy Markdown

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

@cgmillette

cgmillette commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor Author

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

PR #14050 — core dtype review follow-ups: uitofp_f32 source check and native test fixtures — 🟠 Not merge-ready: rebase required (code sound)

Prior item Status
Reviewers 🟢 None yet — only the policy bot
Bots / CI 🟢 Policy bot pass; TheRock CI (gfx94X, gfx110X) and hipDNN superbuild green. Multi-Arch is skipped because the PR is in a conflicting state
Our checks 🟠 Conflicts with develop. merge-tree shows a real content conflict in platform/CMakeLists.txt (the run_all.py / test_run_all.py overlap auto-resolves). Root cause: develop's #13802 ("magic division") landed on the same files — a rebase is required before this can merge or get CI (ran). Code itself is sound: native build of the head is clean (C++ core + pybind compile, rocke_engine imports, 25 ctests OK — ran); rule_scan clean (ran)
# Sev file:line Finding
N1 🟠 platform/CMakeLists.txt Content conflict with develop #13802; GitHub reports the PR CONFLICTING. Rebase onto current develop and re-resolve → then CI can validate the merged logic.

Checked and cleared (not findings): test_u8_arith.py::test_u8_hip_source_compiles fails on the head, but it fails identically on develop (a hipcc-toolchain env limitation on the verifier host, not a regression — ran both). The head adds one passing lane vs develop. The new uitofp_f32 guard (rocke_i_refuse_non_u8_source) is a correct reject-don't-normalize type check and builds natively.

Utility: 🟢 Positive — closes a real IR type-safety gap (u8-only uitofp_f32) and extends native test fixtures; the only thing standing between it and mergeable is the rebase.


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 4.8) verified it on hardware — native build, conflict analysis, and regression-vs-environment checks — 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.

@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 22:34
The uitofp_f32 builder accepts only a u8 input, but IR that did not come
through the builder (assembled by hand, or deserialized and retyped) could
carry another source type. The lowerings would then disagree: LLVM's
uitofp reads an i8 0xff as 255.0, while the HIP cast of an int8_t reads it
as -1.0.

The Python and C++ LLVM and HIP lowerings now refuse such an op with one
message, "arith.uitofp_f32 expects a u8 source, got <type>", from a shared
check in each engine. A test lowers a serialized kernel retyped from u8 to
i8 through every lowering of both engines and expects that refusal. The
u8 path lowers unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
Several pytest files compare the Python engine with a native C++ test
binary named by an environment variable (ROCKE_STORAGE_TEST,
ROCKE_IR_TYPES_TEST, ROCKE_NONTEMPORAL_HIP_TEST,
ROCKE_UNSIGNED_INT_OPS_CLI, ROCKE_ARCH_TARGET_TEST_EXE) and skip
without it. tests/run_all.py set all but the target identity one for a
source build, and the installed rocke_pytest CTest entry set only
ROCKE_UNSIGNED_INT_OPS_CLI, so the other parity cases skipped in the
installed test tree, and the target identity comparison skipped
everywhere.

tests/run_all.py now also resolves ROCKE_ARCH_TARGET_TEST_EXE from the
rocke_arch_target_identity CTest entry. The rocke_pytest entry points
each of the five variables at its installed binary, for every one that
is built and installed, in both the provider and the standalone install
layouts, from one list that replaces the entry's separate
ROCKE_UNSIGNED_INT_OPS_CLI handling. A comment beside run_all.py's list
says the installed entry carries the same variables.

Co-Authored-By: Claude <noreply@anthropic.com>
…oth engines

The C++ engine keeps a hand-written copy of arch_specs.json's
generic_targets table. The test comparing C++ target identity with
Python fed it only a sample of target names, so a member missing from,
or misspelled in, the C++ copy could go unnoticed.

The test now also feeds every generic processor and every member read
from arch_specs.json, in all the prefix, profile and feature forms it
already uses, so each member must resolve to the same catalog row and
generic processor in both engines.

Co-Authored-By: Claude <noreply@anthropic.com>
@cgmillette
cgmillette force-pushed the users/cgmillette/rocke/pr1-review-followups branch from a8d6c4f to d9e8bbc Compare October 22, 2026 15:09
@cgmillette

Copy link
Copy Markdown
Contributor Author
  • N1 (conflict with develop in platform/CMakeLists.txt): fixed by rebasing onto current develop (9a14fac). The conflict was with fix(rocke): reject stale portable-IR engine artifacts #13553, which added ROCKE_PROVENANCE_MANIFEST and its own ROCKE_UNSIGNED_INT_OPS_CLI handling to the installed rocke_pytest entry. The fixtures commit now keeps the single variable list (five native fixtures, including ROCKE_UNSIGNED_INT_OPS_CLI) and adds ROCKE_PROVENANCE_MANIFEST beside ROCKE_REPLAY_CLI, so nothing from develop is lost; tests/tools/test_run_all.py keeps both develop's new fixture checks and ours. Byte identity (llvm20/22/23), ctest, touched tests and the full platform suite pass on the rebased head.
  • N1 side note (test_u8_hip_source_compiles failing on the verifier host): it passes on our gate host on both the rebased head and develop, which agrees with the review's call that it was a toolchain environment limit there, not a regression.
  • CongMa13 (two reviews, no issues; approved): no action needed. The rebase changed only the conflict resolution described above.

@cgmillette

Copy link
Copy Markdown
Contributor Author

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

PR #14050 — core dtype review follow-ups: uitofp_f32 source check and native test fixtures — 🟢 Follow-up: no closer look needed — rebase resolved correctly; the installed lane now runs 127 parity cases it used to skip

Prior item Status
Ours (10-20) N1 🟠: conflict with develop #13802 in platform/CMakeLists.txt 🟢 FIXED — rebased, and only that commit changed. The merged rocke_pytest list moves #13802's ROCKE_UNSIGNED_INT_OPS_CLI into the new fixture loop and keeps -m "not integration" and the compiler PATH (read). Merges cleanly with today's develop (12 behind) (ran)
CongMa13 APPROVED (10-21) at a8d6c4fc 🟡 Given before the rebase; only the conflict resolution has changed since
Bots / CI 🟠 Re-run at head d9e8bbc4 (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 stage) and the hipDNN Linux superbuild are running
Our checks 🟢 Installed tree (merge, standalone cmake --install): rocke_pytest now names all five native fixtures (develop names one), and every path exists. Running the 11 affected test files from the install tree, 127 cases that develop skips now run and pass. The only failures, on both trees, come from missing hipcc headers on our host, and they pass once the headers are supplied (ran). Native build and 27 ctests pass; the PR's tests with the binding and fixtures: 183 passed (develop 182); the representative golden passes with no drift; rule_scan is clean (ran)
# Sev file:line Finding
N1 🟡 platform/CMakeLists.txt (native-parity list); tests/run_all.py:136 Land order with #13886: both add a NATIVE_PARITY_TESTS pair, so run_all.py and test_run_all.py conflict, and #13886's ROCKE_BUFFER_RSRC_HIP_TEST is missing from this list. In a scratch merge of both, 8 native-parity cases in test_buffer_rsrc_hip.py skip in the installed lane. With the pair added here, they run and pass (ran) → whichever PR lands second adds the pair in both places, as the new run_all.py comment asks.

Utility: 🟢 Positive — closes the u8-only uitofp_f32 type gap and lets the installed test lane run its native parity cases.


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, an installed-tree build and run on the merge against develop, the native build and tests, the golden gate, and a scratch merge with #13886, and owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection.

@cgmillette
cgmillette merged commit 11e3828 into develop Oct 22, 2026
143 of 147 checks passed
@cgmillette
cgmillette deleted the users/cgmillette/rocke/pr1-review-followups branch October 22, 2026 19:44
@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