Repository navigation
perf(cuda): derive RMSNorm fold hidden floor from SM count (#1421) - #1582
Merged
Merged
Conversation
justinchuby
marked this pull request as ready for review
August 20, 2026 15:58
justinchuby
force-pushed
the
squad/1421-rmsnorm-device-aware
branch
from
August 20, 2026 16:06
72da204 to
85c9e35
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1582 +/- ##
===========================================
- Coverage 82.19% 80.78% -1.42%
===========================================
Files 12 381 +369
Lines 5471 173786 +168315
Branches 5471 173786 +168315
===========================================
+ Hits 4497 140392 +135895
- Misses 775 28500 +27725
- Partials 199 4894 +4695
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
justinchuby
added a commit
that referenced
this pull request
Aug 20, 2026
…1588) ## Problem `cargo test -p onnx-runtime-ep-cuda --features "cuda,gpu-tests" --lib -- optimizer::tests` was flaky in default parallel mode (~1 failure in 5 runs); `--test-threads=1` never failed. `cargo test` runs tests as parallel threads **inside one process**, so `std::env` is process-wide shared mutable state. Several optimizer tests mutated env vars under a `// SAFETY: single-threaded test` comment that was **never true** under the default harness. The proven failure: - `opt_out_env_preserves_exported_gate_chains` does `set_var(LINEAR_ATTENTION_GATING_DISABLE_ENV, "1")` -> run -> `remove_var`. - `precomputed_neg_exp_a_initializer_does_not_set_neg_exp_marker` sets **no** env but asserts the fusion is enabled by default. If it runs inside the other test's set/remove window, it observes the disable flag and panics. ## Fix — structural, not a workaround New reusable `EnvVarGuard` in `test_support` (RAII over a **process-global mutex**): - Serialises **every** env-touching test — writers *and* default-readers — on one lock. The default-reader half matters: a plain writer-only mutex would leave readers exposed, so both sides take the same lock and the race becomes impossible, not merely unlikely. - Restores each touched variable to its prior value on drop, even on panic. - `acquire()` (lock only), `with_var`/`without_var`, and `set`/`unset` for read-then-toggle tests. All env-touching `optimizer.rs` tests converted, covering **all five** variables set there: linear-attention gating (the proven-flaky one), skip-rmsnorm, const-cast-fold, identity-cast-fold, qkv-fusion. The ad-hoc `IDENTITY_CAST_TEST_LOCK` (which only ever covered one var) is replaced by the shared guard, and every false `// SAFETY: single-threaded test` comment is deleted/replaced with an accurate one. ## Behaviour change you should know about — tests are now immune to external env (intentional) This is a real, deliberate behaviour change, called out explicitly rather than buried: The default-reader tests now `unset` their variable inside the guard, so they assert the **declared default**, not whatever the invoking shell happens to export. Measured on the previously-flaky target with `ONNX_GENAI_CUDA_DISABLE_LINATTN_GATING_FUSION=1` injected **externally**: | | external `DISABLE_LINATTN=1` set before the run | |---|---| | before this PR (measured) | `exit=101` `FAILED. 81 passed; 3 failed` | | after this PR (measured, this branch) | `exit=0` `ok. 81 passed; 0 failed; 0 ignored` | Why this is correct: a test that asserts *default behaviour* must run under a deterministic default. Letting the outcome depend on the runner's ambient environment is just another form of the same non-determinism this PR removes — the pre-fix "3 failed" were the same bug's other face (the tests were reading the **process environment** instead of their **declared premise**). **The cost, stated plainly:** if someone *wants* to force a whole "this fusion disabled" test pass by exporting `ONNX_GENAI_CUDA_DISABLE_*` and running the suite, that path **no longer works for guard-covered tests** — they pin the variable themselves. That is a genuine trade-off; the per-test opt-out assertions (e.g. `opt_out_env_preserves_*`) remain the supported way to exercise the disabled path. ## Regression guard (falsifiable) Added a source-level test, `optimizer_source_routes_all_env_mutation_through_guard`, asserting `optimizer.rs` contains no bare `std::env::set_var`/`remove_var` (needles assembled from fragments so it never matches itself). If a future edit re-introduces a direct env mutation here, this test fails immediately instead of the flake resurfacing under the parallel harness. Production code in this file only *reads* the environment, which is race-free, so a zero-mutation source is the correct invariant. Scope: `optimizer.rs` only; the follow-up issue extends the same guard per file as they are swept. ## Test-only, no runtime footprint (measured) `test_support` is `#[cfg(test)]`-gated. Verified against the non-test dev rlib (`cargo build -p onnx-runtime-ep-cuda --features cuda`): ``` occurrences of 'EnvVarGuard' in non-test rlib = 0 occurrences of 'env_lock' in non-test rlib = 0 occurrences of 'ENV_LOCK' in non-test rlib = 0 ``` (The one `test_support` string hit is a debuginfo file path, not compiled code.) No guard code enters a non-test/release binary. ## Evidence (measured) `optimizer::tests` run **25x in default parallel mode** via the built `cuda,gpu-tests` binary (20 in the first pass + 5 after adding the guard test): zero failures. Representative line: ``` test result: ok. 81 passed; 0 failed; 0 ignored; 0 measured; 398 filtered out ``` (`81` includes the new regression-guard test; the pre-guard passes reported `80`.) `cargo fmt -p onnx-runtime-ep-cuda -- --check` exits 0. ## Scope / deferred — tracked in #1591 - Fully fixes `optimizer.rs` (the proven-flaky file) plus its four latent siblings, with a reusable guard and regression check. - `EnvVarGuard` lives in `test_support` so other files (`matmul_nbits.rs` 33 sites, `provider.rs`, etc.) can adopt the same pattern. **#1591** tracks the sweep and records the decision criterion: *any variable that some test sets must have its default-readers hold the same lock*. - Passes reading vars that **no test sets** (e.g. L2-norm fusion, rmsnorm min-hidden) are left unlocked — no writer means no race. ## Coordination Rebased onto latest `main` (`1c97c9693`, includes #1491). Diff is confined to `optimizer.rs` tests + `test_support.rs` — no overlap with #1491's `standard_attention.rs`/`geometry.rs`. #1582 (RMSNorm) is not yet merged; it edits `optimizer.rs` production code + adds tests, whereas this PR only changes existing tests' env handling, so the conflict surface is small. --------- Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
Replace the global RMSNORM_FUSION_MIN_HIDDEN=1280 with a per-device floor derived from the device's SM count, anchored on the single H200 (132 SM) calibration point rather than a per-device lookup table. A small GPU (few SMs) folds aggressively; a large GPU keeps the protective H200-class floor at M=1. The optimizer cannot see M (the decode graph's batch dim is symbolic and shared across batch sizes), so the SM-derived floor covers the common small-GPU batch case and ONNX_GENAI_RMSNORM_MIN_HIDDEN remains the escape hatch for a small model batched on a large GPU. The fold stays byte-identical. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
The SkipSimplifiedLayerNormalization -> GEMV fold was documented as bit-for-bit identical to the standalone norm. That was never verified and is false: folding the RMS normalization into the following GEMV's prologue changes the fp16 reduction order, so at a greedy-argmax near-tie it can flip a token. Measured on qwen0.5B (hidden 896): 1 of 6 sampled prompts flipped one token at a 0.0156-nats top-2 gap. A CPU-EP full-prefill oracle put the folded token (448) closer to the high-precision reference than the unfused one (304), so the reorder rounds toward the more accurate result -- a reduction-order effect, not a fusion bug. Update the load-bearing comments accordingly; the residual epilogue remains bit-identical. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
Rebasing onto #1588 replaced the ad-hoc RMSNORM_FLOOR_ENV_LOCK and bare std::env::set_var/remove_var in the new floor tests with the crate-wide EnvVarGuard, which serialises env access on a process-global lock (writers and default-value readers share it) and restores touched vars on drop. The two device-derived gate tests read the real ONNX_GENAI_RMSNORM_MIN_HIDDEN default, so they now hold the guard too. Satisfies the include_str! guard test that bans bare env mutation in optimizer.rs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
justinchuby
force-pushed
the
squad/1421-rmsnorm-device-aware
branch
from
August 20, 2026 16:51
85c9e35 to
bf20e41
Compare
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.
perf(cuda): derive the RMSNorm-fold hidden floor from SM count (#1421)
Replaces the global
RMSNORM_FUSION_MIN_HIDDEN = 1280with a per-device floorderived from the device's SM count, anchored on the single H200 calibration point
rather than a per-device lookup table.
Closes #1421.
What the change does
multiprocessor_count()— the property that sets how muchparallel slack the device has to keep the standalone norm "almost free" at M=1.
every other device gets its floor from its own SM count. A big datacenter part
inherits the protective H200-class floor; a small edge part folds aggressively.
Avoids the §40 "regime-tuned threshold" trap.
RTX 4060 Laptop (24 SM) → 256.
model load on a decode graph whose batch dim is symbolic and shared across all
batch sizes, so M ≥ 2 cannot gate the fold per step. The residual uncovered
case (small model batched on a large GPU) is documented and left to the
ONNX_GENAI_RMSNORM_MIN_HIDDENescape hatch.Tests (measured)
cargo test -p onnx-runtime-ep-cuda --features "cuda,gpu-tests" --lib -- optimizer::tests→ 85 passed, 0 failed, 0 ignored (10x default-parallel runs, 0 failures — the new tests route env access through
EnvVarGuardper #1588, closing the process-wide env race; the +1 vs the earlier count is #1588's guard test). (cuda,gpu-tests, notcudaalone, so thesuite is not silently all-ignored.) Four new
derived_min_hiddentests cover theanchor self-reproduction (132→1280), 24→256,
.max(1)clamp at sm 0/1,monotonicity, whole-128-chunk output, the device-derived gate, and env-override
precedence.
The fold is NOT bit-identical — and this PR corrects that long-unverified claim
The
SkipSimplifiedLayerNormalization→ GEMV fold was documented in the code as"byte-identical". That was never verified, and it is false. Folding the RMS
normalization into the following GEMV's prologue changes the fp16 reduction
order, so at a greedy-argmax near-tie it can flip a token. The residual epilogue
(
fp16(fp16(acc) + residual)==__hadd2) remains bit-identical; the reorderingis in the normalization reduction only. The code comments are updated to say this
accurately (see the second commit).
Measured (binary SHA256-pinned across runs, machine quiet)
Switch efficacy proven with
ONNX_GENAI_PROFILE_OPS=1:SkipSimplifiedLayerNormalizationop-count 23 (fold ON) vs 48 (fold OFF) — thetwo sides genuinely run different graphs.
profile_native(native CUDA, greedy,--tokens 128 --decode-skip 8, no--steady), qwen0.5B (hidden 896), fold ON (ONNX_GENAI_RMSNORM_MIN_HIDDEN=256)vs OFF (
=2048):granite-1b-a400m (hidden 1024): identical on the fox prompt. So the divergence is
real but rare and input-dependent (1 of 6 sampled prompts), consistent with a
reduction-order change tipping only near-ties.
Which side is correct? The folded side.
Divergence alone doesn't say which token is right. Adjudicated with a
high-precision oracle: take prompt (11 tok) +
generated[0..48](49 tok) = a60-token prefix and run a full prefill on the CPU EP (a completely different
implementation and precision the CUDA fold never touches), dumping top-k:
The reference top-1 is 448 — the token fold ON selects. Fold OFF (today's
default: keep the standalone norm) selects
304, the runner-up, only 0.0156nats behind. So the fp16 reduction reorder rounded toward the more accurate
result here, not away from it. This is a reduction-order effect, not a fusion
bug — there is no evidence of a numerical defect in the N=896 fused GEMV.
Repro
Not verified / out of scope
single measured flip point only; not proven for every possible near-tie.
derived value in unit tests, not on H200 hardware.