Repository navigation
docs(upstream): correct the CUDA RMSNorm small-axis attribution - #1033
Merged
Merged
Conversation
Issue #830 asked for an upstream bug report against ml-explore/mlx for a small-axis CUDA RMSNorm regression attributed to #3792 and kept alive by #3850. Verifying that attribution against real upstream source inverted it, so no upstream report is filed and none should be. Measured on GB10 with the three kernel bodies copied verbatim from 1700b39 (the pre-#3792 parent), a5a684d (#3792) and b7c3dd6d (#3850, which is the current pin), each launched with its own commit's dispatch configuration and diffed against a per-row float64 reference: the pinned kernel and #3792 are correct at every axis from 8 to 8192 for row counts 1 through 17, and the pre-#3792 kernel that mlxcel carries as an overlay is wrong at every one of the 34 swept axis sizes in [257, 512] with 16-bit activations. Root cause. In that band the pre-#3792 dispatch pairs block_dim == 64 with groups_per_block == 2, so a 128-thread block runs BlockBroadcastReduce<float, 64, 32>, whose TempStorage is sized BLOCK_DIM / WARP_SIZE == 2 while cg::tiled_partition<32> of that block reports meta_group_size() == 4. The second reduction stage therefore indexes two floats past the end of the scratch array and folds in the partials of the other row sharing the block. compute-sanitizer memcheck reports the out-of-bounds shared read for that launch geometry only. At one row per launch the threads that take the row-bounds early return never write their partials, so the normalizer picks up residual shared memory: seeding that window with 0, 1, 100 and 10000 moves the applied normalizer to 99.99%, 92.79%, 24.19% and 2.49% of the float64 reference. #3792 changed that single value to 1 and added static_assert(block_dim <= 32 || groups_per_block() == 1) to pin the invariant; #3850 rewrote the forward path to one row per block. docs/upstream/mlx-cuda-rmsnorm-small-axis-regression.md records the measurements, the compute-sanitizer trace, the quoted source at every cited line, the affected mlxcel surface, and the removal plan. It states plainly that #829's deepseek-v2-lite 0/4 to 4/4 result is not reconciled by these measurements and that resolving it needs a build with the overlay removed, which is deliberately not attempted here. The overlay's tracking comment no longer dangles on "report filed as a follow-up". It points at that document and carries the corrected removal condition: nothing needs to land upstream first, so the overlay can be deleted once deepseek-v2-lite is re-verified on the upstream fast kernel, recovering the #3850 speedup. Only comment lines above the begin-upstream marker changed, so the vendored kernel and its behavior are untouched by this commit. Validated with cargo fmt --all -- --check, cargo clippy -p mlxcel-core --features cuda -- -D warnings, and cargo check -p mlxcel-core --features cuda, the last of which re-applied the overlay and recompiled it through nvcc. Refs #830
be-free-snowboy
pushed a commit
to freestylejs/snowbrain-mlxcel
that referenced
this pull request
Oct 27, 2026
…up#1033) ## Summary Issue lablup#830 asked for an upstream bug report against ml-explore/mlx: MLX #3792 was said to have regressed the small-axis CUDA RMSNorm, with #3850 keeping the same axis broken. Verifying that against real upstream source inverted it, so **no upstream report is filed and none should be**. The kernel at mlxcel's MLX pin is correct on the affected axis. The pre-#3792 kernel that mlxcel carries as `src/lib/mlx-cpp/patches/mlx/backend/cuda/rms_norm.cu` is the one that computes a wrong normalizer, and it reads out of bounds while doing it. ## What was verified, and against what The pin is not `0.32.1` as lablup#830 states. `src/lib/mlx-cpp/CMakeLists.txt:95` pins `b7c3dd6d27f45b5365b08a840310187dc503f1db`, which is upstream PR #3850 itself. The on-disk MLX checkout under `target/` already has the overlay applied, so all three kernels were fetched from GitHub at their own commits instead: `1700b39a` (the direct parent of #3792, confirmed via the commit's `parents`), `a5a684d` (#3792), and the pin. Measured on GB10 (sm_121, CUDA 13) with the three kernel bodies copied verbatim, each launched with its own commit's dispatch configuration and diffed against a per-row float64 reference: | Kernel | Failing axis sizes, 8 to 8192, row counts 1 to 17 | |---|---| | `1700b39` (the overlay mlxcel ships) | all 34 swept sizes in `[257, 512]` | | `a5a684d` (#3792) | none | | `b7c3dd6d` (#3850, the pin) | none | `compute-sanitizer --tool memcheck` reports `Invalid __shared__ read of size 4 bytes` for the overlay's launch geometry only; the other two are clean. ## Root cause At `axis_size` in `(256, 512]` for 16-bit dtypes, the pre-#3792 dispatch pairs `block_dim == 64` with `groups_per_block == 2`. That launches a 128-thread block, but the reduction is `BlockBroadcastReduce<float, 64, 32>`, whose `TempStorage` is sized `BLOCK_DIM / WARP_SIZE == 2` while `cg::tiled_partition<32>` of the launched block reports `meta_group_size() == 4`. The second reduction stage indexes two floats past the end of the scratch array and folds in the partials belonging to the other row that shares the block. Two failure modes follow. Above one row the sum covers both rows, so the normalizer is deterministically wrong (0.55 to 0.94 relative error). At one row the `threadIdx.y == 1` threads take the row-bounds early return and never write their partials, so the normalizer picks up residual shared memory. Seeding that window with 0, 1, 100 and 10000 moves the applied normalizer to 99.99%, 92.79%, 24.19% and 2.49% of the float64 reference. #3792 changed that one value to 1 and added `static_assert(block_dim <= 32 || groups_per_block() == 1)` to pin the invariant. It is a fix, and its title says so. #3850 rewrote the forward path to one row per block, where block size and template parameter agree by construction. lablup#830 got the band, the config, the changed value, and DeepSeek-V2's `kv_lora_rank == 512` landing on it all correct. Only the direction is backwards. ## Not reconciled lablup#829 measured deepseek-v2-lite going 0/4 to 4/4 coherent with the overlay as the only change. Against these measurements the overlay should have made that axis worse. The document says so plainly rather than papering over it, and notes two unverified leads: lablup#817's original "nondeterministic across fresh processes" symptom matches the uninitialized read exactly, and graph capture changes which kernels precede the norm, which is what determines the overlay's answer at one row. Settling it needs a build with the overlay removed, measured the way lablup#829 measured. That is a separate change with real risk and is deliberately not attempted here, so **no runtime behavior changes in this PR**. ## What changed - `docs/upstream/mlx-cuda-rmsnorm-small-axis-regression.md` (new): the report, following the two existing `docs/upstream/` examples. Summary, evidence, root cause with quoted source at every cited line, minimal fix, repro, affected surface. The standalone CUDA repro was run; the `mlx.core` sketch and the model-level repro are labelled not run. - `src/lib/mlx-cpp/patches/mlx/backend/cuda/rms_norm.cu`: the tracking comment no longer dangles on "report filed as a follow-up". It points at the document, marks the superseded paragraph, and carries the corrected removal condition: nothing needs to land upstream first, so the overlay can be deleted once deepseek-v2-lite is re-verified on the upstream fast kernel, recovering the #3850 speedup. Comment lines above the `---- begin upstream ----` marker only; the vendored kernel body is untouched. The patch-file sanity check at `CMakeLists.txt:115-126` globs `patches/mlx/backend/metal/*.cpp` and does not match this file. ## Follow-up worth filing The overlay is wrong on an axis nine model files reach through `kv_a_layernorm`. Dropping it and re-verifying deepseek-v2-lite is the obvious next step, but it needs a real model run and is out of scope for a documentation change. ## Test plan - [x] `cargo fmt --all -- --check` - [x] `cargo clippy -p mlxcel-core -- -D warnings` (clean, 16.23s) - [x] `cargo check -p mlxcel-core --features cuda` (clean, 29.20s; re-applied the overlay and recompiled `rms_norm.cu.o` through nvcc, so the comment edit is confirmed to build) - [x] Overlay diff is comment-only: zero non-`//` lines changed - [x] No em dashes and no AI attribution in the diff or the commit message - [ ] `cargo clippy -p mlxcel-core --features cuda` **skipped**: it hashes to a fresh build directory and starts a cold full MLX CUDA rebuild. Aborted after two minutes and the partial directory removed. No Rust changed in this PR, so it carried no signal. Closes lablup#830
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.
Summary
Issue #830 asked for an upstream bug report against ml-explore/mlx: MLX #3792 was said to have regressed the small-axis CUDA RMSNorm, with #3850 keeping the same axis broken. Verifying that against real upstream source inverted it, so no upstream report is filed and none should be. The kernel at mlxcel's MLX pin is correct on the affected axis. The pre-#3792 kernel that mlxcel carries as
src/lib/mlx-cpp/patches/mlx/backend/cuda/rms_norm.cuis the one that computes a wrong normalizer, and it reads out of bounds while doing it.What was verified, and against what
The pin is not
0.32.1as #830 states.src/lib/mlx-cpp/CMakeLists.txt:95pinsb7c3dd6d27f45b5365b08a840310187dc503f1db, which is upstream PR #3850 itself. The on-disk MLX checkout undertarget/already has the overlay applied, so all three kernels were fetched from GitHub at their own commits instead:1700b39a(the direct parent of #3792, confirmed via the commit'sparents),a5a684d(#3792), and the pin.Measured on GB10 (sm_121, CUDA 13) with the three kernel bodies copied verbatim, each launched with its own commit's dispatch configuration and diffed against a per-row float64 reference:
1700b39(the overlay mlxcel ships)[257, 512]a5a684d(#3792)b7c3dd6d(#3850, the pin)compute-sanitizer --tool memcheckreportsInvalid __shared__ read of size 4 bytesfor the overlay's launch geometry only; the other two are clean.Root cause
At
axis_sizein(256, 512]for 16-bit dtypes, the pre-#3792 dispatch pairsblock_dim == 64withgroups_per_block == 2. That launches a 128-thread block, but the reduction isBlockBroadcastReduce<float, 64, 32>, whoseTempStorageis sizedBLOCK_DIM / WARP_SIZE == 2whilecg::tiled_partition<32>of the launched block reportsmeta_group_size() == 4. The second reduction stage indexes two floats past the end of the scratch array and folds in the partials belonging to the other row that shares the block.Two failure modes follow. Above one row the sum covers both rows, so the normalizer is deterministically wrong (0.55 to 0.94 relative error). At one row the
threadIdx.y == 1threads take the row-bounds early return and never write their partials, so the normalizer picks up residual shared memory. Seeding that window with 0, 1, 100 and 10000 moves the applied normalizer to 99.99%, 92.79%, 24.19% and 2.49% of the float64 reference.#3792 changed that one value to 1 and added
static_assert(block_dim <= 32 || groups_per_block() == 1)to pin the invariant. It is a fix, and its title says so. #3850 rewrote the forward path to one row per block, where block size and template parameter agree by construction.#830 got the band, the config, the changed value, and DeepSeek-V2's
kv_lora_rank == 512landing on it all correct. Only the direction is backwards.Not reconciled
#829 measured deepseek-v2-lite going 0/4 to 4/4 coherent with the overlay as the only change. Against these measurements the overlay should have made that axis worse. The document says so plainly rather than papering over it, and notes two unverified leads: #817's original "nondeterministic across fresh processes" symptom matches the uninitialized read exactly, and graph capture changes which kernels precede the norm, which is what determines the overlay's answer at one row. Settling it needs a build with the overlay removed, measured the way #829 measured. That is a separate change with real risk and is deliberately not attempted here, so no runtime behavior changes in this PR.
What changed
docs/upstream/mlx-cuda-rmsnorm-small-axis-regression.md(new): the report, following the two existingdocs/upstream/examples. Summary, evidence, root cause with quoted source at every cited line, minimal fix, repro, affected surface. The standalone CUDA repro was run; themlx.coresketch and the model-level repro are labelled not run.src/lib/mlx-cpp/patches/mlx/backend/cuda/rms_norm.cu: the tracking comment no longer dangles on "report filed as a follow-up". It points at the document, marks the superseded paragraph, and carries the corrected removal condition: nothing needs to land upstream first, so the overlay can be deleted once deepseek-v2-lite is re-verified on the upstream fast kernel, recovering the #3850 speedup. Comment lines above the---- begin upstream ----marker only; the vendored kernel body is untouched.The patch-file sanity check at
CMakeLists.txt:115-126globspatches/mlx/backend/metal/*.cppand does not match this file.Follow-up worth filing
The overlay is wrong on an axis nine model files reach through
kv_a_layernorm. Dropping it and re-verifying deepseek-v2-lite is the obvious next step, but it needs a real model run and is out of scope for a documentation change.Test plan
cargo fmt --all -- --checkcargo clippy -p mlxcel-core -- -D warnings(clean, 16.23s)cargo check -p mlxcel-core --features cuda(clean, 29.20s; re-applied the overlay and recompiledrms_norm.cu.othrough nvcc, so the comment edit is confirmed to build)//lines changedcargo clippy -p mlxcel-core --features cudaskipped: it hashes to a fresh build directory and starts a cold full MLX CUDA rebuild. Aborted after two minutes and the partial directory removed. No Rust changed in this PR, so it carried no signal.Closes #830