Repository navigation
fix(cpu-ep): keep the prefill fan-out policy present on aarch64+MLAS - #1382
Conversation
5413d58 to
bc8a67c
Compare
5c9a6a5 to
8399cf5
Compare
#1363 shipped this test past a bypassed CI lane, and it fails on every stock runner. It asserts that the flat fan-out reaches the task runtime, but routing reads `rayon::current_num_threads()` and `flat_fan_out` deliberately keeps the fan-out on Rayon below MIN_ROUTED_FAN_OUT_WIDTH (16). Below that width the test asserts a dispatch policy never promised: rayon = 4 / 8 / 15 FAILED rayon = 16 / 32 ok `ubuntu-latest` is a 4-vCPU runner, so "Fast (Linux x86_64)" would have been red. It passed for me only because this host is 16C/32T. The existing `task_runtime::width() <= 1` guard does not cover it: task-runtime width and Rayon width are different numbers, and on a 4-vCPU box the first is > 1 while the second is < 16. `output_chunk_len` reads the same Rayon width, so the *precondition* carried the identical defect -- repairing only the routing moved the failure to rayon = 1 rather than removing it. Fixed by installing a Rayon pool of exactly the routing width, so the decision under test is the same on a 4-vCPU runner as on a 32-thread workstation. Skipping below the threshold would have been weaker: the test would silently guard nothing on every real runner. Now passes at rayon = 1, 2, 4, 8, 15, 16 and 32, and still fails when routing is forced to PrefillFanOut::Wide, so it is not vacuous. Also adds the coverage assertion it should always have had: every output row written exactly once. The aarch64 dead-code break that #1363 also shipped is left to #1382, which was open first and is already armed; I verified its three `allow(dead_code)` restorations are exactly what the cross-target lane needs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Independent confirmation of this fix, and an apology — #1363 was mine, and I merged it with an admin bypass while its checks were still queued, which is why this dead-code break reached main at all. I reproduced the failure on latest main ( I independently arrived at the same three restorations you have here, and verified they clear the cross-target lane. Since #1382 was open first and is already armed, I have dropped my duplicate from #1420 rather than ship a conflicting change to the same lines. #1420 is now scoped to the other #1363 defect, which this PR does not cover: For the record on the gating you flagged: the reachability is Thanks for catching both this and #1385 — you found the two things my bypass hid. |
8399cf5 to
4ef749e
Compare
32834e7 to
1799bdc
Compare
e5adf24 to
a7b7d8f
Compare
#1443 stopped the aarch64 dead-code errors by `#[cfg]`-ing the three prefill fan-out symbols to `target_arch = "x86_64"`. That removes the items outright, and they have two callers gated on *different* things: matmul_nbits.rs:2457 run_mlas_shards #[cfg(feature = "mlas")] matmul_nbits.rs:6914 borrowed_affine_int4_matmul_prefill #[cfg(target_arch = "x86_64")] So on `aarch64 + feature = "mlas"` -- Apple Silicon, the primary aarch64 target -- the MLAS caller is still compiled while its callee is not: error[E0425]: cannot find function `prefill_fan_out` in this scope --> crates/onnx-runtime-ep-cpu/src/kernels/matmul_nbits.rs:2457:27 error[E0425]: cannot find function `prefill_column_grain` in this scope --> crates/onnx-runtime-ep-cpu/src/kernels/matmul_nbits.rs:6915:48 The cross-compile gate cannot see this: its aarch64 pass builds default features, and MLAS is off by default. Gating the items also forced gating the six unit tests that reference them, so the #1363 fan-out policy stopped being checked on aarch64 at all. Those tests are pure functions of explicit arguments -- nothing in them is architecture-specific. `cfg_attr(.., allow(dead_code))` fixes both: the item always exists, so whichever caller survives can reach it, and the lint is silenced only in the configuration where neither caller exists. `prefill_tile_grain` already used exactly this idiom, which is what #1363 dropped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Bypassed merges left `main` failing both required checks: * #1427 introduced `map_or(true, ..)` in `executor/dispatch.rs`, which clippy rejects under `-D warnings` (`this map_or can be simplified`). That aborts the cross-compile gate before its aarch64 pass runs, which is why the gate could not report the aarch64 defect this PR fixes. Replaced with the equivalent `is_none_or`. * #1427 and #1418 also left `dispatch.rs`, `gather_block_quantized.rs` and `gpt_oss_20b_decode_lock.rs` unformatted. `cargo fmt --all -- --check` runs in *both* required jobs while the cross-compile gate runs only in `Rust quality`, so while `main` is broken this way no single-fix PR can go green: a fmt-only PR still fails the cross-compile step, and this PR's own fix still fails fmt. Only a branch carrying both can pass. Drops out on rebase once fixed upstream. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a7b7d8f to
9fb04f5
Compare
|
Status, 2026-08-19 11:06Z — still waiting on required CI, no bypass. Scope changed since this PR was opened. #1443 landed the aarch64 dead-code fix I originally wrote this for, but did it by Local gates at Why it has not merged. Every workflow run in this repo is Auto-merge (squash) stays armed; it will merge on its own the moment both required checks go green. I am not using |
Overlap audit: this PR is a competing design, not a duplicate repair (cross-posted from #1429)Asked to find the duplication across these four and recommend a single minimal repair path. Short version: the aarch64 problem all of this was chasing is already fixed on main, by #1443, which merged at 09:20Z today. Two of the four PRs are now obsolete or actively harmful, and they are obsolete for different reasons. The aarch64 lint is already fixed
and so do the three tests that reference them (17303, 17316, 17332). The items and their only consumers vanish together on aarch64, so there is no dead code and no lint to silence. Nothing further is needed for the aarch64 lint. This is the fifth attempt at the same problem — #1429 — supersededr, and would rebase into a contradiction#1429 adds Rebased onto current main the result is: #[cfg(target_arch = "x86_64")]
#[cfg_attr(not(target_arch = "x86_64"), allow(dead_code))]
const WIDE_PREFILL_MACS: usize = 1 << 29;The #1382 — not a duplicate repair, a competing design; must not merge as-is#1382 is aimed at something genuinely different and arguably better: keep the prefill policy present and tested on aarch64 instead of compiling it away. It deletes But #1443 resolved the same question the opposite way. Merging #1382 now would re-delete the gating #1443 just added, so this is a design disagreement to settle deliberately, not a repair to land. Recommend either closing it, or re-scoping it to only the "restore aarch64 test coverage" argument on top of #1443 — with the ~25 lines of rationale it carries, because that rationale is the most accurate description of the caller structure anyone has written so far and should not be lost. #1393 and #1420 — keep, no overlap between them#1393 is the fmt repair, and it is still needed: Note the overlap that did exist: #1382 also carries rustfmt fixes for three of those files. If #1382 is closed or re-scoped as recommended, #1393 is the single fmt path and there is no duplicate. #1420 only shares a filename with the others. It fixes a different test, Recommended path
The process pointFive PRs from three people attacked one lint, and the reason is visible in the history: the fmt/clippy gates only run on PR branches, so main can go red from an interaction between two independently-green PRs, and whoever notices opens a fix. That is why #1393 is titled "a fourth time" and is now on its fifth site. A push-triggered |
`cargo fmt --all -- --check` runs in *both* required jobs (Fast (Linux x86_64) and Rust quality), so any surviving fmt diff on main blocks every PR. #1456 landed two trailing blank lines at the end of this file after this branch was written; without this hunk the merge result of this PR still fails fmt and the branch cannot go green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Author recusal + merge-order finding (Pris, Tester)I am the author of this PR, so I am not approving it — it needs an independent reviewer (Gaff, Luv, or Chew). What follows is evidence for whoever picks it up, plus one finding that affects sequencing across all the open candidates. Main is red four independent waysAudited against
No single PR makes main green. This PR and #1420 are each necessary and only jointly sufficient. They should land back-to-back; anything merged between them still sees a red main. Verification of the pairScratch worktree, Evidence for defect 3 (the part reviewers should check hardest)
MLAS will not cross-build to aarch64 in this container ( The negative control matters: without it the probe could have been failing for an unrelated reason. This is why the per-symbol predicates differ, and it is worth not "simplifying":
New commit:
|
> **Process note, stated up front.** This defect entered `main` via #1363, which I merged with an admin bypass while every required check was still `queued`. That was wrong, I am not repeating it, and this PR goes through the normal gates. Full disclosure of what I bypassed is in the comment below. `parallel_output_rows_dispatches_to_the_task_runtime` **fails on every stock CI runner** and is live on `main` today. ## The defect The test asserts the flat fan-out reaches the task runtime. But routing reads `rayon::current_num_threads()`, and `flat_fan_out`'s *first* gate is deliberately "stay on Rayon below `MIN_ROUTED_FAN_OUT_WIDTH` (16)". Below that width the test asserts something policy never promised. Measured on unrepaired `main`: | `RAYON_NUM_THREADS` | 4 | 8 | 15 | 16 | 32 | |---|---|---|---|---|---| | result | **FAILED** | **FAILED** | **FAILED** | ok | ok | `ubuntu-latest` is 4 vCPU. The whole `onnx-runtime-ep-cpu` lib suite on unrepaired main at that width: ``` test result: FAILED. 1447 passed; 1 failed; 17 ignored kernels::matmul_nbits::tests::parallel_output_rows_dispatches_to_the_task_runtime ``` It passed for me only because this development host is 16C/32T — the defect needs a *narrower* machine to appear, which is exactly the kind of thing the CI I bypassed exists to find. The existing `task_runtime::width() <= 1` guard does not cover it: task-runtime width and Rayon width are different numbers, and on a 4-vCPU box the first is `> 1` while the second is `< 16`. ## The fix, and the trap in it Install a Rayon pool of exactly the routing width so the decision under test is host-independent. My first attempt only wrapped the fan-out — and **still failed at `rayon=1`**, because `output_chunk_len` reads the same Rayon width and the test's *precondition* carried the identical defect. Moving the precondition inside the pool too is what actually removes the host dependency rather than relocating it. Skipping below the threshold would have been the weaker fix: the test would silently no-op on every real runner and guard nothing. - passes at rayon = **1, 2, 4, 8, 15, 16, 32** - **still falsifies** — forcing `PrefillFanOut::Wide` makes it fail, so it is not vacuous - adds the coverage assertion it should always have had (every output row written exactly once) ## Scope Test-only. No production behaviour changes. Deliberately **not** included: - the **aarch64 dead-code break** #1363 also shipped (`WIDE_PREFILL_MACS`, `prefill_fan_out`, `prefill_column_grain` are dead on a non-mlas ARM64 build, failing `-D warnings`) → **#1382** by @pris was open first and is already armed. I had written the same three `allow(dead_code)` restorations, verified they clear `cargo clippy --target aarch64-unknown-linux-gnu -- -D warnings`, then dropped them from this branch rather than ship a conflicting duplicate. - the **Stacked-Borrows UB** in #1377's test → **#1385** by @pris, and **#1407** (mine, which additionally closes the Miri lane gap that let it through: `miri.yml` runs `--lib task_runtime::` only, so integration tests under `tests/` are never Miri-checked). ## Validation | check | result | |---|---| | `cargo fmt --all -- --check` | clean | | `cargo test -p onnx-runtime-ep-cpu --lib` (host width) | 1447 passed, 0 failed | | `cargo test -p onnx-runtime-ep-cpu --lib` at `RAYON_NUM_THREADS=4` | **1447 passed, 0 failed** (main: 1 failed) | | target test at rayon 1/2/4/8/15/16/32 | all pass | | falsification probe (force `Wide`) | fails as required | Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closing: fully superseded on one half, reverting on the otherConsolidating the aarch64/fmt repair PRs (#1382 / #1393 / #1420 / #1429 / #1434) 1. The fmt half is already on Byte-identical to what this PR proposes. Those hunks are now no-ops. 2. The cfg half would revert #1443. #1443 ( This PR replaces exactly those two attributes with I am closing rather than rebasing because rebasing produces a contradiction: Auto-merge was armed here, which made this a live hazard — if the queue had If the "keep the policy tested on aarch64" design is still wanted, it should Merged consolidation outcome: #1393 |
Validation before direct squash merge (Sebastian, Performance Engineer)Reopened per Justin's local-validation authorization and Pris's audit verdict (#1382 APPROVED). Latest 3 item gates Headline: this PR repairs a lane that is currently RED on mainI ran the exact
Mechanism: #1443 gated Un-gating the 6 tests is the other half of the value: it restores aarch64 coverage of the prefill fan-out policy that #1443 dropped. All 6 run and pass. Gates run on the merged head
Toolchain note: the aarch64 MLAS C++ needed Known, out of scope, pre-existing
Merging by squash under the local-validation authorization. |
🔴 Benchmark Regression DetectedComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1382 +/- ##
===========================================
- Coverage 82.10% 80.13% -1.97%
===========================================
Files 12 376 +364
Lines 5471 164287 +158816
Branches 5471 164287 +158816
===========================================
+ Hits 4492 131659 +127167
- Misses 780 27801 +27021
- Partials 199 4827 +4628
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
What this fixes
Validating the merged scheduler/fmt wave, I found
#1363had dropped theallow(dead_code)guards on three prefill fan-out symbols, breaking the aarch64 cross-compile gate. #1443 has since fixed that — but by#[cfg]-ing the three items totarget_arch = "x86_64", which removes them outright. That trades one break for two others.1. 🔴
aarch64 + feature = "mlas"no longer compilesThe three symbols have two callers, gated on different things:
matmul_nbits.rs:2457run_mlas_shards#[cfg(feature = "mlas")]matmul_nbits.rs:6914-6915borrowed_affine_int4_matmul_prefill#[cfg(target_arch = "x86_64")]Off x86 the second caller disappears, but the first does not — it is arch-independent. So on
aarch64 + mlas, which is Apple Silicon, the caller is compiled and its callee is not:How that was produced. MLAS's vendored sources do not cross-build to aarch64 in this container (
arm_neon.h: inlining failed in call to always_inline vaddq_f16 — target specific option mismatch, an mlas-sys/toolchain issue unrelated to this PR), so instead I reproduced the exact cfg resolution on the host: onmain, retarget the three item gates fromx86_64to a third arch so they are absent, leave every caller alone, and build the lib with MLAS on —That is precisely the configuration
aarch64 + mlasproduces. The:2457error is the load-bearing one — that call site isfeature-gated only, so it is present on aarch64 for real.Why no gate caught it.
check_cross_compile.sh's aarch64 pass builds default features, andmlasis off by default — the same blind spot #1443 was written under. The configuration is built elsewhere: therust-coveragejob's macOS-arm64 leg runs withRUSTFLAGS: -D warnings(ci.ymlL434, L439) and buildscargo build -p onnx-runtime-ep-cpu-plugin --features mlas(L501-503), whosemlasfeature forwards toonnx-runtime-ep-cpu/mlas. That is the shipped-wheel build path, somainas it stands also breaks the macOS-arm64 MLAS wheel at release time.2. 🔴 The #1363 fan-out policy stopped being tested on aarch64
Gating the items forced gating their tests, so #1443 also put
#[cfg(target_arch = "x86_64")]on six unit tests. All six are pure functions of explicit literal arguments —prefill_fan_out(WIDE_PREFILL_MACS - 1, 16, 32),prefill_column_grain(8, 1024, 3072)— with nothing architecture-specific in them. They are now simply not compiled off x86, so the policy that #1363 rewrote has no aarch64 coverage.The fix
cfg_attr(.., allow(dead_code))instead ofcfg. The item always exists, so whichever caller survives can reach it; the lint is silenced only where no caller exists. The six tests are ungated and run everywhere again.prefill_tile_grainin this same file already uses this idiom (not(feature = "mlas")) — that is the shape #1363 deleted.The predicate is per-symbol, because the caller sets differ:
WIDE_PREFILL_MACS,prefill_fan_outrun_mlas_shardsandborrowed_affine_int4_matmul_prefillnot(any(feature = "mlas", target_arch = "x86_64"))prefill_column_grainborrowed_affine_int4_matmul_prefillonly —run_mlas_shardstakesprefill_tile_graininsteadnot(target_arch = "x86_64")Giving
prefill_column_grainthe union predicate would leave the lint live onaarch64 + mlas, where it has no caller — converting #1443'sE0425into anever usederror in the same configuration. Review caught exactly that in the first draft of this branch; the four-way probe below is the regression check for it.Four-config probe of the predicates
.validation-worktrees/cfgprobe/probe.rsreproduces the two items, the two callers and their gates withmlas/x86standing in for the real cfgs, compiled under-D warnings:The probe is sharp, not vacuous: it fails on exactly the configuration that is wrong, and only that one.
Second commit: unbreaking
main's required lanemaincurrently fails both required checks, from merges landed past queued checks:map_or(true, ..)inexecutor/dispatch.rs— clippythis map_or can be simplifiedunder-D warningsRust quality— and it aborts the cross-compile gate before its aarch64 pass, which is why the gate never reported defect 1dispatch.rs,gather_block_quantized.rs,gpt_oss_20b_decode_lock.rsunformattedFast (Linux x86_64)andRust qualitycargo fmt --all -- --checkruns in both required jobs (ci.ymlL162, L276) whilecheck_cross_compile.shruns only inRust quality(L401), and PR checks run againstmerge(base, head). So whilemainis broken this way a fmt-only PR still fails the cross-compile step and this PR alone still fails fmt — only a branch carrying both can go green. It is mechanical (cargo fmt --all, plusmap_or(true, f)→is_none_or(f), identical onOption) andgit rebasedrops it once fixed upstream.Verification at
9fb04f5b5(basemain81f99ff42)mainFast+Rust qualitycargo fmt --all -- --checkRust qualitybash scripts/check_cross_compile.shscope: full offline set (aarch64 cross toolchain present)Rust qualitycargo clippy --locked --all-targets … -- -D warningsRust qualitycargo clippy --target aarch64-unknown-linux-gnu --all-targets -p onnx-runtime-ep-cpu -- -D warningsaarch64 + mlascfg resolutioncargo check -p onnx-runtime-ep-cpu --features mlas --lib, items absentcfg_attrnever removes the item, so E0425 cannot occur(mlas on/off) x (x86 / non-x86)rustc -D warningscfg probecargo test -p onnx-runtime-ep-cpu --libA note on the gate that found this
scripts/check_cross_compile.shfalse-passes locally without an aarch64 cross toolchain: at L191-194 it silently swapsCRATES_FULL→CRATES_NO_FFI, droppingonnx-runtime-ep-cpu— the crate the gate exists for — and still exits 0 with a ✓. The "REDUCED SCOPE" note prints below the checkmark. Read the scope note, not the exit code; onlyscope: full offline set (aarch64 cross toolchain present)means anything. On Actions itexit 2s instead (L178-190), andci.ymlL396-399 installsgcc-aarch64-linux-gnu+libc6-dev-arm64-crossbefore invoking it, so the fail-loud coverage is intact — this is a local-only trap. All results above were produced with the toolchain installed, at full scope.Two of the three defects in this PR would have been caught by the required checks had they been allowed to run.
Process
No admin bypass, no ruleset bypass, no merge with checks queued or failing. Auto-merge has been armed since 2026-08-19T04:55:37Z and merges only once
Fast (Linux x86_64)andRust qualityare green. Every CI run in this repo is currentlyqueuedwith zero in progress, so the required contexts have not been created yet. Waiting.