Repository navigation
fix(cpu): undo the 2.3x unary regression #1130 shipped - #1136
Merged
Merged
Conversation
#1130 wrapped `Clip` in `run_chunked` from `selection.rs`. `run_chunked` is generic, so that added an instantiation in a new module. The runtime path of every other unary op was untouched, and every test still passed, but the crate's codegen units repartitioned and the AVX2 unary kernels in simd_activations.rs stopped being vectorised. Measured at n = 65536, one thread, same commit, only that instantiation moved: Sqrt 21.5 -> 48.5 us Tanh 30.4 -> 54.7 us Sigmoid 32.2 -> 57.1 us QuickGelu 42.6 -> 64.6 us FastGelu 55.7 -> 79.2 us Bisected file by file: reverting selection.rs alone restored all five, and reverting relu.rs or conv.rs alone restored none of them. `run_chunked` is private to simd_activations.rs again. Callers elsewhere go through `run_chunked_fn`, which is not generic, or `clip_chunked`, both instantiated in that module. `clip_chunked` takes the serial decision itself so the short case stays a direct call. A test now walks the crate source and fails if any other module instantiates `run_chunked`. Verified to falsify. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1136 +/- ##
========================================
Coverage 79.94% 79.94%
========================================
Files 368 368
Lines 160611 160826 +215
Branches 160611 160826 +215
========================================
+ Hits 128395 128579 +184
- Misses 27499 27525 +26
- Partials 4717 4722 +5
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
|
Independent review findings on #1136. MINOR: `run_chunked_fn` was ungated while both callers are mlas-gated, so the non-mlas build warned it was never used. Gated to match. MINOR: inserting the two new functions left `parallel_dispatches`' doc comment attached to `run_chunked_fn`, where it contradicted the function below it. Moved back. MINOR: the source guard matched the bare substring `run_chunked(`, which missed `run_chunked ::<T>(` and would trip on prose. It now matches the identifier on a word boundary and requires the next token to open a call or a turbofish, so it rejects `run_chunked_fn`, `run_chunked_rows`, test names and string literals. Verified to falsify against the turbofish spelling. NIT: the failure message said 2.3x for all three of Sqrt, Tanh and Sigmoid; only Sqrt is 2.3x. Corrected to "up to 2.3x". Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two NITs from the re-review. The guard's comment claimed it rejects prose inside string literals. It does not: it is a substring heuristic, so a call reached through an aliased import, split across two lines or generated by a macro slips past, and the literal text inside a string would trip it. The comment now says so, and says what it does catch, which is the accidental case. The PR body said "2.3x on five ops". Only Sqrt is 2.3x; the other four are 1.5-1.8x. Same imprecision that was already corrected in the guard's failure message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
marked this pull request as ready for review
August 17, 2026 19:25
This was referenced Aug 18, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 18, 2026
…oss the activation family) The AVX2 elementwise kernels only vectorise when the compiler can see the dispatcher, the chunk loop and the per-vector body together. The release profile's default sixteen codegen units splits that chain and the loops come out scalar. #1136 found one *cause* of the repartition -- instantiating the generic `run_chunked` from another module -- and fixed it by making `run_chunked` private. That closed one door; the compiler was still free to split the crate on its own, and it does. Pin `codegen-units = 1` for `onnx-runtime-ep-cpu` alone. The rest of the workspace keeps parallel codegen; this crate goes from ~19 s to ~65 s. Kernel level (`activation_bench`, 5 interleaved rounds, medians, `taskset -c 8-15`): 77 of 105 cases improve by more than 1.15x, worst case `Sqrt` f32 at 4096 elements at 2.65x. The 16-element shapes are flat at 0.99-1.00x, which is the control: they are dispatch-bound, so a de-vectorized loop cannot show up in them, and nothing else should. Session level, against ORT's own CPU EP through the ORT API, `intra_op = 1` on both sides, 31 interleaved iterations, p50 of whole-`Run`, as `ours / ORT` where above 1.00 means we are slower: | case, 1 Mi f32 | before | after | |---|---|---| | `Tanh` | 2.45 | 1.43 | | `Sigmoid` | 2.34 | 1.38 | | `Erf` | 2.06 | 1.57 | | `Gelu` (tanh) | 2.02 | 1.40 | | `Gelu` (exact) | 1.95 | 1.51 | | `Exp` | 2.41 | 1.29 | | `FastGelu` | 2.00 | 1.40 | | `QuickGelu` | 1.45 | 0.95 | | `Sqrt` | 1.53 | 0.72 | | `Relu` | 1.04 | 1.03 | `Sqrt` and `QuickGelu` cross from loss to win; `Relu` is the control and does not move, being memory-bound at this size. The f16 rows move the same way (`Tanh` 2.06 -> 1.36, `Exp` 2.02 -> 1.33), as does the 4 Ki grid (`Tanh` 2.06 -> 1.52). This also explains a discrepancy: `main` had been measurably slower than the ratios published in `CPU_ACTIVATION_GAPS.md` -- `Tanh` at 1 Mi was 0.41 against a published 0.82 -- with no source change to account for it. The published numbers were taken from a build whose partition happened to be favourable, and were not reachable on `main` until this pin. Two supporting pieces: * `unary_bench_cases()` extends the session A/B harness, which until now only covered the matmul family, to the elementwise grid the activation doc is written against. Same harness, same pinning refusal, so a number from it is directly comparable to a matmul number. * `codegen_units_are_pinned` reads the setting back out of the workspace manifest. A build setting is exactly the kind of thing a rebase drops silently -- no test fails, no path changes, everything just gets slower -- which is how this class of regression reached `main` in the first place. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 18, 2026
…oss the activation family) (#1174) ## What Pin `codegen-units = 1` for `onnx-runtime-ep-cpu` in the workspace release profile. The AVX2 elementwise kernels only vectorise when the compiler can see the dispatcher, the chunk loop and the per-vector body together. The default sixteen codegen units split that chain and the loops come out **scalar**. #1136 found one *cause* of the repartition — instantiating the generic `run_chunked` from another module — and fixed it by making `run_chunked` private. That closed one door. The compiler was still free to split the crate on its own, and it does. ## Why this is a regression, not a tuning knob `main` has been measurably slower than the ratios this repo publishes in `docs/performance/CPU_ACTIVATION_GAPS.md`, with no source change to account for it. `Tanh` at 1 Mi is published at 0.82 of ORT; measured on `main` today it is **0.41**. With the pin it is 0.70. The published numbers came from a build whose partition happened to be favourable and were not reachable on `main` at all. ## Evidence — kernel level `activation_bench`, two binaries built from **identical source** (default vs `CARGO_PROFILE_RELEASE_CODEGEN_UNITS=1`), run interleaved 5 rounds, medians, `taskset -c 8-15`. 105 cases (7 ops × 5 shapes × 3 dtypes). | case | 16 CGUs | 1 CGU | ratio | |---|---|---|---| | `Sqrt` f32 / 4096 | 0.639 ns/elem | 0.241 | **2.65x** | | `Sqrt` f32 / 3072 | 0.642 | 0.244 | 2.63x | | `Tanh` f32 / 3072 | 0.731 | 0.381 | 1.92x | | `Tanh` f32 / 2 Mi | 0.189 | 0.099 | 1.90x | | `Sigmoid` f32 / 3072 | 0.735 | 0.389 | 1.89x | | `Sqrt` f16 / 4096 | 0.915 | 0.499 | 1.83x | | `Sigmoid` f32 / 2 Mi | 0.196 | 0.108 | 1.81x | | `QuickGelu` f32 / **16** (control) | 8.556 | 8.657 | 0.99x | | `Tanh` bf16 / **16** (control) | 21.149 | 21.305 | 0.99x | **77 of 105 cases move by more than 1.15x.** The ones that do not are the 16-element shapes — dispatch-bound, so a de-vectorized loop cannot show up in them, and nothing else should. That split is the signature of de-vectorization, not of noise. Reproduced independently through the per-package override actually being landed here (`[profile.release.package.onnx-runtime-ep-cpu]`), same 77/105 and same 2.65x worst case. ## Evidence — session level, against ORT Through the ORT session API, our EP vs ORT's own CPU EP in one process, interleaved iteration by iteration, `intra_op = 1` on **both** sides (the harness refuses a half-pinned comparison), 31 iterations after 3 warmups, p50/p90 of whole-`Run`, `taskset -c 8-15`. Columns are `ours_ms / ort_ms`, so **lower is better and below 1.00 means we win**. ### float32, 1 Mi, one thread | case | before p50 | after p50 | before p90 | after p90 | |---|---|---|---|---| | `Tanh` | 2.449 | **1.433** | 3.135 | 1.381 | | `Sigmoid` | 2.342 | **1.383** | 2.286 | 1.372 | | `Erf` | 2.058 | **1.565** | 2.049 | 1.573 | | `Gelu` (tanh) | 2.020 | **1.397** | 2.017 | 1.379 | | `Gelu` (exact) | 1.951 | **1.507** | 1.934 | 1.497 | | `Exp` | 2.408 | **1.285** | 2.333 | 1.271 | | `FastGelu` | 2.003 | **1.402** | 1.996 | 1.394 | | `QuickGelu` | 1.450 | **0.953** | 1.428 | 0.945 | | `Sqrt` | 1.530 | **0.718** | 1.508 | 0.734 | | `Relu` (control) | 1.038 | 1.033 | 1.060 | 1.058 | `Sqrt` and `QuickGelu` cross from loss to **win**. `Relu` is the control: it is memory-bound at 1 Mi, so the codegen partition cannot move it, and it does not. ### float32 4 Ki and float16 1 Mi, one thread | case | before p50 | after p50 | |---|---|---| | `Tanh` f32 4 Ki | 2.058 | **1.523** | | `Sigmoid` f32 4 Ki | 2.051 | **1.503** | | `Erf` f32 4 Ki | 1.905 | **1.598** | | `Sqrt` f32 4 Ki | 1.616 | **1.150** | | `Tanh` f16 1 Mi | 2.060 | **1.359** | | `Exp` f16 1 Mi | 2.015 | **1.332** | Reproduce: ```sh NXRT_MM_BENCH=1 NXRT_MM_BENCH_THREADS=1 ONNX_GENAI_MLAS_THREADPOOL_THREADS=1 \ NXRT_MM_BENCH_CASE=f32_1m NXRT_MM_BENCH_ITERS=31 taskset -c 8-15 \ cargo test --release -p onnx-runtime-ep-cpu-plugin --test plugin_ort_e2e \ plugin_path_ab -- --nocapture --ignored ``` ## Also in this PR - **`unary_bench_cases()`** — extends the session A/B harness, which until now covered only the matmul family, to the elementwise grid the activation doc is written against. Same harness, same refusal to report a half-pinned ratio, so a number from it is directly comparable to a matmul number. `#[ignore]`d like the rest of the harness, so no CI cost. - **`codegen_units_are_pinned`** — reads the setting back out of the workspace manifest. A build setting is exactly what a rebase drops silently: no test fails, no path changes, everything just gets slower. That is how this class of regression reached `main` in the first place, so it gets a test. - Doc update recording the before/after grid and the reproduce line. ## Numerics Unchanged — this is a compiler partitioning setting, not a code change. Full suites green: `onnx-runtime-ep-cpu` 1324 passed / 0 failed, and every `onnx-runtime-ep-cpu-plugin` suite passed with `NXRT_REQUIRE_ORT_TESTS=1` (so ORT-dependent tests are hard failures rather than skips). ## Cost `onnx-runtime-ep-cpu` builds in ~65 s instead of ~19 s. Scoped to this one package, so nothing else in the workspace changes. 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
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.
What happened
#1130 (mine) wrapped the
ClipMLAS call inrun_chunkedso it would use thethread pool.
run_chunkedis generic, so doing that fromselection.rscreated a newinstantiation in a module that had never had one.
The runtime path of every other unary op was untouched — not one instruction — and all
1332 tests still passed. But the crate's codegen units repartitioned and the AVX2 unary
kernels in
simd_activations.rsstopped being vectorised.This shipped. It was found while collecting numbers for the final report:
Sqrthadgone from beating ORT by 1.9× to losing at 0.7×, and the ORT-relative position of five
ops had collapsed in a way no code change explained.
Evidence
Bisected by rebuilding
mainwith one file at a time reverted to1f1ce4b74(the commit before #1130). n = 65536, 1 thread,
taskset -c 8-23, 4 interleavedrounds, µs p50. Only
selection.rsmatters — revertingrelu.rsorconv.rsalone restored nothing:
Reproduced independently by building the same commit in a second worktree, so it is not
a build-directory artefact. Adding
#[inline]torun_chunkeddid not help, whichrules out a plain inlining decision and points at codegen-unit partitioning.
The diagnosis, confirmed mechanically
If the cause really is codegen-unit partitioning, then forcing the crate into a single
codegen unit should erase the regression with no source change at all. It does.
CARGO_PROFILE_RELEASE_CODEGEN_UNITS=1on the unfixed commit34095af0f, samemachine, 4 interleaved rounds, µs p50:
codegen-units=1So the diagnosis is not inferred from a bisect alone — the proposed mechanism, applied
directly, reproduces the cure.
codegen-units = 1(or LTO) in the release profile would remove this whole fragilityclass permanently, and is the more durable answer. It is deliberately not in this PR:
it is a workspace-wide build-policy change that affects every crate and every
contributor's build time (the plugin alone went 14 s to 63 s here), it would need its own
measurement across the whole EP rather than the activation kernels, and it does not
belong in a regression fix. Filed as the follow-up this PR's limitation section points
at. The source fix costs nothing and is independent of it.
The fix
run_chunkedis private tosimd_activations.rsagain — the compiler now enforces therule, not a convention. Callers elsewhere go through one of two entry points that are
instantiated in that module:
run_chunked_fn(input, output, body: fn(&[f32], &mut [f32]))— deliberately afnpointer, not
impl Fn, so every caller shares one instantiation. Used byReluandSiLU.clip_chunked(input, output, min, max)—Clipneeds captured bounds. It takes theserial decision itself so the short case is a direct call rather than one through a
closure the optimiser can no longer see into; without that,
Clipitself paid 12%.Result
n = 65536 and 1 Mi, 1 thread, 5 interleaved rounds, µs p50:
Everything is back to its pre-#1130 level and #1130's own win is kept:
Swishisstill 1.92× faster than before #1130, and
Clip/Relustill reach the pool at≥
PAR_MIN_LEN.Regression guard
chunking_instantiation_is_local::no_module_outside_this_one_instantiates_run_chunkedwalks the crate source and fails if any module other than
simd_activations.rsinstantiates
run_chunked, naming the offending file and line.Verified to falsify: re-widening the visibility and pointing
relu.rs:144back atrun_chunkedmakes it fail withOffending call sites: ["…/kernels/relu.rs:144"].This class of bug produces no wrong answers, no test failures and no diff in the file
that slows down, so a mechanical guard is the only thing that catches it.
Tests
cargo test -p onnx-runtime-ep-cpu --features mlas --lib→ 1332 passed, 0 failedcargo test -p onnx-runtime-ep-cpu --lib→ 1305 passed, 0 failedcargo fmt --allclean.clip_chunked's serial branch calls exactly the function theclosure called, and
run_chunked_fnforwards unchanged.Limitations
about. The guard encodes the rule that was measured to work on this toolchain; it
cannot prove the next refactor is safe. That is why the guard names the symptom and
the measured cost in its failure message.
taskset-pinned, 5 rounds).The direction is unambiguous — up to 2.3× on
Sqrtand roughly 1.5–1.8× on theother four — but the exact figures are not portable.