Skip to content

fix(rocke): compare arch-domain baselines across compiler builds - #15025

Open
tenpercent wants to merge 12 commits into
developfrom
users/tenpercent/rocke/aick-2970-arch-domain-provenance
Open

tenpercent wants to merge 12 commits into
developfrom
users/tenpercent/rocke/aick-2970-arch-domain-provenance

Conversation

@tenpercent

@tenpercent tenpercent commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Make arch-domain validation a monotonic support check rather than exact compiler-snapshot reproduction. Compiler identity remains diagnostic provenance, and an explicit transition matrix allows support gains while failing loss of previously working support.

JIRA ID : AICK-2970

This changes the comparison policy and its validation/test coverage only. It does not introduce per-compiler baseline storage, change runtime baseline selection, or re-bless any committed baseline. Related history: #14692 and #14328.

Context: hashes and baseline comparisons

Value / check Meaning Role in this PR
Compiler source commit hash LLVM revision embedded in clang --version, persisted in toolchain.clang. Retained and printed as provenance. A different identity no longer skips probes or fails the comparison by itself.
Emitted-IR SHA-256 Hash of rocKE-generated LLVM IR text before LLVM compilation, in the separate IR golden gates. Unchanged. Detects text changes, not correctness; intended emission changes require reviewed golden updates.
Arch-domain comparison Compiler probe outcomes per intrinsic/target, plus scope and probe-method metadata. No IR, assembly, or HSACO digest comparison. Checks retained support and reports other observations.

Neither compiler identity nor IR byte stability establishes numerical correctness. This PR changes neither numerical references nor their tolerances.

Risk Assessment

Risk level 3: this changes the pass/fail contract of a host-side compiler-validation gate. The changes are confined to comparison/validation tooling, tests, installation comments, and documentation. There is no kernel emission, runtime API, artifact-schema, or compiler-probe algorithm change. Host-only policy tests now join the existing installed rocke_pytest entry; the dedicated compiler-sweep registration is unchanged.

ASIC Coverage

The real-toolchain checks are offline compilation, linking, and target assembly—not GPU execution. Each full sweep covers 247 intrinsic keys × 18 targets = 4,446 cells.

The current head was replayed with the cached 2026-11-04 LLVM 24 installation reporting ROCm LLVM revision 36c1564ff815158e4fdff0880ba90b5d628aa045. The four-flavor test matrix exercises rocKE's software profiles and policies; it is not evidence that four real compiler majors were installed. No new numerical, performance, or blanket release qualification is claimed.

Technical Changes

  • Retain compiler identity as provenance rather than an applicability gate.
  • Define all known-status transitions in an explicit 7×7 matrix. Previously ok cells must remain ok, and accepted processor names must remain accepted.
  • Allow and report support gains and unknown-to-unavailable measurement improvements. Report degraded negative measurements and remaining no-data cells without qualifying them.
  • Reject invalid probes, malformed/missing records, incompatible scope or methodology, changed positive probe witnesses, and loss of accepted processor names.
  • Require explicit matching per-cell verified_on, usable compiler provenance, diagnostic evidence for non-ok cells and rejected processors, and integer witnesses only on ok cells. Diagnostic wording and canonical-name changes remain observations.
  • Never write the baseline from --check. Reviewed gains need a separate baseline update to protect them from future loss and update runtime availability policy.
  • Separate per-document record validation into _validate_column() from cross-document comparison, while retaining independent structural assertions and malformed-input tests.
  • Move the 20 support-policy tests into an installed, source-less-capable module and add four metadata/diagnostic/local-generator controls. The exhaustive compiler sweep remains outside pytest.
  • Name changed method/metadata fields and baseline/current values, including missing-versus-null distinctions. Preserve the existing total/omitted-count failure summary.
  • Update test-enrollment documentation and remove the stale no-op/golden description. The runtime no-data set is deliberately unchanged: it includes probe_error, which the comparer treats as an invalid probe rather than reportable unqualified data.

Testing Summary — current head 9194745f2e16

  • Source owner matrix: LLVM 20/22/23/24 profiles × Python/strict C++/strict both, with a fresh native binding: 173 tests and 19,457 subtests passed per run, zero skips across all 12 runs. The committed head was replayed successfully.
  • Fresh build/install: native engine, binding, and installed assets built successfully; 46/46 non-sweep native CTests passed.
  • Physically relocated pytest: the complete generated provider-layout rocke_pytest CTest passed with the source tree hidden: 9,725 passed, 1,063 skipped, 3 deselected, and 14,864 subtests passed. All 24 support-policy cases ran. Skipped cases remain uncovered.
  • Standalone relocated policy module: 24 tests and 495 subtests passed, again with the source tree hidden.
  • Real full LLVM 24 support sweep: source CLI and relocated compiler CTest passed. The source sweep retained all 2,249 baseline-working cells, reported three gains and 84 measurement improvements, and left 62 cells explicitly unqualified.
  • Real CLI negative controls: all 12 full checks—source and relocated paths × missing processor mappings, missing flavor tags, missing clang provenance, changed metadata, changed method, and deliberate support loss—failed with exit 1 and the expected diagnostics. Private references and all committed baselines remained unchanged.
  • Differential controls: 1,057 old/new comparisons agreed on pass/fail and on observations for passing inputs. Metadata-guard, generic-diagnostic, and permissive-loss mutations were rejected. The prior implementation fails the new detailed-diagnostic tests.
  • Scope verification: AST comparison confirms the transition matrix, probing algorithm, CLI orchestration, and emission code are unchanged by this follow-up. Committed generator/test bytes match the validated relocated assets.
  • Formatting/static checks: applicable file hooks, full PR-range pre-commit hooks, and whitespace checks passed.

The relocated validation used the matching nightly COMGR/LLVM environment, pytest-timeout 2.4.0, and ml_dtypes 0.6.0. An initial broad run exposed missing dependencies and a mismatched COMGR; correcting the environment, without project-code changes, produced the passing result above. Provider-layout assets and verbatim CMake-generated test entries were exercised locally; this is not a claim of a full CI superbuild replay.

Known unchanged limitation: the developer relative-path guard still flags the monorepo bundle reference at platform/CMakeLists.txt:428. The prior head reproduces the same failure. It was left outside this seven-file follow-up, so a clean complete developer run_all.py verdict is not claimed.

Historical validation

The earlier comparison-policy implementation was tested against seven cached nightly installations. These are prior-head results, not seven fresh sweeps of 9194745f2e16:

Cache snapshot Reported LLVM source revision Historical full support check
2026-10-28 9628a5b15b30be73e6533a1431f85926ea08de1e PASS
2026-10-29 4208a88993d31a4311127ddae671ec6a6b1000c6 PASS
2026-10-30 e095f1b3becf0c1bfdc5e7647003d60976a2460b PASS
2026-10-31 e095f1b3becf0c1bfdc5e7647003d60976a2460b PASS
2026-11-01 9fa7f0cb60bbe1b4f9e602b1bf12a94df7e53cf7 PASS
2026-11-02 36c1564ff815158e4fdff0880ba90b5d628aa045 PASS
2026-11-03 36c1564ff815158e4fdff0880ba90b5d628aa045 PASS

These existing ROCm-cache binaries report ROCm source revisions, but their original download/build provenance was not independently audited. Earlier third-party LLVM-fork runs are diagnostic only, not official ROCm qualification evidence.

The prior-head hipDNN Linux superbuild failure reported 186 passed and one failed. Its 87 changes were three BF16 gfx1250 cells changing arch_absent -> ok and 84 cells changing toolchain_crash -> arch_absent, with no reported loss of working support. This is the gain/improvement scenario the new policy accepts.

Testing Checklist

  • Current-head source owner matrix and committed replay — python -m pytest tests/tools/test_arch_domain_artifact.py tests/tools/test_arch_domain_support.py tests/tools/test_gen_arch_domain_cpp.py tests/tools/test_run_all.py -q -rs from platform/ — passed with the fresh binding.
  • Fresh native build/install and 46/46 non-sweep CTests — passed.
  • Current-head full LLVM 24 CLI — python tools/gen_arch_domain.py --check --unvalidated-exit-code 77 --jobs 4 — passed without filters or baseline writes.
  • Relocated compiler CTest and full rocke_pytest CTest, source hidden — passed in the matching toolchain environment.
  • Standalone relocated support-policy tests — passed.
  • Real source/installed negative controls and mutation/differential controls — behaved as required.
  • Full PR-range pre-commit hooks and whitespace checks — passed.
  • New-head CI — in progress at drafting time; local validation is not a CI completion claim. hipDNN Superbuild · TheRock Multi-Arch.

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

No active review issues remain for the cross-build arch-domain validation update.

No open issues 🎉

What changed in this PR

This PR updates rocKE’s arch-domain validation to compare baselines across compiler builds within the same LLVM flavor. Compiler identity remains diagnostic provenance; measurement and other metadata differences still fail validation.

Changes:

  • Remove the compiler-identity mismatch skip and exclude only toolchain.clang from artifact comparison.
  • Add regression coverage for unchanged results, measurement drift, probe execution, and baseline immutability.
  • Update the documented reproducibility and skip behavior.
File Description
dnn-providers/​hip-kernel-provider/​rocke/​TESTING.md Documents cross-build comparison and remaining skip conditions.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tools/​gen_arch_domain.py Compares measurements despite differing compiler identities.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​tools/​test_arch_domain_artifact.py Tests cross-build pass/fail behavior and applies mechanical formatting.

Review effort: Balanced

@therock-pr-bot

therock-pr-bot Bot commented Nov 4, 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

therock-pr-bot Bot commented Nov 4, 2026

Copy link
Copy Markdown

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

@tenpercent
tenpercent marked this pull request as ready for review November 4, 2026 01:01
@tenpercent
tenpercent requested review from a team as code owners November 4, 2026 01:01
@tenpercent
tenpercent marked this pull request as draft November 4, 2026 01:15
@tenpercent tenpercent changed the title fix(rocke): compare arch-domain baselines across compiler builds fix(rocke): (do not merge) compare arch-domain baselines across compiler builds Nov 4, 2026
@tenpercent
tenpercent requested a max review from Copilot November 4, 2026 04:16

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

Missing processor records and provenance can pass validation, allowing incomplete baselines to produce fictitious support gains.

4 open issues

Review effort: Max

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

The artifact checker accepts missing required diagnostics, unusable compiler provenance, and invalid probe witnesses that can cause ArchDomain.lookup() to raise ValueError.

4 open issues
4 resolved since last review

Review effort: Max

@tenpercent tenpercent changed the title fix(rocke): (do not merge) compare arch-domain baselines across compiler builds fix(rocke): compare arch-domain baselines across compiler builds Nov 4, 2026
@tenpercent
tenpercent marked this pull request as ready for review November 4, 2026 17:16
Require nonblank compiler identities and negative diagnostics in both comparison columns. Reject present probe witnesses unless they are non-boolean integers on successful cells. Preserve cross-build support gains, diagnostic wording freedom, and the positive-witness equality guard.

Add symmetric malformed-record regressions, CLI exit/immutability controls, and committed-artifact schema assertions. The new rejection tests reproduce the prior comparison gaps.

Validation: 166 owner tests and 19444 subtests passed in each of 12 LLVM-profile/backend runs with a fresh native binding; 45 native CTests passed; real LLVM24 source and relocated support CTests passed; 28 real CLI controls passed without changing baselines.

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.

🔵 Needs a closer look

Human assessment is needed because a compiler-banner mismatch with the declared toolchain can allow incompatible baseline comparisons to pass.

No open issues 🎉

4 resolved since last review

Review effort: Max

@tenpercent
tenpercent requested a review from CongMa13 November 4, 2026 17:57
@cgmillette

Copy link
Copy Markdown
Contributor

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

PR #15025 — fix(rocke): compare arch-domain baselines across compiler builds (tenpercent, AICK-2970) — 🟠 First-round: rules duplicated, tests outside CI

Prior item Status
Copilot (11-04) ×4: processor records, verified_on, summary, docs 🟢 FIXED 8a570aef0f
• missing maps, entries, verified_on now fail, either side (ran)
• summary prints total and (+N more) (ran)
• cited docs and CLI help state the floor (read)
Copilot (11-04) ×4: clang identity, evidence, probe_imm, rejected processors 🟢 FIXED 5ab13a7acc
• all four now fail, either side; none crash (ran)
Copilot (11-04) overview: banner mismatch needs a human look 🟢 BY DESIGN
• another LLVM major still skips (flavor gate) (read)
• same-major builds compare: CI's passes, an older one fails (ran)
Bots / CI 🟢 Green
• 45 ✓ · 51 skipped · 0 ✗; Math CI Summary ✓
• Linux shards ran the real gate: support kept, 3 gains (ran)
• Windows shards skip it: no clang there (ran)
• merge: mergeable; blocked on review
Our checks 🟢 Clean
• merge vs d0abb00640: clean (ran)
• build + ctest 46 ✓ / 1 skip; differential golden OK (ran)
• tests 106 ✓; unforced llvm24 with lane test 191 ✓ (ran)
• comparer: 57 crafted records judged right, none crash (ran)
• older llvm24: gate fails, barrier decls invalid there; baseline untouched (ran)
• rules 1–3: hits judged harmless (ran)
Code quality 🟠 Should-fix
• N1 record rules copied; N2 tests outside CI
• 🟡 policy restated in 7 places; arch_axis_proposal.md:40 keeps the no-op gate (read)
• 🟡 gen_arch_domain.py:1225 re-lists NO_DATA_STATUSES as "unvalidated" (read)
• 🟡 gen_arch_domain.py:1169, :1174 failures name no field or value (ran)
• both engines n/a: Python tooling only (read)
# Sev file:line Finding
N1 🟠 platform/tools/gen_arch_domain.py:1232 Record rules copied into the comparer, which both validates and compares
• impact: 5ab13a7acc wrote four rules twice; the copies will drift
• fix: one _validate_column(doc, label); comparer and structure tests call it
N2 🟠 platform/tests/tools/test_arch_domain_artifact.py:359 The policy's tests run in no CI lane
• impact: CI runs only the gate's pass path (ran)
• the metadata check (gen_arch_domain.py:1173) has no test (ran)
• fix: move SupportComparisonTest where rocke_pytest collects it; test the metadata check

Utility: 🟢 Positive — CI now checks the support floor instead of skipping the gate.


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

First-round review: first pass by a local qwen3.6 model, verified by the orchestrator (Claude Opus 5.5). Checks: merge with develop, build + ctest, differential golden, changed tests (forced and unforced flavor), CI shard logs, the real gate on this host's llvm24 build, record probes and mutants, rule scan, code quality. Flags whether a closer look is needed; not an approval or rejection.

@tenpercent

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Addressed N1/N2 and the accepted smaller suggestions in 9194745f2e16, keeping the follow-up to seven files.

  • N1: extracted _validate_column() so record validation is separate from cross-document support comparison. Structural tests exercise it but retain independent assertions; the shared validator is not their sole oracle.
  • N2: moved the 20 SupportComparisonTest cases to test_arch_domain_support.py, which the existing installed rocke_pytest entry collects. It resolves the local source or staged generator without reaching back into a checkout. Added four metadata/diagnostic/local-generator controls; the full compiler sweep remains a separate CTest.
  • Smaller suggestions: covered the metadata guard with addition/removal/change and missing-versus-null controls, made method/metadata failures identify fields and baseline/current values, and corrected the remaining no-op/golden sentence. Enrollment documentation now reflects the split.
  • No-data classification: deliberately left unchanged. Runtime NO_DATA_STATUSES includes probe_error; the comparer treats that state as an invalid probe, not reportable unqualified data. A direct substitution would conflate the two contracts.

Evaluation: all 12 source profile/backend runs passed with 173 tests and 19,457 subtests per run; full relocated rocke_pytest passed with the source hidden (9,725 passed, 1,063 skipped), including all 24 policy cases. The source and relocated LLVM 24 gates passed; all 12 real CLI rejection controls failed as intended without rewriting references. Metadata/diagnostic/loss mutations were detected, and 1,057 old/new comparisons agreed. Full PR-range hooks passed.

The transition matrix, probe algorithm, runtime/emitter code, and committed baselines are unchanged. The existing relative-path-guard finding at platform/CMakeLists.txt:428 reproduces on the prior head and remains out of scope. New-head CI was still in progress when this reply was drafted; skipped tests and local passes are not blanket qualification.

@cgmillette

Copy link
Copy Markdown
Contributor

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

PR #15025 — fix(rocke): compare arch-domain baselines across compiler builds (tenpercent, AICK-2970) — 🟡 Follow-up: fixes hold; CMake comment conflict

Prior item Status
Copilot (11-04) ×4: processor records, verified_on, summary, docs 🟢 FIXED 8a570aef0f
• rules now in _validate_column; CI's tests fail without each (ran)
• summary and docs unchanged since 11-05 (read)
Copilot (11-04) ×4: clang identity, evidence, probe_imm, rejected processors 🟢 FIXED 5ab13a7acc
• one copy each, in _validate_column; CI's tests fail without each (ran)
Copilot (11-04) overview: banner mismatch needs a human look 🟢 BY DESIGN
• CI's gate names the other build, then checks the floor (ran)
Ours (11-05) N1 record rules copied into the comparer 🟢 FIXED 9194745f2e
• comparer and structure test both call _validate_column (ran)
• status rule also still in the matrix: N3
Ours (11-05) N2 policy tests in no CI lane 🟢 FIXED 9194745f2e
• test_arch_domain_support.py: 24 ✓ in rocke_pytest on every shard (ran)
• metadata check tested; failures name fields and values (ran)
Ours (11-05) code quality 🟡 points 🟡 OPEN
• messages and arch_axis_proposal.md:40 fixed 9194745f2e (ran)
• no-data list kept: "an invalid probe, not reportable unqualified data"; validator now rejects probe_error first, so NO_DATA_STATUSES is exact (ran)
• policy still restated: see Code quality
Approvals 🟢 None yet
• needs one rocke-platform code owner's approval
Bots / CI 🟢 Green
• 46 ✓ · 52 skipped · 0 ✗, latest attempts
• Windows shard 1/2 attempt 1: rocke_pytest timed out in develop's fresh-build test (replaced Azure pool); attempt 2 ✓
• Linux gate keeps the floor, 3 gains; Windows skips it (ran)
• merge: conflicts since 14811 landed; blocked on review
Our checks 🟡 Conflict
• merge vs 10f12f0d49: platform/CMakeLists.txt conflicts, comments only (ran)
• resolve: PR's three comment lines, then develop's two; arch-domain tests 197 ✓ on the be0e8acf7b merge (ran)
• build + ctest 48 ✓ / 1 skip; differential golden OK, at 2e8e72951d (ran)
• changed tests 137 ✓ / 1 skip; installed tree 24 ✓ (ran)
• mutants: 12 of 16 validator rules caught by CI's tests (ran)
• rules 1–3: perf hit is develop's sentence, rewrapped (ran)
Code quality 🟡 Minor
• N1, N2 fixed: one validator; tests in rocke_pytest (ran)
• N3: status rule stated twice; matrix copy unreachable (ran)
• 🟡 gen_arch_domain.py:1282 re-lists NO_DATA_STATUSES; tests hand-list status sets (read)
• 🟡 docs restate policy and enrollment; :1724, :1746 name the old drift check (read)
# Sev file:line Finding
N3 🟡 platform/tools/gen_arch_domain.py:1278 Status rule stated twice; four validator rules have no test of their own
• impact: deleting the schema, method-field, row-mapping or status check keeps CI's tests green (ran)
• the matrix's invalid entries and this branch run only in unit tests (ran)
• fix: status rule in _validate_column only (matrix over six states); one negative test per rule

Utility: 🟢 Positive — policy tests now run in CI; one validator owns the record rules.


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

Follow-up review: first pass by a local qwen3.6 model, verified by the orchestrator (Claude Opus 5.5). Checks: merge with develop (before and after 14811; conflict resolved and tested), build + ctest, differential golden, changed tests (installed tree too), CI shard logs, validator mutants, saved probes, rule scan, code quality. Flags whether a closer look is needed; not an approval or rejection.

@cgmillette cgmillette 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.

lgtm

tenpercent and others added 3 commits November 6, 2026 12:30
…erage

Merge develop while keeping monotonic support tests in their installed module. Use architecture-specific fixtures in CLI controls and run the support-policy cases for gfx942 and gfx950.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants