Repository navigation
Conversation
✅ 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. |
…e differently (AICK-2762) Defect: the include picker took a type's header from typedef_file, the first header (alphabetically) that defines it. rocke_tensor_descriptor_t is two different structs, in helper_rocke.helpers.tensor_view.h and in helper_rocke.helpers.transforms.h; a source that names the type and calls transforms.h functions got both headers. How it showed: the e2e port of transpose_bc against the PR #14538 headers. A build_transpose_bc fill that spells its descriptors rocke_tensor_descriptor_t* (and calls rocke_tensor_descriptor_naive / _transform / _unmerge_lower of transforms.h) regenerates with both includes; g++: "redefinition of 'struct rocke_tensor_descriptor'". The fill had to write auto* to dodge it. Fix: Headers records every header that typedefs a name and the bodies of the complete definitions. A type several headers define comes from a header the source already needs (the one of the functions it calls with it), else from the first that adds no conflicting definition. If two headers the source needs still define a name differently, that is an include conflict: tr.translate lists "conflict <type> (...)", writes an #error in the source and exits 1, and tr.regen writes nothing. A type defined in one header is picked as before (goldens unchanged). Evidence: tests/test_include_conflicts.py (4 tests; all 4 fail before the fix): a transforms.h call picks transforms.h only and the source compiles; calls into both headers are a reported conflict and regen writes nothing. The e2e transpose_bc regen with the typed fill now includes transforms.h only. Self-test 105 pass; measure.py unchanged (all 54.4%, 7/7, 7/7). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eturns ok (AICK-2762) Defect: in the hand-ABI shim (translating against an existing header), a Python (ok, reason) function behind a C `bool f(..., char* reason, size_t reason_cap)` became `return (bool)py_is_valid_spec(...);` inside py_adapt::with_reason: a std::tuple cast to bool, which does not compile, and the reason is never written. The generated-ABI shim already had the right body. How it showed: the e2e regen of batched_transpose and transpose_bc against the PR #14538 headers (rocke_*_is_valid_spec); g++: "invalid cast from type 'std::tuple<bool, const char*>' to type 'bool'". Hand edit needed in both. Fix: one routine, ok_reason_body(), builds the gate's body (the reason copied into the buffer, truncated and NUL-terminated, ok returned) and both make_shim (hand ABI) and make_gen_shim (generated ABI) use it. Any other tuple result behind a reason entry is refused as an ABI hole instead of a cast that cannot compile. The generated-ABI output is unchanged. Tests: tests/test_hand_abi_shims.py translates batched_transpose against the reviewed instance_batched_transpose.h of PR #14538 (fixtures/hand_abi/, put into a symlink copy of the tree by trtest.overlay_root): the gate writes the reason and returns ok, both paths share the routine, the source compiles (-Wall -Werror). All 3 fail before the fix. trtest.strip_holes also strips an #error that clang-format wrapped after padding. Evidence: self-test 108 pass (goldens unchanged); measure.py unchanged (all 54.4%, headers 7/7, cpp 7/7). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… counts ABI holes (AICK-2762) Defect: translating against an existing header (hand ABI), a public function whose Python int (int64_t inside) the header returns as C int got '#error "abi hole: <c>: shim: Python return int64_t as C int"'. --analyze reported it as translated (0 holes) and tr.regen wrote the file anyway, a source that cannot compile. How it showed: the e2e regen of batched_transpose and transpose_bc against the PR #14538 headers (rocke_batched_transpose2d_block_size, rocke_transpose_bc_block_size): "0 holes", files written, #error in both; a hand edit was needed in both. Fix: (a) make_shim narrows an int64_t result to a C int / int32_t with a range check. A builder entry raises on the builder when out of range (its existing convention, -1). Any other entry takes the failure value its header documents (documented_fail reads the comment above the declaration, e.g. "Returns 0 for a NULL spec or vec == 0"): a NULL pointer argument the comment names, a Python raise (ckc::Error) and an out-of-range result all return it. A header that documents no failure value leaves an ABI hole. (b) An ABI hole is a hole of the report: status "hole", reason "abi hole: ...", counted by --analyze and the summary; tr.translate exits 1 and tr.regen writes nothing while one is open. A hand fill of the function replaces its C entry (status "filled"), as for any refused function. Tests (tests/test_hand_abi_shims.py, against the reviewed header in fixtures/hand_abi/): the narrowing follows the header (NULL guard, range check, 0 on a raise); with the comment removed the entry is a hole that --analyze counts, translate exits 1 and regen writes nothing; a fill replaces it; documented_fail takes only failure values ("Returns 1 if the spec is valid" is not one). 5 of these fail before the fix. Evidence: e2e regen (scratch copy) of both modules: block_size entries generated, both sources pass g++ -std=c++20 -Wall -Werror -fsyntax-only. Self-test 113 pass (goldens unchanged); measure.py unchanged (all 54.4%, headers 7/7, cpp 7/7). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-2858) (#14706) ## rocKE: buffer address space in the shared C++ tensor view ISSUE ID : AICK-2858 Three commits (feature, tests, then a review fix); please use "Rebase and merge". ### Why The Python tensor view supports global, LDS and buffer views. Its C++ twin only supports global and LDS: `rocke_tensor_view_t` has nowhere to hold a `BufferResource`, so every `ROCKE_ADDR_BUFFER` branch in `cpp/helpers/tensor_view.cpp` is a "NAMED GAP" that returns NULL, and `make_buffer_view` has no C++ twin. That has three costs: - **Duplicate code.** Seven C++ instances that need buffer access roll their own private buffer-resource structs and shims (pooling, img2col, conv_implicit_gemm wgrad and build glue, deep_fused_conv_pool on gfx950 and gfx1201). Each copy has to be kept in step with Python by hand. - **New twins.** A Python instance that calls `make_buffer_view` (today `transpose_bc`, #14538) cannot use the shared helper in its C++ twin. - **The translator** (#14534) can port Python helpers into their C++ twin, but not `make_buffer_view`, because there was no struct to fill. ### What this does | Python (`helpers/tensor_view.py`) | C++ twin | |---|---| | `BufferResource` | `rocke_tv_buffer_resource_t` | | `make_buffer_resource` | `rocke_tv_make_buffer_resource` | | `make_buffer_view` | `rocke_tv_make_buffer_view` | | `TensorView.buffer` | `rocke_tensor_view_buffer` | | buffer branches of `load_scalar`, `store_scalar`, `load_vec`, `store_vec`, `load_vec_at`, `store_vec_at` | same functions, branch bodies filled in | | `load_vec_at(..., mask=)`, `store_vec_at(..., mask=)` | `rocke_tensor_view_load_vec_at_masked`, `rocke_tensor_view_store_vec_at_masked` | - `rocke_tensor_view_t` gains one field, `buffer`, at its end. All zeros means "no buffer", and it is only read when `addr_space == ROCKE_ADDR_BUFFER`. The existing view factories zero it. - Every port follows Python line for line: same builder calls, same order. Python raises become builder errors with the same type and message. - The struct grows, so `ROCKE_ABI_VERSION` (and `portable_ir` `BINARY_ABI`) goes from 4 to 5, as `abi.h` requires. - The `rocke_tv_` prefix avoids a clash with pooling.cpp's private `rocke_buffer_resource_t`, which has a different layout. ### Existing lowerings do not change The global and LDS branches are untouched, and nothing in develop builds a buffer view through the shared helper today. To prove it, the full gate set was run on develop first and then on this branch: | Check | develop | this branch | |---|---|---| | Byte-identity gate, llvm22 and llvm20 | 85 + 4 (gfx1250 @ llvm23) families, 2,010 configs GREEN | 86 + 4 families, 2,115 configs GREEN (+ `tensor_view_buffer`, 105 configs) | | Byte-identity gate, llvm23 | 88 families, 1,897 configs GREEN | 89 families, 2,002 configs GREEN | | clang `-Wall -Werror` build of `rocke_core` | pass | pass | | ctest | 33/33 | 34/34 (+ `rocke_tensor_view_buffer_errors`) | | Differential golden check | OK | OK; only the new family's hashes are added, no existing line changes | | Python suites (platform runner, both backends; library CPU lane under `ROCKE_BACKEND=both`) | 5,796 platform tests pass; library 66,803 tests, 39 fail | 5,811 pass (+15 new); library: same 66,803 tests, the same 39 fail (none related) | Numbers are for the current develop (`b0f07f86451`) and this PR head. Full test plan: #14706 (comment) Full results (develop vs this PR, including the Python platform and library suites): #14706 (comment) ### Tests - Parity family `tensor_view_buffer` (`tests/instances/parity/tensor_view_buffer_emit.{py,c}`): 35 cases on gfx950, gfx1151 and gfx1201. 17 accepted f16 cases cover every ported path, with default and explicit soffset, packed and runtime strides, with and without a mask; all byte-identical. 18 rejected cases hit each Python raise (unsupported dtype, width outside {2, 4, 8}, mask on a global view, buffer view with no resource). - `test_tensor_view_buffer_errors` (`.py` and native `.cpp`) checks that both engines raise the same error kind and message for each rejected case, since the gate only checks that both reject. ### Not in this PR - Moving the seven instances onto the shared helper (follow-ups, one instance at a time). - `load_scalar_at`, `store_scalar_at`, `async_load_lds_at`, `load_vec_tr_at`: these have no C++ twin for any address space yet. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Add a C++ twin of rocke/instances/common/batched_transpose.py:
- instance_batched_transpose.h: the C spec struct, spec_default,
block_size, kernel_name, is_valid_spec (bool + reason buffer, NULL arch
=> gfx950) and build_batched_transpose2d_new, named
rocke_batched_transpose2d_* like the Python module.
- instances/common/batched_transpose.cpp: the build mirrors the Python op
by op (global -> LDS vector copy through the tensor_view TileWindow,
barrier, column-strided LDS reads packed into one global store per
thread), so the emitted IR is byte-identical. The build entry runs under
ckc::guard_builder. block_size uses Python floor division, an unknown
arch reports the Python KeyError text and validate_io formats its reason
into a scratch arena, so every reject reason matches the Python.
- parity/batched_transpose_emit.{py,c}: 14 configs. Nine build: tiles
16/32/64, vec 2/4/8, f16/bf16 and the fp16 alias, LDS padding 0/4/8/16,
a custom name, the 1024-thread block and one gfx1151 target. Five are
rejected (unknown arch, f32, vec 3, a non-square tile, a negative tile);
a rejected config prints its reason on stdout on both sides, so the gate
compares the reject reasons byte for byte.
- instances/test_batched_transpose.py: CPU-only checks that valid specs
build and verify clean and that the reject reasons are the expected ones.
The byte-identity gate is GREEN at llvm20 and llvm22 for batched_transpose
with 14 configs. The differential golden gains the batched_transpose hashes
(ll at llvm20/22/23 and canonical IR).
The first draft was written by a Python-to-C++ translator and then
reviewed and completed by hand.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-2830)
Add a C++ twin of rocke/instances/common/transpose_bc.py, the 2D transpose
in which each lane transposes one vec x vec sub-tile in its own registers:
- instance_transpose_bc.h: the C spec struct, spec_default, block_size,
kernel_name, is_valid_spec (bool + reason buffer, NULL arch => gfx950)
and build_transpose_bc_new. transpose_bc_grid and transpose_bc_signature
are host-side helpers with no C twin.
- instances/common/transpose_bc.cpp: the build mirrors the Python op by
op (lane unmerge, vec row loads over flat global or buffer IO, the
in-register transpose, vec row stores), so the emitted IR is
byte-identical. The global and buffer views and their load_vec_at /
store_vec_at are the shared tensor_view twins; TensorCoordinate has no
C twin yet, so its moves are written inline in Python order.
- instances/common/transpose_bc_lanes.cpp: the lane unmerge, in its own
translation unit because helper_rocke.helpers.transforms.h and
helper_rocke.helpers.tensor_view.h define struct
rocke_tensor_descriptor differently.
- parity/transpose_bc_emit.{py,c}: 18 configs, each built at its own
arch. Ten build: vec 2/4/8, f16 and bf16, block sizes 64/256/576, flat
global and buffer IO, a custom kernel name and one wave32 gfx1151
target. Eight are rejected (a block below one wave, a block that is not
whole wave32 waves, unknown arch, vec 3, a non-square sub-tile, f32, a
tile that is not a multiple of vec, a block over the thread cap); a
rejected config prints its reason on stdout on both sides, so the gate
compares the reject reasons byte for byte.
- instances/test_transpose_bc.py: CPU-only checks that valid specs build
and verify clean and that the reject reasons are the expected ones.
The byte-identity gate is GREEN at llvm20, llvm22 and llvm23 for
transpose_bc with 18 configs. The differential golden gains the transpose_bc hashes (ll at
llvm20/22/23 and canonical IR).
The first draft was written by a Python-to-C++ translator and then
reviewed and completed by hand.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7a304b2 to
0483fb8
Compare
There was a problem hiding this comment.
Blocking — merge after #14365. CI never compares these twins with Python.
check_byte_identity.pyruns in no CI job;test_transpose_bc.py/test_batched_transpose.pyare Python-only.- Fix once feat(rocke): C++ twins of the SDPA backward kernel builders #14365 lands: add
transpose_bcandbatched_transposetorocke_native_golden_ll(tests/cmake/NativeGoldenTests.cmake). That list reads emitters fromlibrary/tests/parityonly; these live inplatform/tests/instances/parity, so it needs a per-family source dir. The golden hashes are already blessed in this PR.
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14538 — C++ twins of the batched_transpose and transpose_bc instances (afagaj, AICK-2830) — 🟠 First-round: twins not compared in CI
Utility: 🟢 Positive — C++ coverage for two Python-only instances; byte-identical, other output unchanged. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted First-round review: first pass by a local qwen3.6 model, verified by the orchestrator (Claude Opus 5.5). Checks: merge with develop, native build + ctest, byte identity at llvm20/22/23/24, a 3,717-case C-vs-Python grid at llvm22/23, goldens and differential goldens, the PR's tests, compile, a GCC LTO ODR probe, CI at head. Flags whether a closer look is needed; not an approval or rejection. |
|
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. |
There was a problem hiding this comment.
🟡 Changes recommended
Unchecked integer arithmetic can overflow and bypass validation, while builder failure paths can omit documented sticky errors or allow C++ exceptions to escape the public C API.
5 open issues
Widen tile-size arithmetic before overflow-prone calculations · New Widen LDS padding addition before computing allocation size · New Initialize an error-carrying builder when generated names are too long · New Check lane-count product in a wide type before narrowing · New Catch builder initialization exceptions at the C API boundary · New
What changed in this PR
Extends rocKE’s C++ engine with counterparts to the existing Python batched and register-only transpose builders, with fixtures for checking byte-identical LLVM IR and rejection reasons.
Changes:
- Adds spec, validation, naming, and build APIs for both transpose families.
- Reuses shared tensor-view helpers and isolates lane-coordinate transforms.
- Adds parity emitters, differential golden hashes, CPU tests, and test-inventory documentation.
| File | Description |
|---|---|
dnn-providers/hip-kernel-provider/rocke/platform/tests/README.md |
Documents the new tests and parity coverage. |
dnn-providers/hip-kernel-provider/rocke/platform/tests/instances/test_transpose_bc.py |
Checks Python register-transpose builds and rejection reasons. |
dnn-providers/hip-kernel-provider/rocke/platform/tests/instances/test_batched_transpose.py |
Checks Python batched-transpose builds and rejection reasons. |
dnn-providers/hip-kernel-provider/rocke/platform/tests/instances/parity/transpose_bc_emit.py |
Emits Python reference output for register-transpose configurations. |
dnn-providers/hip-kernel-provider/rocke/platform/tests/instances/parity/transpose_bc_emit.c |
Emits matching C++ register-transpose output. |
dnn-providers/hip-kernel-provider/rocke/platform/tests/instances/parity/batched_transpose_emit.py |
Emits Python reference output for batched-transpose configurations. |
dnn-providers/hip-kernel-provider/rocke/platform/tests/instances/parity/batched_transpose_emit.c |
Emits matching C++ batched-transpose output. |
dnn-providers/hip-kernel-provider/rocke/platform/tests/instances/differential/golden/llvm_gfx_all.json |
Records golden hashes for both new families. |
dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/common/transpose_bc.cpp |
Implements the register-only transpose twin using shared views. |
dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/common/transpose_bc_lanes.cpp |
Isolates lane-coordinate transforms from conflicting headers. |
dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/common/batched_transpose.cpp |
Implements the LDS-staged batched-transpose twin. |
dnn-providers/hip-kernel-provider/rocke/platform/cpp/include/rocke/instance_transpose_bc.h |
Declares the register-transpose spec and C-compatible APIs. |
dnn-providers/hip-kernel-provider/rocke/platform/cpp/include/rocke/instance_batched_transpose.h |
Declares the batched-transpose spec and C-compatible APIs. |
Review effort: Max
| return 0; | ||
| } | ||
| /* (tile_m * tile_n) // vec */ | ||
| return bt_floordiv(spec->tile_m * spec->tile_n, spec->vec); |
| const long lds_bytes = (long)spec->tile_m * (long)(spec->tile_n + spec->lds_pad) * 2L; | ||
| if(!rocke_arch_fits_lds(target, lds_bytes)) | ||
| { | ||
| snprintf(buf, | ||
| sizeof(buf), | ||
| "LDS staging %ld > %d cap on %s", | ||
| lds_bytes, | ||
| target->lds_capacity_bytes, | ||
| arch); | ||
| rocke_spec_set_reason(reason, reason_cap, buf); | ||
| return false; | ||
| } |
| if(rocke_batched_transpose2d_kernel_name(spec, name, sizeof(name)) != ROCKE_OK | ||
| || rocke_ir_builder_init(b, name) != ROCKE_OK) | ||
| { | ||
| return NULL; | ||
| } | ||
| return ckc::guard_builder(b, [&]() -> rocke_kernel_def_t* { return bt_build(b, spec, arch); }); |
| const int bs = lanes_m * lanes_n; | ||
| const int max_threads = rocke_arch_max_threads_per_block(target); | ||
| if(bs > max_threads) | ||
| { | ||
| snprintf(buf, sizeof(buf), "block_size %d > %d hardware cap on %s", bs, max_threads, arch); | ||
| rocke_spec_set_reason(reason, reason_cap, buf); | ||
| return false; | ||
| } |
| char name[256]; | ||
| if(rocke_transpose_bc_kernel_name(spec, name, sizeof(name)) != ROCKE_OK) | ||
| { | ||
| if(rocke_ir_builder_init(b, "rocke_invalid_spec") == ROCKE_OK) | ||
| rocke_i_set_err_msg(b, ROCKE_ERR_VALUE, "transpose_bc kernel name too long"); | ||
| return nullptr; | ||
| } | ||
| if(rocke_ir_builder_init(b, name) != ROCKE_OK) | ||
| return nullptr; | ||
| return ckc::guard_builder(b, [&]() -> rocke_kernel_def_t* { return tbc_build(b, spec, arch); }); |

rocKE: C++ twins of the batched_transpose and transpose_bc instances
ISSUE ID : AICK-2830
Two commits, one per instance; please use "Rebase and merge".
What this does
Two platform kernel instances were Python-only. This adds their C++ twins, which emit byte-identical LLVM IR, so the C++ engine covers them:
instances/common/batched_transpose.pycpp/instances/common/batched_transpose.cppinstance_batched_transpose.h(rocke_batched_transpose2d_*)instances/common/transpose_bc.pycpp/instances/common/transpose_bc.cppinstance_transpose_bc.h(rocke_transpose_bc_*)Each commit adds the twin, its header, a parity driver pair (
tests/instances/parity/<instance>_emit.py/_emit.c), the family's differential golden hashes and a CPU test (tests/instances/test_<instance>.py). The headers follow the existing instance ABI (spec_default,block_size,kernel_name,is_valid_specreturningboolwith a reason buffer, and the build entry).transpose_bcuses the shared tensor_view twins (from #14706) for its global and buffer views and theirload_vec_at/store_vec_at, as the Python does. Its lane unmerge is intranspose_bc_lanes.cpp, a translation unit of its own, becausehelper_rocke.helpers.transforms.handhelper_rocke.helpers.tensor_view.hdefinestruct rocke_tensor_descriptordifferently (as forbindings/rocke_engine_moe_gemm_fused.cpp). TensorCoordinate has no C twin yet, so its moves stay inline.The parity drivers include configs the Python rejects. On those, both drivers print the reject reason, so the gate also compares the reason text byte for byte.
Checks
On develop 7cd54ff:
batched_transpose14 configs (5 rejects),transpose_bc18 configs (8 rejects).rocke_corebuilds with clang-Wall -Werror.Not in this PR
rocke_enginepybind bindings for these two families. Nothing in Python calls them yet.How it was written
The first draft came from an experimental Python-to-C++ translator (#14534). Most of the code was then written or rewritten by hand, and every line was reviewed. The gate is the acceptance test, as for every twin.
🤖 Generated with Claude Code