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.
🟡 Changes recommended
Changed crash evidence can bypass serial confirmation and cause load-dependent reproduction failures.
1 open issue
What changed in this PR
This PR reduces compiler orchestration overhead in rocKE’s exhaustive arch-domain reproducibility gate while preserving per-cell coverage and target-assembly validation.
Changes:
- Batches declaration discovery and expected-positive probes, recursively isolating failures.
- Adds target preflights, explicit operand witnesses, bounded concurrency, and selective serial retries.
- Expands scheduler tests and documents the optimized reproduction process.
| File | Description |
|---|---|
dnn-providers/hip-kernel-provider/rocke/TESTING.md |
Documents scheduling, coverage, and retry behavior. |
dnn-providers/hip-kernel-provider/rocke/platform/tools/gen_arch_domain.py |
Implements batching, preflights, witnesses, and retry scheduling. |
dnn-providers/hip-kernel-provider/rocke/platform/tests/tools/test_arch_domain_artifact.py |
Adds coverage for scheduling, batching, witnesses, and retries. |
dnn-providers/hip-kernel-provider/rocke/platform/tests/README.md |
Explains reproduction scheduling and concurrency limits. |
Review effort: Balanced
CongMa13
left a comment
There was a problem hiding this comment.
No new P0 or P1 issues were identified during this review. The changes are approved.
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14355 — optimize arch-domain reproduction scheduling — 🟢 First-round (posted after the merge): no closer look needed — on a real compiler the scheduled reproduction agrees with the old per-cell one and names planted drift exactly; one 🟡: none of the new code runs in CI
Utility: 🟢 Positive — the exhaustive reproduction gate gets much cheaper without dropping a cell, and it still names drift exactly, including inside a batch. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted First-round review, posted after the merge: a local qwen3.6 model made the independent first pass. The orchestrator (Claude Opus 5.5) verified it. The checks were generation and reproduction on a real compiler with pre-merge develop and this PR, on three key subsets, plus planted-drift controls. They also included the unit tests on both trees, CI-enrollment reads, a scratch merge with #14359 and the CI logs at head. The orchestrator owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection. |
TheRock Submodule Bump ActivityNewest first
|

ISSUE ID : AICK-2713
Summary
Optimize the exhaustive rocKE arch-domain reproducibility CTest by roughly 10x at the measured LLVM 23 configuration, without reducing its matrix, frequency, compiler isolation, or verdict strength.
Why
The gate validates every intrinsic declaration against every enrolled architecture, but its original implementation repeated work that belongs to broader logical axes:
optprocess per declaration;The expensive part was process orchestration, not the coverage requirement. Expected-negative cells still need independent probes because a compiler fatal can terminate a combined module before later calls are evaluated.
Design decisions
ProbeSpecwitnessesArtifact generation remains per-cell. Optimized scheduling applies to reproduction and shares the same probe construction and result classification.
Improvements
Bug fixes
Cleaner design
ProbeSpecrecords the valid witnesses used to construct a probe.Better test coverage
Faster execution
Performance
Measured scope: the focused host-only
rocke_arch_domain_reproducibilityCTest over the complete LLVM 23 column: 205 declaration keys acrossgfx11-generic,gfx1151,gfx12-generic,gfx1201,gfx1250,gfx90a,gfx942, andgfx950.On the same host with AMD clang 23 build
8f497e0992fb7513f7f78a6f6b6f1056c375e961, the final implementation reduced focused-CTest wall time by approximately 91%—about an 11x speedup—relative to parent commit784462004d, at this measured configuration. Both arms reproduced the committed artifact cell-for-cell. The result is sensitive to host load, filesystem/compiler page cache, CPU quota, and the selected worker count; it is not a general performance claim.Replay:
Validation
developwith the exact recorded ROCm 7.1.1 toolchainLLVM 20's committed column has pre-existing evidence/coverage drift under its recorded toolchain: both
developand this branch fail--checkwith identical classification counts, while their freshly generated documents are byte-identical. This PR preserves that baseline behavior and does not attempt to add gfx1250 support to LLVM 20.TheRock CI coverage note
TheRock CI for this repository currently uses LLVM 24. rocKE does not yet have a committed LLVM 24 arch-domain column, so
rocke_arch_domain_reproducibilityintentionally reports a CTest skip in those jobs rather than claiming reproduction coverage for the wrong compiler flavor. Those jobs validate registration, packaging, relocation, and skip semantics, but they do not execute the full compiler matrix added by this PR.The full reproduction path for this change was validated locally with matching committed toolchains, including LLVM 23 from ROCm 10.0. LLVM 24 arch-domain and CI qualification support will follow in a separate PR after this change.
Deferred follow-up
Kept out of this PR intentionally: