Repository navigation
fix(rocke): make both engines emit the same HIP source - #14845
cgmillette wants to merge 12 commits into
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. |
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14845 — make both engines emit the same HIP source (cgmillette, AICK-2903) — 🟡 First-round: gate holds, one repr gap
Utility: 🟢 Positive — engines now emit identical HIP for every parity kernel; gate local-only. 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 and goldens at four LLVM flavors, the new gate and all HIP-route tests with the engine, a both-engine sweep over every wired target, literal and kernel compiles, CI at head. Flags whether a closer look is needed; not an approval or rejection. |
|
This is OK as-is, with the noted N1 formatting finding above. If you do make changes, I would recommend having agents look at this changes in the context of single-responsibility principle (SRP) and don't-repeat yourself (DRY). This may make the design cleaner and easier to test and maintain. Keeping python and C++ in sync can be subtle, so a good design may help simplify future improvements. Here's how my agents responded to my questions in SRP and DRY: What it gets right
Problems
Better abstractionMove the primitive to a neutral support module, for example: With responsibilities such as: void rocke_append_python_float_repr(rocke_strbuf_t*, double);Then use that single implementation from:
Keep the HIP policy adapter separate: rocke_h_f32_literal(...) // NaN/Inf spelling + "f"
rocke_h_f64_literal(...) // NaN/Inf spelling, no suffixAdd a focused table/property test covering:
So my answer is: the HIP-local helper is reasonable, but the PR does not yet provide the right shared, directly testable SRP/DRY abstraction for float formatting. A small consolidation would materially improve it. |
|
I reviewed this PR again. The following review threads are still unresolved, or comments have no author reply: If you have already fixed these, please reply on each comment to say so and resolve the threads. |
8cc38ce to
83770a6
Compare
|
Thanks @shumway. I took the SRP / DRY suggestion across the whole change, and N1 (2**-24) is fixed. The fixes are folded into the commits they belong to. Float formatting: one primitive, one adapter per engine
Tests for the formatter
The same treatment for the MMA port and the refusals
Results
|
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14845 — make both engines emit the same HIP source (cgmillette, AICK-2903) — 🟠 Follow-up: N1 fixed, ABI bump owed
Utility: 🟢 Positive — both engines emit identical HIP for every parity kernel; repr-exact floats. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted Follow-up review: first pass by a local qwen3.6 model, verified by the orchestrator (Claude Opus 5.5). Checks: range-diff, merge with develop (conflict resolved), native build + ctest, byte identity and goldens at four LLVM flavors, differential, the PR's tests in three modes, HIP route tests and the identity gate with the engine, both-engine float and MMA sweeps, ABI layout, CI at head, code quality. Flags whether a closer look is needed; not an approval or rejection. |
CongMa13
left a comment
There was a problem hiding this comment.
One new issue found; see the inline comment about multiline inline-assembly templates producing an unescaped newline in the generated C++ string literal.
83770a6 to
dd6473f
Compare
|
Rebased on develop and addressed the open review feedback. This is a summary; the replies on each item have the details.
Local gates on the head:
|
|
Re our review (#14845 (comment)): Follow-up on N2-N5 (rebased on develop, head dd6473f):
The rebase also needed one C++ fix: develop added the unsigned integer WMMA rows, and the C++ HIP lowering hard-coded signed flags. Both lowerings now read the flags from the catalog row through one primitive, |
|
Automated overlap notice (rocKE merge monitor) This PR and #12167 both add the HIP lowering of
They differ on mismatched operands: this PR pairs constraints with operands "stopping at the shorter list" and truncates ( Owner: #12167 keeps it (cgmillette's priority, and it lands first). Resolution: this PR drops its inline-asm lowering and its escaper in both engines and keeps the rest of its HIP parity work on top of #12167's lowering. Suggested order: #12167 lands first, then this PR rebases onto develop, then #14881, which is stacked on this PR, rebases onto it. The merge plan already has #12167 in its first group with auto-merge on, and it has this PR wait for #12167 because both change Rebase: after #12167 lands:
Then run that file with If you think this PR should own it instead, reply here and on #12167, and the reviews will follow the new decision. Automated overlap check: one owner per shared piece of code, coordinated on the PRs. Nothing else changed. |
There was a problem hiding this comment.
🟡 Changes recommended
The integer and FP8 WMMA emitters access results and operands without arity checks, allowing malformed serialized IR to crash the host process.
5 open issues
What changed in this PR
Aligns rocKE’s Python and C++ HIP-source lowerers and extends the existing engine-parity checks to cover HIP output.
Changes:
- Centralizes Python-compatible float formatting and escapes inline assembly strings.
- Adds missing C++ MMA and inline assembly lowerings, aligns target refusals, and updates the binary ABI.
- Adds HIP-source identity tests, target-tagged emitter output, and float-format test wiring.
Paths are relative to dnn-providers/hip-kernel-provider/.
| File | Description |
|---|---|
rocke/TESTING.md |
Documents the HIP-source identity test. |
rocke/platform/tests/tools/test_run_all.py |
Checks float-format fixture discovery and propagation. |
rocke/platform/tests/test_engine_linkage.py |
Adds the installed float-format fixture expectation. |
rocke/platform/tests/run_all.py |
Registers the native float-format fixture. |
rocke/platform/tests/README.md |
Documents float-format parity execution. |
rocke/platform/tests/instances/parity/moe_sorting_emit.py |
Adds target-tagged IR output. |
rocke/platform/tests/instances/parity/moe_fused_mega_fp8_emit.py |
Adds the shared target-ir mode. |
rocke/platform/tests/instances/parity/layernorm2d_emit.py |
Adds the shared target-ir mode. |
rocke/platform/tests/instances/parity/ir_serialize_emit.py |
Supports target-tagged serialization. |
rocke/platform/tests/instances/parity/grouped_gemm_stress_emit.py |
Adds target-tagged grouped GEMM output. |
rocke/platform/tests/instances/parity/gemm_stress_emit.py |
Adds target-tagged GEMM output. |
rocke/platform/tests/instances/parity/fused_moe_emit.py |
Recognizes and emits target-tagged IR. |
rocke/platform/tests/instances/parity/fused_moe_e2e_emit.py |
Emits one target-tagged block per stage. |
rocke/platform/tests/instances/parity/batched_gemm_stress_emit.py |
Adds target-tagged batched GEMM output. |
rocke/platform/tests/instances/parity/add_rmsnorm2d_rdquant_emit.py |
Adds the shared target-ir mode. |
rocke/platform/tests/instances/parity/_emit_common.py |
Centralizes emitter modes and target tagging. |
rocke/platform/tests/instances/differential/test_hip_source_identity.py |
Compares corpus-wide HIP source and refusals. |
rocke/platform/tests/instances/differential/test_gfx950_scaled_mfma_emit.py |
Uses the public emitter-environment helper. |
rocke/platform/tests/instances/differential/run_diff.py |
Exposes environment and emission-record helpers. |
rocke/platform/tests/instances/differential/python_batch_worker.py |
Delegates mode validation to emitters. |
rocke/platform/tests/core/test_mma_operand_register_type.py |
Extends HIP operand checks to both engines. |
rocke/platform/tests/core/test_integer_wmma_signedness.py |
Checks both HIP engines’ signedness flags. |
rocke/platform/tests/core/test_hip_lowering_parity.py |
Adds atom, inline assembly, and refusal parity cases. |
rocke/platform/tests/core/test_float_format.py |
Compares native formatting with Python repr. |
rocke/platform/tests/core/test_float_format.cpp |
Adds formatter cases, locale checks, and sampling support. |
rocke/platform/tests/core/matrix_call_lowering.cpp |
Tests the shared signedness primitive. |
rocke/platform/tests/cmake/CoreTests.cmake |
Builds, registers, and installs the formatter test. |
rocke/platform/python/rocke/tests/engines.py |
Adds comparable HIP-source and refusal outcomes. |
rocke/platform/python/rocke/portable_ir/src/abi.py |
Updates the Python binary ABI to 8. |
rocke/platform/python/rocke/core/lower_hip.py |
Aligns literals, escaping, validation, and refusals. |
rocke/platform/cpp/support/float_format.cpp |
Implements Python-compatible float formatting. |
rocke/platform/cpp/include/rocke/matrix_calls_internal.h |
Declares shared signedness and integer-row helpers. |
rocke/platform/cpp/include/rocke/lower_hip.h |
Updates the HIP architecture representation. |
rocke/platform/cpp/include/rocke/lower_hip_internal.h |
Updates literal, escaping, and target-gate interfaces. |
rocke/platform/cpp/include/rocke/ir_serialize.h |
Documents shared float formatting. |
rocke/platform/cpp/include/rocke/float_format.h |
Declares the common float formatter. |
rocke/platform/cpp/include/rocke/abi.h |
Records the layout change and ABI version 8. |
rocke/platform/cpp/core/lower_llvm/matrix_call.cpp |
Reuses the shared signedness primitive. |
rocke/platform/cpp/core/lower_hip/lower_hip_mma.cpp |
Adds MMA and inline assembly emitters. |
rocke/platform/cpp/core/lower_hip/lower_hip_mem.cpp |
Updates the LDS-refusal comment only. |
rocke/platform/cpp/core/lower_hip/core.cpp |
Adds shared formatting, escaping, and target checks. |
rocke/platform/cpp/core/isa/matrix_calls.cpp |
Centralizes signedness derivation and integer rows. |
rocke/platform/cpp/core/ir/serialize.cpp |
Replaces the duplicated float formatter. |
rocke/platform/cpp/core/ir/print.cpp |
Uses the common formatter for float attributes. |
rocke/platform/CMakeLists.txt |
Propagates the installed formatter-test path. |
rocke/library/tests/parity/gfx950_attention_tiled_3d_emit.py |
Tags segment and reduction kernels with their target. |
rocke/library/tests/parity/gfx942_attention_tiled_3d_emit.py |
Adds target-tagged attention output. |
rocke/library/tests/parity/fmha_splitkv_decode_emit.py |
Adds target-tagged decode output. |
rocke/library/tests/parity/conv_direct_grouped_emit.py |
Emits each convolution kernel’s target. |
rocke/library/tests/parity/_emit_common.py |
Adds shared target-tagged library emission. |
ROCKE_ENGINE_test_categories_external.yaml |
Enrolls the formatter test in quick tiers. |
Review effort: Max
The C++ HIP lowering printed float constants with %g, which keeps only six
significant digits and drops the decimal point from whole numbers. That
emitted invalid C++ ("0f", "1024f") and changed values ("-1.4427f" for
-1.4426950408889634), so the C++ HIP source differed from the Python
engine's and did not compile.
The Python engine writes every float with repr(): in printed IR, in
serialized IR and in HIP literals. The C++ engine now writes all three with
one formatter, rocke_append_python_float_repr in support/float_format.cpp,
which replaces the two copies the IR printer and the serializer each kept.
It takes the shortest round-trip digits from std::to_chars and lays them out
the way CPython does. The copies it replaces probed printf precisions until
strtod read the value back; that picked 17 digits for some values that have
a 16-digit spelling (2**-24 printed as 5.9604644775390625e-08, repr is
5.960464477539063e-08) and depended on the C locale's decimal point.
Each engine's HIP lowering adds one thin adapter on top: repr() plus an "f"
suffix for a float literal, repr() alone for a double literal, and the
<math.h> NAN / INFINITY macros cast to the literal's type for the values repr()
spells in a way C++ does not accept. The float and double entry points are
one-line calls into that adapter.
rocke_float_format checks the formatter against a table of values whose repr
is known (zeros, integral values, both sides of the 1e-4 and 1e16 notation
switches, 2**-24 and its neighbours, subnormals, the finite extremes, nan
and the infinities), again under a comma-decimal locale when one is
installed. test_float_format.py compares it with repr() over every power of
two and of ten, the doubles either side of each, and a fixed random sample,
and lowers float and double constants (zeros, integral values, the notation
switches, 2**-24, the finite extremes, nan and the infinities) through both
engines' HIP lowerers and checks every literal. tests/run_all.py and the
installed rocke_pytest point it at the rocke_float_format binary
(ROCKE_FLOAT_FORMAT_TEST), as they do for the other native parity tests.
The ctest is in the quick and ffm-quick tiers of
ROCKE_ENGINE_test_categories_external.yaml, beside rocke_ir_types.
Co-Authored-By: Claude <noreply@anthropic.com>
A newer target replaced the monolithic s_waitcnt with split wait counters, and clang cannot select __builtin_amdgcn_s_waitcnt there. The two HIP lowerings still emitted it, each with a different immediate: Python used the gfx9 layout, C++ the gfx11 layout. Both now ask the target's ISA backend whether it emits the monolithic s_waitcnt (the fact the LLVM lowering already uses) and refuse with the same NotImplementedError message when it does not. The C++ HIP arch seam records this as a new waitcnt family, ROCKE_HIP_WAITCNT_NONE. Every other target keeps its current encoding. test_hip_lowering_parity.py lowers tile.s_waitcnt and tile.sync_lds_only through both engines: on the target with split wait counters both refuse with that exact message (C++ with ROCKE_ERR_NOTIMPL), and on gfx942, gfx950, gfx1151 and gfx1201 both emit the same source. Co-Authored-By: Claude <noreply@anthropic.com>
The C++ HIP lowering refused kernels the Python one lowers: every f16, bf16, fp8 and bf8 MFMA atom outside the f32/f64 controls, and the gfx11, gfx12 and newer WMMA atoms all failed with "no HIP lowering for op". Each is now ported from its Python handler and emits the same text. The atoms are grouped by how their builtin takes its operands, with one table of op_id-to-builtin names and one small emitter per group: - MFMA f16/bf16 atoms, including the 4x4x4 ones, pass A, B and C to their builtin as they are; - fp8/bf8 MFMA atoms copy the packed operands into long scalars first; - the gfx11 and gfx12 f16 WMMA atoms pass A, B and C as they are, and a newer target's K=32 ones use the builtin's 8-operand form; - a newer target's fp8/bf8 WMMA atoms and the iu8/iu4 ones take A and B as packed i32 vectors. The iu8/iu4 atoms lower under every signedness row of the catalog, and their builtin's signedness bools come from the row through rocke_matrix_operand_is_signed, which the LLVM lowering's flags now read too (Python's Signedness.flag is the twin). ROCKE_INTEGER_MMA_ROWS lists a table's rows for each signedness, as integer_mma_op_ids does in Python. The accumulator width of each atom, and the packed word count of A and B, come from the arch MMA catalog (c_frag_len, a_frag_len, b_frag_len) rather than a second table. rocke_h_require_wmma_arch now admits an atom by the target's MMA catalog, as Python's _require_wmma_arch does. The has_wmma and family fields of rocke_hip_arch_t, which it used to read, are removed. That changes the struct's size, so ROCKE_ABI_VERSION and the Python BINARY_ABI go to 9. Both HIP lowerings of tile.inline_asm now read its template and constraints attrs through one helper each (_inline_asm_string_attr, h_inline_asm_string_attr), so a serialized kernel that lacks one is refused with the same KeyError, and a non-string one with the same TypeError, by both engines. Python used to raise a bare KeyError for a missing template and a TypeError from re.sub for a non-string one. test_hip_lowering_parity.py lowers every native MFMA and WMMA atom in each target's catalog and the 4x4x4 atoms through both engines and requires the same source or the same refusal, and checks the type and message of each template and constraints refusal. It builds each A and B operand from zero words, so the formats with no zero constant (the 6- and 4-bit floats, the unsigned integers) are lowered too. The HIP cases of test_integer_wmma_signedness.py and test_mma_operand_register_type.py, which ran the Python lowering alone for the atoms the C++ one did not have, now run both engines, and the check that no engine writes a signedness flag as a literal also reads the C++ HIP format strings. Co-Authored-By: Claude <noreply@anthropic.com>
The LLVM-IR route is held byte-identical between the engines; the HIP source route was not, and drifted. This adds a test that lowers every parity kernel through both HIP lowerers (Python lower_kernel_to_hip and rocke_engine.lower_serialized_ir_hip) and requires the same text, or a refusal from both. It is parametrized by parity family, covers the platform and library parity trees, and skips without rocke_engine. The serialized IR does not record the target a kernel is built for, so the parity emitters gain a "target-ir" mode: a "target <gfx>" line, then the serialization, once per kernel. run_emit accepts it, and every emitter that parses its own argv does too, except conv_abi_emit.py, which builds no kernel. The batch worker no longer keeps its own list of modes: it passes the mode to the emitter, which accepts it or answers "unknown mode". The test fails if an emitter does not accept the mode or builds no kernel. Co-Authored-By: Claude <noreply@anthropic.com>
…r in HIP The Python HIP lowering raised RuntimeError when an op reads or writes an LDS allocation whose smem_alloc has not been lowered yet; the C++ HIP lowering refuses the same kernel with the same message and status 1, which stands for ValueError (there is no RuntimeError status). The Python lowering now raises ValueError at each of those eleven sites, so both engines refuse such a kernel with the same error type. Co-Authored-By: Claude <noreply@anthropic.com>
The HIP lowering tests compared the two engines through helpers of their own. They now use the ones in rocke.tests.engines: - assert_both_engines_reject and assert_both_engines_reject_hip compare the Python message by its first argument, so a KeyError refusal compares as written rather than quoted. - hip_outcomes_of_both_engines returns each engine's HIP source for a serialized kernel, or its refusal as a HipRefusal: the error type (the Python exception's, or the one the C++ status code stands for) and the message. test_hip_lowering_parity.py requires equal outcomes for every MMA atom and checks each refusal it pins (s_waitcnt, malformed inline asm) against a HipRefusal, so the engines must agree on the error type as well as the message. - test_hip_source_identity.py requires equal outcomes too: the same source, or the same refusal, where it used to accept any two refusals. test_hip_source_identity.py also takes the emitter environment from run_diff.python_emitter_env (public now, so the debug-location capture stays off as it does for the gate), detects an emitter refusing the target-ir mode with run_diff.mode_unsupported, which reads only the last line of its stderr, and reads each worker record with the new run_diff.emission_from_record, which the gate's batch reader now uses as well. The MMA atom cases now cover each target with its own catalog (arches_with_own_facts): gfx908 is added, and the gfx11 and gfx12 atoms run on gfx11-generic and gfx12-generic, whose catalogs gfx1151 and gfx1201 take, instead of on those two. Co-Authored-By: Claude <noreply@anthropic.com>
A kernel read from serialized IR is not verified, so a tile.mma op can arrive without its accumulator or its result. The C++ HIP lowering of the fp8 and integer WMMA atoms, the f32 and f64 MFMA controls and the TF32 atoms indexed both regardless and could read past the op's arrays; the Python lowering failed with whatever unpacking or indexing error came first. A scaled WMMA op without its two scales was refused by C++ with "<op_id> expects 5 operands and 1 result" and by Python with an unpacking error. Every path of both lowerings that lowers an atom from A, B and C now checks first that the op has those three operands and one result, and refuses it otherwise with the same ValueError: "tile.mma <op_id> expects 3 operands and 1 result" (h_require_abc, Python's _require_abc_atom). An op_id neither engine lowers is still refused as unknown, before any count is checked. The Python scaled WMMA lowering checks for five operands with the C++ message. test_hip_lowering_parity.py drops the accumulator, and separately the result, from a kernel for every native atom in each target's catalog and requires both engines to refuse it alike, with that message wherever the well-formed kernel lowers. It also gives every scaled atom an op with A, B and C only, and requires the same refusal from both, the five-operand one for the scaled WMMA atoms. Co-Authored-By: Claude <noreply@anthropic.com>
The test compared the kernels each config printed and ignored how the config's emitter run ended. A config that failed after printing part of its output was compared as if it had built, and one that failed with no output dropped out of the comparison, so a broken emitter could remove kernels from the gate unnoticed. Each config now passes only if it printed its kernels and exited 0, or reached the end of the config list. A config that fails with no output passes only as the builder refusing it: it must have raised one of the exception types that stand for a refusal (REFUSAL_ERROR_TYPES in rocke.tests.engines: ValueError, TypeError, KeyError and NotImplementedError, the types the C++ engine's status codes stand for), and the ir mode, which builds the same kernel and only serializes it, must refuse it with the same error. A config that exits non-zero after printing, exits 0 without printing, fails with another exception type, fails only in the target-ir mode, or fails in the ir mode with another error fails the family. run_diff.py makes public the two facts the test reads from a config's run: is_end_emission (was the index past the last config) and last_error_line (the last stderr line, which mode_unsupported also reads). test_hip_source_identity.py checks the classification against an emitter with one config for each way a run can end; that case needs no engine. Co-Authored-By: Claude <noreply@anthropic.com>
The test lowers each kernel through the C++ engine's rocke_engine binding and skipped without it. CI runs CTest against the installed artifact, whose wheel has no binding, and the test was not installed, so it never ran there. ir_lower_cli.cpp, the tool that lowers serialized IR with the C++ engine, gains a --hip option: it lowers with rocke_lower_kernel_to_hip and the defaults the binding uses, writes the source unchanged (all three streams binary on Windows), and on a refusal writes the engine's status and message in the form the binding's errors take. CMake now builds it as rocke_ir_lower_cli and installs it beside the test binaries, outside the list that registers each installed binary as a test. In rocke.tests.engines, hip_outcomes_of_both_engines runs the C++ HIP lowering through the binding when it is built and otherwise through the program ROCKE_IR_LOWER_CLI names (ir_lower_cli_hip_outcome); skip_without_native_hip_lowering skips a test that has neither, and a name that is not a file fails. test_hip_lowering_parity.py requires the program and the binding to report the same source and the same refusal where both are present. The test is installed with the differential files the golden checks already install, and the rocke_hip_source_identity CTest runs it with ROCKE_IR_LOWER_CLI set, a 120 s limit per family and 900 s in all; one family's emitter run is bounded at 100 s, below the per-family limit. It is listed in the quick and ffm-quick tiers. The rocke_pytest CTest does not collect it. Co-Authored-By: Claude <noreply@anthropic.com>
dd6473f to
92241b6
Compare
|
Rebased onto current develop and addressed the open review items. Summary of what changed since the last push: Rebase. #12167 landed with its own HIP lowering of MMA arity (Copilot, the fp8 and integer WMMA emitters). Every path of both HIP lowerings that lowers an MMA atom from A, B and C now checks first that the op has three operands and one result, and refuses it with the same ValueError otherwise. That covers the fp8 and integer WMMA emitters and also the f32/f64 MFMA controls and TF32 atoms, which indexed the same arrays unchecked. The Python scaled WMMA lowering now counts its five operands with the C++ message.
Identity gate in CI (Copilot). CI runs installed CTest without the Emission status (Copilot). The gate now fails a config that exits non-zero after printing, exits 0 without printing, or fails with an exception other than a refusal type (ValueError, TypeError, KeyError, NotImplementedError). A config that fails with no output counts as the builder refusing it only if the Our follow-up review (11-03). The CI. The red Checked locally: build and ctest, byte identity at llvm20/22/23/24 GREEN, the identity gate with the binding and from an installed tree through |
The C++ parser of serialized IR kept the names of the values it had defined in an array and found each operand by comparing it with every name defined before it, so parsing a kernel took time quadratic in its size. For a large attention kernel, almost all of the time the C++ HIP lowering took, through rocke_engine.lower_serialized_ir_hip or the serialized-IR lowering entry points, went to that search. The table is now an open-addressing hash table on the name (64-bit FNV-1a, linear probing, kept at most three-quarters full), in the builder's arena as before. It keeps the same contract: a later definition of a name replaces the earlier one, and the names a loop defines are removed when the loop ends, so a sibling loop can define them again. test_serialized_ir_name_scopes.py builds a kernel of 200 sibling loops that each define the same induction variable and iter-arg names, with over a thousand values in all, and requires both engines to lower it to the same LLVM IR and the same HIP source. Co-Authored-By: Claude <noreply@anthropic.com>
…test The HIP source identity test ran the C++ lowering of every kernel on its own: one rocke_ir_lower_cli process per kernel where the binding is absent, and it ran each family's emitter only when that family's test started. - rocke_ir_lower_cli's --hip option becomes --hip-batch, which lowers a list of kernels in one run: frames of "<arch> <byte count>" and the serialized IR in, frames of "<status> <byte count>" and the source or the engine's refusal out, with the binding's fallback messages. An empty or malformed frame, or a reply that cannot be written, ends the run with status 2, as the binding refuses empty IR outright. All three streams are binary on Windows. - In rocke.tests.engines, native_hip_outcomes lowers a list of kernels through the binding or one program run (ir_lower_cli_hip_outcomes, which checks every reply frame), python_hip_outcome is the Python half, and hip_outcomes_of_both_engines is built from the two. - The test starts each selected family's emitter run ahead of the test that reads it, in the session's order and a few at a time, so the emitters overlap the Python lowering of the families before them. It starts none where the C++ lowering cannot run, and cancels the runs not yet started when the module ends. - The tie test between the program and the binding sends one batch that holds source and refusals. The rocke_hip_source_identity CTest limit drops from 900 s to 300 s. Co-Authored-By: Claude <noreply@anthropic.com>
The Windows hipkernelprovider CI shards run close to their 30-minute step limit, and rocke_hip_source_identity pushed one of them over it. The CTest is no longer registered on Windows, as rocke_library_gdn_pytest is not; it stays registered, in the quick and ffm-quick categories, on every other platform, so the Linux hipkernelprovider jobs still run it on every pull request. The cost is that CI no longer compares the MSVC-built engine's HIP output with Python's. The CMake comment, the category list and TESTING.md say so, and say to register it again once the Windows shards have room. Co-Authored-By: Claude <noreply@anthropic.com>
|
The new
Every lane runs
The test stays in the quick and ffm-quick categories, so every Linux hipkernelprovider job runs it on every PR. Checked locally:
|
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14845 — make both engines emit the same HIP source (cgmillette, AICK-2903) — 🟠 Follow-up: rebase owed, CI coverage gap
Utility: 🟢 Positive — repr-exact floats, ported MMA atoms, identity gate in Linux CI. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted Follow-up review: first pass by a local qwen3.6 model, verified by the orchestrator (Claude Opus 5.5). Checks: range-diff, merge with develop (conflict resolved and built), native build + ctest, byte identity and goldens at four LLVM flavors, differential, the PR's tests in three modes, every |
CongMa13
left a comment
There was a problem hiding this comment.
One new issue: the HIP source identity gate can pass a family without comparing any successfully emitted source; see the inline comment.
I also agree with two existing open concerns in cgmillette's comment 6655831567: the focused synthetic both-engine HIP cases are skipped in installed CI, and test-duration claims remain in public PR materials.
| if difference is not None: | ||
| disagreements.append(f"{where}: {difference}") | ||
| compared = len(kernels) | ||
| assert compared, f"{name}: the emitter built no kernel in any config" |
There was a problem hiding this comment.
If both lowerers regress to refusing every kernel in this family with the same error, disagreements stays empty and this check passes: compared counts inputs, not successful source comparisons. Please require at least one successful HIP source match for each expected-lowerable family, with explicit exceptions for intentionally unsupported families, so the parity gate does not pass with no output to compare.


Motivation
Both rocKE engines lower a kernel to HIP source as well as to LLVM IR. The LLVM-IR route is held byte-identical by the byte-identity gate; the HIP route had no gate and had drifted. Lowering every parity kernel through both HIP lowerers on develop:
The differences, by class (a kernel can be in more than one):
0.0f/0f-1.4426950408889634f/-1.4427f0.0/0s_waitcnton a target with split wait counters49279/64519tile.inline_asm(now lowered by develop, #12167)Technical Details
%g. It now prints Python'srepr(float), the shortest decimal that reads back as the same double. One formatter,rocke_append_python_float_repr(cpp/include/rocke/float_format.h,cpp/support/float_format.cpp), now serves the C++ IR printer, the IR serializer and the HIP lowerer, and replaces the two copies the printer and serializer kept. It takes the shortest round-trip digits fromstd::to_chars, so it matchesreprexactly (the old copies printed 2**-24 with 17 digits) and does not depend on the C locale. Each engine's HIP lowering adds one thin adapter on top: anfsuffix for float literals and theNAN/INFINITYmacros for the valuesreprspells in a way C++ does not accept. Python's spelling is the one kept: it is valid C++ and exact.s_waitcnton a target with split wait counters (both change). A newer target has no monolithics_waitcnt. Both HIP lowerings now ask the target's ISA backend (the fact the LLVM route already uses) and refuse with the same message. The C++ HIP arch seam records this asROCKE_HIP_WAITCNT_NONE. Every other target keeps its encoding. Nine kernels on that target (1 platform, 8 library) that Python used to lower to uncompilable source are now refused by both.rocke_matrix_operand_is_signed, which the LLVM matrix-call lowering now uses as well (the twin of Python'sSignedness.flag). The WMMA gate now admits an atom by the target's MMA catalog, as Python's does; the unusedhas_wmmaandfamilyfields ofrocke_hip_arch_tare removed, which changes its size, soROCKE_ABI_VERSIONandBINARY_ABIgo to 9 (develop + 1).tile.mma <op_id> expects 3 operands and 1 result); the C++ fp8 and integer WMMA emitters, the f32/f64 MFMA controls and the TF32 atoms used to index the arrays unchecked. The Python scaled WMMA lowering counts its five operands with the C++ message. An op_id neither engine lowers is still refused as unknown.tile.inline_asm(feat(rocke): add NVFP4 GEMM with tensor scales and focused example #12167) is kept as it is. Both engines now read itstemplateandconstraintsattrs through one helper each (_inline_asm_string_attr,h_inline_asm_string_attr): a missing one is refused with KeyErrortile.inline_asm: missing '<key>' attrand a non-string one with TypeErrorHIP inline asm <key> must be a string, by both engines (Python used to raise a bare KeyError, or a TypeError fromre.sub).platform/tests/instances/differential/test_hip_source_identity.pyruns every platform and library parity emitter (through the differential gate's batch worker) and lowers each kernel through both HIP lowerers: the text must match, or both must refuse with the same error type and message. The serialized IR does not carry the target, so the emitters gain atarget-irmode (target <gfx>line, then the serialization). The test fails if an emitter does not accept the mode or builds no kernel (conv_abiis listed as building none), and fails any config that exits non-zero after printing, exits 0 without printing, or fails with an exception other than a refusal type (ValueError, TypeError, KeyError, NotImplementedError); a config that fails with no output counts as the builder refusing it only if theirmode refuses it with the same error.rocke_enginebinding.tests/core/ir_lower_cli.cppgains--hip-batch, which lowers a list of kernels to HIP in one run, and the test starts each family's emitter run ahead of the test that reads it; it is built and installed asrocke_ir_lower_cli(not a test itself), androcke.tests.engineslowers through it when the binding is absent (ROCKE_IR_LOWER_CLI). Therocke_hip_source_identityCTest runs the gate there (120 s per family, 300 s in all) and is in the quick and ffm-quick tiers. It is not registered on Windows, whose CI shards run close to their time limit, so CI does not compare the MSVC-built engine's HIP output with Python's (it matched on every parity kernel on the Windows lanes when the gate was added).rocke_pytestdoes not collect it.test_serialized_ir_name_scopes.pyholds both engines to the same output for a kernel whose sibling loops reuse their names.tests/core/test_hip_lowering_parity.pylowers every native MFMA and WMMA atom of each target with its own catalog (operands built from zero words, so the fp6/fp4 and unsigned formats are included), and the 4x4x4 atoms through both engines (same source or same refusal), refuses every atom without its accumulator or result and every scaled atom without its scales alike in both, checks the type and message of thes_waitcntrefusal on the target with split wait counters and of the inline-asmtemplate/constraintsrefusals, and holdsrocke_ir_lower_clito the binding. The HIP cases oftest_integer_wmma_signedness.pyandtest_mma_operand_register_type.pynow run both engines. The both-engine HIP helpers live inrocke.tests.engines(hip_outcomes_of_both_engines,HipRefusal).tests/core/test_float_format.pyand therocke_float_formatctest check the formatter againstrepr(a table of hard cases, about 150k sampled values, and every HIP literal spelling).LLVM-IR output is unchanged.
Test Plan
hipcc --gencoof every kernel whose C++ HIP text changed (up to two per family, arch and kind): gfx942, gfx950 and gfx90a with ROCm 7.1; gfx1151, gfx1201 and gfx11-generic in the CI image.ROCKE_BACKEND=bothstrict, compared with develop by failure name; library tests that run parity emitters.Test Result
Head after the rebase on develop (#12167, #14835 and the rest):
ROCKE_BACKEND=bothstrict: 99 families pass. From an installed tree throughrocke_ir_lower_cli(ctest -R rocke_hip_source_identity): 100 passed in 47 s, slowest family about 6 s.ROCKE_BACKEND=bothstrict: pass.ROCKE_BACKEND=bothstrict: 10853 passed, 1 failed (test_the_cpp_fallback_does_not_warn_twice, which failed on develop in this mode when last compared).Earlier revisions (before #12167 landed; they included this PR's own inline-asm lowering):
rocke_float_formatincluded).autoloop carries,atomicAddoverloads, a bf16/fp16 assignment, a 2-byte float atomic); one large fp8 MoE kernel did not finish compiling in 15 minutes and is likewise develop's Python text. On develop the C++ text of the literal class does not compile (invalid digit 'f' in octal constant).ROCKE_BACKEND=bothstrict 6413 passed, 3 failed. Every failure also fails on develop:test_binding_compiler_selection,test_installed_pytest_gets_distinct_artifact_environment_variables(develop's installed test environment has aROCKE_REQUIRE_COMPILER_TOOLSentry that test does not expect) and, with both engines,test_the_cpp_fallback_does_not_warn_twice.test_barrier_fence_parity_emit,test_direct_wgrad_specs, strided-KV emitter golden: pass.ISSUE ID : AICK-2903
🤖 Generated with Claude Code