Repository navigation
style: run cargo fmt on onnx-runtime-session (unblocks Rust quality on main) - #1686
Merged
Merged
Conversation
…n main) `Rust quality` has been failing on `main` since #828 landed: `cargo fmt --all -- --check` reports five hunks in `crates/onnx-runtime-session/src/executor/{geometry,tests}.rs`. Reproduced locally and against the CI log for job 96899749480 on `0f40538b2`, which names the same `geometry.rs:582`. This is `cargo fmt --all` output only — no semantic change. It is split out rather than folded into a feature branch so the unblock is reviewable on its own. Refs #1600. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1686 +/- ##
==========================================
- Coverage 80.55% 80.20% -0.36%
==========================================
Files 394 394
Lines 184855 184853 -2
Branches 184855 184853 -2
==========================================
- Hits 148911 148258 -653
- Misses 30819 31468 +649
- Partials 5125 5127 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
justinchuby
marked this pull request as ready for review
August 21, 2026 22:20
Owner
Author
|
CI result, read honestly.
Four jobs still fail. They are pre-existing main breakage, not caused by this PR, which changes nothing but whitespace in
All four are downstream of #828's inference-metadata IR redesign, in crates this PR does not touch. Tracked by #1600. Merging so the formatting gate stops masking real failures on every other PR. |
justinchuby
added a commit
that referenced
this pull request
Aug 21, 2026
`2ef5e793e` landed `crates/onnx-runtime-ep-cuda/src/provider.rs` unformatted and with a `clippy::needless_question_mark`. That takes out **four jobs on every PR in the repo**, including mine: | job | failure | |---|---| | `Rust quality` | `Diff in .../onnx-runtime-ep-cuda/src/provider.rs:2807` | | `Fast (Linux x86_64)` | same `cargo fmt --all -- --check` diff | | `CUDA compile (Windows x86_64)` | `error: enclosing \`Ok\` and \`?\` operator are unneeded` at `provider.rs:1636` | | `CUDA compile (Linux x86_64)` | same clippy error | This is `cargo fmt --all` output plus clippy's own suggested rewrite. The `?` removal is semantically identical — the function returns the same `Result<bool>` the callee already returns. Verified locally on `59368509d` with the **exact CI commands**: | command | result | |---|---| | `cargo fmt --all -- --check` | clean | | `cargo clippy -p onnx-runtime-ep-cuda --features cuda -- -D warnings` | clean | | `cargo check --locked -p onnx-runtime-ep-cuda -p onnx-runtime-python --features onnx-runtime-python/cuda` | clean | | `cargo test -p onnx-runtime-ep-cuda` | ok, 0 failed | Not fixed here, deliberately: `cargo clippy --all-targets -p onnx-runtime-ep-cuda` reports 16 further errors in that crate's *test* code (e.g. unsafe-block lints at `provider.rs:2154`). Those are pre-existing, are not in any CI gate, and belong to the crate's owner — widening this unblock into them would make it unreviewable. This is the second formatting unblock in a day (#1686 was the same for `onnx-runtime-session`). Refs #1600. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 22, 2026
… closing #1381 (#1687) Closes #1381. #1381 asked why equivalent-math f16 `Gemm` and `MatMul` decode diverge. The answer is **layout, not operator plumbing**: the same weight was read by two different kernels depending on how it happened to be stored, and one of them is much slower *and* much less accurate than the other. ## The divergence, enumerated Before this change, at m=1 with a constant 16-bit weight (min of 6 interleaved reps, pinned to physical cores — see #1680 for why unpinned multi-thread numbers on this host are not trustworthy): | operator | stored order | kernel taken | |---|---|---| | `Gemm` transB=1 | `[N,K]` | `gemv_half_nk` | | `Gemm` transB=0 | `[K,N]` | `gemv_half_kn` | | `MatMul` | `[K,N]` | `gemv_half_kn` | | `FusedMatMulBias` | `[K,N]` | **no 16-bit GEMV at all** (f32 prepack) | The existing dispatch comment claimed this was already closed — "both operators, both stored orders and both 16-bit formats reach the same GEMV backend". Same *backend*, not same *kernel*. Route counters confirmed the fourth row empirically (`matmul_gemv=1 fused_gemv=0`), which matters because the optimizer fuses `MatMul + Add(bias)`, so a biased projection runs the fused op. ## Root cause: page-crossing stride, not arithmetic Direct kernel A/B on identical bytes: | shape (K x N) | `gemv_half_kn` | `gemv_half_nk` | speedup | |---|---|---|---| | qkv 4096 x 6144 | 5022 us | 1688 us | **2.98x** | | o 4096 x 4096 | 1967 us | 979 us | **2.01x** | | gate 4096 x 14336 | 11332 us | 3939 us | **2.88x** | | down 14336 x 4096 | 11015 us | 7068 us | **1.56x** | `[K,N]` walks a column tile with an `n * 2`-byte stride between consecutive `p` — 12 KB at n=6144 — so consecutive reads cross pages and the L2 prefetcher, which does not cross a page boundary, cannot run ahead. Byte rates say the same thing: 10.0-17.1 GB/s versus 16.6-34.3 GB/s, against ~83 GB/s achievable on this host. `[N,K]` makes each output one contiguous `k`-element dot. ### Negative result recorded first Software prefetch (`_mm_prefetch`, distance 12) into the strided inner loop was tried **before** the layout change because it costs no memory. It made `kn` *worse*: 5580 us vs 5022 us on qkv. Reverted. The stride penalty is not prefetch-recoverable, which is what justifies paying memory for it. ## Accuracy moves the same way There is no speed/accuracy trade here to weigh. `kn` carries one serial accumulator per column across the whole contraction; `nk` carries four combined pairwise as `(acc0+acc1)+(acc2+acc3)`. Max relative error against an f64 oracle: | K | `gemv_half_kn` | `gemv_half_nk` | `nk` better by | |---|---|---|---| | 4096 | 1.961e-3 | 7.147e-4 | 2.7x | | 1024 | 2.007e-4 | 3.707e-5 | 5.4x | | 256 | 9.689e-5 | 1.046e-5 | 9.3x | A first version of that test used `* 0.125` / `* 0.0625` operands. Those are exactly representable, so every partial sum is exact and the test reported zero error and 100% bit-identity — it could not detect the effect it existed to measure. Redone with non-representable wide-dynamic-range data, only ~3% of elements are bit-identical. ## The change `half_decode_gemv_dispatch` routes a constant `[K,N]` 16-bit weight through the existing transpose cache (previously Apple-only) into `gemv_half_nk`, for both operators. End-to-end MatMul decode: | shape | before | after | speedup | |---|---|---|---| | qkv 4096 x 6144 | 5166 us | 1830 us | **2.82x** | | o 4096 x 4096 | 2712 us | 1188 us | **2.28x** | | gate 4096 x 14336 | 11772 us | 4069 us | **2.89x** | | down 14336 x 4096 | 11619 us | 7086 us | **1.64x** | MatMul now also beats the fused f32-prepack path (1830 vs 2845 us on qkv) while holding half the bytes resident. `every_operator_and_stored_order_decode_bit_identically` asserts all three operator/order combinations agree **bit for bit**, with route counters proving assignment == execution. Mutation check: `if false &&` on the dispatch fails it. ## Two costs, both deliberate, both disclosed **1. Resident memory.** The transpose is `2*K*N` bytes per constant 16-bit decode weight. `node_weight_transpose_cache_bytes` — what `engine/load.rs` budgets against under the #1056 admission governor — was `cfg(macos/ios)` for `MatMul`, so on x86 this copy would have been **invisible to the memory plan** and the plan would have under-budgeted by gigabytes. It now predicts the x86 case. `FusedMatMulBias` is deliberately *excluded*, because it still has no x86 16-bit GEMV and budgeting it would over-reserve on every fused projection. Two guard tests encoded the opposite decision ("a transposed variant would cost a permanent 2*K*N bytes"). Rather than delete them, they are rewritten to assert the stronger invariant they were reaching for: no **unbudgeted** copy, and never an f32 widening. They now check `predicted >= retained` against the predictor itself. **2. Declining the cache now changes output bits**, because it changes which kernel runs. That weakens the #1056 doc's framing of admission as a "pure performance tradeoff". `a_declined_transpose_cache_falls_back_to_reading_in_place` pins the fallback behaviour explicitly. ## A real bug found on the way The f16 transpose cache stores raw `u16` keyed `(addr, k, n)` with **no dtype** — safe only while exactly one dtype used it. Routing bf16 through it let a bf16 weight hit an f16 entry left behind by a freed buffer at a recycled address: `Bf16 column 0: -0.000021640852 != -0.8984375`, reproducible only when run in company. Guarding the *view* dtype is not enough; the *key* needs the discriminator. `WeightTransposeKey` now carries a format tag (0 = f16, preserving existing keys), with a regression test that asserts on the key rather than hoping for address reuse. ## Validation Branch has `origin/main` (`4c15a64b3`) merged in. | gate | result | |---|---| | `cargo test -p onnx-runtime-ep-cpu` | **1579 passed, 0 failed** (3x consecutive) | | `--features mlas` | pass | | `--no-default-features` | pass | | `--all-features` check | pass | | clippy offline / native-backend / aarch64 `-D warnings` | pass | | aarch64-unknown-linux-gnu suite under QEMU | pass | | `cargo xwin check --target aarch64-pc-windows-msvc` | pass | | no-MLAS default artifact | pass | | `check_cross_compile.sh` + 8 repo scripts | pass | | `cargo fmt --all -- --check` | **fails on `onnx-runtime-session`, pre-existing** | `PASS=20 FAIL=1 SKIP=0`. The one failure is `cargo fmt` on `crates/onnx-runtime-session/src/executor/{geometry,tests}.rs`, which is **byte-identical to `origin/main`** here and is the same failure CI reports on main (job `96899749480`, `geometry.rs:582`). Fixed separately in #1686 rather than folded in. Also filed while validating: **#1685** — `sdpa`'s MLAS fast path intermittently leaves an 8-row hole in its output (SIGSEGV / "identity specialization diverged"), ~3% on clean `main` under `--features mlas`, reproducible on a single test single-threaded. Not caused by this branch; rate is the same on clean `main`. ## Out of scope, quantified, not fixed here `FusedMatMulBias` still takes no 16-bit GEMV on x86 (2845 us on qkv vs MatMul's 1830 us). It is a separate mechanism with its own memory-plan consequence and deserves its own change; the predictor deliberately does not budget for it. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rust qualityis red onmainand has been since #828:cargo fmt --all -- --checkreports five hunks incrates/onnx-runtime-session/src/executor/geometry.rsand.../tests.rs.96899749480on0f40538b2fails withDiff in .../executor/geometry.rs:582.4c15a64b3; both files are byte-identical toorigin/main, so this is not branch-local drift.This PR is
cargo fmt --alloutput and nothing else. Verified locally on4c15a64b3:cargo fmt --all -- --checkcargo check --locked -p onnx-runtime-sessioncargo test --locked -p onnx-runtime-sessionSplit out of my MatMul work rather than folded into it, so the unblock is reviewable on its own and lands for everyone.
Refs #1600.