Skip to content

fix(rocke): emit indexer scalar-body zero const before its loop - #14073

Merged
cgmillette merged 1 commit into
developfrom
users/arai/indexer_scalar_cpp_parity_fix
Oct 29, 2026
Merged

cgmillette merged 1 commit into
developfrom
users/arai/indexer_scalar_cpp_parity_fix

Conversation

@arai713

@arai713 arai713 commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

JIRA ID : AICK-2670

Motivation

The scalar lightning-indexer cases do not lower through the C++ engine, so the Python and C++ backends disagree for that body. test_attention_ir_cpp_parity.py::test_attention_ir_cpp_python_byte_identity fails when rocke_engine is built, with "rocke_engine.lower_serialized_ir: parse failed (status 1): operand '%c15' used before definition in op 'arith.select'". All six scalar-body harness cases (deepseek_hi64, glm_hi32, small on gfx942 and gfx950) fail at llvm20, llvm22 and llvm23. The four mfma_* cases are unaffected. CI missed it because the test skips when rocke_engine is not importable. This PR restores byte-identity for the scalar body.

Technical Details

  • In _emit_scalar_body (library/kernels/common/lightning_indexer.py) the i32 zero constant was created after the key scf_for.
  • IRBuilder.scf_for appends the loop op to the block immediately, so the constant landed after the loop that consumes it (the head-loop lower bound and the causal select).
  • The Python lowerer folds constants to immediates and never noticed, but the C++ engine's parser enforces define-before-use and rejected the serialized IR.
  • The fix moves the constant ahead of the scf_for so every operand is defined before the op that uses it, with a comment explaining the eager-append ordering constraint.
  • Emitted LLVM IR is unchanged, so the goldens are untouched.

Test Plan

  • test_attention_ir_cpp_parity.py with rocke_engine built and ROCKE_CPP_STRICT=1 at llvm20, llvm22 and llvm23
  • test_dsa_indexer_golden.py at llvm20/22/23 to confirm the emitted IR is unchanged
  • platform byte-identity gate (tools/check_byte_identity.py) across all families, to confirm no sibling define-before-use regression

Test Result

  • Parity test was red before this change (define-before-use parse error) and is green after, at all three llvm flavors.
  • test_dsa_indexer_golden.py passes at llvm20/22/23, confirming the IR did not change.
  • Platform byte-identity gate is green across all 81 families plus the gfx1250 family at llvm23.

Submission Checklist

@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 ⚠️ Warning Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: dnn-providers/hip-kernel-provider/rocke/library/kernels/common/lightning_indexer.py; no test file found
🔎 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.

@arai713
arai713 force-pushed the users/arai/indexer_scalar_cpp_parity_fix branch from 6b4ab45 to 4229eb3 Compare October 27, 2026 17:52
scf_for appends the loop op immediately, so the i32 zero created after it
was defined after the loop that uses it. The Python lowerer folds constants
to immediates, but the C++ parser enforces define-before-use and rejected
the serialized IR.
@cgmillette

Copy link
Copy Markdown
Contributor

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

PR #14073 — emit indexer scalar-body zero const before its loop (arai713, AICK-2670) — 🟢 First-round: no closer look needed — verified fix for #14630

Prior item Status
AnaghaRaoAMD (10-28): APPROVED at 916306eb 🟢 Current — given at head, the PR's only commit; no comments or threads (read)
Develop break (#14630, 10-29): parity gate red since #12548; engine rejects 6 scalar indexer kernels 🟢 FIXED 916306eb — a native engine built from each tree (develop d6957099 and the merge differ only in this file). Develop: test_attention_ir_cpp_python_byte_identity fails at llvm20/22/23/24 with operand '%c15' used before definition in op 'arith.select'; case by case 67/73 match, and the 6 failures are exactly the scalar indexer kernels. Merge: the file passes (2 passed, no skips) at all four flavors, 73/73 byte-identical. verify() is clean and parse(serialize(k)) round-trips for all 10 indexer cases; on develop the 6 scalar ones fail both (ran)
Bots / CI 🟢 Policy bot pass; its unit-test warning is moot, as the existing parity test is the regression test. At 916306eb every workflow passes (Multi-Arch CI and ASAN, Component CI, pre-commit, clang-tidy, hipDNN superbuild). hipkernelprovider passed on Linux gfx94X and Windows gfx110X (2 shards each), including rocke_golden_static and the indexer numerics on gfx942 hardware (5 scalar + 5 MFMA, Python-lowered; gfx950 skips without a GPU). No lane builds rocke_engine (14 "engine not importable" skips per shard) or runs the parity test, so CI cannot see this fix; #14630 proposes the CI change (read)
Our checks 🟢 Clean merge with develop d6957099 (17 behind) (ran). No re-bless: the indexer's Python-lowered LLVM matches develop for all 10 cases × 4 flavors, test_dsa_indexer_golden.py passes 8/8, the representative golden gate passes at all four flavors, and the differential goldens are OK (ran). Rule 9: all 312 harness cases (28 families) round-trip on the merge; an AST scan of the 282 x = b.scf_for / scf_for_iter / scf_if / scf_if_else(…) bindings (all four emit the op on creation) finds code emitted before the with only at this site on develop, and nowhere on the merge (ran). Rules 1–3 clean (ran)
# Sev file:line Finding
N1 🟡 PR description Nothing ties this PR to #14630: no Fixes keyword and no manual link, so there is no closing reference in either direction (read). Merging will not close the issue → add Fixes #14630 to the description. The rocKE merge monitor treats this PR as the fix either way

Utility: 🟢 Positive — one-line move restores C++/Python parity for six indexer kernels; LLVM unchanged.


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

First-round review: a local qwen3.6 model made the independent first pass. The orchestrator (Claude Opus 5.5) verified it. On a clean merge with today's develop, and on develop alone, each with its own freshly built native engine, it ran the C++/Python parity test at llvm20/22/23/24 (as a whole and case by case), verify() on the indexer cases, the serialize/parse round trip over all 312 parity-harness cases, the indexer, representative and differential goldens, and an AST scan for the same bug shape. It also read the CI logs at head. The orchestrator owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection.

@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, fixes develop

@cgmillette
cgmillette enabled auto-merge (squash) October 29, 2026 04:13
@cgmillette
cgmillette merged commit 77a7a40 into develop Oct 29, 2026
149 of 152 checks passed
@cgmillette
cgmillette deleted the users/arai/indexer_scalar_cpp_parity_fix branch October 29, 2026 04:15
@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.

3 participants