Repository navigation
fix(cpu): keep the int4 prefill fan-out policy off the aarch64 dead-code lint - #1429
justinchuby wants to merge 2 commits into
Conversation
…ode lint `Rust quality`'s cross-arch step fails on main: `WIDE_PREFILL_MACS`, `prefill_fan_out` and `prefill_column_grain` are consumed only by `borrowed_affine_int4_matmul_prefill`, which is `#[cfg(target_arch = "x86_64")]`, so a lib-only build for aarch64 sees three unused items under `-D warnings`. Bisected to bf72272 (#1363); its parent dcea14c is clean. Mark them `#[cfg_attr(not(target_arch = "x86_64"), allow(dead_code))]` rather than `cfg`-gating them, because the unit tests that pin the policy are portable and reference all three -- gating would fix the aarch64 lib build by breaking the aarch64 test build. Lint-only, so no behaviour changes anywhere. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review found two inaccuracies in the prose. `prefill_fan_out` also has a caller in the `mlas`-gated `run_mlas_shards`, so "only the x86_64 prefill entry point" was only true because `mlas` is off by default; say that. And bf72272 did not introduce the two older items -- it removed their `cfg_attr(not(feature = "mlas"), allow(dead_code))` guards while adding an x86-only consumer, which is what actually lost them their cover. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Superseded — closing without merging, and not merging any overlapping aarch64 fix. Two things changed since I opened this:
Deferring to #1382. Branch |
Pull request was closed
Overlap audit: #1382 / #1393 / #1420 / #1429, and why only two of them should mergeAsked 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 |
🔴 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
|
|
Confirmed superseded: #1382 merged as Independent evidence that this was the correct fix rather than a cosmetic one: the exact No action needed here — closing stands. cc @justinchuby (and Leon for the duplicate-path audit): #1382 is the single surviving repair, no duplicate landed. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1429 +/- ##
===========================================
+ Coverage 80.26% 82.64% +2.38%
===========================================
Files 362 12 -350
Lines 157159 5475 -151684
Branches 157159 5475 -151684
===========================================
- Hits 126137 4525 -121612
+ Misses 26406 757 -25649
+ Partials 4616 193 -4423
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Rust qualityis red onmainright now. Its cross-arch step fails:Rust qualityis a required check, so this blocks every open PR, not just this one.Cause
In a default-feature build all three are reachable only from
borrowed_affine_int4_matmul_prefill, which is#[cfg(target_arch = "x86_64")].(
prefill_fan_outhas one further caller in themlas-gatedrun_mlas_shards, butmlasis not a default feature, so the lint's build compiles it out too.) On anynon-x86 target the lib-only build therefore has no consumer left, and the step runs with
-D warnings.This is exactly the failure mode
scripts/check_cross_compile.shdocuments in its ownerror text (the
#1037case).Bisected to
bf722725a(#1363, "route the int4 flat output-row fan-out through the taskruntime"). Precisely, that commit removed the
#[cfg_attr(not(feature = "mlas"), allow(dead_code))]guards that had been protecting the pre-existingWIDE_PREFILL_MACSand
prefill_fan_out, addedprefill_column_grain, and introduced the x86-onlyconsumer -- so the items lost their only dead-code cover in the same change that made
them x86-only:
dcea14c74(parent)bf722725a(#1363)Fix
#[cfg_attr(not(target_arch = "x86_64"), allow(dead_code))]on the three items, which isthe second remedy the check prescribes.
cfg-gating them outright is the other option butit is wrong here: the unit tests
(
small_prefill_work_stays_on_the_task_runtime,prefill_column_grain_*, and friends)reference all three and are portable, so gating the items would break the aarch64 test
build to fix the aarch64 lib build.
No behaviour change on any target: an
allowattribute is lint-only.Verification
cargo clippy --target aarch64-unknown-linux-gnu -p onnx-runtime-ep-cpu --lib -- -D warningsbash scripts/check_cross_compile.sh(full offline set, both dimensions)cargo clippy --locked --all-targets -p onnx-runtime-ep-cpu(-D warnings)cargo test --locked -p onnx-runtime-ep-cpucargo fmt --all --checkFound while replicating the CI lanes locally on
main@f8f3878ba, because Actions hasnot concluded a run on this repo in some time. Every other lane of
Fast (Linux x86_64)and
Rust qualityis green on that commit (3942 tests over 188 targets); this was the onlyred.