Repository navigation
fix(rocke): refuse a buffer descriptor as a loop-carried value - #14294
Merged
cgmillette merged 1 commit intoOct 26, 2026
Merged
Conversation
A buffer descriptor (tile.buffer_rsrc) is a <4 x i32> in the IR but an opaque ptr addrspace(8) (HIP rsrc_t) once lowered. A descriptor passed as an scf.for iter_arg, as its initial value or as a yielded value, became a phi over those pointers (HIP: an i32x4 variable), and the compiler refused the module with an error that did not point at the loop. Refusing it loses nothing: no op edits an existing descriptor, so a loop can only use the one built before it, advancing the offset it passes to each access, or build a new one in its body. Both forms keep working. One rule per engine (ir._scf_for_carried_descriptor_error and rocke_i_scf_for_carried_descriptor_error) recognises a descriptor as the result of tile.buffer_rsrc and refuses it as an iter_arg init or a yielded loop-carried value with one text in both engines: scf.for iter_arg <i> is a buffer descriptor, which a loop cannot carry; build the descriptor before the loop and advance the offset, or rebuild it inside the loop body The builders (scf_for_iter, and scf_yield inside a loop body), both verifiers and every scf.for lowering (LLVM, looped and unrolled, and HIP, in both engines) apply it, so serialized IR that bypasses the builder is refused too. The rule only slices the operand and region lists, so it accepts any counts. New tests cover the Python builder and verifier, all four lowering routes on serialized IR, and a C++ ctest case for the C++ builder and verifier. Controls check that a descriptor built before the loop and one rebuilt in the body lower to identical LLVM IR and HIP source in both engines and compile. op_vocabulary.md states the rule next to scf.for. Co-Authored-By: Claude <noreply@anthropic.com>
github-actions
Bot
added
documentation
project: hip-kernel-provider
rocke: platform
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
Contributor
Author
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14294 — refuse a buffer descriptor as a loop-carried value — 🟢 First-round: no closer look needed — both engines refuse a carried descriptor with the same text at every entry point, and no valid kernel's IR moves
No net-new findings. Utility: 🟢 Positive — turns a compiler failure that pointed nowhere near the loop into an up-front refusal that says what to do instead. It changes nothing for valid kernels, in either engine. 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. The checks were the native build and ctests, the new tests with both engines in strict mode, a differential sweep of every anchored family against develop, byte identity, the golden gate and the platform suite on the merge and develop. They also covered a scratch merge with #14263 and 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
enabled auto-merge (squash)
October 26, 2026 17:46
CongMa13
approved these changes
Oct 26, 2026
CongMa13
left a comment
CongMa13
left a comment
Contributor
There was a problem hiding this comment.
No new P0 or P1 issues were identified during this review. The changes are approved.
cgmillette
deleted the
users/cgmillette/rocke/refuse-carried-descriptor
branch
October 26, 2026 19:19
Contributor
TheRock Submodule Bump ActivityNewest first
|
cgmillette
added a commit
that referenced
this pull request
Oct 30, 2026
…y width, if/else results, pointer offset, trap (#14263) ## Motivation Tiling kernels that handle arbitrary sizes and ragged batches need a few core ops that rocKE does not have yet: buffer accesses of every element width (so the hardware's range check can mask the last, partial element), an if/else that yields values, a pointer offset that makes no in-bounds promise, and a trap. This PR adds them to both engines. Nothing that uses them is in this PR. ISSUE ID : AICK-2579 ## Technical Details Four commits. Each one is in both engines, with byte-identical LLVM IR, identical HIP source and the same error texts. Each one also refuses bad input in the builders, the verifiers and every lowering, so serialized IR that bypasses the builder gets the same message; for a built op the operand, result and region counts are checked before the op is read. 1. **Buffer load and store for every element width.** - `buffer_load_vN` takes any scalar whose width is whole bytes (i8, u8, fp8, bf8, i16, f16, bf16, i32, f32, i64, f64), with any count whose access is 1, 2, 4, 8 or 16 bytes. - A new `buffer_store_vN` stores the same element types and widths. - Each access is one raw buffer load or store of its width, bitcast to or from the vector. The 1- and 2-byte forms are new. - Whole-dword loads keep their `dwords` attr, so existing kernels are unchanged. - The count must be an int; the C++ engine computes the width in 64 bits, so a large count is refused as in Python. 2. **If/else that yields values.** - `scf_if_else` takes `result_types`. Each branch ends in `scf_yield` of one value per type, and the results are the values of the branch that ran. - The LLVM lowering puts one phi per result at the head of the join block. - The HIP route gets its first `scf.if_else` handler: it declares the results before the `if` and assigns them at the end of each branch. - A buffer descriptor cannot be a result, for the reason a loop cannot carry one (#14294): it is `<4 x i32>` in the IR but an opaque `ptr addrspace(8)` (HIP `rsrc_t`) once lowered. The if/else rule recognises a descriptor with the same check as the loop rule (`_is_buffer_descriptor` / `rocke_i_is_buffer_descriptor`), and its message has the same form, naming the result and the branch. An if/else inside a loop body that yields a descriptor gets the if/else message. - In C++, `rocke_b_scf_if_else_results` is new; its results are the op's results. `rocke_b_scf_if_else` and the handle it returns are unchanged, so the binary ABI is unchanged. 3. **A non-inbounds global pointer offset, and pointer-to-integer on HIP.** - `global_ptr_offset(ptr, byte_off)` lowers to `getelementptr i8` without `inbounds` in LLVM IR. In HIP it becomes integer arithmetic that keeps the pointer's element type: `(T*)((uintptr_t)p + (uint64_t)off)`. - The offset must be i64. It is never widened implicitly, so a negative offset stays negative. - `global_ptr_to_i64` gains its HIP lowering, and a rule of its own: the operand is a global pointer and the result is i64. 4. **A trap op.** - `trap()` lowers to `llvm.trap` (`s_trap 2`) and to `__builtin_trap()` on HIP. - A trap with operands, results or regions is refused: the lowerings emit only the call, so a result would be undefined. - A taken trap ends the kernel with a hardware exception. Depending on the HIP runtime the launch reports an error or the process is aborted; either way the process cannot use the GPU afterwards. The docstring and the op reference therefore describe it as a last resort. - `llvm.trap` is measured into the llvm20, llvm22 and llvm23 arch-domain artifacts. `dsl_docs/reference/op_vocabulary.md` documents each new op with its LLVM and HIP forms. ## Test Plan - One new test file per commit: - `test_buffer_access_widths.py` and `test_buffer_oob_facts.py` for the buffer accesses; - `test_scf_if_else_results.py`; - new parity families built with the C++ builder: `buffer_load_vN`, `buffer_store_vN` and `scf_if_else_results`; - `test_global_ptr_offset.py`; - `test_trap.py`. - Each file checks the LLVM IR shape, both engines' IR at llvm20, llvm22 and llvm23, identical HIP source, the builder, verifier and lowering refusals with their exact texts (malformed ops included), and, on a GPU, the LLVM and hipcc routes. - The pointer-offset tests also check that hipcc's optimized device IR has no in-bounds step. - The trap tests refuse a trap with a result, an operand or a region, check that the code object contains `s_trap 2`, and run a launch that does not take the trap in a child process. No test takes the trap on a GPU: what happens then is the HIP runtime's behaviour. - The `future_intrinsic_lowering` ctest covers the new C++ builders, their refusals, the C++ verifier's refusals and the ops' purity. - Each commit was built and tested on its own. ## Test Result Rebased onto current develop (e8b9598). - Byte identity green at llvm20, llvm22 and llvm23. - ctest 33 / 33. - Portable IR against a freshly built shared engine: 195 passed. - The touched tests pass with both engines in strict mode: 242 passed. The one exception is `test_the_cpp_fallback_does_not_warn_twice`, which stubs a C++ failure to exercise the Python fallback that strict mode forbids; it passes in the normal mode. - Full platform suite: 4828 passed, 0 failed. - Library smoke finds no new failure. - pre-commit passes. - Every commit builds and passes ctest and its own tests on its own. - The GPU tests ran on gfx90a (the trap test runs only the launch that does not take the trap). ## 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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
A buffer descriptor (
tile.buffer_rsrc) is<4 x i32>in the IR but an opaqueptr addrspace(8)(HIPrsrc_t) once lowered. A kernel that passed a descriptor through anscf.forloop, as an iter_arg's initial value or as a yielded value, produced a phi over those pointers (on HIP, ani32x4variable), and the compiler then refused the module with an error that did not point at the loop. This PR refuses that form up front, in both engines, with a message that says what to do instead.ISSUE ID : AICK-2736
Technical Details
Refusing a carried descriptor loses nothing: no op edits an existing descriptor, so a loop can only use the descriptor built before it (advancing the offset it passes to each access) or build a new one in its body. Both forms keep working.
One rule per engine recognises a descriptor as the result of
tile.buffer_rsrcand refuses it as an iter_arg init or a yielded loop-carried value:ir._scf_for_carried_descriptor_errorrocke_i_scf_for_carried_descriptor_errorBoth engines give the same text:
The rule is applied by the builders (
scf_for_iter, andscf_yieldinside a loop body), both verifiers, and everyscf.forlowering in both engines (LLVM looped, LLVM unrolled, HIP), so serialized IR that bypasses the builder is refused too. The rule only slices the operand and region lists, so it accepts any counts.No emission changes for valid kernels: byte identity is unchanged and no golden is re-blessed.
dsl_docs/reference/op_vocabulary.mdstates the rule next toscf.for.feat(rocke): core ops for ragged tile access: buffer accesses of every width, if/else results, pointer offset, trap #14263 (open) refuses descriptors as if/else results with the same "is this a descriptor" test. Whichever of the two lands second will merge the two helpers into one; the overlap is a few lines in
core/ir.py.Test Plan
tests/core/test_loop_carried_descriptor.py:tests/core/test_loop_carried_descriptor.cpp(ctestrocke_loop_carried_descriptor): the C++ builder and verifier refuse both forms.Test Result
Submission Checklist
🤖 Generated with Claude Code