Repository navigation
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
There was a problem hiding this comment.
🟢 Approval recommended
The narrow tooling change matches the intended behavior, preserves existing failure handling, and has focused regression coverage with no unresolved findings.
No open issues 🎉
What changed in this PR
Adds per-toolchain-flavor exceptions to rocKE’s strict IR validity gate, allowing expected compiler limitations without accepting unrelated unvalidated cases. No IR emission code changes.
Changes:
- Defines skip lists for
llvm20,llvm22, andllvm23. - Makes strict runs reject unlisted unvalidated cases and stale entries that now validate, while preserving non-strict exit behavior.
- Adds tests for skip-list verdicts, selection boundaries, unknown flavors, and corpus references.
| File | Description |
|---|---|
dnn-providers/hip-kernel-provider/rocke/platform/tools/check_ir_validity.py |
Adds flavor-specific skip lists, verdict checks, and diagnostics. |
dnn-providers/hip-kernel-provider/rocke/platform/tests/core/test_arch_domain_artifact.py |
Adds focused skip-list regression tests. |
Review effort: Balanced
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14211 — add a per-flavor skip-list to the IR validity gate — 🟢 First-round: no closer look needed — the skip-list logic is right; the red CI checks come from develop, not this PR
Utility: 🟢 Positive — lets the strict gate pass on llvm20 / llvm22 images while still failing on any new unmeasured case or stale entry; a needed step toward running it in CI. 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 on the merge, the strict gate on an LLVM 24 host (native and forced) against develop, and the failing CI log traced to its source, and owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection. |
|
I've been fixing this test because it's a blocker for running tests locally. I'm also adding it to CI on TheRock. If we can hold off on this PR until I get my changes in that would be much better. |
|
I reviewed this PR again. The following review threads are still unresolved, or comments have no author reply:
If you have already fixed these, please reply on each comment to say so and resolve the threads. Please request my review again once all comments and threads are resolved. |
CongMa13
left a comment
There was a problem hiding this comment.
No new issues found.
|
@shumway can you request changes until your work lands? I want the bots to understand the dependency you highlighted. |
|
The seven failing LLVM 23 test may be the ones I documented in #14084 |
Under --strict, `check_ir_validity.py` failed on any UNVALIDATED case. A host with an older clang cannot measure some gfx1250 cases. The gate could not run in CI on the llvm20 and llvm22 toolchains for that reason. Add `SKIP_UNVALIDATED`, a table of the cases each flavor is expected to leave UNVALIDATED, with a reason for each. Under --strict: - A listed UNVALIDATED case passes. - An UNVALIDATED case that is not listed fails. - A listed case that now validates is stale and fails. The list only shrinks, like `KNOWN_BAD`. A flavor with no entry expects no UNVALIDATED case. Runs without --strict keep their behavior. ISSUE ID : AICK-2694 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The gate printed one total for validated, known-bad and UNVALIDATED cases. A run could not show which arch lost coverage. Print a table of validated, failed and unvalidated counts for each arch. The table counts every case once, so its rows add up to the corpus. ISSUE ID : AICK-2694 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
5191e44 to
d84f6ba
Compare
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14211 — add a per-flavor skip-list to the IR validity gate — 🟢 Follow-up: no closer look needed — the new per-arch table adds up to the corpus, and the red CI is develop's break plus a timeout this diff cannot reach. Check that shumway's hold is lifted before merging
Utility: 🟢 Positive — the skip-list lets the strict gate pass on llvm20 / llvm22 images while still failing on any new unmeasured case or stale entry. The per-arch table shows which arch lost coverage. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted Follow-up review: a local qwen3.6 model made the independent first pass. The orchestrator (Claude Opus 5.5) verified it. The checks were a range-diff against the head we reviewed and the PR's tests on a merge with today's develop. They also included the gate under |
cgmillette
left a comment
There was a problem hiding this comment.
LGTM, @shumway please weigh in on current readiness
|
This solution hides the problem instead of solving it. Close it and open another PR to solve this issue. |
ISSUE ID : AICK-2694
Summary — For human reviewers
The short version
check_ir_validity.py --strictfails on every host whoseclangis older than the newest wired arch.That
clangcannot measure some gfx1250 cases.The tool reports them as
UNVALIDATED, and--stricttreats anyUNVALIDATEDcase as a failure.This PR adds a per-flavor skip-list.
A case on the list may stay
UNVALIDATED.Any other
UNVALIDATEDcase still fails.This PR is one step toward running the gate in CI.
It changes only the tool and its tests.
It does not add the CI entry.
Why this has to be a list and not a count
A simple floor on the validated count also lets old toolchains pass.
But a floor hides which cases went unmeasured.
A new case that stops validating could stay under the floor without notice.
The list names each expected case and gives a reason.
A new
UNVALIDATEDcase fails the run.A listed case that starts to validate also fails the run.
This forces the entry out of the list in the change that fixes it.
The list only shrinks, like
KNOWN_BAD.How it works
SKIP_UNVALIDATEDmaps each flavor to its expectedUNVALIDATEDcases, with a reason.llvm20: 8 cases. Four needllvm23to lower. Four target gfx1250, which thisclangcannot target.llvm22: 4 cases. All four needllvm23to lower.llvm23: no cases.UNVALIDATEDcase.--strictbehave as before.--strict.A developer host with a newer
clangof the same flavor does not fail the default gate.How this was tested
I ran
check_ir_validity.py --strictin four toolchain images on the rebased tree (246 cases).llvm20llvm22llvm23buildllvm23llvm23buildllvm23I also forced two failures.
An empty
llvm22list fails and names the four cases.A listed
llvm23case that validates fails as stale.Each run takes about 12 seconds.
One finding outside this PR
On the second
llvm23build, seven gfx950 fp16 attention cases fail to compile.The error is
Operand has incorrect register class.The first
llvm23build compiles all 246 cases.This PR does not touch emission.
The failure depends on the compiler build and exists without this PR.
Check which
llvm23build the CI image carries before the gate runs in CI.Reviewing this
One commit.
Two files: the tool (+70 −6) and its test file (+55).
No emitter file changes.
Summary — For AI reviewers
Change surface
No file under
core/,helpers/,instances/,cpp/orlibrary/changes.The change cannot alter emitted IR.
The gap being closed
--strictUNVALIDATED, listed for the flavorUNVALIDATED, not listedProperties to verify
UNVALIDATEDcase fails under--strict._skip_list_verdictreturns it inunexpected.main()printsREDand returns 1 before theGREENline.OKfails under--strict._skip_list_verdictreturns it instale.Confirm that a listed case outside the selection (
--only) is not stale.--strictis required for both new failures.Confirm that a run without
--strictreturns the same exit code as before.KNOWN_BADhandling is unchanged.new_badandstalereturn before the new checks.test_every_entry_names_a_corpus_case_and_a_reasonchecks this.SKIP_UNVALIDATED.get(flavor, {})returns an empty dict.Repo rules that apply
gfx1250, no product name.Known weak points
llvm20,llvm22andllvm23only. A new flavor needs a new entry.Scope calibration
A finding that this PR does not add the CI entry, the ctest registration or the installed artifact is out of scope.
Those are the next changes.
A finding that this PR does not fix the seven
llvm23failures is also out of scope.What review cannot establish from the diff
clangis the build that passes or the build that fails.Test plan
check_ir_validity.py --strictGREEN onllvm20(ROCm 7.1.1)check_ir_validity.py --strictGREEN onllvm22(ROCm 7.2.0)check_ir_validity.py --strictGREEN on onellvm23buildValiditySkipListTestandValidityGateTestpasspre-commit run --from-ref origin/develop --to-ref HEADpassesllvm23build: 7 failures, independent of this PR (deferred)🤖 Generated with Claude Code