Repository navigation
style: reformat the int4 tail-block slice #1104 left unformatted - #1109
Merged
Merged
Conversation
`cargo fmt --all -- --check` fails on `main` at 9b7a458, so every open PR's `Rust quality` job fails for a reason that has nothing to do with the PR. The merge queue does not re-run formatting on the merge result, which is how this keeps landing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1109 +/- ##
==========================================
- Coverage 80.40% 79.93% -0.48%
==========================================
Files 369 369
Lines 160217 160217
Branches 160217 160217
==========================================
- Hits 128829 128064 -765
- Misses 26655 27423 +768
+ Partials 4733 4730 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
🔴 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
|
justinchuby
added a commit
that referenced
this pull request
Aug 17, 2026
## The bug `Kernel::set_constant_inputs` is how a kernel learns that an input is a weight ORT owns for the life of the session. Kernels use it to decide whether a prepack may be built once and kept, and `MatMulNBits` goes further: `mlas_sqnbit_owns_fp32_compute(can_prepack, …)` gates the **MLAS SQNBit path itself** on the same flag. It was called from exactly one place in the workspace — `onnx-runtime-session`'s executor. The ORT plugin EP never called it. So on the only path a real ORT model takes, every kernel saw every input as a runtime tensor: `MatMulNBits` rebuilt its packed weights on every `Run` *and* ran the slower non-SQNBit kernel, and `QLinearMatMul` re-packed `B` per call. A decoder paid a load-time cost per token. ## Why the existing test could not be reused `node_inputs_all_routable` already distinguishes weights, as "no producer and not a graph input". That is right for the whole model at `GetCapability` time and wrong at `Compile` time: ORT hands a fused node's subgraph over with the initializers it kept inside listed as **graph inputs of that subgraph**, because they are inputs of the fused node. Instrumenting the compile path on a `MatMulNBits` model prints ``` [const] op=MatMulNBits flags=[false, false, false] is_graph_input=[true, true, true] has_producer=[false, false, false] ``` `Graph_GetInitializers` still tells the two apart, so `OutboundGraphReader` now records initializer **names** (no tensor data: the existing `read_initializers_int64` deliberately copies only small int64 tensors, and a 1 GB `B` must not be copied to answer a yes/no question), and `constant_input_flags` keys against that set. ## Measurement Interleaved A/B against **plain ORT through the ORT session API** — the same generated single-node model loaded twice, once with this EP registered and `session.disable_cpu_ep_fallback=1`, once with no EP appended at all. 3 warmups, then 41 interleaved iterations each side, p50/p90 of `Run` only (input `OrtValue`s built once). Assignment is asserted before any timing, so a ratio is never reported for a node ORT actually ran. Host: AMD EPYC 9V74, 32 vCPU / 16 cores, AVX2+FMA+F16C (no AVX-512/VNNI), ORT 1.27.0, release build **with `--features onnx-runtime-ep-cpu/mlas`**, K=N=2048. The box is shared, so p90s are contended; the `ours ms` columns are the load-bearing evidence and the ORT column moves between runs. | case | ours ms before | ours ms after | speedup | ours/ORT before | ours/ORT after | |---|---|---|---|---|---| | `MatMulNBits` int4 M=1 | 1.144 | **0.425** | 2.7x | 15.0 | 2.34 | | `MatMulNBits` int4 M=128 | 108.4 | **7.79** | 13.9x | 91.6 | 6.36 | | `MatMulNBits` int4 f16-act M=1 | 3.659 | **0.416** | 8.8x | 16.0 | 1.80 | | `MatMulNBits` int8 M=1 | 8.226 | **4.845** | 1.7x | 3.85 | 2.53 | | `MatMulNBits` int8 M=256 | 47.98 | **12.04** | 4.0x | 5.03 | **1.05** | | `QLinearMatMul` u8 M=1 | 2.234 | **0.079** | 28x | 43.6 | 1.84 | | `QLinearMatMul` u8 M=128 | 13.32 | **11.11** | 1.2x | 2.15 | 3.78 | | `QLinearMatMul` i8 M=1 | 1.968 | **0.091** | 21.6x | 0.52 | **0.055** | | `MatMul` f32 M=1 (control) | 0.147 | 0.178 | — | 1.72 | 1.49 | | `MatMul` f32 M=128 (control) | 9.675 | 8.698 | — | 1.36 | 1.43 | | `MatMul` f16 M=1 (control) | 1.368 | 2.358 | — | 1.08 | 0.37 | | `MatMul` f16 M=128 (control) | 21.04 | 22.64 | — | 7.04 | 2.84 | The four dense cases are the control: they declare **both** operands as graph inputs, so they have no initializer, no flag changes for them, and they move only with host noise. Every case that moved has a constant weight. Cold session-creation time is reported by the same harness (`cold_ours_ms`) and does not regress: the prepack moved from per-`Run` to first-`Run`, not to `CreateSession`. ## Still losing after this change Reporting all of it, per the standing rule: - `MatMulNBits` int4 M=128 at 6.36x and M=1 at 2.34x. - `QLinearMatMul` u8 M=128 at 3.78x (u8 M=1 is now 1.84x). - `MatMul` f16 M=128 at 2.84x — no initializer involved, so untouched here. - `MatMul` f32 at 1.43-1.49x — likewise. These are kernel gaps, not wiring gaps, and they stay open on my task list. **Separate finding, not fixed here:** the plugin cdylib is built **without** the `mlas` feature by default, and `docs/architecture/CROSS_PLATFORM.md` documents that feature as x86-64-Linux-only. Rebuilding the same benchmark against the default cdylib gives, after this fix, `MatMulNBits` int4 M=128 at 80x and `QLinearMatMul` u8 M=128 at 56x ours/ORT: the pure-Rust paths are the ones a portable build actually ships, and they are far behind. That is a distinct piece of work and needs its own PR. ## Tests - `constant_weights_are_reported_to_kernels_as_constant` (new, `plugin_ort_e2e`) — reads the counter this PR exports from the very cdylib ORT loaded and asserts symmetric int4 reports 2 constant inputs, asymmetric int8 reports 3, and the all-graph-input dense case reports **0**. A wiring that marks everything constant fails the third assertion; caching a prepack of an activation is wrong, not merely slow. Falsified by reverting `constant_input_flags` to the producer/`is_graph_input` form: `reported 0 constant inputs, expected 2`. - `initializers_are_constant_even_when_the_subgraph_calls_them_inputs` and `nothing_is_constant_without_an_initializer_list` (new, `onnx-runtime-ep-plugin` unit) — hardware-independent, pin the exact shape ORT presents and the absent-optional-input case. - `no_matmul_family_node_escapes_to_the_ort_cpu_ep` (extended) — every case now runs a **second time in the same session** with the activation rotated by one element, and is compared against ORT again. A weight cache still matches ORT; anything activation-derived that outlived the call does not. ORT is the oracle for whether a case can detect staleness at all (a saturating u8 output cannot), and the suite asserts at least 5 of the 10 cases are activation-sensitive so the check cannot go vacuous. - `plugin_path_ab_vs_plain_ort` (new, `#[ignore]` + `NXRT_MM_BENCH=1`) — the harness that produced the table, committed so the numbers are reproducible rather than asserted. - `plugin_export_abi` — the two new exports are added to both the required-symbol list and the unexpected-symbol filter. ## Verification - `cargo test -p onnx-runtime-ep-cpu-plugin` — 51 + 9 + 6 + 1 passed, 1 ignored (the benchmark) - `cargo test -p onnx-runtime-ep-plugin --lib` — 231 passed (was 229) - `cargo fmt --all -- --check` clean (given #1109, which repairs `main`) - `cargo clippy --all-targets` clean with and without `--features onnx-runtime-ep-cpu/mlas` --- ## Post-review fix (commit 2): overridable initializers are not constant Independent review found a correctness hole in commit 1, and it was right. `Graph_GetInitializers` is documented in ORT's own header as including *"constant and non-constant initializers"*. From ONNX IR version 4, an initializer whose name also appears in the graph's input list is only a **default**: the caller may hand a different tensor in on any `Run`. ORT says so at load time: ``` [W:onnxruntime:, graph.cc:1419 Graph] Initializer B appears in graph inputs and will not be treated as constant value/weight. ``` Keying the flags on that name list alone therefore marked such a value constant, and `MatMulNBits` would have cached a session-lifetime prepack of the default weight and returned its answer for every later `Run` — a wrong result, not a slow one. **Fix:** each name from `Graph_GetInitializers` is now filtered through `ValueInfo_IsConstantInitializer` (ORT ≥1.23, reads no tensor data). Fail-closed: if the entry point is missing or the call errors, the initializer is treated as **non-constant**. A false negative costs a repeated prepack; a false positive costs correctness. **New regression test** `an_overridable_initializer_is_not_treated_as_a_constant_weight` builds exactly that model — `B` and `scales` declared both as initializers and as graph inputs — and asserts two independent things: 1. the EP reports **0** constant inputs (the classification), and 2. running the session twice with two different `B` payloads matches plain ORT run for run, with the test first proving via ORT that the two payloads *do* produce different outputs (the consequence — a stale prepack would return run one's answer twice). Falsified by deleting the `is_constant` filter and rebuilding the cdylib: ``` assertion `left == right` failed: an initializer that is also a graph input may be replaced on any Run, but this EP reported 2 of them as constant weights ``` The two doc comments that asserted "every initializer is constant" are corrected, and the unit test is renamed to `constant_initializers_are_flagged_even_when_the_subgraph_calls_them_inputs`. ### The measured win survives the stricter gate Re-measured after the fix on the same host, same build (`--features onnx-runtime-ep-cpu/mlas`, release, K=N=2048, 3 warmups + 41 interleaved iterations, p50 ms): | case | ours before PR | ours after commit 1 | ours after commit 2 | |---|---|---|---| | `nbits4_m1` | 1.144 | 0.425 | 0.400 | | `nbits4_m128` | 108.4 | 7.79 | 8.80 | | `nbits4_f16_m1` | 3.659 | 0.416 | 0.437 | | `nbits8_m256` | 47.98 | 12.04 | 13.86 | | `qlinear_u8_m1` | 2.234 | 0.079 | 0.092 | | `qlinear_i8_m1` | 1.968 | 0.091 | 0.092 | The benchmark models declare their weights as initializers only, so they are still constant under the stricter rule; the residual movement is host noise on a shared machine (ORT's own p50 for `nbits4_m1` moved 0.125 → 0.077 ms between the two runs, so the *ratio* column is the noisier of the two and the absolute `ours` column is the honest comparison here). ### Verification (re-run) - `cargo test -p onnx-runtime-ep-cpu-plugin --test plugin_ort_e2e` — 52 passed, 1 ignored (was 51 + 1) - `cargo test -p onnx-runtime-ep-plugin --lib` — 231 passed - `cargo fmt --all -- --check` clean (rebased onto #1109, now merged) - `cargo clippy -p onnx-runtime-ep-cpu-plugin -p onnx-runtime-ep-plugin --all-targets` clean with and without `--features onnx-runtime-ep-cpu/mlas` --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 17, 2026
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.
cargo fmt --all -- --checkfails onmainat 9b7a458 (#1104), incrates/onnx-runtime-ep-cpu/src/kernels/matmul_nbits.rs:6239. Every open PR'sRust qualityjob therefore fails for a reason unrelated to the PR.This is the third time (see #1089, #1102): the merge queue does not re-run formatting against the merge result, so an individually-green PR can still land unformatted
main.Pure
cargo fmt -p onnx-runtime-ep-cpuoutput, two lines, no behaviour change.