Repository navigation
fix(rocke): never give two values the same name - #14846
cgmillette wants to merge 5 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 #14846 — never give two values the same name (cgmillette, AICK-2799) — 🟡 First-round: C++ test not in CI
Utility: 🟢 Positive — closes a latent duplicate-name bug; all re-blessed IR is names-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, build + ctest, byte identity, goldens, collision repro on both trees, names-only audit of every re-blessed entry, code-object compare, CI at head. Flags whether a closer look is needed; not an approval or rejection. |
shumway
left a comment
There was a problem hiding this comment.
This touches a lot of files, and it looks like we have a good window to merge this.
It seems like this convention could be abstracted, so that we don't have to make changes across all files, but I'm OK with this large-scale change. In the future, let's consider design improvements to go hand in hand with large scale changes, with this idea that future PRs won't have to be this large and conflict-prone.
|
Thanks, agreed. Filed as a follow-up (AICK-2909): one naming policy per engine that owns the hint, separator and counter, and goldens that hash a canonical form (values renumbered in order of first use, as the differential harness's canonical section already does), so a future naming change is a one-place edit and re-records no golden. This PR stays as is and lands at a quiet moment to limit the rebases it causes. |
CongMa13
left a comment
There was a problem hiding this comment.
One new issue is called out inline.
I agree with the following existing feedback:
- The new
rocke_value_namesCTest is missing from tiered CI — cgmillette, comment #6566925563.
|
Review follow-up:
The branch currently conflicts with develop (one kernel file and several goldens); I will rebase and re-bless those goldens with the tools listed in the description in a separate step. |
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14846 — never give two values the same name (cgmillette, AICK-2799) — 🟠 Follow-up: N1 fixed; rebase needed
Utility: 🟢 Positive — ends a latent name clash; its C++ test joins tiered 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: merge with develop and each conflict's cause; scratch merge with the MLA port (MLA tests, representative, library and differential goldens, develop's changed tests); tier check at head, on the old YAML and in the installed layout; CI lane logs; 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 is noted inline.
I agree with the following existing comments:
- Tier-check coverage omits loop-generated tests — cgmillette, comment #6604923588. The check runs through the quick
rocke_pytestentry, but its literal-name scan skips two loop-generated C++ test targets. - Resolve develop merge conflicts — cgmillette, comment #6595870040. GitHub still reports the current head as conflicting with the base branch.
|
Rebased on develop and addressed the open feedback:
Byte identity is GREEN at all four flavors; the platform and library suites show no failure that develop does not also have. |
ed1e980 to
c8d8552
Compare
|
Targeted to merge in wave 6. Do not rebase / merge till then. |
There was a problem hiding this comment.
🟡 Changes recommended
The new naming rules break replay of legacy and roller-generated artifacts, while unresolved collisions between declared names and lowering-generated names can still produce invalid LLVM or HIP code.
4 open issues
What changed in this PR
This PR prevents ambiguous SSA value names in rocKE’s Python and C++ builders, updating dependent kernels, tests, and IR baselines to use the new naming convention.
Changes:
- Separates builder hints from counters and validates author-chosen identifiers.
- Renames affected kernel parameters and loop values, keeping runtime argument mappings aligned.
- Adds naming regression tests and CTest-category checks, and refreshes name-sensitive assertions and goldens.
| File | Description |
|---|---|
| dnn-providers/hip-kernel-provider/rocke/platform/tests/test_rocke.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/test_gpu_references.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/test_cxx_test_categories.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/golden/rocke_representative_ir_sha256/nontemporal.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/golden/rocke_representative_ir_sha256/grouped_gemm.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/golden/rocke_representative_ir_sha256/fused_moe.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/golden/rocke_representative_ir_sha256/conv_winograd_gemm_wmma.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/golden/rocke_representative_ir_sha256/conv_winograd_fused_1d_wmma.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/golden/rocke_representative_ir_sha256/conv_wgrad_two_stage.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/golden/rocke_representative_ir_sha256/conv_wgrad_reduce.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/golden/rocke_representative_ir_sha256/conv_fwd_split_k_reduce.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/golden/rocke_representative_ir_sha256/conv_direct_fwd_wmma.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/golden/rocke_representative_ir_sha256/conv_direct_fwd_smallk_wmma.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/golden/rocke_representative_ir_sha256/conv_direct_fwd_smallc_wmma.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/golden/rocke_representative_ir_sha256/conv_dgrad_smallc_wmma.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/golden/rocke_representative_ir_sha256/conv_dgrad_patch_wmma.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/golden/rocke_representative_ir_sha256/attention_d256.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/golden/rocke_representative_ir_sha256/attention_d128_swa.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/core/test_value_names.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/core/test_unsigned_int_ops.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/core/test_scf_if_else_results.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/core/test_mx_scale_decode.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/core/test_mx_scale_decode.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/core/test_mma_operand_register_type.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/core/test_lowbit_loop_types.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/core/test_lds_alias_scopes.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/core/test_global_ptr_offset.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/core/test_gfx1250_tdm.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/core/test_gfx1250_prefetch_users.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/core/test_gfx1250_cluster.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/core/gfx1250_scaled_wmma_llvm23.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/core/future_intrinsic_lowering.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/conftest.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/tests/cmake/CoreTests.cmake | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/tests/helpers/tiling/test_demo_ir_snapshot.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/tests/helpers/tiling/tensor_transforms/golden/emit_xor.txt | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/tests/helpers/tiling/tensor_transforms/golden/emit_two_inputs.txt | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/tests/helpers/tiling/tensor_transforms/golden/emit_rebase.txt | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/instances/common/moe_fused_mega_fp8.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/fusion_lowering.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/fuse.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/examples/gfx950/fused_mega_moe/levels/level_09_iglp.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/examples/gfx950/fused_mega_moe/levels/level_08_dtla.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/examples/gfx950/fused_mega_moe/levels/level_07_k128.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/examples/gfx950/fused_mega_moe/levels/level_05_gateup_interleave.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/examples/gfx950/fused_mega_moe/levels/level_04_mtile_fix.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/examples/gfx950/fused_mega_moe/levels/level_03_down_pipeline.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/examples/gfx950/fused_mega_moe/levels/level_02_fusequant.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/examples/gfx950/fused_mega_moe/levels/level_01_tilem16.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/examples/gfx950/fused_mega_moe/levels/level_00_baseline.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/core/ir.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/ir_lowering/ir_model.md | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/gfx950/attention_tiled_3d_segment_prologue.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/gfx950/attention_tiled_2d_public_entry_glue.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/gfx950/attention_tiled_2d_ctx_prologue.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/gfx942/attention_tiled_3d_gfx942_attn_tiled_3d_segment_setup.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/gfx942/attention_tiled_2d_ctx_prologue.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/common/moe_fused_mega_fp8_stage1_gateup.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/common/moe_fused_mega_fp8_glue.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/common/conv_direct_wgrad_tiled.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/common/conv_direct_grouped_build_generic.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/common/conv_direct_grouped_build_depthwise_tiled.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/common/conv_direct_grouped_build_depthwise_spatial.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/common/conv_direct_grouped_build_depthwise_packed.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/common/conv_direct_grouped_build_depthwise_dgrad_win.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/common/conv_direct_grouped_build_depthwise_dgrad_stream.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/instances/common/conv_direct_dw_wgrad_mfma.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/include/rocke/ir_internal.h | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/include/rocke/instance_gfx950_attention_tiled_3d_internal.h | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/include/rocke/instance_gfx942_attention_tiled_3d_internal.h | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/core/ir/ir_flow.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/platform/cpp/core/ir/core_builder.cpp | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_attention_unified_lse.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_attention_unified_2d_lse_ir.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_attention_tiled_3d_running_max.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/test_attention_builds.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/tests/golden/kda_gfx942_ir_sha256.json | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/mla/_mla_body.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/gfx950/attention_tiled_3d.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/gfx950/attention_tiled_2d.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/gfx950/attention_dense.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/gfx942/attention_tiled_3d.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/gfx942/attention_tiled_2d.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/gfx942/attention_dense.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/gfx1250/attention_tiled_3d.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/gfx1250/attention_tiled_2d.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/common/conv_direct_wgrad_tiled.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/common/conv_direct_grouped.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/common/conv_direct_dw_wgrad_mfma.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/common/attention_unified.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/common/attention_dense_bwd_dq.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/kernels/common/attention_bwd_dq.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/builders/gfx1250/attention/tiled_2d_verify.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/builders/gfx1151/attention/fmha_singlewave.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/rocke/library/builders/gfx1151/attention/fmha_regblocked.py | Updated as part of this pull request. |
| dnn-providers/hip-kernel-provider/ROCKE_ENGINE_test_categories_external.yaml | Updated as part of this pull request. |
Review effort: Max
The builder named each value "%<hint><n>": the op's name hint followed by the builder's counter. When a hint ended in digits the two ran together. The 16-wide zero vector made as value 5 (hint "cz16") and the 1-wide zero vector made as value 65 (hint "cz1") were both "%cz165", and the lowered module defined that name twice. Both engines now put "_" between the hint and the counter: "%cz16_5" and "%cz1_65". The counter is always the digits after the last "_", so two builder names are equal only when their hints and their counters are equal. The separator is "_" because the HIP lowering prints value names as C identifiers, and because the LLVM lowering names its own temporaries "<hint>.<n>", so the two kinds of name cannot meet. Names the author picks (kernel parameters, loop induction variables and loop-carried values) are refused when they end in "_" and digits, the form builder names now take, or when they are not identifiers. A name hint that is empty or starts with a digit is refused too. Both engines give the same messages. The names that broke the rule are renamed, in both engines where both build them: the attention parameter qq_bias_stride_0 (now qq_bias_stride0), the fused epilogue's residual parameters (now residual<i> and residual_mul<i>), and loop names in the direct convolution kernels, the fused MoE fp8 kernel and its example levels, dense attention on gfx942 and gfx950, MLA prefill, the attention backward dQ kernels and two gfx1151 attention builders. The gfx950 unified attention packs of the hipDNN kernel ingestor compare their kernel argument names with the builder's, so they use the new qq_bias_stride0 too. The IR of almost every kernel changes, in its value names only. This re-blesses, each with its own tool or by the procedure its test checks: the representative IR golden (every family shard, at every flavor), the library IR goldens, the tiling demo snapshots and transform emit goldens, the scaled-MMA and NVFP4 hash tables in tests/core, the tests that pin value names, and every entry the differential golden already has, at every flavor and in the canonical section (where only the fused MoE end-to-end entries change, since they carry the renamed loop names). tests/core/test_value_names.py and .cpp check the separator with the cz16 / cz1 pair and every refusal, in both engines. Co-Authored-By: Claude <noreply@anthropic.com>
Add rocke_value_names to the quick and ffm-quick lists in the provider CTest categories, so tiered CI selects it like the other installed C++ unit tests. Add a check that every installed C++ test registered in tests/cmake gets the same provider tier labels, so a new test cannot silently drop out of tiered CI again. Tests registered before this check that still have no tier are listed by name; the check also fails if one of them is tiered or removed without leaving the list. The code that labels CTest names through the shared category parser moves into a conftest fixture used by both this check and the reference-label check. Co-Authored-By: Claude <noreply@anthropic.com>
The HIP lowering prints the names an author picks (kernel parameters,
loop induction variables and loop-carried values) unchanged, as C++
identifiers, so a parameter named "for" lowered to "int for", which does
not compile. Both engines now refuse such a name when it is a C++20
keyword or an alternative operator token ("and", "not", ...), with the
same message. Builder names always end in "_" and digits, so they are
never keywords.
tests/core/test_value_names.py and .cpp check the refusal for a
parameter, an induction variable and a loop-carried value in both
engines, and that a name that only starts with a keyword is accepted.
The Python test also checks that the two engines' keyword tables hold
the same words.
Co-Authored-By: Claude <noreply@anthropic.com>
The tier check read test names from tests/cmake by pattern, so it missed the two portable IR tests, whose names a CMake loop builds. It now configures the platform and reads the C++ tests from the installed CTest file the build writes, so it sees every test the build installs. The two portable IR tests, rocke_portable_ir_dom_decoders and rocke_portable_ir_recipe_vm_replay, join the quick and ffm-quick lists in the provider CTest categories like the other installed C++ tests. Co-Authored-By: Claude <noreply@anthropic.com>
When a loop-carry fan's Python names are not "<prefix><lane>" (a 2-D "acc_m<i>_n0" fan, say), the portable-IR roller names the fan's iter-args itself, as "__fa<fan>_<lane>". Those names reach the builder as loop-carried value names, and a name ending in "_" and digits is refused there, so the C replay of such a recipe failed. The roller now names them "__fa<fan>_l<lane>". A new test rolls such a fan, replays it through the standalone C replay at sampled and held-out lane counts, and checks the result matches Python's LLVM IR once value names are renumbered. The renumbering helper the parity driver already used is now public so the test can share it. Co-Authored-By: Claude <noreply@anthropic.com>
c8d8552 to
581e472
Compare
|
Rebased on develop and addressed the open feedback:
Byte identity is GREEN at all four flavors; ctest passes; the platform and library suites show no failure that develop does not also have. |
CongMa13
left a comment
There was a problem hiding this comment.
No new issues found. I agree with the following existing comments:
- Legacy portable-IR recipes can be rejected after an engine upgrade — Copilot, thread
PRRT_kwDOOej8qc6zYsQ_, comment #4461936586. The portable-IR documentation describes persisted bundles as readable by older reader levels, but this change makes the current reader reject some names those recipes can contain. - A shared-memory declaration can collide with an accepted parameter — Copilot, thread
PRRT_kwDOOej8qc6zYsSk, comment #4461936742. A parameter namedsmem_1_storagecan collide with the storage declaration generated from%smem_1by both HIP lowerers. - Two caller-selected values can still receive the same SSA name — Copilot, thread
PRRT_kwDOOej8qc6zYsS1, comment #4461936765. A parameter and a default loop induction variable can both be namedk0. - Resolve the develop merge conflict — cgmillette, comment #6595870040. GitHub currently reports
mergeable_state=dirty.
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: high PR #14846 — never give two values the same name (cgmillette, AICK-2799) — 🟠 Follow-up: 29 conflicts; rebase needed
Utility: 🟢 Positive — closes a latent SSA name clash in both engines; rebase pending. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted Follow-up review: first pass by qwen3.6 (local), verified by the orchestrator (Claude Opus 5.5). Checks: merge with develop and each conflict's cause; old names left outside the conflicts; goldens develop changed since the base; the delta's tests at head (value names, roller with its fix reverted, tier check); CI logs; code quality. Flags whether a closer look is needed; not an approval or rejection. |

The bug
Both engines named every value the builder makes
%<hint><n>: the op's name hint, then thebuilder's counter. When the hint ends in digits, the hint and the counter run together:
cz16%cz165cz1%cz165The kernel then defines
%cz165twice. The C++ engine refuses to read the IR back("SSA id '%cz165' redefined") and the LLVM module is invalid. Even when nothing clashes the
names mislead:
%c4holds 64 and%cz425cannot be split by eye.The fix
Both engines now always put
_between the hint and the counter:%cz16_5,%cz1_65,%c_4. The counter is always the digits after the last_, and it never repeats inside onebuilder, so two builder names are equal only when their hints and their counters are both
equal. That closes the bug by construction; nothing has to remember which names were issued.
The rule, in both engines with the same error texts:
%<hint>_<n>. A hint that is empty or starts with a digit is refused.refused when it is not an identifier or ends in
_and digits, the form builder names take,so an author name can never spell a builder name.
(
for,int,and, ...): the HIP lowering prints these names unchanged, as C++identifiers, so a parameter named
forlowered toint for. A builder name always ends in_and digits, so it is never a keyword. Other names the HIP output already uses (its typenames such as
uint32_t, builtins such asthreadIdx, names C++ reserves such as__x)are not refused.
The names that broke the second rule are renamed, in both engines where both build them:
qq_bias_stride_0qq_bias_stride0(the host keyword is unchanged; the gfx950 unified attention packs of the hipDNN kernel ingestor match kernel argument names against the builder's, so their signatures use the new name too)residual_<i>,residual_mul_<i>residual<i>,residual_mul<i>kg_0,g_out_0_1,dg_d0_1kg_m0,g_out_m0_n1,dg_d_m0_n1rg_y0_3,dwt_acc0_3,sp_acc_2,dww_y0_3,dwm_y0_3,dfw_y0_3,dgs_tail_acc_3..._i3,sp_acc_kh2acc1_3,a1_3,r1_0acc1_i3,a1_i3,r1_m0ro0_2,rl_2ro0_rs2,rl_rs2wo<dt>_<tag>wo<dt>_t<tag>kernels/mla/_mla_body.py)acc0_3,rs000_3acc0_t3,rs000_t3dq_ki_0dq_ki0m0_1,acc0_2m0_r1,acc0_d2dwp_acc_<f>_<k>,dwp_tail_<i>,dwtp_acc<ph>_<i>,dwtp_x<ph>_r<t>_<c>,dwtp_w<ph>_<i>dwp_acc_<f>_k<k>,dwp_tail_i<i>,dwtp_acc<ph>_i<i>,dwtp_x<ph>_r<t>_c<c>,dwtp_w<ph>_i<i>dgs_tail_acc_<i>,dgp_acc_<f>_<k>,dgp_tail_<i>dgs_tail_acc_i<i>,dgp_acc_<f>_k<k>,dgp_tail_i<i>The portable-IR roller names a loop-carry fan's iter-args itself when the Python names are
not
<prefix><lane>(a 2-Dacc_m<i>_n0fan, say). It named them__fa<fan>_<lane>, which theC replay now refuses as a loop-carried name; it now names them
__fa<fan>_l<lane>.Every kernel the byte-identity check, the representative golden, the library convolution tests
and the library attention tests build was first built with the rule logged rather than enforced:
no other name breaks it. On the current develop the rule is enforced in both engines, and every
kernel byte identity (all four flavors), the platform suite and the library smoke build passes it.
Why
_, and why not the alternatives.is ruled out. The HIP lowering prints value names as C identifiers (int add.1doesnot compile), and the LLVM lowering already names its own temporaries
<hint>.<n>, sharingsome hints with the builder, so
%splat.7could be defined twice. With_, builder namesnever contain
.and lowering temporaries always do.-breaks the HIP lowering the same way;$makes LLVM quote the name.no IR today, but keeps the misleading names, needs a per-builder set of every name for the
builder's whole life (and a binary ABI bump for the C++ builder struct), and leaves author
names unchecked. The separator needs no state and no ABI change.
_only after a digit-final hint still changes almost every kernel (digit-final hints suchas
cz16,av8,acc32are in nearly all of them) and leaves two naming rules.Re-bless scope
The IR of almost every kernel changes, in value names only. Each artifact is re-blessed with its
own tool, or, where it has none, by the procedure its test checks, and only the entries it already
has are changed (nothing added or removed). Run on develop without this change, every
one of these tools reproduces develop's committed file byte for byte, so each change below comes
from this PR alone.
platform/tests/golden/rocke_representative_ir_sha256/(all 39 family shards)rocke_ir_parity_harness.py --write platform/tests/golden/rocke_representative_ir_sha256library/tests/golden/attention_bwd_ir_sha256.json--writeattention_dense_bwd_ir_sha256.json--write(python -m tests.test_attention_dense_bwd_ir --write)attention_dense_gfx942_ir_sha256.json--writeattention_strided_kv_ir_sha256.jsondsa_indexer_ir_sha256.json--writegdn_decode_gfx950_ir_sha256.json--writekda_gfx942_ir_sha256.json--writekda_gfx950_ir_sha256.json--writemla_prefill_gfx942_ir_sha256.json--writetests/core: the four scaled-MMA golden tables (gfx950_scaled_mfmaand three WMMA tables) and the NVFP4 GEMM table, llvm23test_demo_ir_snapshot.py)tensor_transformsemit goldens (emit_conv,emit_rebase,emit_two_inputs,emit_xor)_separator removed again is develop's filellvm_gfx_all.json,llsectionrun_diff.py --python-only --record-golden, keeping only the entries the golden already hasconv_abi,barrier_fence,target_intrinsicsand two configurations of a newer WMMA target's lowering family at every flavor, whose IR names no builder value)ir_canonicalsectionrun_diff.py --python-only --mode ir --canonical --record-golden, samefused_moe_e2eentries, whose kernel's loop names are renamed)The emitted LLVM IR of every representative case (349 at llvm20 and llvm22, 395 at llvm23 and
llvm24) was compared with develop's after renaming every value, and every LDS name in an
alias-scope string, to its order of first use: identical in all of them, so the change is value
names only. Also re-blessed: the tests
that pin value names (
test_mx_scale_decodein both engines,future_intrinsic_lowering.cpp, thethe newer target's cluster / prefetch / TDM tests,
test_lds_alias_scopes,test_lowbit_loop_types,test_mma_operand_register_type,test_unsigned_int_ops,test_global_ptr_offset,test_scf_if_else_results,test_optimization_barrierin both engines, the DPP checks intest_rocke.py, and the library tests that name theQQ-bias stride parameter:
test_attention_builds,test_attention_unified_lse).The C++ text-IR reader reads the new names (every both-engine test round-trips through it);
test_xf32_bf16x3_lowering.cppkeeps its hand-written IR, which does not come from the builder.Tests
tests/core/test_value_names.pyandtests/core/test_value_names.cppbuild thecz16/cz1pair and check the names, that both engines lower the kernel to the same LLVM IR with every name
defined once, and every refusal with its exact text, keywords included. The Python test also
checks that the two engines' keyword tables hold the same words.
rocke_value_namesis in the quick and ffm-quick lists of the provider CTest categories, andtests/test_cxx_test_categories.pyfails if any installed C++ test has no tier or gets differenttiers from the others (tests that had no tier before this check are listed by name in that file).
It configures the platform and reads the installed CTest file the build writes, so it also sees
tests whose names a CMake loop builds; the two portable IR tests it found
(
rocke_portable_ir_dom_decoders,rocke_portable_ir_recipe_vm_replay) join the quick andffm-quick lists.
tests/portable_ir/test_recipe_roller.py::test_standalone_cli_replays_a_fan_with_synthetic_lane_namesrolls a fan the roller has to name itself, replays it through the standalone C replay at sampled
and held-out lane counts, and checks the roller's names are in the output and that the LLVM IR
matches Python's once value names are renumbered; it fails on the old
__fa<fan>_<lane>names.For other open PRs
Every PR that touches an IR golden, a library golden, the differential golden or a test that
pins a value name must regenerate it on rebase after this lands (with the same tools as above;
for the representative golden,
--write <shard dir> --only <family>re-blesses just that PR'sfamilies);
a kernel that declares a parameter, induction variable or loop-carried value ending in
_anddigits must rename it.
Gate (run on this branch on develop c6bca3b; it now sits on 3ab1173, which changes nothing under
dnn-providers)testsandpython/rocke/tests), both engines strict--check, differential golden--check-golden(llat four flavors andir_canonical)library/tests)Not run: GPU execution (names do not reach the code object), and the hipDNN kernel-ingestor gtests, which need the provider build (they check the renamed argument name against the packs' own lists).
ISSUE ID : AICK-2799
🤖 Generated with Claude Code