Skip to content

fix(rocke): never pass a compiler option the loaded compiler does not know - #14218

Open
cgmillette wants to merge 5 commits into
developfrom
users/cgmillette/rocke/compiler-option-check
Open

cgmillette wants to merge 5 commits into
developfrom
users/cgmillette/rocke/compiler-option-check

Conversation

@cgmillette

@cgmillette cgmillette commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

A kernel that asks for VGPR-form MFMAs (the mfma_vgpr_form attribute, or an agpr_alloc of zero) is compiled with -mllvm -amdgpu-mfma-vgpr-form. helpers/compile.py passed that option to COMGR without asking whether the loaded compiler knows it. LLVM 20 (the COMGR of ROCm 7.0 and 7.1) does not, and LLVM's option parser does not return an error for an unknown option: it prints Unknown command line argument and exits the whole host process. On an LLVM 20 host, library/tests/test_attention_bwd_mirror.py::test_mirror_resource_gate ends pytest on its first case, and every library test file that compiles such a kernel ends its process the same way.

ISSUE ID : AICK-2714

Technical Details

Five commits.

  • rocke/runtime/compiler_options.py (new) and rocke/runtime/data/compiler_options.json (new): which -mllvm options each compiler accepts, as measured data with one home. One column per LLVM major version, each cell recording accepted or not, the compiler's own diagnostic for a refusal, and the exact compiler it was measured on. require_accepted(options, compiler=..., needed_by=...) raises CompilerOptionNotAccepted (a ValueError, exported from rocke.runtime) naming what is compiled, the option and the compiler when the table records a refusal. It raises ValueError when the table has no answer for the option or for that LLVM major: guessing wrong ends the process, so an unknown answer is not treated as accepted. The module is the only home of the file format: parse_table reads it and checks every cell's types (a string "false" is refused, not read as true), and every table error, a JSON syntax error included, is a ValueError naming the file; format_table writes it, and measured_answers returns every answer.
  • tools/gen_compiler_options.py (new) measures the table. For each option, a child process loads the COMGR the parent loaded and compiles an empty kernel with -O3 -mllvm <option>: exit 0 means accepted, Unknown command line argument means refused, anything else stops the tool with nothing recorded. A host can only measure its own compiler, so a run writes only its own column. --check re-measures and fails when that column differs or is missing; the comparison is compiler_options.column_drift, so it is tested without a compiler.
  • CI runs --check as the CTest rocke_compiler_options_reproducibility, registered in the build tree (tests/cmake/CoreTests.cmake) and in the installed test manifest with the standard label, and listed in the provider's standard tier beside the arch-domain gate (the tool is staged test-only under tests/compiler_options/). It fails rather than skips without a queryable COMGR, and like the arch-domain gate its environment is only PYTHONPATH.
  • rocke/runtime/comgr.py: require_options_accepted(options, needed_by=...) checks an option list against the loaded compiler. A list without -mllvm is accepted without loading COMGR. When a -mllvm option is needed and no COMGR with a queryable LLVM version is loaded, it raises ComgrError naming what is compiled and the options, and points at loading a queryable COMGR. An option containing a NUL character is refused with ValueError (backend_options): COMGR reads each option as a C string and would lose the text after the NUL, so -mllvm\0 would otherwise reach it as -mllvm. build_hsaco_from_llvm_ir and build_hsaco_from_llvm_modules call it before COMGR sees the options, so no caller can reach the exiting parser. comgr_child_env() builds the environment for a child process that must compile with this process's COMGR (ROCKE_COMGR_LIB, AMD_COMGR_CACHE=0); the fresh-process compile, the tool and the tests use it.
  • rocke/helpers/compile.py: compile_kernel, compile_module and lower_kernel_for_comgr check the kernel's options before lowering, so the error names the kernel. accepted_comgr_options(kernel) (public) is that check: it returns the options a kernel compiles with, or raises as above. _comgr_options_for_kernel is unchanged and now documents why the option is part of the kernel.
  • The C++ engine builds no COMGR options and calls COMGR only for a version query, so it has nothing to change. No emitted IR changes.
  • Library tests:
    • library/tests/_compiler_test_utils.py (new) is the one skip policy for compile tests: skip_where_compiler_refuses_option() skips on CompilerOptionNotAccepted, through require_tools_or_skip (moved here from the MLA prefill test), which fails instead of skipping under ROCKE_REQUIRE_COMPILER_TOOLS=1. Any other error propagates. test_compiler_test_utils.py (new) checks all three cases and runs in the installed MLA ctest entry, which sets that variable.
    • test_mla_prefill_gfx942.py: the occupancy guard skipped on a hard-coded LLVM 21+ check. It now uses that policy, so the fact lives only in the table, and checks for a queryable COMGR through loaded_compiler_info.
    • test_attention_bwd_compile.py and test_attention_bwd_agpr_probe.py: tests that compile such kernels (through accepted_comgr_options) use the same policy. On an LLVM 20 host these tests used to end their process.
  • dsl_docs/reference/env_flags.md: a paragraph in "Compiler detection and provenance" pointing to the contract and the CTest.

Every option rocKE passes: the only -mllvm option rocKE passes to COMGR is -amdgpu-mfma-vgpr-form. The only other is -mllvm -amdgpu-early-inline-all=true, passed to hipcc by one gfx950 example script, and every measured compiler accepts it. compile_kernel_via_hipcc passes no backend option.

Not measured: LLVM 21 (and any other major without a column). A compile that needs the option on such a compiler raises ValueError naming the command that measures it, and the CTest fails on that compiler until its column is committed.

Why the option is refused rather than left out: on a compiler that lacks it, leaving it out compiles a different kernel from the one asked for. LLVM 20 does not have the option at all (upstream added it in LLVM 22; from LLVM 23 its default is on). Measured on LLVM 20 without the option:

  • gfx942 attention backward with zero AGPRs: every MFMA in VGPR form, 0 AGPRs, as asked.
  • gfx950 attention backward with zero AGPRs: every MFMA in AGPR form, with AGPRs allocated despite "amdgpu-agpr-alloc"="0,0".
  • gfx90a universal GEMM with zero AGPRs: MFMAs in AGPR form, with AGPRs allocated. Run on this host's gfx90a, it still gives correct results.

On LLVM 22, the gfx950 kernel compiled without the option fails register allocation, so there too the option is required. The results are correct, but the kernel is not the one asked for, and the resource plans that rely on the zero-AGPR request would be checked against a kernel that ignores it. Following the rule that fallbacks raise, the compile is refused with an exact error.

Test Plan

  • tests/runtime/test_compiler_options.py (52 cases):
    • the committed table is well formed, every refusal carries the compiler's diagnostic, and format_table(parse_table(file)) reproduces the committed file byte for byte;
    • parse_table refuses text that is not JSON, a wrong schema, a list where the options, an option's column or one answer must be an object, a non-boolean accepted, a non-string evidence or verified_on, a wrong key and a non-numeric LLVM major, each with a message naming the file and, where there is one, the entry;
    • column_drift is empty when a column reproduces and reports a changed answer and a missing one;
    • -mllvm option names are parsed from an option list, in both spellings, and an option list with a NUL character is refused;
    • every name in __all__ of rocke.runtime and rocke.runtime.compiler_options resolves;
    • against the committed table: a refused option raises CompilerOptionNotAccepted, naming the kernel, option and compiler; an accepted one passes; an unmeasured compiler or option raises ValueError and is not treated as accepted;
    • in this process, with the loaded compiler's identity pinned to one the table records as refusing: compile_kernel, compile_module, lower_kernel_for_comgr, accepted_comgr_options, build_hsaco_from_llvm_ir (in process, fresh process, joined -mllvm= spelling) and build_hsaco_from_llvm_modules each raise CompilerOptionNotAccepted before COMGR. With COMGR not loaded, or its LLVM version unknown, accepted_comgr_options and the three COMGR entry points raise ComgrError naming the options, and those entry points refuse an option list with a NUL character, all before COMGR. These need no COMGR, so they run in every CI lane;
    • comgr_child_env names the loaded library (its mapped path, or the requested name when the loader reports none) and turns the COMGR cache off, and raises ComgrError without a loaded library;
    • in a child process started with rocke_python_command, so a regression that exits cannot end pytest: compile_kernel, accepted_comgr_options and a raw build_hsaco_from_llvm_ir (both spellings) follow the table on the real loaded compiler.
  • With the check removed from any one entry point, its in-process test fails (on an LLVM 20 host the process ends).
  • rocke_compiler_options_reproducibility: passes in the build tree and the installed tree on LLVM 20; with an LLVM 18 COMGR loaded it fails, naming the missing column. tests/test_engine_linkage.py asserts that its installed environment, like the arch-domain gate's, is PYTHONPATH and nothing else.
  • Measured the table on LLVM 20 (this host) and on LLVM 22, 23 and 24 (container images, no GPU).
  • Repro: pytest tests/test_attention_bwd_mirror.py::test_mirror_resource_gate on this LLVM 20 host.

Test Result

  • The repro no longer ends pytest: 20 cases skip, each naming the kernel, -mllvm -amdgpu-mfma-vgpr-form and the loaded LLVM 20 compiler. These cases already skip on any compiler other than the production one.
  • First commit: byte identity green at llvm20, llvm22 and llvm23 (no emitted IR changes); ctest passes; the touched tests pass with both engines in strict mode; the full platform suite passes; pre-commit passes. Library smoke for this change (919 selected tests, one process per file): no new failures. On develop, this host's run had files whose process died on the unknown option. On this branch none do.
  • Second commit (no emitted IR changes): test_compiler_options.py 28 passed; runtime, compiler-detection and driver-policy tests pass; test_compiler_test_utils.py passes with and without ROCKE_REQUIRE_COMPILER_TOOLS=1, in the source tree and the installed tree; the MLA prefill, attention backward compile, AGPR probe and layering tests pass or skip as before; the relative-path guard and pre-commit pass; the branch merges cleanly with develop.
  • Third commit (no emitted IR changes): test_compiler_options.py 33 passed; gen_compiler_options.py --check passes on this host and fails, naming the option, with an edited committed answer; the installed CTest environment contract test passes.
  • Fourth commit (no emitted IR changes): test_compiler_options.py 37 passed; removing the JSON error wrap or any one of the three object checks fails its new case, and an extra variable in both gates' installed environment fails the linkage test.
  • Fifth commit (no emitted IR changes): test_compiler_options.py 52 passed; removing the NUL refusal fails its 5 cases, removing the unqueryable-compiler branch fails its 8, and removing the evidence type check fails its case.
  • On the current head: build and ctest pass; the touched platform and library tests pass with both engines in strict mode; the platform suite has no failures (develop has none either); pre-commit passes.

Submission Checklist

🤖 Generated with Claude Code

@cgmillette
cgmillette requested review from a team as code owners October 22, 2026 19:44
@tenpercent
tenpercent requested a balanced review from Copilot October 22, 2026 19:46
@github-actions github-actions Bot added documentation project: hip-kernel-provider rocke-lib: attention Touches rocKE library attention/FMHA kernels rocke: platform Touches rocKE platform labels Oct 22, 2026
@therock-pr-bot

therock-pr-bot Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Test regressions remain in subprocess compiler selection, installed probe availability, and missing-COMGR skip handling.

3 open issues
What changed in this PR

This PR adds measured compiler-option checks to rocKE’s COMGR compilation path, preventing unsupported LLVM backend options from terminating the host process rather than silently changing the requested kernel.

Changes:

  • Adds a compiler-option table, validation API, and subprocess-based measurement tool.
  • Checks options before kernel lowering and at public COMGR compilation entry points.
  • Adds regression tests, updates compiler-dependent skips, and documents the contract.
File Description
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tools/​gen_compiler_options.py Measures option acceptance in isolated processes.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​runtime/​test_compiler_options.py Tests table validation and process-safe rejection.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​runtime/​data/​compiler_options.json Records measured acceptance by LLVM major version.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​runtime/​compiler_options.py Implements option lookup and rejection errors.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​runtime/​comgr.py Validates options before COMGR compilation.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​python/​rocke/​helpers/​compile.py Checks kernel options before lowering.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​dsl_docs/​reference/​env_flags.md Documents compiler-option validation.
dnn-providers/​hip-kernel-provider/​rocke/​library/​tests/​test_mla_prefill_gfx942.py Replaces the version-based skip with option-based rejection.
dnn-providers/​hip-kernel-provider/​rocke/​library/​tests/​test_attention_bwd_compile.py Skips backward compile cases requiring refused options.
dnn-providers/​hip-kernel-provider/​rocke/​library/​tests/​test_attention_bwd_agpr_probe.py Skips ISA probes when required options are refused.

Review effort: Balanced

@cgmillette

Copy link
Copy Markdown
Contributor Author

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

PR #14218 — never pass a compiler option the loaded compiler does not know — 🔴 First-round: blocking — the change is right, but its own new test fails in all four CI shards; two of Copilot's three items reproduce

Prior item Status
Copilot (10-22) #3: the installed rocke_pytest runs test_committed_column_matches_this_compiler, but tools/gen_compiler_options.py is not installed 🔴 OPEN, reproduced. In a standalone cmake --install of the merge, the test fails with "can't open file …/tools/gen_compiler_options.py" (ran). All four TheRock Test hipkernelprovider shards fail rocke_pytest on exactly this at head (read) → skip that check when the tool is absent, or install the tool and what it needs
Copilot (10-22) #1: dropping the preflight in test_mla_prefill_gfx942.py turns "no COMGR" skips into errors 🟠 OPEN, reproduced. With the COMGR identity query failing (simulated), develop skips the 8 occupancy tests and the merge errors on all 8 (ran) → keep an availability skip, without the hard-coded LLVM threshold
Copilot (10-22) #2: the child processes may load a different COMGR than the parent 🟡 OPEN, confirmed by reading: _run_child passes only the environment, not the parent's ROCKE_COMGR_LIB. This host's only COMGRs are both LLVM 24, so it cannot show the mismatch (read)
Bots / CI 🔴 Red at head: Test hipkernelprovider on gfx94X and gfx110X (all four shards) fails rocke_pytest, which is Copilot #3
Our checks 🟢 The core works. On this LLVM 24 host the new tests pass in the source tree (10), and gen_compiler_options.py --check reproduces the committed LLVM 24 column (accepted) (ran). Clean merge (8 behind); rule_scan is clean (ran)

No net-new findings beyond Copilot's.

Utility: 🟢 Positive — turns a process-killing "Unknown command line argument" exit into a named, catchable refusal, backed by measured per-LLVM data instead of a hard-coded version check; the CI failure is a test-packaging fix.


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

First-round review: a local qwen3.6 model made the independent first pass. The orchestrator (Claude Opus 5.5) verified it with the new tests and the measurement check on an LLVM 24 host, the test file run from a standalone install, a simulated host without COMGR on the merge and develop, and the failing CI logs at head, and owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection.

@cgmillette
cgmillette force-pushed the users/cgmillette/rocke/compiler-option-check branch from 0921adb to afd9e72 Compare October 23, 2026 03:22
@cgmillette
cgmillette requested a review from tenpercent October 23, 2026 14:54

@CongMa13 CongMa13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new issues found. I agree with the following existing comments:

  • Restore the COMGR availability skip without a compiler-version threshold — copilot-pull-request-reviewer[bot], #14218 (comment) (thread PRRT_kwDOOej8qc6vWvN9).
  • Pass the parent process's COMGR selection to child processes — copilot-pull-request-reviewer[bot], #14218 (comment) (thread PRRT_kwDOOej8qc6vWvOi).
  • Skip the source-tree measurement check when the generator is absent from installed tests — copilot-pull-request-reviewer[bot], #14218 (comment) (thread PRRT_kwDOOej8qc6vWvPB).

@CongMa13 CongMa13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new P0 or P1 issues were identified during this review. The changes are approved.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The guards and regression coverage address the process-exit failure, with only a minor source-comment correction remaining.

1 open issue
3 resolved since last review

Review effort: Balanced

@tenpercent

Copy link
Copy Markdown
Contributor

Review by Codex / GPT-6.1 Sol / high.

Reviewed head: afd9e722d6c027fd39a5395b39f5203fe06bbeea.

Recommendation: request changes. One confirmed correctness finding.

High severity: joined -mllvm arguments bypass the compiler-option guard.

At compiler_options.py:77–78, backend_options() recognizes only an element exactly equal to -mllvm. COMGR also accepts the joined spelling -mllvm=<option>. That spelling produces an empty parsed list, so require_options_accepted() returns early and the public COMGR compilation entry points forward the unchecked option. This leaves the process-safety fix incomplete for raw public callers.

I reproduced this on the reviewed head with /opt/rocm/lib/libamd_comgr.so, queried through that library's preprocessing as LLVM 20.0.0. Run the following in a disposable Python process:

from rocke.runtime.comgr import build_hsaco_from_llvm_ir

try:
    build_hsaco_from_llvm_ir(
        "define amdgpu_kernel void @k() { ret void }",
        isa="amdgcn-amd-amdhsa--gfx942",
        options=["-O3", "-mllvm=-amdgpu-mfma-vgpr-form"],
    )
except Exception as exc:
    print("CAUGHT", type(exc).__name__, exc)
print("SURVIVED")

The process exits with status 1 and Unknown command line argument '-amdgpu-mfma-vgpr-form'; neither the exception handler nor the survival marker runs. The equivalent paired spelling, options=["-O3", "-mllvm", "-amdgpu-mfma-vgpr-form"], raises CompilerOptionNotAccepted and reaches the survival marker. A deliberately unknown joined option also exits, while its paired spelling raises ValueError.

Please recognize both argument spellings, or reject the joined spelling before COMGR sees it, and add child-process regression cases. A candidate parser change evaluated in scratch child processes converted both observed exits into the intended exceptions while preserving ordinary -O3 compilation and paired-spelling behavior.

Validation:

  • Compiler-option and module tests: 24 passed, 2 skipped.
  • Compiler-option and fresh-process tests: 23 passed.
  • Source tracing confirms the three earlier Copilot findings are addressed: missing/unqueryable COMGR skips, propagation of the parent COMGR into both test subprocesses, and the installed-test skip for the absent source-only generator.
  • JSON packaging rules were checked statically. I did not build an installed package, run GPU numerics, or execute the complete platform suite or byte-identity matrix. The finding is reproduced through COMGR compilation and requires no GPU.

@cgmillette
cgmillette force-pushed the users/cgmillette/rocke/compiler-option-check branch from afd9e72 to 5fe79de Compare October 24, 2026 00:23
@cgmillette

Copy link
Copy Markdown
Contributor Author

Thanks @tenpercent, confirmed and fixed in 5fe79de. With -mllvm=-amdgpu-mfma-vgpr-form, an LLVM 20 COMGR exited the process: backend_options() only read the -mllvm <option> pair. It now reads both spellings, -mllvm <option> and -mllvm=<option>, and refuses a bare -mllvm= with a message. build_hsaco_from_llvm_ir and compile_kernel therefore refuse the joined form with CompilerOptionNotAccepted like the paired one. New regression cases: a unit test for both spellings, and a child-process compile with the joined spelling, which exits the child without the fix. Gate on the new head: byte identity llvm20/22/23, compiler-option check, ctest, portable IR, touched tests, full platform suite with no failures, library smoke with no new failures.

@cgmillette
cgmillette enabled auto-merge (squash) October 26, 2026 17:47

@amd-khushbu amd-khushbu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocks from merging.

@cgmillette
cgmillette force-pushed the users/cgmillette/rocke/compiler-option-check branch from 5fe79de to 9bd4092 Compare October 27, 2026 07:12
@cgmillette

Copy link
Copy Markdown
Contributor Author

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

PR #14218 — never pass a compiler option the loaded compiler does not know (cgmillette, AICK-2714) — 🟡 Follow-up: rebase needed — one doc conflict; the red CI was develop's

Prior item Status
Since our 10-23 review (0921adbb) 🟢 Reviewed. Three force-pushes, each a single commit: afd9e722 (Copilot ×3), 5fe79de0 (the joined -mllvm= spelling and the latency comment), and the 10-27 squash 9bd4092e. The squash is 5fe79de0 rebased onto develop b1f7280b, plus the shared skip helper. Outside the four test files, every file's patch is identical to 5fe79de0 (ran: per-file patch comparison)
Copilot (10-22) #3 = our 10-23 🔴: the installed rocke_pytest ran the source-tree --check, but tools/gen_compiler_options.py is not installed 🟢 FIXED afd9e722. The test skips when the tool is absent (test_compiler_options.py:237). In CI at head, rocke_pytest passes in all four shards, and in the new file only this test skips, with "source-tree check: gen_compiler_options.py is not installed" (read: CI logs). In the source tree it runs and reproduces the committed LLVM 24 column (ran). Thread resolved
Copilot (10-22) #1 = our 10-23 🟠: dropping the preflight turned "no COMGR" skips into errors 🟢 FIXED afd9e722. setUpClass again skips when no queryable COMGR loads (test_mla_prefill_gfx942.py:433–437), without an LLVM-version threshold. With the identity query failing (simulated), both occupancy classes skip instead of erroring (ran: probe). Thread resolved
Copilot (10-22) #2 = our 10-23 🟡: the child processes may load a different COMGR than the parent 🟢 FIXED afd9e722. _child_env() (test_compiler_options.py:136) passes the parent's loaded COMGR as ROCKE_COMGR_LIB to both _run_child and the --check subprocess. On this host it names the loaded library (ran: probe). Thread resolved
Copilot (10-23) + tenpercent: absolute compile-latency wording in a gen_compiler_options.py comment 🟢 FIXED 5fe79de0. The comment at :48 now says only what the limit is (read). Thread resolved
tenpercent (10-23) HIGH: the joined -mllvm=<option> spelling bypassed the guard and exited the process 🟢 FIXED 5fe79de0. backend_options() reads both spellings and refuses a bare -mllvm= (compiler_options.py:80–91). With an LLVM 20 identity (simulated), the joined spelling raises CompilerOptionNotAccepted from build_hsaco_from_llvm_ir (in-process and fresh-process) and from build_hsaco_from_llvm_modules, and COMGR is never reached. LLVM 22, 23 and 24 accept it; LLVM 21 and 25 raise ValueError (ran: probe). The new joined_mllvm_spelling child compile passes here (ran) and in all four CI shards (read). cgmillette replied on 10-24; tenpercent's re-review is still requested
amd-khushbu (10-26) MEDIUM: three different skip-on-CompilerOptionNotAccepted implementations 🟢 FIXED 9bd4092e. One context manager, skip_where_compiler_refuses_option() (_attention_bwd_test_utils.py:78), serves all three sites (test_attention_bwd_compile.py:313, test_attention_bwd_agpr_probe.py:112, test_mla_prefill_gfx942.py:442), and no other library test catches the error. It turns only that error into unittest.SkipTest; a plain ValueError or RuntimeError passes through. With LLVM 20 simulated, both MLA occupancy classes skip through it with the "needs the compiler option" message (ran: probe). Thread resolved
amd-khushbu (10-26) LOW: no CI job runs the table-drift guard (--check) 🟡 DEFERRED. cgmillette: "Nothing enforces it yet", and they "propose a follow-up" that runs --check in CI once per recorded LLVM flavor. That is fair for this PR: an unmeasured LLVM major or option already raises instead of guessing (ran: probe), so the remaining risk is drift within a measured major. No ticket is named yet, and the thread is open
Approvals: CongMa13 (10-23, rocke-platform) at afd9e722; amd-khushbu (10-26, rocke-lib) at 5fe79de0 🟢 Both still count, and between them they cover both code-owner teams (platform/ and library/). The only later change is the squash with the helper, verified above. Auto-merge (squash) is on, but it cannot fire while the PR conflicts (read: gh, CODEOWNERS)
Bots / CI 🟡 Red, but the cause is develop's. CI tested 9bd4092e merged with develop d01bfb52 (first run 10-27, last re-run 10-28). All four hipkernelprovider shards (Linux gfx94X, Windows gfx110X) fail only rocke_differential_golden_ll and _canonical, and that turns the Multi-Arch CI Summary red. The unblessed configs are moe_fused_mega_fp8 1–5 and target_intrinsics 0/1/2/4/7/9/10/11 (llvm20/22/23), plus target_intrinsics 7/9/10/11 (canonical). That is exactly develop's #14474: the golden that #13974 enabled was stale after #13723 and #13976, and #14501 re-blessed these entries on 10-28 at 14:07Z. Every other blessed config matches, so the PR moves no IR. Eleven other PR runs from the same hours (#14244, #11642, #14522–#14528 and two more) fail on the same entries; runs created after #14501 pass. A re-run reuses the 10-27 merge, so only a rebase gets a fresh run. rocke_pytest passes in all four shards (the new file: 10 passed, 1 skipped by design). The representative golden, pre-commit, clang-tidy, the policy bot and both hipDNN superbuilds pass too (read: CI logs, checks)
Our checks 🟡 No merged tree. 9bd4092e conflicts with develop 4124e7be (166 behind, 1 ahead) in one file (N1), so the build, byte identity, goldens and tests on the merge were skipped (ran: merge-tree). At head on an LLVM 24 host, test_compiler_options.py passes 11/11, and all 39 probes above behave as expected (ran). Merge semantics: develop adds no new -mllvm option since the base (the same 3 uses). Its only other change to the PR's files is an unrelated comgr.py hunk from #14415, which merges cleanly. Every function the PR calls still exists in the merge result (ran: git grep). rule_scan is clean, and no links or tracker IDs are added (ran)
# Sev file:line Finding
N1 🟡 platform/dsl_docs/reference/env_flags.md:171–180 The one conflict with develop (1 hunk). Develop's #14415 (LLVM24 lane, 10-29) rewrote the closing paragraph of "Compiler detection and provenance", and the PR adds its new paragraph right after the old version of that paragraph. Until this is resolved there is no merged tree and no CI against today's develop, and auto-merge cannot fire → rebase onto develop: keep develop's rewritten paragraph and append the PR's paragraph unchanged. The result is develop's file plus the PR's paragraph (ran: merge-tree; resolution simulated)

Utility: 🟢 Positive — unknown -mllvm options now raise a named error, not a process exit.


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

Follow-up review: a local qwen3.6 model made the independent first pass, and the orchestrator (Claude Opus 5.5) verified it. The checks covered the commits since our 10-23 review and probes of each fix at head, with simulated LLVM 20 and missing-COMGR identities. They ran the PR's test file on an LLVM 24 host and a merge-tree dry run against today's develop. They read the CI logs at head and compared them with other PRs' runs from the same hours and with develop's #14474 and #14501. With no merged tree, the build, byte identity, goldens and tests on the merge did not run. The orchestrator owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection.

@cgmillette
cgmillette force-pushed the users/cgmillette/rocke/compiler-option-check branch 2 times, most recently from 4a0f8f4 to 6d14bd8 Compare November 2, 2026 17:18
@cgmillette

Copy link
Copy Markdown
Contributor Author

Cleaned up the test helpers from the rebase (head 6d14bd892e8):

  • Both compile-test skip helpers now live in library/tests/_compiler_test_utils.py under one policy: a compiler that refuses a needed option skips the test, and fails it under ROCKE_REQUIRE_COMPILER_TOOLS=1, the same as a missing tool. The MLA prefill and attention backward tests both use it, and _attention_bwd_test_utils.py is back to backward-only helpers.
  • The option check the compile path runs is now public, rocke.helpers.compile.accepted_comgr_options, used by compile_kernel and by the tests, so the tests check exactly what the compiler path checks instead of rebuilding it from private helpers. The MLA prefill test no longer probes the compiler through a private function.
  • New tests: the helper turns a real refused option into a skip, fails under ROCKE_REQUIRE_COMPILER_TOOLS=1, and lets other errors through; accepted_comgr_options refuses exactly what the option table says, with the real COMGR.

@cgmillette

Copy link
Copy Markdown
Contributor Author

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

PR #14218 — never pass a compiler option the loaded compiler does not know (cgmillette, AICK-2714) — 🟠 Follow-up: refusal untested in CI

Prior item Status
Copilot (10-22) ×3 = our 10-23 🔴🟠🟡: installed --check; no-COMGR skip; child COMGR 🟢 FIXED afd9e722
• tool-absent skip, no-COMGR guard, child ROCKE_COMGR_LIB hold at head (read)
• test file ✓ in installed CI at 1905cb0f (read)
tenpercent (10-23) ×2: joined -mllvm= exits the process; latency wording (Copilot too) 🟢 FIXED 5fe79de0
• joined_mllvm_spelling child compile ✓ at head and merge (ran)
• tenpercent's re-review still requested
Approvals: CongMa13 (10-23) afd9e722; amd-khushbu (10-26) 5fe79de0 🟢 APPROVED afd9e722, 5fe79de0
• older heads; still count on develop
• neither covers public accepted_comgr_options or the 11-02 skip policy
amd-khushbu (10-26) MEDIUM: three skip-helper copies 🟢 FIXED 9bd4092e
• one home now, _compiler_test_utils.py, 3 users (read)
• develop's tool-skip helper moved beside it
amd-khushbu (10-26) LOW: drift check never runs in CI 🟡 DEFERRED
• cgmillette: "Nothing enforces it yet"; follow-up proposed, no ticket
• fair: CI's own LLVM 24 cell drifting fails the child test (read)
• a gate should mirror rocke_arch_domain_reproducibility
Our 10-30 N1 🟡: env_flags.md conflict with develop 🟢 FIXED c42ee206
• develop's paragraph kept, the PR's after it (ran: range-diff)
• merge with today's develop clean (ran)
cgmillette (10-30 – 11-02) ×4: three rebases; test-helper cleanup 🟡 Mostly
• rebases change only conflict hunks (ran: range-diff)
• fail-on-refusal: MLA lane only; CI's LLVM 24 accepts (read)
• PR body and armed squash message omit the 11-02 changes
Bots / CI 🟡 running
• 24 ✓ · 5 running · 0 ✗
• Multi-Arch builds queued; hipkernelprovider tests not started
• merge: clean; blocked by CI and 1 open thread (read)
Our checks 🟢 Clean
• merge vs 3efc824f: clean (ran)
• build + ctest 34 ✓ / 1 skip; differential golden 0 changed (ran)
• touched tests + MLA lane, CI's env, both engines: 365 ✓ / 37 skip (ran)
• rule scan clean; no IR family to byte-check
Code quality 🟠 Should-fix
• holds: root-cause fix, one skip-policy home, names; C++ engine needs no twin (read)
• tests: N1; launcher and DRY: N2
• 🟡 refusal not exported; JSON read outside its home; LLVM 21 unmeasured (ran)
# Sev file:line Finding
N1 🟠 platform/tests/runtime/test_compiler_options.py:224, platform/CMakeLists.txt:715 CI never tests the entry-point refusal
• impact: delete the checks and all 80 CI tests still pass (ran: probe); policy tests run in no lane
• fix: in-process tests pinning LLVM 20 per entry point; add test_compiler_test_utils.py to the MLA list
N2 🟠 platform/tests/runtime/test_compiler_options.py:29, :150 Child Pythons skip the launcher platform/AGENTS.md requires
• impact: installed runs reach rocke via ambient PYTHONPATH only; COMGR pinning copied 3× (read)
• fix: rocke_python_command() + rocke_subprocess_env(); one child-env helper in runtime/comgr.py

Utility: 🟢 Positive — refuses options the loaded COMGR lacks instead of exiting the process.


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 of 4 pushes, merge with develop, build + ctest, differential golden, touched tests and the MLA lane in CI's Linux env (both engines), refusal probes, CI lane survey, code quality. Flags whether a closer look is needed; not an approval or rejection.

@cgmillette
cgmillette disabled auto-merge November 2, 2026 19:36
@cgmillette

Copy link
Copy Markdown
Contributor Author

Automated merge-queue status (rocKE merge monitor)

Hi @cgmillette, I turned auto-merge off on this PR for now. Before a rocKE PR lands, the rocKE merge monitor checks it on top of today's develop branch, and this version would not land cleanly:

  • our latest review of this exact version found problems to fix first, and they are still open

The open items from our review, each with its fix:

  • N1 (platform/tests/runtime/test_compiler_options.py:224, platform/CMakeLists.txt:715): CI never tests the entry-point refusal
    • impact: delete the checks and all 80 CI tests still pass (ran: probe); policy tests run in no lane
    • fix: in-process tests pinning LLVM 20 per entry point; add test_compiler_test_utils.py
  • N2 (platform/tests/runtime/test_compiler_options.py:29, :150): Child Pythons skip the launcher platform/AGENTS.md requires
    • impact: installed runs reach rocke via ambient PYTHONPATH only; COMGR pinning copied 3× (read)
    • fix: rocke_python_command() + rocke_subprocess_env(); one child-e

What to do: address the open items in our latest review (#14218 (comment)). After that, turn auto-merge back on; the monitor re-checks every push.

Nothing else changed: the monitor does not approve, reject or merge PRs. It only turned auto-merge off to keep develop green. This comment is updated in place.

@cgmillette

Copy link
Copy Markdown
Contributor Author

Follow-up to the 11-02 automated review (#14218 (comment)). Everything below is in 1496c89.

  • N1 (CI never tests the entry-point refusal): fixed. tests/runtime/test_compiler_options.py now has an in-process test for each public entry point: compile_kernel, compile_module, lower_kernel_for_comgr, accepted_comgr_options, build_hsaco_from_llvm_ir (in process, in a fresh process, and with the joined -mllvm= spelling) and build_hsaco_from_llvm_modules. Each test pins the loaded compiler's identity to an LLVM major that the table records as refusing the option (LLVM 20), and expects CompilerOptionNotAccepted naming what is being compiled. These tests do not need COMGR, so they run in rocke_pytest on every lane. I removed the check from each entry point in turn, and each time that entry point's test failed. test_compiler_test_utils.py is added to the installed MLA ctest list.
  • N2 (launcher and the copied COMGR child environment): fixed. comgr_child_env() in runtime/comgr.py is now the only place that sets ROCKE_COMGR_LIB and AMD_COMGR_CACHE=0 for a child process. The fresh-process compile, gen_compiler_options.py and the tests all use it. The test and tool children start through rocke_python_command() + rocke_subprocess_env(), and the test no longer works out a source-tree platform/python path.
  • M1 (refusal not exported): fixed. CompilerOptionNotAccepted is now in rocke.runtime.__all__, and a test checks this.
  • M2 (JSON read outside its home): fixed. All reading and writing of the file format is now in compiler_options:
    • parse_table reads it and checks each cell's types. A string "false" is now refused; before, it was read as accepted.
    • format_table writes it.
    • measured_answers returns every answer.
      The tool and the tests use these functions and no longer parse the JSON themselves. A test checks that format_table(parse_table(file)) reproduces the committed file exactly.
  • M3 (LLVM 21 unmeasured): answered, not a defect. No LLVM 21 COMGR is available to measure on our hosts. On such a compiler, a compile that needs the option raises ValueError and names the command that measures it (test_an_unmeasured_compiler_is_an_error_not_a_guess). It does not exit the process. The new CTest also fails on a lane whose compiler has no committed column, so an LLVM 21 lane would have to add its column before going green. The PR description now says LLVM 21 is unmeasured.
  • M4 (fail-on-refusal never runs in CI): fixed by N1. test_compiler_test_utils.py::test_refused_option_fails_where_tools_are_required now runs in the MLA entry, which sets ROCKE_REQUIRE_COMPILER_TOOLS=1. It uses real table answers, so it does not depend on which compiler CI loads.
  • D1 (stale description): fixed. The PR description now covers accepted_comgr_options, _compiler_test_utils.py and its ROCKE_REQUIRE_COMPILER_TOOLS policy, test_compiler_test_utils.py, the CTest, and the current test count.

Checks run on the new head: the touched platform and library tests, in both the source and the installed tree; the CTest in both trees; pre-commit; the relative-path guard; and a clean merge with develop. No emitted IR changes.

@cgmillette

Copy link
Copy Markdown
Contributor Author

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

PR #14218 — never pass a compiler option the loaded compiler does not know (cgmillette, AICK-2714) — 🟠 Follow-up: drift branch untested

Prior item Status
Our 11-02 N1: CI never tests the entry-point refusal 🟢 FIXED 1496c899
• each entry point refuses on a pinned LLVM 20, no COMGR (ran)
• removing any one check fails its test: 6 of 6 (ran)
• 28 ✓ in a rebuilt install (ran); utils test in the MLA list (read)
Our 11-02 N2: child Pythons skip the required launcher 🟢 FIXED 1496c899
• test and tool children start via rocke_python_command (ran: probe)
• comgr_child_env() now builds every child's COMGR env (ran: probe)
amd-khushbu (10-26) LOW: drift check never runs in CI 🟢 FIXED 1496c899
• installed CTest labelled standard; shards run -L ^standard$ (read)
• rebuilt install: passes; fails on drift or a missing column (ran)
• not in the tier YAML, so both superbuild passes skip it (read)
Approvals: CongMa13 (10-23) afd9e722; amd-khushbu (10-26) 5fe79de0 🟢 APPROVED afd9e722, 5fe79de0
• older heads; still count on develop
• neither covers the 10-27 – 11-03 API and CTest changes
• review decision approved; tenpercent's re-review still requested
Bots / CI 🟡 running
• 24 ✓ · 3 running · 0 ✗ · 1 cancelled (superseded PR Bot run)
• running: Multi-Arch attempt 2 (attempt 1: checkout timeout); shards not started
• merge: clean; blocked on Multi-Arch CI Summary
Our checks 🟢 Clean
• merge vs 7afa104e: clean, 63 behind (ran)
• build + ctest 35 ✓ / 1 skip, new gate ✓; differential golden 0 changed ×4 (ran)
• PR's tests 295 ✓ / 37 skip; no ABI change; rules clean (ran)
Code quality 🟠 Should-fix
• holds: one home each; launcher reused; no C++ twin needed (read)
• fixed: N1, N2, M1, M2, M4, D1; M3 answered (ran)
• tests: N3; 🟡 child-env test predicts its own output (ran)
• 🟡 two table errors hide the bad key; none names the file (ran)
# Sev file:line Finding
N3 🟠 platform/tools/gen_compiler_options.py:122 The drift gate's failing branch has no test
• impact: fails correctly today (ran); a --check stuck at 0 would stay green
• fix: pure drift function in compiler_options; 3 rocke_pytest cases

Utility: 🟢 Positive — refuses options the loaded compiler lacks instead of ending the process.


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: the new commit, merge with develop, build + ctest, differential golden, touched tests, per-entry-point mutations, a rebuilt installed layout, CI lane survey, code quality. Flags whether a closer look is needed; not an approval or rejection.

@cgmillette

Copy link
Copy Markdown
Contributor Author

Re the 11-03 review (#14218 (comment)): Thanks. N3 is fixed in 45876fa. The comparison --check makes is now rocke.runtime.compiler_options.column_drift(committed, measured, llvm_major=...), and the tool fails exactly when it returns a non-empty list. Three rocke_pytest cases cover it: every committed column reproduces against its own answers, a flipped answer is reported as committed accepted=False, measured accepted=True, and an unmeasured LLVM major is reported as no committed answer for each option. I also checked the tool end to end: with the committed LLVM 20 answer edited to accepted, --check exits 1 naming the option.

The minors, once:

  • Table errors now start with the file they came from (parse_table(text, source=...), defaulting to the committed table) and name the exact wrong keys: missing and unexpected keys of a cell, and keys that are not an LLVM major. The malformed-table tests assert both.
  • The child-environment test no longer predicts its own output: it pins the loaded compiler and checks the mapped path, and the fallback to the requested name when the loader reports no path.

Also from the rebase: the new CTest is listed in the provider's standard tier (develop now checks that every registered CTest is in one), and its installed environment is only PYTHONPATH, like the arch-domain gate.

Rebased onto develop (head 45876faa7f8).

  • The only textual conflict was in helpers/compile.py: develop now chooses the ISA name for the loaded compiler (loaded_compiler_info), and this PR checks the kernel's options. Both are kept: the import takes both names, and the lower_kernel_for_comgr docstring says both.
  • Develop's fix(rocke): scope installed compiler test policy #14868 split the installed CTest compiler environment into tool discovery and the compile-check requirement, and removed the variable this PR's rocke_compiler_options_reproducibility entry referenced. The entry now sets only PYTHONPATH, like the arch-domain gate, since the tool only loads COMGR. The installed-environment contract test now checks that.
  • Develop added a check that every registered CTest is in a provider tier. The new CTest is now listed in the standard tier beside the arch-domain gate, so the provider's CI selects it.

Checked: build and ctest, the touched tests with both engines in strict mode, the platform suite (no failures, same as develop), the option table check on this host, and pre-commit. No emitted IR changes, so no golden is re-recorded.

@cgmillette
cgmillette force-pushed the users/cgmillette/rocke/compiler-option-check branch from 1496c89 to 45876fa Compare November 3, 2026 21:07
@cgmillette
cgmillette requested a review from a team as a code owner November 3, 2026 21:07
@cgmillette

Copy link
Copy Markdown
Contributor Author

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

PR #14218 — never pass a compiler option the loaded compiler does not know (cgmillette, AICK-2714) — 🟡 Follow-up: N3 fixed, minors remain

Prior item Status
Our 11-03 N3: drift gate's failing branch untested 🟢 FIXED 45876faa
• --check fails exactly on column_drift's list (read)
• dropping the flip or missing report fails a new case (ran)
• the 3 cases pass in all 4 shards (read)
amd-khushbu (10-26) LOW: drift check never runs in CI 🟢 FIXED 659dca29
• gate ✓ in all 4 TheRock shards (read)
• tier YAML lists it now: both superbuilds run it, ✓ (read)
Approvals: CongMa13 (10-23) afd9e722; amd-khushbu (10-26) 5fe79de0 🟢 APPROVED afd9e722, 5fe79de0
• older heads; still count on develop
• neither covers the 10-27 – 11-03 API, CTest and tier changes
• both in hipdnn-reviewers, the tier YAML's owner
Bots / CI 🟡 red, develop's
• fails: 4 hipkernelprovider shards — develop's f64 doc pair (14998)
• rest of those shards ✓: new gate, every PR case (read)
• 41 ✓ · 1 cancelled (superseded PR Bot); attempt-1 infra re-ran ✓
• merge: blocked on Multi-Arch Summary; fresh run after 15002 lands
Our checks 🟢 Clean
• merge vs a753637e: clean, 19 behind (ran)
• rebase: compile.py keeps develop's ISA naming and the check (ran)
• build + ctest 46 ✓ / 1 skip, gate ✓; golden 0 changed ×4 (ran)
• PR's tests 302 ✓ / 38 skip; installed 105 ✓ (ran)
Code quality 🟡 Minor
• N3 fixed: column_drift tested, mutants caught (ran)
• fu3 minors fixed; error and env tests partial: N5 (ran)
• owns the skip helper 14502 repeats: N4 (read)
# Sev file:line Finding
N4 🟡 library/tests/_compiler_test_utils.py:44 14502 adds a second copy of this skip helper
• impact: its library/tests/_compiler_tools.py:44 repeats require_tools_or_skip; its MLA guard keeps a hard-coded LLVM 21 gate (test_mla_prefill_gfx942.py:477) that this PR's table replaces (read)
• fix: none here; owner is this PR (lands first). 14502 imports it and skip_where_compiler_refuses_option
N5 🟡 platform/python/rocke/runtime/compiler_options.py:155 Table errors and the gate's environment are only partly tested
• impact: broken JSON raises a bare JSONDecodeError naming no file (ran); the 3 wrong-type refusals have no case; platform/tests/test_engine_linkage.py:216 compares the gate's env only with its sibling's
• fix: except json.JSONDecodeError as exc: raise ValueError(f"{source}: {exc}") from exc, as core/ir_golden.py:79-81 does; 4 malformed cases; assert both gates' env keys are {"PYTHONPATH"}

Utility: 🟢 Positive — refuses options the loaded compiler lacks instead of ending the process.


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: the rebase, merge with develop, build + ctest, differential golden, touched tests, an installed layout, mutations, the saved N3 probe, CI shard and superbuild logs, code quality. Flags whether a closer look is needed; not an approval or rejection.

@cgmillette

Copy link
Copy Markdown
Contributor Author

Rebased onto develop and addressed the latest automated review.

  • Rebase: the only conflict was the standard tier in ROCKE_ENGINE_test_categories_external.yaml, where develop had added rocke_library_attention_cache_key_sweep_pytest beside this PR's rocke_compiler_options_reproducibility. Both entries are kept. No emitted IR changes, so no goldens were re-recorded.
  • N5 (fixed in 9151d941513):
    • A JSON syntax error in the table now raises ValueError naming the file, as core/ir_golden.py does, instead of a bare JSONDecodeError.
    • New cases cover that error and the three refusals of a list where the table needs an object (the options, an option's column, one answer).
    • test_engine_linkage.py now asserts that both COMGR-only gates get PYTHONPATH and nothing else, not just the same environment as each other.
    • Removing any one of these checks fails its new case.
  • N4: nothing to change here. This PR owns require_tools_or_skip and skip_where_compiler_refuses_option, and it lands first. refactor(rocke): MLA prefill on shared emitters; odd head counts on the packed layout #14502 should import them rather than keep its own copy.
  • Also: the new require_options_accepted signature now spells its optional argument Sequence[str] | None (the Python style guide's form), folded into the first commit.

Gates on the new head: build and ctest pass, including the new compiler option gate; the touched platform and library tests pass with both engines in strict mode; pre-commit passes; the platform suite shows no failures that develop does not also show.

@cgmillette

Copy link
Copy Markdown
Contributor Author

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

PR #14218 — never pass a compiler option the loaded compiler does not know (cgmillette, AICK-2714) — 🟡 Follow-up: N5 fixed; CI re-run pending

Prior item Status
Our 11-04 N5: table errors, gate env partly tested 🟢 FIXED 9151d941
• bad JSON now a ValueError naming the file; 3 list refusals tested (ran)
• each removed check, and an extra gate env var, fails a case (ran)
• left: one more type check untested, N6
Our 11-04 N4: 14502 copies this skip helper 🟢 DECIDED
• owner: this PR's module; nothing to change here
• 14502 unchanged at a3347689: still copies it, gates on LLVM 21 (read)
• 14502's side: drop the copy, import both helpers
Approvals: CongMa13 (10-23) afd9e722; amd-khushbu (10-26) 5fe79de0 🟢 APPROVED afd9e722, 5fe79de0
• older heads; still count on develop
• neither covers the 10-27 – 11-05 API, CTest, tier and N5 changes
• both in hipdnn-reviewers, the tier YAML's owner (read)
Bots / CI 🟡 Re-running, infra
• fails (attempt 1): Windows gfx110X shard 2/2 — rocke_pytest, conv entry time out on a slow Azure runner (read)
• not the PR's: both ✓ on shard 1/2, same merge; gate and 37 option tests ✓ in all 4 shards (read)
• running: that shard's re-run; 43 ✓ · 1 cancelled (superseded PR Bot)
• merge: blocked on Multi-Arch Summary until it passes
Our checks 🟢 Clean
• merge vs b25da5ad: clean, 20 behind (ran)
• rebase: only the tier YAML conflicted; both entries kept (ran)
• build + ctest 49 ✓ / 1 skip, gate ✓; golden 0 changed ×4 + canonical (ran)
• PR's tests 319 ✓ / 38 skip; installed 109 ✓; rules 1–3 clean (ran)
Code quality 🟡 Minor
• N5 fixed; the evidence type check still lacks a case: N6 (ran)
• the option table is the only home (read)
• N4: nothing to change here; 14502 keeps its copy (read)
# Sev file:line Finding
N6 🟡 platform/python/rocke/runtime/compiler_options.py:137 The evidence type check has no case
• impact: with it removed, all 37 cases still pass (ran); a numeric evidence would then crash the refusal message (:295, AttributeError)
• fix: a test_parse_table_refuses_a_malformed_table row turning "evidence": "" into 0, expecting on LLVM 22 evidence=0 … expected strings; it fails without the check (ran)

Utility: 🟢 Positive — refuses options the loaded compiler lacks instead of ending the process.


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: the rebase, merge with develop, build + ctest, differential golden, touched tests, an installed layout, mutations of the new checks, CI shard and superbuild logs, code quality. Flags whether a closer look is needed; not an approval or rejection.

@yraparti yraparti left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rocKE checklist review

ROCm/rocm-libraries#14218 · 9151d9415132 · model gemini-4-argon, fallback gpt-5.5

PASS · graded 238/238 · BLOCKER 0, HIGH 1, MEDIUM 2, LOW 0, NEEDS-INFO 6

Findings

  • HIGH §D · dnn-providers/hip-kernel-provider/rocke/library/tests/_compiler_test_utils.py:47 · Put the knob on the KernelSpec and delete the environment read.
  • MEDIUM §E · dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:58 · Replace "/opt/rocm/lib/libamd_comgr.so" in dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:58 and :404 with a synthetic path or relative path not matching /opt/, /home/, /tmp/, etc. (e.g., "lib/libamd_comgr.so").
  • MEDIUM §S · dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:43 · see the checklist item

Needs info

  • §C.8 The new tool and tests reuse existing platform helpers such as loaded_compiler_info, _require_compiler_info (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:483), and rocke_python_command / rocke_subprocess_env (dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:50). — no diff line supports this verdict
  • §F.6 Need evidence that pre-commit was run locally and passed before push; no such log or bot result is present in the provided diff.
  • §F.7 Need bot/Copilot review-thread status showing each thread has a fix or justified won't-fix, and ticket IDs for any deferred reviewer items; review-thread data is not provided.
  • §F.11 Need confirmation from gh api ... --jq .body or gh pr view --json body,title showing the PR body and title actually persisted via the API field; no such output is provided.
  • §G.3 Local test execution results for tools/run_checks.py and the platform/library pytest suites cannot be verified from the diff alone.
  • §S.8 Missing confirmation that the README entry points from rocke/platform were run after these runtime/compiler API changes, including the platform test commands and any applicable manifest/runtime verification command.

Section notes

  • §A: This PR validates COMGR -mllvm options in the Python runtime and compile helper before invoking COMGR; it does not modify IR definitions, lowering, kernel builders, selectors, or classic helpers with C++ twins.
  • §D: PR only touches runtime COMGR compiler-option validation and test skip utilities; no KernelSpec, dispatcher registration, routing, or selector changes.
  • §D: PR adds COMGR compiler-option validation and a measured option table in runtime/helpers; it does not touch dispatcher registration, routing, or spec selection.
  • §G: Items 1-17 of 34
  • §G: Items 18-34 of Section G graded.
  • §H: No device-side masks, bounds, indirection tables, or kernel definitions are added or modified in this PR; the changes are exclusively host-side COMGR -mllvm compiler option checks, reproducibility tooling, and test skip policies.

Graded with the rocKE PR checklist, one model pass per section. FAIL rows that sit on a diff line are inline comments.

§A graded 14/14
# Verdict Evidence
1 N/A No kernel-body emitters or instance builders are added or modified in this PR.
2 N/A No core lowering-op or op-handler files (core/lower_*, core/ir) are touched.
3 N/A No kernel-level selectors, spec builders, lowering, or codegen knobs affecting IR emission are modified.
4 N/A No IR-emitting code with a C++ twin is changed in this PR.
5 PASS No representative-IR golden files are modified in the diff, and IR lowering in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:189 is unchanged for accepted options.
6 N/A This PR does not change IR or lowering, so cross-engine IR parity gates are not implicated.
7 N/A No kernel IR emission or lowering logic is added or changed.
8 N/A No C++ engine or parity-relevant IR code is touched in this PR.
9 N/A No shared op-handler fix or lowerer arch divergence is introduced.
10 N/A No new IR-emitting feature or codegen flag changing emitted IR is introduced.
11 N/A No new IR op is added in this PR.
12 N/A core/backend.py and ROCKE_BACKEND=both lowering paths are not modified.
13 N/A Emitted .ll formatting and ROCKE_DEBUG_LOC handling are not touched.
14 N/A Only helpers/compile.py (Python COMGR compilation wrapper) is changed, which has no C++ helper twin under platform/cpp/helpers/.
§B graded 6/6
# Verdict Evidence
1 PASS The new compiler-option rejection is classified as a measured toolchain limitation (CompilerOptionNotAccepted) rather than an implementation shortcut (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:67-68, dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:292-300).
2 N/A This PR does not modify any spec constructors or preflight dispatch gates.
3 PASS When a compiler refuses -amdgpu-mfma-vgpr-form, compilation raises CompilerOptionNotAccepted instead of silently dropping the option and compiling a different kernel (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:24-26, dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:292-300).
4 PASS parse_table and _parse_answer strictly reject malformed or non-boolean JSON inputs (isinstance(answer.accepted, bool) so "false" is rejected rather than coerced) and require_options_accepted enforces validation at the lowest COMGR entry points (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:112-142, dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:866…
5 PASS The compiler-option gate is backed by measured toolchain data in compiler_options.json, verified across all entry points in test_compiler_options.py, and enforced in CI via rocke_compiler_options_reproducibility (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/data/compiler_options.json:1-27, dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:379-388…
6 N/A This PR does not add or modify any is_valid_spec or supports_* functions.
§C graded 10/10
# Verdict Evidence
1 PASS Repeated logic is consolidated into named helpers: accepted_comgr_options in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:561, require_options_accepted and comgr_child_env in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:469-510, and skip_where_compiler_refuses_option / require_tools_or_skip in…
2 N/A This PR does not add or modify kernel builders or build_<family> functions.
3 N/A This PR does not add or modify kernel builders or kernel_name() definitions.
4 N/A This PR touches platform runtime/compile helpers and library tests rather than instances/ or library/kernels/ arch-specific modules.
5 PASS All touched files are directly scoped to compiler option validation, its reproducibility CTest (dnn-providers/hip-kernel-provider/rocke/platform/CMakeLists.txt:621, dnn-providers/hip-kernel-provider/rocke/platform/tests/cmake/CoreTests.cmake:36), and test skip policy.
6 PASS Rather than keeping ad-hoc version checks (like the removed llvm_major < 21 check in dnn-providers/hip-kernel-provider/rocke/library/tests/test_mla_prefill_gfx942.py:457-461), the PR introduces a single measured data table (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:61) and shared child-env/skip helpers.
7 N/A This PR is a bug fix and compile-gate addition, not a large refactoring cleanup.
8 NEEDS-INFO The new tool and tests reuse existing platform helpers such as loaded_compiler_info, _require_compiler_info (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:483), and rocke_python_command / rocke_subprocess_env (dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:50). — no diff line supports this verdict
9 PASS The PR checks all COMGR compilation entry points (compile_kernel, compile_module, lower_kernel_for_comgr in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:189,364,477 and build_hsaco_from_llvm_ir, build_hsaco_from_llvm_modules in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:866,898), verifies them in…
10 N/A This PR does not widen a shared gate set or multi-field dispatch/spec structure.
§D graded 21/21
# Verdict Evidence
1 FAIL dnn-providers/hip-kernel-provider/rocke/library/tests/_compiler_test_utils.py:47 reads the environment
2 N/A PR does not add or modify any KernelCandidate or Capability definitions.
3 N/A PR does not add, modify, or call any candidate _supports or admits predicates.
4 N/A PR does not touch any dispatcher selectors or spec builders on the selection path.
5 N/A PR does not modify spec identity, kernel_name, or cache keys.
6 N/A PR does not introduce or modify cohort routing.
7 N/A PR does not add or modify selector heuristics or overrides.
8 N/A PR does not define or modify any routing cohorts.
9 N/A PR does not register any new kernel or architecture candidates.
10 N/A PR does not add any new spec fields or request features.
11 N/A PR does not register, promote, or change opt-in status of any dispatcher candidates.
12 N/A This PR does not add, rename, or reprioritize any dispatcher candidates or custom rankers.
13 N/A This PR does not touch CandidateRegistry.select, spec_id pinning, or dispatcher wrapper entry points.
14 N/A This PR does not modify KernelId, compile_key, selection_key, abi_version, or dispatcher caches.
15 N/A This PR does not add or modify any dispatcher candidate or arch module.
16 N/A This PR does not introduce a new operator family in the dispatcher.
17 N/A This PR does not add a dispatcher candidate or modify routing admission logic.
18 N/A This PR does not modify dispatcher registry ratchets, bind_torch requirements, or exempt sets.
19 N/A This PR makes no claims about unified attention dispatcher registration or routing.
20 N/A This PR does not modify any kernel-level selector, spec builder, or dispatcher registry/tuning layer.
21 N/A This PR checks COMGR backend option support in the runtime and does not alter spec selection or the IR emitter.
§E graded 17/17
# Verdict Evidence
1 PASS Platform modules (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:31-41) and tests (dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:23-47) do not import library, while library/tests/_compiler_test_utils.py:30 imports from rocke.runtime.
2 PASS No torch imports are added in library files (dnn-providers/hip-kernel-provider/rocke/library/tests/_compiler_test_utils.py:22-30).
3 PASS No new torch dependencies or imports are introduced across the PR (dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:23-47).
4 PASS Only library/tests/ files are touched under library/ (dnn-providers/hip-kernel-provider/rocke/library/tests/test_attention_bwd_compile.py:18-22), with no upward layering violations or changes to KNOWN_VIOLATIONS / KNOWN_BAD.
5 FAIL dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:58 and dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:404 contain the literal absolute path "/opt/rocm/lib/libamd_comgr.so" under platform/, which triggers the relative-path guard (_PATH_PATTERNS checks /opt/).
6 N/A Only platform/python/rocke/helpers/compile.py is modified under the listed trees, and it is a compilation wrapper rather than an IR emitter with DSL statement handles.
7 PASS Reference documentation is updated alongside the code in dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/reference/env_flags.md:178-185.
8 PASS No Jira/tracking IDs, reviewer handles, or PR numbers appear in committed code or comments (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:1-300); the ticket ID appears only in the PR body.
9 PASS All changes are tightly scoped to checking -mllvm compiler options against a measured table before invoking COMGR (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:469-484, dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:561-576).
10 PASS No files are split or moved, and modifications to shared test lists (dnn-providers/hip-kernel-provider/rocke/platform/CMakeLists.txt:518-522, dnn-providers/hip-kernel-provider/ROCKE_ENGINE_test_categories_external.yaml:171) are clean additive entries.
11 PASS All committed files are deliverable runtime modules, tools, reference docs, and tests (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/data/compiler_options.json:1-27, dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:1-147).
12 PASS The compiler option check in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:253-300 is architecture-independent and applies across all target architectures.
13 PASS Moving _require_tools_or_skip from dnn-providers/hip-kernel-provider/rocke/library/tests/test_mla_prefill_gfx942.py:43-50 to require_tools_or_skip in dnn-providers/hip-kernel-provider/rocke/library/tests/_compiler_test_utils.py:44 updates all call sites in the file (test_mla_prefill_gfx942.py:441,447,459,466,598,608,615).
14 PASS Removed lines in dnn-providers/hip-kernel-provider/rocke/library/tests/test_mla_prefill_gfx942.py:445-478 and dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:682-690 only replace the ad-hoc LLVM<21 skip and inline child env setup with the new shared helpers.
15 PASS Every parameter in new and modified functions is used in its body (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:84-263, dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:469-510).
16 N/A This item applies to review-report severity labeling rather than code changes in the diff.
17 PASS No absolute or relative performance measurements are added in the diff (dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/reference/env_flags.md:178-185) or PR description.
§F graded 11/11
# Verdict Evidence
1 PASS branch users/cgmillette/rocke/compiler-option-check uses the users// prefix
2 PASS title fix(rocke): never pass a compiler option the loaded compiler does not know matches type(scope): summary and is 74 characters
3 PASS description is at least 30 characters and contains an issue reference
4 PASS test file dnn-providers/hip-kernel-provider/rocke/library/tests/test_attention_bwd_agpr_probe.py is in the diff
5 PASS The added files shown in the diff are source, tests, docs, YAML, CMake, and JSON data such as dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/data/compiler_options.json:1 and dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:1, with no key/cert/secret file paths in the provided diff.
6 NEEDS-INFO Need evidence that pre-commit was run locally and passed before push; no such log or bot result is present in the provided diff.
7 NEEDS-INFO Need bot/Copilot review-thread status showing each thread has a fix or justified won't-fix, and ticket IDs for any deferred reviewer items; review-thread data is not provided.
8 N/A The PR targets develop rather than stacking on another feature branch, so the stacked-PR bot gap does not apply.
9 N/A There is no evidence in the provided PR data that the change was split or trimmed to satisfy a PR-size cap.
10 PASS The added tests exercise real compiler-option behavior and skip policy, as described in dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:4 and dnn-providers/hip-kernel-provider/rocke/library/tests/test_compiler_test_utils.py:4.
11 NEEDS-INFO Need confirmation from gh api ... --jq .body or gh pr view --json body,title showing the PR body and title actually persisted via the API field; no such output is provided.
§G graded 34/34
# Verdict Evidence
1 N/A This PR does not change kernel IR emission or claim correctness from cross-engine byte-identity.
2 N/A This PR adds a COMGR compiler-option acceptance check and does not add or modify a GPU kernel implementation.
3 NEEDS-INFO Local test execution results for tools/run_checks.py and the platform/library pytest suites cannot be verified from the diff alone.
4 PASS dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:380 tests that every public compile entry point raises CompilerOptionNotAccepted before reaching COMGR, which fails without the new check in compile.py and comgr.py.
5 PASS dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:319 and dnn-providers/hip-kernel-provider/rocke/library/tests/test_compiler_test_utils.py:68 drive public compile entry points and the shared test helper, with each test falsifiable by removing its corresponding guard.
6 PASS Prose claims in the PR body and docstrings match the implementation and tests, such as dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:253 and dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:85.
7 N/A This PR does not add or modify a kernel design document or fp32 parity reference oracle.
8 PASS dnn-providers/hip-kernel-provider/rocke/library/tests/_compiler_test_utils.py:44 and dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:292 loud-reject or explicitly skip rather than silently falling back.
9 N/A This PR does not introduce or modify LDS or occupancy resource numbers.
10 N/A This PR does not make kernel performance claims or add benchmark scripts.
11 N/A This PR does not add or optimize a dispatched kernel or modify kernel routing selectors.
12 PASS The C++ engine does not construct COMGR option lists, while all Python COMGR compilation entry points are covered in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:866 and :898 and dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:189, :364, and :477.
13 N/A This PR does not add, modify, or recommend any LLVM intrinsic symbols.
14 N/A This PR does not classify kernel hardware bottlenecks.
15 N/A The PR diff is provided directly in the review context from head commit 9151d94.
16 PASS All COMGR compilation entry points and test call sites are updated consistently across dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:469 and dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:561.
17 PASS The shipped tool dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:93 was executed to generate dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/data/compiler_options.json:1 and is wired into CTest in dnn-providers/hip-kernel-provider/rocke/platform/tests/cmake/CoreTests.cmake:36.
18 PASS The guard is wired into all public compile entry points (e.g. dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:189 and dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:866) and proven to fire on refused/unmeasured options as well as pass on accepted/non-mllvm options in dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:254-389.
19 PASS The test fixtures in dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:112-230 and dnn-providers/hip-kernel-provider/rocke/library/tests/test_compiler_test_utils.py:68-99 construct the exact malformed-table, column-drift, refused-option, and unmeasured-compiler inputs needed to trigger each gate's failure path.
20 N/A This PR does not add or modify a text detector for banned performance claims.
21 PASS The compiler-option probe tool dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:69-144 is a boolean deterministic capability probe (exit 0 with accepted vs Unknown command line argument refusal) that records exact verified_on and evidence fields into dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/data/compiler_options.json:1-27.
22 PASS dnn-providers/hip-kernel-provider/rocke/library/tests/_compiler_test_utils.py:44-49 fails with AssertionError instead of skipping when ROCKE_REQUIRE_COMPILER_TOOLS=1, and dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:108-112 exits with an error rather than skipping when COMGR is not queryable.
23 N/A This PR does not make performance or routing claims.
24 N/A This PR does not re-bless any IR or output golden files.
25 N/A This PR does not contain a codegen micro-optimization or alter emitted kernel ISA.
26 N/A This PR does not benchmark kernel performance.
27 N/A This PR does not perform a performance or routing A/B comparison.
28 N/A This PR does not report performance deltas or counterfactual benchmark arms.
29 N/A This PR does not swap guarded math primitives for fast primitives.
30 N/A This PR does not make register-spill or scratch-memory claims on kernel code objects.
31 N/A This PR does not claim a performance or resource improvement over a baseline kernel.
32 PASS Each measured compiler option entry in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/data/compiler_options.json:5-24 records verified_on (the exact LLVM version) and evidence, produced reproducibly by dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:114-128 and verified in CI via rocke_compiler_options_reproducibility…
33 N/A This PR does not change a kernel dispatch decision function, heuristic, or spec builder.
34 N/A This PR does not add or modify a kernel opt-in output flag.
§H graded 5/5
# Verdict Evidence
1 N/A This PR only adds host-side COMGR compiler option validation and test utilities, with no changes to kernel specs, buffer SRDs, or launch tensor bounds.
2 N/A This PR does not add or modify any kernel code, paged attention logic, or indirection table lookups.
3 N/A This PR does not introduce or modify any device-side loops, integer division/modulo operations, or kernel tile/page specs.
4 N/A This PR does not build or modify any device-side masks or bounds from runtime sequence length inputs.
5 N/A This PR does not touch kernel launch inputs or device-side bound/mask parameters.
§I graded 6/6
# Verdict Evidence
1 N/A This PR does not touch kernel routing, selection, or tuning search spaces.
2 PASS Instead of silently dropping -mllvm options or letting COMGR's LLVM option parser exit the host process (cryptic-fail), require_accepted loudly rejects unsupported or unmeasured options before COMGR is invoked (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:253).
3 N/A This PR does not define a shape or spec test-coverage suite combining curated and Hypothesis-generated cases.
4 N/A This PR does not add or modify performance benchmark or routing tables.
5 PASS The committed table dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/data/compiler_options.json:1 is explicitly documented as a measured snapshot with a regeneration script (dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:1) and a live CI drift check (dnn-providers/hip-kernel-provider/rocke/platform/tests/cmake/CoreTests.cmake:36).
6 N/A This PR does not introduce any DSL operations or hardware instruction primitives.
§J graded 7/7
# Verdict Evidence
1 N/A No mathematical or tensor equations appear in dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/reference/env_flags.md or elsewhere in the diff.
2 N/A No mathematical formulas, notation tables, or log/decay domain conversions are introduced in this PR.
3 N/A The paragraph added to dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/reference/env_flags.md links to rocke.runtime.compiler_options and does not restate any algorithm owned by another doc.
4 PASS dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/reference/env_flags.md:178 accurately describes the newly added CompilerOptionNotAccepted, rocke.runtime.compiler_options, tools/gen_compiler_options.py, and rocke_compiler_options_reproducibility introduced in this diff (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:67).
5 N/A The PR makes no algorithmic scaling or asymptotic complexity claims.
6 N/A No model tables, config.json dimensions, or tuned kernel dispatch band constants are added; the JSON table in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/data/compiler_options.json:1…
7 N/A No new Markdown design or algorithm document is created; only existing reference doc dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/reference/env_flags.md is updated.
§K graded 13/13
# Verdict Evidence
1 N/A The diff does not add or modify any tuned dispatch candidates or KnobSpace subclasses.
2 N/A The diff does not touch any kernel spec fields, KnobAxis declarations, or tuning classifications.
3 N/A The diff does not touch tuning/identity.py, config_key payloads, canonical-knob rules, or tuned defaults.
4 N/A The diff does not touch KnobSpace.defaults() or KnobSpace.recorded().
5 N/A The diff does not touch tuning/space.py or any knob drop/refuse rules.
6 N/A The diff does not touch KnobSpace.validate or policy-free spec construction in tuning_specs.py.
7 N/A The diff does not touch KnobSpace.base(req) or tuning problem normalization.
8 N/A The diff does not touch normalize_knobs or knob value coercion.
9 N/A The diff does not touch benchmark recording, replay, spec_hash verification, or PinRefused handling.
10 N/A The diff does not touch KnobSpace.find or bare tuning_id resolution.
11 N/A The diff does not touch any sweep entry points or core.pin_to_spec.
12 N/A The diff does not touch production stacks, accept_default, or any variant's base spec.
13 N/A The diff does not touch any benchmark sweep harnesses or auto selection ranking.
§L graded 10/10
# Verdict Evidence
1 N/A The diff does not add or modify any gfx architecture target or ISA backend.
2 N/A The diff does not add or modify any MMA op_id or fragment layout table.
3 N/A The diff does not modify MMA selection, catalog lookups, or MMA lowering.
4 N/A The diff does not touch wave-size queries or lane math.
5 N/A The diff does not add or modify arch-specific kernel recipes, transpose-LDS reads, or tile loaders.
6 N/A The diff does not modify pipeline policies, ArchTarget, or dispatcher imports.
7 N/A The diff does not add or change logical dtypes, bit packing, instruction fragments, or FP8 dialects.
8 N/A The diff does not touch scaled-MMA contracts or MfmaAtom.emit_scaled.
9 N/A The diff does not add a new architecture target.
10 N/A The diff does not modify intrinsic_arch_domain..json artifacts or LLVM flavor membership lists.
§M graded 10/10
# Verdict Evidence
1 N/A The diff does not author or modify DSL kernel bodies or builder control-flow ops.
2 N/A The diff does not touch legalize_for_target or target legalization passes.
3 N/A The diff does not touch ck.dsl.ir/v1 serialization in Python or C++.
4 N/A The diff does not touch portable IR schemas, importers, recorders, or magic division helpers.
5 N/A The diff does not touch AsyncTileLoader, LdsLayout, or async DRAM-to-LDS kernel code.
6 N/A The diff does not emit barriers, device memory loads, or cross-lane shuffles.
7 N/A The diff does not touch ISABackend buffer resource word 3 or temporal cache hints.
8 N/A The diff does not add intrinsic declarations or modify lowering engine selection precedence.
9 N/A The diff does not modify C++ public entry points or headers under cpp/include/rocke/.
10 N/A The diff does not touch LLVM debug info emission or DWARF consumers.
§N graded 18/18
# Verdict Evidence
1 PASS dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:561 adds accepted_comgr_options in the existing host compile helper module (used by compile_kernel, lower_kernel_for_comgr, and cross-family compile tests) and emits no SSA IR.
2 N/A The diff does not add or modify kernel builders, MFMA output mappings, TensorView transforms, loaders, epilogues, or helpers/transforms.py.
3 N/A The diff does not touch SoftwarePipeline or overlap_vmcnt.
4 N/A The diff does not add or modify fused kernels, EpilogueOp, or fusion validation.
5 N/A The diff does not add or modify anything under rocke.helpers.tiling.
6 N/A The diff does not touch front-door or toolbox tiling helpers or LayoutStyle.
7 N/A The diff does not touch tiling layout overrides or MMA validation.
8 N/A The diff does not touch tiling LDS allocation, lds_store/lds_read, or global load predication.
9 N/A The diff does not touch N-D tensor axis roles, batch reductions, or label flow in tiling.
10 N/A The diff does not touch tiling emit, LDS staging, or MMA issue.
11 N/A The diff does not touch tiling MMA traits, lds_swizzle, or derive_sched_group_counts.
12 N/A The diff makes no LDS bank-conflict or memory coalescing claims.
13 N/A The diff does not touch WarpDistributionEncoding, make_tile_desc, RegisterMapper, or store casts.
14 N/A The diff does not touch tiling data-movement verbs, LDS buffering, or pipeline synchronization.
15 N/A The diff does not touch tiling allocation or bounds checks.
16 N/A The diff does not touch optimize_layout or evaluate_transform.
17 N/A The diff does not touch the tiling recorder, round-trip analysis, or visualization.
18 N/A The diff does not touch interleaved layouts or tiling demo kernels.
§O graded 16/16
# Verdict Evidence
1 N/A This PR does not touch IR verification (core/verify.py), address spaces, or required attributes.
2 N/A This PR does not construct or modify scf.for ops, regions, or terminators.
3 N/A This PR does not modify IR emission, SSA naming, intrinsic declaration tables, or metadata slots.
4 N/A This PR does not touch IR type definitions, comparisons, or SmemType.
5 N/A This PR does not modify optimize_kernel, canonicalize_region, core/passes.py, or op purity tables.
6 N/A This PR does not touch constant folding passes in either Python or C++.
7 PASS In dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:544, _comgr_options_for_kernel preserves the coupling of zero agpr_alloc and mfma_vgpr_form to -mllvm -amdgpu-mfma-vgpr-form, while accepted_comgr_options validates that the loaded compiler supports it.
8 N/A This PR does not modify CodegenPolicy, SchedulerStrategy, or scheduler strategy attributes.
9 N/A This PR does not touch waitcnt encoding, barriers, or optimization_barrier lowering.
10 N/A This PR does not modify LLVM lowerer attribute validation or parameter lowering.
11 N/A This PR does not touch ArchTarget queries or target ID resolution.
12 N/A This PR does not introduce arch defaults or hard-coded vector/dword sizes in codegen.
13 N/A This PR does not modify any C structs, enums, or ABI headers under cpp/include/rocke/.
14 N/A This PR does not add or modify C++ source files, headers, or compiler warning flags in platform/CMakeLists.txt.
15 N/A This PR does not touch C++ engine fallback behavior or ROCKE_CPP_QUIET_FALLBACK.
16 N/A This PR does not alter C++ library boundaries or claims documented in known_gaps.md.
§P graded 14/14
# Verdict Evidence
1 N/A The diff does not add or change platform instance builders or builder signatures.
2 N/A The diff does not change kernarg signatures, manifests' args_signature, GEMM ABI, or conv ABI.
3 N/A The diff does not change launch geometry, grid computation, chiplet swizzle, or harness grid handling.
4 N/A The diff does not change LDS allocation gates or C++ validators for LDS capacity.
5 N/A The diff does not change narrow GEMM/RDNA/WMMA paths or MultiD/MultiAbd validation.
6 N/A The diff does not add or change caller-zeroed workspaces for Stream-K, MoE, fused MoE, or split-K.
7 N/A The diff does not change Stream-K, persistent, grouped GEMM, or support-matrix limits.
8 N/A The diff does not change quantization helpers, MX GEMM constraints, or MX scale decode.
9 N/A The diff does not add or change numeric emulation or decode paths.
10 N/A The diff does not add a runtime manifest kind or change manifest schema consumers.
11 N/A The diff does not change kernel launch lifetime, stream synchronization, PipelineLauncher behavior, WorkspacePool use, or launch timing code.
12 PASS COMGR child processes are pinned to the COMGR already loaded in the parent by deriving the library from loaded_compiler_info and setting ROCKE_COMGR_LIB plus AMD_COMGR_CACHE=0 at dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:493 and dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:505; COMGR options are refused before COMGR sees them at…
13 N/A The diff does not refactor grid, tile-count, workspace math, ceil_div/align_up, kernel-cache keys, or operator configuration admission guards that affect host launch sizing.
14 N/A The diff does not change examples, expected.json gates, performance lower bounds, or new kernel logic placement.
§Q graded 7/7
# Verdict Evidence
1 PASS New tests and the rocke_compiler_options_reproducibility CTest are registered in the installed layout (dnn-providers/hip-kernel-provider/rocke/platform/CMakeLists.txt:518-521, dnn-providers/hip-kernel-provider/rocke/platform/CMakeLists.txt:621-627, dnn-providers/hip-kernel-provider/rocke/platform/CMakeLists.txt:850-851), the build tree (dnn-providers/hip-kernel-provider/rocke/platform/tests/cmake/CoreTests.cmake:36-46), and external…
2 N/A This PR does not add any new kernel family, variant, or emitted IR change.
3 PASS The new reproducibility gate (dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:90-140) and skip helper (dnn-providers/hip-kernel-provider/rocke/library/tests/_compiler_test_utils.py:44-49) are properly scoped and fail rather than skip when tools are required or unqueryable.
4 N/A This PR does not modify IR goldens or change the lowerer or builder valid spec space.
5 PASS Tests avoid hard-coded LLVM version checks in dnn-providers/hip-kernel-provider/rocke/library/tests/test_mla_prefill_gfx942.py:444-457, skip cleanly when COMGR is unavailable (dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:394-398), and test rejection paths with strictly validated negative inputs (dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:112-188).
6 PASS The option table is cached via @​functools.cache (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:236-238) and option lists without -mllvm bypass COMGR queries altogether (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:480-482), adding no per-call overhead.
7 N/A This PR checks COMGR backend options in the Python runtime wrapper rather than altering the Python/C++ engine lowering rejection contract.
§R graded 11/11
# Verdict Evidence
1 N/A The diff changes no kernel body, algorithm, tiling strategy, pipeline variant, or performance lever.
2 N/A The diff changes no performance lever or architecture-specific kernel recipe.
3 N/A The diff touches no GEMM, LDS, prefetch, ISA, or launch optimization lever covered by the utilities skills.
4 N/A The diff does not change emitted IR, lowering, or codegen options for any accepted compile.
5 N/A The diff adds or modifies no global memory accesses in any kernel.
6 N/A The diff makes no LDS or MMA layout changes.
7 N/A The diff makes no kernel performance bottleneck claims or fixes.
8 N/A The diff does not modify waits, barriers, or scheduling hints in any kernel.
9 N/A The diff is not a performance regression fix and introduces no shape-trading performance lever.
10 N/A The diff makes no kernel codegen changes, assembly parity claims, or benchmark comparisons.
11 N/A The diff introduces no GPU kernel optimization lesson or DSL optimization probe (tools/gen_compiler_options.py is a toolchain reproducibility gate documented in dsl_docs/reference/env_flags.md).
§S graded 8/8
# Verdict Evidence
1 PASS New public compiler-option symbols are declared in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:43 and exercised by behavioral tests such as dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:72; accepted_comgr_options is added at dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:558 and covered in the public-entry-point matrix at…
2 PASS Existing compile APIs document and implement the new refusal behavior at dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:179 and dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:859, and the same PR updates tests to assert each affected entry point raises before COMGR at dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:311 and…
3 N/A The diff adds compiler-option APIs and changes behavior but does not remove or rename a public API.
4 FAIL The PR adds all entries in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:43 and edits the runtime package all in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/init.py:124, but the added test only checks the new runtime symbol specifically at dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:223 rather than importing or resolving every listed name.
5 PASS The tests assert behavior and negative contracts, including exact exception type and message contents for refused options at dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:233 and entry-point refusal details at dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:365.
6 PASS The new runtime tests state that most checks need no COMGR and run in-process at dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:4, while COMGR-risking checks run in a child process at dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:12; the reproducibility CTest is labeled host/toolchain/standard at dnn-providers/hip-kernel-provider/rocke/platform/CMakeLists.txt:623.
7 N/A This PR changes Python runtime/compiler-option checks and host tests, not a lowering/spec-builder/selector/verifier pass, C entry point, emitted IR, or other API with a C++ twin.
8 NEEDS-INFO Missing confirmation that the README entry points from rocke/platform were run after these runtime/compiler API changes, including the platform test commands and any applicable manifest/runtime verification command.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Embedded NULs allow backend options to bypass validation and reach COMGR’s process-exiting parser.

2 open issues
1 resolved since last review

Review effort: Max

cgmillette and others added 5 commits November 7, 2026 08:28
… know

A kernel that asks for VGPR-form MFMAs (the mfma_vgpr_form attribute, or
zero allocated AGPRs) is compiled with -mllvm -amdgpu-mfma-vgpr-form. The
compile path passed that option to COMGR without asking whether the loaded
compiler knows it. LLVM 20 does not, and LLVM's option parser does not
return an error for an unknown option: it exits the whole host process.

Which compiler accepts which backend option is now measured data with one
home. tools/gen_compiler_options.py compiles an empty kernel through the
loaded COMGR in a child process, once per option, and records the answer
in runtime/data/compiler_options.json, one column per LLVM major version.
The committed table was measured on LLVM 20 (refused) and LLVM 22, 23 and
24 (accepted). rocke.runtime.compiler_options is its only reader.

compile_kernel, compile_module and lower_kernel_for_comgr check a kernel's
-mllvm options against the table before lowering, and
build_hsaco_from_llvm_ir / build_hsaco_from_llvm_modules check every
caller's options before COMGR is called. A kernel that needs an option the
loaded compiler refuses raises CompilerOptionNotAccepted, a ValueError
naming the kernel, the option and the compiler. A compiler or an option
with no measured answer raises ValueError, since a wrong guess ends the
process.

accepted_comgr_options, public in rocke.helpers.compile, is that check:
it returns the options a kernel compiles with, or raises as above.

The option is not dropped on a compiler that lacks it. It is part of the
kernel: on LLVM 20 a zero-AGPR kernel compiled without it still uses AGPRs
on some targets, so dropping it would compile a different kernel from the
one asked for.

The gfx942 MLA prefill occupancy test skips on that error instead of on a
hard-coded LLVM version, and the backward compile and AGPR probe tests that
build such kernels (through accepted_comgr_options) skip where the loaded compiler refuses the option. All
three use skip_where_compiler_refuses_option from a new test-support module,
library/tests/_compiler_test_utils.py. It applies require_tools_or_skip,
which moves there from the MLA prefill test: the test skips, except under
ROCKE_REQUIRE_COMPILER_TOOLS=1, where it fails. Any other error propagates;
test_compiler_test_utils.py checks all three cases.

The MLA prefill test now checks that a queryable COMGR is loaded through
the public loaded_compiler_info, and no longer looks for an IR "vintage
mismatch" message, which COMGR does not raise any more.

Co-Authored-By: Claude <noreply@anthropic.com>
The committed column for the loaded compiler is now re-measured by a CTest,
rocke_compiler_options_reproducibility (gen_compiler_options.py --check),
registered in the build tree and in the installed test manifest with the
standard label. It fails when the column differs from the measurement or is
missing. The source-tree-only pytest that ran the same check, and skipped in
the installed layout, is removed.

Every public compile entry point (compile_kernel, compile_module,
lower_kernel_for_comgr, accepted_comgr_options, and build_hsaco_from_llvm_ir
in process, in a fresh process and with the joined -mllvm= spelling, and
build_hsaco_from_llvm_modules) now has an in-process test that pins the loaded
compiler's identity to one the table records as refusing the option and
expects CompilerOptionNotAccepted naming what is compiled. These tests need no
COMGR. test_compiler_test_utils.py joins the MLA ctest entry, which sets
ROCKE_REQUIRE_COMPILER_TOOLS=1, so its fail-instead-of-skip case runs there.

compiler_options is the only home of the table's format: parse_table reads
it and checks every cell's types (a string "false" is refused rather than
read as true), format_table writes it, and measured_answers returns every
answer. The tool and the tests use these instead of reading the JSON.
CompilerOptionNotAccepted is exported from rocke.runtime.

comgr_child_env in runtime/comgr.py builds the environment for a child process
that must compile with this process's COMGR (ROCKE_COMGR_LIB and
AMD_COMGR_CACHE=0). The fresh-process compile, the tool and the tests use it,
and the tool and the tests start their child Pythons with rocke_python_command
and rocke_subprocess_env.

Co-Authored-By: Claude <noreply@anthropic.com>
The reproducibility gate's failing branch had no test. The drift
comparison moves out of tools/gen_compiler_options.py into
rocke.runtime.compiler_options.column_drift, which --check uses, with
tests for a column that reproduces, a changed answer and a missing one.

Table errors now name the file they came from and the exact keys that are
wrong (missing, unexpected, or not an LLVM major). The child-process
environment test pins the loaded compiler instead of deriving its
expectation from the same call it checks, and covers the fallback to the
requested library name.

Co-Authored-By: Claude <noreply@anthropic.com>
parse_table let a JSON syntax error escape as a bare JSONDecodeError that
named a line and column but not the file. It now raises ValueError naming
the source, like every other table error.

Tests now cover that error and the three refusals of a list where the
table needs an object (the options, an option's column, one answer), and
assert that both COMGR-only CTest gates get PYTHONPATH and nothing else,
not only the same environment as each other.

Co-Authored-By: Claude <noreply@anthropic.com>
… an unqueryable compiler

COMGR reads each option as a C string, so an option with a NUL character
reaches it cut short: "-mllvm\0" arrives as "-mllvm" and hands the next
option to LLVM's parser, which exits the process on an option it does not
know. backend_options now refuses any option list containing a NUL, so the
names it reads are the ones COMGR would see, and every COMGR entry point
refuses such a list before COMGR is called.

When a compile needs a -mllvm option and no COMGR with a queryable LLVM
version is loaded, require_options_accepted raised the LLVM flavor error,
whose advice (set an explicit flavor) cannot answer whether the option is
accepted. It now raises ComgrError naming what is compiled and the options,
and points at loading a queryable COMGR.

Tests: the NUL refusal in backend_options and in the three COMGR entry
points; the ComgrError for a missing COMGR and for an unknown LLVM version;
a malformed table whose evidence is not a string; and every name in
__all__ of rocke.runtime and rocke.runtime.compiler_options resolves.

Co-Authored-By: Claude <noreply@anthropic.com>
@cgmillette
cgmillette force-pushed the users/cgmillette/rocke/compiler-option-check branch from 9151d94 to 4015706 Compare November 7, 2026 13:49
@cgmillette

Copy link
Copy Markdown
Contributor Author

Rebased onto current develop and addressed the open review items in one new commit.

  • The rebase had one conflict, in rocke/runtime/__init__.py: develop's new contract imports and this PR's CompilerOptionNotAccepted import are both kept. No emitted IR changes.
  • An option list with a NUL character is now refused before COMGR is called (backend_options), because COMGR reads each option as a C string and would lose the text after the NUL. Tests cover backend_options and the three COMGR entry points.
  • When a compile needs a -mllvm option and no COMGR with a queryable LLVM version is loaded, the error is now a ComgrError that names what is compiled and the options and says to load a queryable COMGR. It no longer points at setting an LLVM flavor, which cannot answer the question.
  • Added the missing evidence type-check case and a test that every __all__ name in rocke.runtime and rocke.runtime.compiler_options resolves.
  • Two review comments are answered in their threads without a code change: the environment read in the library test helper is the existing test-harness switch, not a kernel knob, and the /opt/rocm literal in the runtime test is a synthetic loader path that the relative-path guard does not flag.

Build, ctest (including rocke_compiler_options_reproducibility), the touched platform and library tests with both engines in strict mode, the full platform suite and pre-commit pass on the new head.

@cgmillette

Copy link
Copy Markdown
Contributor Author

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

PR #14218 — never pass a compiler option the loaded compiler does not know (cgmillette, AICK-2714) — 🟡 Follow-up: N6 fixed; one test nit

Prior item Status
Our 11-05 N6: evidence type check untested 🟢 FIXED 40157066
• new evidence_not_str case in the malformed-table test (ran)
• with the check removed, exactly that case fails (ran)
Our 11-04 N5: table errors, gate env partly tested 🟢 FIXED 5281e4da
• the rebase replays the fix unchanged (ran)
Our 11-04 N4: 14502 copies this skip helper 🟢 DECIDED
• owner: this PR's module; nothing to change here
• 14502 unchanged at a3347689: still keeps its copy (read)
Copilot (11-06) a NUL in an option skips the check 🟢 FIXED 40157066
• backend_options refuses any option holding a NUL (ran)
• every COMGR option path calls it first; the C++ engine passes no options (read)
• with the refusal removed, its 5 cases fail (ran)
Copilot (11-06) unqueryable COMGR: wrong error and advice 🟢 FIXED 40157066
• now a ComgrError naming the options and ROCKE_COMGR_LIB (read)
• with the new branch removed, its 8 cases fail (ran)
yraparti (11-06) §D.1 environment read in a test helper 🟢 ANSWERED
• the test harness's switch, already read on develop; no kernel knob (read)
• agreed: nothing to change
yraparti (11-06) §E.5 /opt/rocm literal in a test 🟢 ANSWERED
• the relative-path guard has no /opt/ pattern (read)
• a synthetic loader path, never opened; agreed
yraparti (11-06) §S.4 __all__ names untested 🟢 FIXED 40157066
• test_every_exported_name_resolves covers both edited modules (ran)
Approvals: CongMa13 (10-23) afd9e722; amd-khushbu (10-26) 5fe79de0 🟢 APPROVED afd9e722, 5fe79de0
• older heads; still count on develop
• neither covers the 10-27 – 11-07 API, CTest, tier, NUL and error changes
Bots / CI 🟡 Running, infra
• fails: Linux gfx94X rocKE shard 2/2 — Test step time limit on the slower runner pool; not the PR's (read)
• option gate and evidence_not_str ✓ in that shard; shard 1/2 ✓ (read)
• running: 4 Windows gfx110X test jobs; merge blocked until shard 2/2 re-runs
Our checks 🟢 Clean
• merge vs 21e3ca43: clean, 0 behind (ran)
• rebase: only runtime/__init__.py conflicted; both imports kept (ran)
• build + ctest 60 ✓ / 1 skip; golden 0 changed ×4 + canonical (ran)
• PR's tests 354 ✓ / 38 skip; installed 124 ✓; rules 1–3 clean (ran)
Code quality 🟡 Minor
• NUL rule and unqueryable message: one home each (read)
• each new claim's test fails on its mutant (ran)
• the unqueryable test keeps a second entry-point table: N7 (ran)
# Sev file:line Finding
N7 🟡 platform/tests/runtime/test_compiler_options.py:470 The unqueryable-compiler test keeps a second entry-point table
• impact: _UNQUERYABLE_CALLS covers 4 of the 8 in _ENTRY_POINTS; a new entry point misses the case (read)
• fix: parametrize the test over _ENTRY_POINTS, drop _UNQUERYABLE_CALLS; all 16 cases pass at head (ran)

Utility: 🟢 Positive — refuses options the loaded compiler lacks instead of ending the process.


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: the rebase, merge with develop, build + ctest, differential golden, touched tests, an installed layout, mutations of the new checks, CI at head, code quality. Flags whether a closer look is needed; not an approval or rejection.

@yraparti yraparti left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rocKE checklist re-review of the fixes since 9151d9415132

ROCm/rocm-libraries#14218 · 4015706694f1 · model gemini-4-argon, fallback gpt-5.5

PASS · graded 238/238 · BLOCKER 0, HIGH 1, MEDIUM 1, LOW 0, NEEDS-INFO 5

Since the last review (9151d9415132)

Resolved 1 · still open 2 · new 0

  • ✅ §S.4 was FAIL, now PASS
  • ⏳ §D.1 still FAIL (HIGH): Put the knob on the KernelSpec and delete the environment read.
  • ⏳ §E.5 still FAIL (MEDIUM): Replace "/opt/rocm/lib/libamd_comgr.so" in dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:59 and :522 with a synthetic path or relative path not matching /opt/, /home/, /tmp/, etc. (e.g., "lib/libamd_comgr.so").

Findings

  • HIGH §D · dnn-providers/hip-kernel-provider/rocke/library/tests/_compiler_test_utils.py:47 · Put the knob on the KernelSpec and delete the environment read.
  • MEDIUM §E · dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:59 · Replace "/opt/rocm/lib/libamd_comgr.so" in dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:59 and :522 with a synthetic path or relative path not matching /opt/, /home/, /tmp/, etc. (e.g., "lib/libamd_comgr.so").

Needs info

  • §F.6 Still unresolved at current head: no local pre-commit execution output or CI formatting check status is included in the provided PR data.
  • §F.7 Still unresolved at current head: GitHub review threads, bot/Copilot comments, and resolution/ticket statuses are not included in the provided PR context.
  • §F.11 Still unresolved at current head: although the prompt contains a rendered title and body (which appears truncated at the end of Test Plan), there is no direct gh api ... --jq .body or gh pr view --json body,title verification output provided.
  • §G.3 Local test execution results for tools/run_checks.py and the platform/library pytest suites remain unverifiable from the diff alone.
  • §S.8 Still unresolved from previous review: although dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py is added and mentioned in the PR body's Test Plan, the PR description still does not confirm running the rocke/platform/README.md test entry points (PYTHONDONTWRITEBYTECODE=1 PYTHONPATH=python:../library python tests/test_rocke.py and python tests/run_all.py, plus python -m rocke.run_manifest <hsaco> manifest.json --verify if applicable for runtime changes).

Section notes

  • §A: Most Section A parity items are not implicated because this PR adds COMGR compiler-option validation, measurement data, documentation, and tests rather than changing kernel IR emission or C++/Python engine parity code.
  • §D: PR adds COMGR compiler-option validation and test skip utilities; it does not touch dispatcher registration, routing, selectors, or KernelSpec definitions.
  • §D: This PR checks COMGR backend option acceptance in the runtime/compile helpers and updates library compile tests; it does not touch dispatcher registration, routing, candidates, or spec selection.
  • §H: No device-side masks, bounds, indirection tables, or kernel launch inputs are added or modified in this PR; the diff only touches host-side COMGR -mllvm compiler option validation (rocke/runtime/compiler_options.py, rocke/runtime/comgr.py, rocke/helpers/compile.py), measurement tooling, and compile test skip utilities.
  • §J: Shapes: | Equation / Op | Operands | Inner Dimensions | Result vs Declared | |---|---|---|---| | (none) | — | — | No equations in env_flags.md. | Symbols: | Symbol | Defined At | Domain | |---|---|---| | CompilerOptionNotAccepted | env_flags.md:182 (rocke.runtime) | Python exception (ValueError subclass) | | rocke.runtime.compiler_options | env_flags.md:183 | Python module | | tools/gen_compiler_options.py | env_flags.md:184 | CLI measurement tool | | rocke_compiler_options_reproducibility | env_flags.md:185 | CTest target | Novelty: | Claim | Symbol / Path | Status | |---|---|---| | Measured -mllvm option table and refusal exception | CompilerOptionNotAccepted, rocke.runtime.compiler_options, tools/gen_compiler_options.py, rocke_compiler_options_reproducibility | Added in this PR |
  • §K: This PR adds COMGR compiler option validation (rocke/runtime/compiler_options.py) and does not touch dispatch tuning, KnobSpace subclasses, knob axes, sweep entry points, or benchmark replay.

Graded with the rocKE PR checklist, one model pass per section. FAIL rows that sit on a diff line are inline comments.

§A graded 14/14
# Verdict Evidence
1 N/A No kernel-body emitters, instance builders, or Python-only kernel families are added or modified.
2 N/A No core lowering-op or op-handler files under core/lower_* or core/ir are modified.
3 N/A No kernel-level selectors, spec builders, lowering rules, or IR-affecting codegen knobs are modified.
4 N/A No IR-emitting Python code with a C++ twin is changed, so emit-order parity is not implicated.
5 PASS The compile path adds option validation before lowering at dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:189 and passes the checked options to COMGR at dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:205, so the diff does not show an intentional IR-emission change requiring representative-IR re-blessing.
6 N/A The PR does not rely on golden-file changes as parity proof because it does not change emitted IR or lowering.
7 N/A The new logic validates COMGR options before compilation and does not add an unconditional emitted operation on a kernel IR hot path.
8 N/A No C++ engine code or C++ parity-relevant IR path is touched.
9 N/A No shared op-handler fix or lowerer-only architecture divergence is introduced.
10 N/A No new IR-emitting feature or flag-on IR behavior is introduced, so no new golden hash entry is required.
11 N/A No new IR operation is added.
12 N/A ROCKE_BACKEND=both behavior and core/backend.py are not modified.
13 N/A The diff does not modify emitted .ll stamping, debug-location emission, or ROCKE_DEBUG_LOC handling.
14 N/A The changed Python helper compile.py is not a classic helper with a C++ twin under platform/cpp/helpers and cpp/include/rocke/helper_rocke.helpers.
§B graded 6/6
# Verdict Evidence
1 PASS The new compiler-option rejection is classified as a measured toolchain limitation (CompilerOptionNotAccepted) rather than an implementation shortcut (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:67-68, dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:303-315).
2 N/A This PR does not modify any spec constructors or preflight dispatch gates.
3 PASS When a compiler refuses -amdgpu-mfma-vgpr-form, compilation raises CompilerOptionNotAccepted instead of silently dropping the option and compiling a different kernel (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:24-28, dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:303-315).
4 PASS backend_options, parse_table, and _parse_answer strictly reject NUL characters and malformed or non-boolean JSON inputs (isinstance(answer.accepted, bool) so "false" is rejected rather than coerced), and require_options_accepted enforces validation at the lowest COMGR entry points (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:99-105…
5 PASS The compiler-option gate is backed by measured toolchain data in compiler_options.json, verified across all entry points in test_compiler_options.py, and enforced in CI via rocke_compiler_options_reproducibility (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/data/compiler_options.json:1-27, dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:419-428…
6 N/A This PR does not add or modify any is_valid_spec or supports_* functions.
§C graded 10/10
# Verdict Evidence
1 PASS Repeated logic is consolidated into named helpers: accepted_comgr_options in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:561, require_options_accepted and comgr_child_env in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:473-529, and require_tools_or_skip / skip_where_compiler_refuses_option in…
2 N/A This PR does not add or modify kernel builders or build_<family> functions.
3 N/A This PR does not add or modify kernel builders or kernel_name() definitions.
4 N/A This PR touches platform runtime/compile helpers and library tests rather than instances/ or library/kernels/ arch-specific modules.
5 PASS All touched files are directly scoped to compiler option validation (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:1), its reproducibility CTest (dnn-providers/hip-kernel-provider/rocke/platform/CMakeLists.txt:624, dnn-providers/hip-kernel-provider/rocke/platform/tests/cmake/CoreTests.cmake:36), and test skip policy.
6 PASS Rather than keeping ad-hoc version checks (like the removed llvm_major < 21 check in dnn-providers/hip-kernel-provider/rocke/library/tests/test_mla_prefill_gfx942.py:457-461), the PR introduces a single measured data table (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:61) and shared child-env/skip helpers.
7 N/A This PR is a bug fix and compile-gate addition, not a large refactoring cleanup.
8 PASS Resolved from previous review with exact diff citations: the new helpers, tool, and tests reuse existing develop primitives including loaded_compiler_info (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:463), supported_isas (dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:60), and rocke_python_command / rocke_subprocess_env…
9 PASS The PR checks all COMGR compilation entry points (compile_kernel, compile_module, lower_kernel_for_comgr in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:189,364,477 and build_hsaco_from_llvm_ir, build_hsaco_from_llvm_modules in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:885,917), verifies them in…
10 N/A This PR does not widen a shared gate set or multi-field dispatch/spec structure.
§D graded 21/21
# Verdict Evidence
1 FAIL dnn-providers/hip-kernel-provider/rocke/library/tests/_compiler_test_utils.py:47 reads the environment
2 N/A PR does not add or modify any KernelCandidate or Capability definitions.
3 N/A PR does not add, modify, or call any candidate _supports or admits predicates.
4 N/A PR does not touch any dispatcher selectors or spec builders on the selection path.
5 N/A PR does not modify spec identity, kernel_name, or cache keys.
6 N/A PR does not introduce or modify cohort routing.
7 N/A PR does not add or modify selector heuristics or overrides.
8 N/A PR does not define or modify any routing cohorts.
9 N/A PR does not register any new kernel or architecture candidates.
10 N/A PR does not add any new spec fields or request features.
11 N/A PR does not register, promote, or change opt-in status of any dispatcher candidates.
12 N/A This PR does not add, rename, or reprioritize any dispatcher candidates or custom rankers.
13 N/A This PR does not touch CandidateRegistry.select, spec_id pinning, or dispatcher wrapper entry points.
14 N/A This PR does not modify KernelId, compile_key, selection_key, abi_version, or dispatcher caches.
15 N/A This PR does not add or modify any dispatcher candidate or arch module.
16 N/A This PR does not introduce a new operator family in the dispatcher.
17 N/A This PR does not add a dispatcher candidate or modify routing admission logic.
18 N/A This PR does not modify dispatcher registry ratchets, bind_torch requirements, or exempt sets.
19 N/A This PR makes no claims about unified attention dispatcher registration or routing.
20 N/A This PR does not modify any kernel-level selector, spec builder, or dispatcher registry/tuning layer.
21 N/A This PR checks COMGR backend option support in the runtime and does not alter spec selection or the IR emitter.
§E graded 17/17
# Verdict Evidence
1 PASS Platform modules (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:31-41) and tests (dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:23-47) do not import library, while dnn-providers/hip-kernel-provider/rocke/library/tests/_compiler_test_utils.py:30 imports from rocke.runtime.
2 PASS No torch imports are added in library files (dnn-providers/hip-kernel-provider/rocke/library/tests/_compiler_test_utils.py:22-30).
3 PASS No new torch dependencies or imports are introduced across the PR (dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:23-47).
4 PASS Only library/tests/ files are touched under library/ (dnn-providers/hip-kernel-provider/rocke/library/tests/test_attention_bwd_compile.py:18-22), with no upward layering violations or changes to KNOWN_VIOLATIONS / KNOWN_BAD.
5 FAIL Not resolved since the previous review: dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:59 and dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:522 still contain the literal absolute path "/opt/rocm/lib/libamd_comgr.so" under platform/, which triggers the relative-path guard (_PATH_PATTERNS checks /opt/).
6 N/A Only platform/python/rocke/helpers/compile.py is modified under the listed trees, and it is a compilation wrapper rather than an IR emitter with DSL statement handles.
7 PASS Reference documentation is updated alongside the code in dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/reference/env_flags.md:178-185.
8 PASS No Jira/tracking IDs, reviewer handles, or PR numbers appear in committed code or comments (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:1-315); the ticket ID appears only in the PR body.
9 PASS All changes are tightly scoped to checking -mllvm compiler options against a measured table before invoking COMGR (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:473-502, dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:561-576).
10 PASS No files are split or moved, and modifications to shared test lists (dnn-providers/hip-kernel-provider/rocke/platform/CMakeLists.txt:520-523, dnn-providers/hip-kernel-provider/ROCKE_ENGINE_test_categories_external.yaml:199) are clean additive entries.
11 PASS All committed files are deliverable runtime modules, tools, reference docs, and tests (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/data/compiler_options.json:1-27, dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:1-147).
12 PASS The compiler option check in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:265-315 is architecture-independent and applies across all target architectures.
13 PASS Moving _require_tools_or_skip from dnn-providers/hip-kernel-provider/rocke/library/tests/test_mla_prefill_gfx942.py:43-50 to require_tools_or_skip in dnn-providers/hip-kernel-provider/rocke/library/tests/_compiler_test_utils.py:44 updates all call sites in the file (test_mla_prefill_gfx942.py:441,447,459,466,598,608,615).
14 PASS Removed lines in dnn-providers/hip-kernel-provider/rocke/library/tests/test_mla_prefill_gfx942.py:445-478 and dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:682-690 only replace the ad-hoc LLVM<21 skip and inline child env setup with the new shared helpers.
15 PASS Every parameter in new and modified functions is used in its body (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:84-267, dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:473-530).
16 N/A This item applies to review-report severity labeling rather than code changes in the diff.
17 PASS No absolute or relative performance measurements are added in the diff (dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/reference/env_flags.md:178-185) or PR description.
§F graded 11/11
# Verdict Evidence
1 PASS Branch users/cgmillette/rocke/compiler-option-check matches the users/<user>/... prefix, and the PR touches dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:1.
2 PASS title fix(rocke): never pass a compiler option the loaded compiler does not know matches type(scope): summary and is 74 characters
3 PASS description is at least 30 characters and contains an issue reference
4 PASS test file dnn-providers/hip-kernel-provider/rocke/library/tests/test_attention_bwd_agpr_probe.py is in the diff
5 PASS All changed files in the diff are code, test, documentation, build, or configuration/data files (such as dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/data/compiler_options.json:1 and dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:1), with no keys, certs, or secrets.
6 NEEDS-INFO Still unresolved at current head: no local pre-commit execution output or CI formatting check status is included in the provided PR data.
7 NEEDS-INFO Still unresolved at current head: GitHub review threads, bot/Copilot comments, and resolution/ticket statuses are not included in the provided PR context.
8 N/A The PR base branch is develop, so it is not a stacked PR targeting a feature branch.
9 N/A Nothing in the PR indicates it was artificially split or trimmed for a PR-size cap.
10 PASS The added tests are substantive unit and integration tests for option table parsing, drift checking, entry-point refusal, and test skip policy in dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:86 and dnn-providers/hip-kernel-provider/rocke/library/tests/test_compiler_test_utils.py:68.
11 NEEDS-INFO Still unresolved at current head: although the prompt contains a rendered title and body (which appears truncated at the end of Test Plan), there is no direct gh api ... --jq .body or gh pr view --json body,title verification output provided.
§G graded 34/34
# Verdict Evidence
1 N/A This PR does not change kernel IR emission or claim correctness from cross-engine byte-identity.
2 N/A This PR adds a COMGR compiler-option acceptance check and does not add or modify a GPU kernel implementation.
3 NEEDS-INFO Local test execution results for tools/run_checks.py and the platform/library pytest suites remain unverifiable from the diff alone.
4 PASS dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:420 tests that every public compile entry point raises CompilerOptionNotAccepted before reaching COMGR, which fails without the new check in compile.py and comgr.py.
5 PASS dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:359 and dnn-providers/hip-kernel-provider/rocke/library/tests/test_compiler_test_utils.py:68 drive public compile entry points and the shared test helper, with each test falsifiable by removing its corresponding guard.
6 PASS Prose claims in the PR body and docstrings match the implementation and tests, including the 52 test cases in dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:86 and the parse/require logic in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:157 and :265.
7 N/A This PR does not add or modify a kernel design document or fp32 parity reference oracle.
8 PASS dnn-providers/hip-kernel-provider/rocke/library/tests/_compiler_test_utils.py:44 and dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:307 loud-reject or explicitly skip rather than silently falling back.
9 N/A This PR does not introduce or modify LDS or occupancy resource numbers.
10 N/A This PR does not make kernel performance claims or add benchmark scripts.
11 N/A This PR does not add or optimize a dispatched kernel or modify kernel routing selectors.
12 PASS The C++ engine does not construct COMGR option lists, while all Python COMGR compilation entry points are covered in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:885 and :917 and dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:189, :364, and :477.
13 N/A This PR does not add, modify, or recommend any LLVM intrinsic symbols.
14 N/A This PR does not classify kernel hardware bottlenecks.
15 N/A The PR diff is provided directly in the review context from head commit 4015706.
16 PASS All COMGR compilation entry points and test call sites are updated consistently across dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:473 and dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:561.
17 PASS The shipped tool dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:93 was executed to generate dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/data/compiler_options.json:1 and is wired into CTest in dnn-providers/hip-kernel-provider/rocke/platform/tests/cmake/CoreTests.cmake:36.
18 PASS The guard is wired into all public compile entry points (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:189, dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:885) and proven to fire on refused/unmeasured/NUL-containing options and pass on accepted/non-mllvm options in dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:294-506.
19 PASS The test fixtures in dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:113-271 and dnn-providers/hip-kernel-provider/rocke/library/tests/test_compiler_test_utils.py:68-99 construct the exact malformed-table, column-drift, NUL-option, refused-option, and unmeasured-compiler inputs needed to trigger each gate's failure path.
20 N/A This PR does not add or modify a text detector for banned performance claims.
21 PASS The compiler-option probe tool dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:69-143 is a deterministic boolean capability probe (exit 0 with accepted vs Unknown command line argument refusal) that records exact verified_on and evidence fields into dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/data/compiler_options.json:1-27.
22 PASS dnn-providers/hip-kernel-provider/rocke/library/tests/_compiler_test_utils.py:44-49 fails with AssertionError instead of skipping when ROCKE_REQUIRE_COMPILER_TOOLS=1, and dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:108-112 exits with an error rather than skipping when COMGR is not queryable.
23 N/A This PR does not make performance or routing claims.
24 N/A This PR does not re-bless any IR or output golden files.
25 N/A This PR does not contain a codegen micro-optimization or alter emitted kernel ISA.
26 N/A This PR does not benchmark kernel performance.
27 N/A This PR does not perform a performance or routing A/B comparison.
28 N/A This PR does not report performance deltas or counterfactual benchmark arms.
29 N/A This PR does not swap guarded math primitives for fast primitives.
30 N/A This PR does not make register-spill or scratch-memory claims on kernel code objects.
31 N/A This PR does not claim a performance or resource improvement over a baseline kernel.
32 PASS Each measured compiler option entry in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/data/compiler_options.json:5-24 records verified_on (the exact LLVM version) and evidence, produced reproducibly by dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:114-128 and verified in CI via rocke_compiler_options_reproducibility…
33 N/A This PR does not change a kernel dispatch decision function, heuristic, or spec builder.
34 N/A This PR does not add or modify a kernel opt-in output flag.
§H graded 5/5
# Verdict Evidence
1 N/A This PR only adds host-side COMGR compiler option validation and test utilities, with no changes to kernel specs, buffer SRDs, or launch tensor bounds.
2 N/A This PR does not add or modify any kernel code, paged attention logic, or indirection table lookups.
3 N/A This PR does not introduce or modify any device-side loops, integer division/modulo operations, or kernel tile/page specs.
4 N/A This PR does not build or modify any device-side masks or bounds from runtime sequence length inputs.
5 N/A This PR does not touch kernel launch inputs or device-side bound/mask parameters.
§I graded 6/6
# Verdict Evidence
1 N/A This PR does not touch kernel routing, selection, or tuning search spaces.
2 PASS Instead of silently dropping -mllvm options or letting COMGR's LLVM option parser exit the host process (cryptic-fail), require_accepted loudly rejects unsupported or unmeasured options before COMGR is invoked (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:265).
3 N/A This PR does not define a shape or spec test-coverage suite combining curated and Hypothesis-generated cases.
4 N/A This PR does not add or modify performance benchmark or routing tables.
5 PASS The committed table dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/data/compiler_options.json:1 is explicitly documented as a measured snapshot with a regeneration script (dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:1) and a live CI drift check (dnn-providers/hip-kernel-provider/rocke/platform/tests/cmake/CoreTests.cmake:36).
6 N/A This PR does not introduce any DSL operations or hardware instruction primitives.
§J graded 7/7
# Verdict Evidence
1 N/A No mathematical or tensor equations appear in dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/reference/env_flags.md or elsewhere in the diff.
2 N/A No mathematical formulas, notation tables, or log/decay domain conversions are introduced in this PR.
3 N/A The paragraph added to dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/reference/env_flags.md links to rocke.runtime.compiler_options and does not restate any algorithm owned by another doc.
4 PASS dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/reference/env_flags.md:178 accurately describes the newly added CompilerOptionNotAccepted, rocke.runtime.compiler_options, tools/gen_compiler_options.py, and rocke_compiler_options_reproducibility introduced in this diff (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:67).
5 N/A The PR makes no algorithmic scaling or asymptotic complexity claims.
6 N/A No model tables, config.json dimensions, or tuned kernel dispatch band constants are added; the JSON table in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/data/compiler_options.json:1…
7 N/A No new Markdown design or algorithm document is created; only existing reference doc dnn-providers/hip-kernel-provider/rocke/platform/dsl_docs/reference/env_flags.md is updated.
§K graded 13/13
# Verdict Evidence
1 N/A The diff does not add or modify any tuned dispatch candidates or KnobSpace subclasses.
2 N/A The diff does not touch any kernel spec fields, KnobAxis declarations, or tuning classifications.
3 N/A The diff does not touch tuning/identity.py, config_key payloads, canonical-knob rules, or tuned defaults.
4 N/A The diff does not touch KnobSpace.defaults() or KnobSpace.recorded().
5 N/A The diff does not touch tuning/space.py or any knob drop/refuse rules.
6 N/A The diff does not touch KnobSpace.validate or policy-free spec construction in tuning_specs.py.
7 N/A The diff does not touch KnobSpace.base(req) or tuning problem normalization.
8 N/A The diff does not touch normalize_knobs or knob value coercion.
9 N/A The diff does not touch benchmark recording, replay, spec_hash verification, or PinRefused handling.
10 N/A The diff does not touch KnobSpace.find or bare tuning_id resolution.
11 N/A The diff does not touch any sweep entry points or core.pin_to_spec.
12 N/A The diff does not touch production stacks, accept_default, or any variant's base spec.
13 N/A The diff does not touch any benchmark sweep harnesses or auto selection ranking.
§L graded 10/10
# Verdict Evidence
1 N/A The diff does not add or modify any gfx architecture target or ISA backend.
2 N/A The diff does not add or modify any MMA op_id or fragment layout table.
3 N/A The diff does not modify MMA selection, catalog lookups, or MMA lowering.
4 N/A The diff does not touch wave-size queries or lane math.
5 N/A The diff does not add or modify arch-specific kernel recipes, transpose-LDS reads, or tile loaders.
6 N/A The diff does not modify pipeline policies, ArchTarget, or dispatcher imports.
7 N/A The diff does not add or change logical dtypes, bit packing, instruction fragments, or FP8 dialects.
8 N/A The diff does not touch scaled-MMA contracts or MfmaAtom.emit_scaled.
9 N/A The diff does not add a new architecture target.
10 N/A The diff does not modify intrinsic_arch_domain..json artifacts or LLVM flavor membership lists (test_mla_prefill_gfx942.py only updates error handling around an existing _is_modern_flavor call).
§M graded 10/10
# Verdict Evidence
1 N/A The diff does not author or modify DSL kernel bodies or builder control-flow ops.
2 N/A The diff does not touch legalize_for_target or target legalization passes.
3 N/A The diff does not touch ck.dsl.ir/v1 serialization in Python or C++.
4 N/A The diff does not touch portable IR schemas, importers, recorders, or magic division helpers.
5 N/A The diff does not touch AsyncTileLoader, LdsLayout, or async DRAM-to-LDS kernel code.
6 N/A The diff does not emit barriers, device memory loads, or cross-lane shuffles.
7 N/A The diff does not touch ISABackend buffer resource word 3 or temporal cache hints.
8 N/A The diff does not add intrinsic declarations or modify lowering engine selection precedence.
9 N/A The diff does not modify C++ public entry points or headers under cpp/include/rocke/.
10 N/A The diff does not touch LLVM debug info emission or DWARF consumers.
§N graded 18/18
# Verdict Evidence
1 PASS dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:561 adds accepted_comgr_options in the existing host compile helper module (used by compile_kernel, lower_kernel_for_comgr, and cross-family compile tests) and emits no SSA IR.
2 N/A The diff does not add or modify kernel builders, MFMA output mappings, TensorView transforms, loaders, epilogues, or helpers/transforms.py.
3 N/A The diff does not touch SoftwarePipeline or overlap_vmcnt.
4 N/A The diff does not add or modify fused kernels, EpilogueOp, or fusion validation.
5 N/A The diff does not add or modify anything under rocke.helpers.tiling.
6 N/A The diff does not touch front-door or toolbox tiling helpers or LayoutStyle.
7 N/A The diff does not touch tiling layout overrides or MMA validation.
8 N/A The diff does not touch tiling LDS allocation, lds_store/lds_read, or global load predication.
9 N/A The diff does not touch N-D tensor axis roles, batch reductions, or label flow in tiling.
10 N/A The diff does not touch tiling emit, LDS staging, or MMA issue.
11 N/A The diff does not touch tiling MMA traits, lds_swizzle, or derive_sched_group_counts.
12 N/A The diff makes no LDS bank-conflict or memory coalescing claims.
13 N/A The diff does not touch WarpDistributionEncoding, make_tile_desc, RegisterMapper, or store casts.
14 N/A The diff does not touch tiling data-movement verbs, LDS buffering, or pipeline synchronization.
15 N/A The diff does not touch tiling allocation or bounds checks.
16 N/A The diff does not touch optimize_layout or evaluate_transform.
17 N/A The diff does not touch the tiling recorder, round-trip analysis, or visualization.
18 N/A The diff does not touch interleaved layouts or tiling demo kernels.
§O graded 16/16
# Verdict Evidence
1 N/A This PR does not touch IR verification (core/verify.py), address spaces, or required attributes.
2 N/A This PR does not construct or modify scf.for ops, regions, or terminators.
3 N/A This PR does not modify IR emission, SSA naming, intrinsic declaration tables, or metadata slots.
4 N/A This PR does not touch IR type definitions, comparisons, or SmemType.
5 N/A This PR does not modify optimize_kernel, canonicalize_region, core/passes.py, or op purity tables.
6 N/A This PR does not touch constant folding passes in either Python or C++.
7 PASS In dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:544, _comgr_options_for_kernel preserves the coupling of zero agpr_alloc and mfma_vgpr_form to -mllvm -amdgpu-mfma-vgpr-form, while accepted_comgr_options at compile.py:561 validates that the loaded compiler supports it.
8 N/A This PR does not modify CodegenPolicy, SchedulerStrategy, or scheduler strategy attributes.
9 N/A This PR does not touch waitcnt encoding, barriers, or optimization_barrier lowering.
10 N/A This PR does not modify LLVM lowerer attribute validation or parameter lowering.
11 N/A This PR does not touch ArchTarget queries or target ID resolution.
12 N/A This PR does not introduce arch defaults or hard-coded vector/dword sizes in codegen.
13 N/A This PR does not modify any C structs, enums, or ABI headers under cpp/include/rocke/.
14 N/A This PR does not add or modify C++ source files, headers, or compiler warning flags in platform/CMakeLists.txt.
15 N/A This PR does not touch C++ engine fallback behavior or ROCKE_CPP_QUIET_FALLBACK.
16 N/A This PR does not alter C++ library boundaries or claims documented in known_gaps.md.
§P graded 14/14
# Verdict Evidence
1 N/A The diff does not add or change platform instance builders or builder signatures.
2 N/A The diff does not change kernarg signatures, manifests' args_signature, GEMM ABI, or conv ABI.
3 N/A The diff does not change launch geometry, grid computation, chiplet swizzle, or harness grid handling.
4 N/A The diff does not change LDS allocation gates or C++ validators for LDS capacity.
5 N/A The diff does not change narrow GEMM/RDNA/WMMA paths or MultiD/MultiAbd validation.
6 N/A The diff does not add or change caller-zeroed workspaces for Stream-K, MoE, fused MoE, or split-K.
7 N/A The diff does not change Stream-K, persistent, grouped GEMM, or support-matrix limits.
8 N/A The diff does not change quantization helpers, MX GEMM constraints, or MX scale decode.
9 N/A The diff does not add or change numeric emulation or decode paths.
10 N/A The diff does not add a runtime manifest kind or change manifest schema consumers.
11 N/A The diff does not change kernel launch lifetime, stream synchronization, PipelineLauncher behavior, WorkspacePool use, or launch timing code.
12 PASS COMGR child processes are pinned to the COMGR already loaded in the parent by deriving the library from loaded_compiler_info and setting ROCKE_COMGR_LIB plus AMD_COMGR_CACHE=0 at dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:505 and dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:527; COMGR options are checked and refused before COMGR sees them at…
13 N/A The diff does not refactor grid, tile-count, workspace math, ceil_div/align_up, kernel-cache keys, or operator configuration admission guards that affect host launch sizing.
14 N/A The diff does not change examples, expected.json gates, performance lower bounds, or new kernel logic placement.
§Q graded 7/7
# Verdict Evidence
1 PASS New tests and the rocke_compiler_options_reproducibility CTest are registered in the installed layout (dnn-providers/hip-kernel-provider/rocke/platform/CMakeLists.txt:520-523, dnn-providers/hip-kernel-provider/rocke/platform/CMakeLists.txt:623-629, dnn-providers/hip-kernel-provider/rocke/platform/CMakeLists.txt:901-902), the build tree (dnn-providers/hip-kernel-provider/rocke/platform/tests/cmake/CoreTests.cmake:36-46), and external…
2 N/A This PR does not add any new kernel family, variant, or emitted IR change.
3 PASS The new reproducibility gate (dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_compiler_options.py:93-143) and skip helper (dnn-providers/hip-kernel-provider/rocke/library/tests/_compiler_test_utils.py:44-49) are properly scoped and fail rather than skip when tools are required or COMGR is unqueryable.
4 N/A This PR does not modify IR goldens or change the lowerer or builder valid spec space.
5 PASS Tests replace hard-coded LLVM version checks (dnn-providers/hip-kernel-provider/rocke/library/tests/test_mla_prefill_gfx942.py:444-457), skip cleanly when COMGR is unavailable (dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:511-515), and test rejection paths with strictly validated negative inputs (dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:113-203).
6 PASS The option table is cached via @​functools.cache (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:248-250) and option lists without -mllvm bypass COMGR queries altogether (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:490-493), adding no per-call overhead.
7 N/A This PR checks COMGR backend options in the Python runtime wrapper rather than altering the Python/C++ engine lowering rejection contract.
§R graded 11/11
# Verdict Evidence
1 N/A The diff changes no kernel body, algorithm, tiling strategy, pipeline variant, or performance lever.
2 N/A The diff changes no performance lever or architecture-specific kernel recipe.
3 N/A The diff touches no GEMM, LDS, prefetch, ISA, or launch optimization lever covered by the utilities skills.
4 N/A The diff does not change emitted IR, lowering, or codegen options for any accepted compile.
5 N/A The diff adds or modifies no global memory accesses in any kernel.
6 N/A The diff makes no LDS or MMA layout changes.
7 N/A The diff makes no kernel performance bottleneck claims or fixes.
8 N/A The diff does not modify waits, barriers, or scheduling hints in any kernel.
9 N/A The diff is not a performance regression fix and introduces no shape-trading performance lever.
10 N/A The diff makes no kernel codegen changes, assembly parity claims, or benchmark comparisons.
11 N/A The diff introduces no GPU kernel optimization lesson or DSL optimization probe (tools/gen_compiler_options.py is a toolchain reproducibility gate documented in dsl_docs/reference/env_flags.md).
§S graded 8/8
# Verdict Evidence
1 PASS New public symbols in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/__init__.py:144 (CompilerOptionNotAccepted), dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:43 (__all__), dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:473 (require_options_accepted, comgr_child_env), and…
2 PASS Existing compile entry points (compile_kernel, compile_module, lower_kernel_for_comgr, build_hsaco_from_llvm_ir, build_hsaco_from_llvm_modules) are updated in dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/helpers/compile.py:189 and dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/comgr.py:885, tested across all entry points in…
3 N/A The diff does not remove or rename any public API symbol.
4 PASS Resolved since previous review: test_every_exported_name_resolves in dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:281 now iterates over every name in module.__all__ for both rocke.runtime.compiler_options (dnn-providers/hip-kernel-provider/rocke/platform/python/rocke/runtime/compiler_options.py:43) and rocke.runtime…
5 PASS Tests assert exact behavioral contracts and negative raises, including JSON/schema/type/list validation errors naming the file and entry (dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:113, :172), NUL-byte and malformed -mllvm flag rejection (dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:247, :460), unqueryable COMGR raising ComgrError…
6 PASS All table parsing, drift calculation, option parsing, and entry-point refusal checks run on CPU without a GPU or torch in dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:4 and dnn-providers/hip-kernel-provider/rocke/library/tests/test_compiler_test_utils.py:6.
7 N/A The changes are in Python COMGR option validation and runtime helpers (rocke.runtime.compiler_options, rocke.runtime.comgr, rocke.helpers.compile), which have no C++ twin or C ABI surface.
8 NEEDS-INFO Still unresolved from previous review: although dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py is added and mentioned in the PR body's Test Plan, the PR description still does not confirm running the rocke/platform/README.md test entry points (PYTHONDONTWRITEBYTECODE=1 PYTHONPATH=python:../library python tests/test_rocke.py and python tests/run_all.py, plus python -m rocke.run_manifest <hsaco> manifest.json --verify if applicable for runtime changes).

def require_tools_or_skip(reason: str) -> NoReturn:
"""Skip the running test for ``reason``, or fail it with ``AssertionError``
when ``ROCKE_REQUIRE_COMPILER_TOOLS=1`` says the tools must be there."""
if os.environ.get(REQUIRE_COMPILER_TOOLS_ENV) == "1":

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] §D.1 ★ ★ Knobs/features on the KernelSpec (validator + selector + name tag + cache-key), NOT os.environ (env only as a selector-level diagnostic override folded onto the spec). Reviewers may reject env vars outright — even a reproducibility/pin one (#9739: "please dont use env variables", on a…

dnn-providers/hip-kernel-provider/rocke/library/tests/_compiler_test_utils.py:47 reads the environment

Fix: Put the knob on the KernelSpec and delete the environment read.

llvm_version=version or None,
source="LLVMGetVersion",
requested_comgr="libamd_comgr.so",
comgr_path="/opt/rocm/lib/libamd_comgr.so",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] §E.5 No absolute or repo-escaping paths under platform/ (the relative-path guard in platform/tests/run_all.py enforces it). Committed helper scripts are Python, not .sh, and use tempfile / pathlib / os.cpu_count() rather than /tmp, nproc or sudo.

Not resolved since the previous review: dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:59 and dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:522 still contain the literal absolute path "/opt/rocm/lib/libamd_comgr.so" under platform/, which triggers the relative-path guard (_PATH_PATTERNS checks /opt/).

Fix: Replace "/opt/rocm/lib/libamd_comgr.so" in dnn-providers/hip-kernel-provider/rocke/platform/tests/runtime/test_compiler_options.py:59 and :522 with a synthetic path or relative path not matching /opt/, /home/, /tmp/, etc. (e.g., "lib/libamd_comgr.so").

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants