Accumulate low-precision float scans in float32 on the CPU#3907
Open
v-code01 wants to merge 1 commit into
Open
Accumulate low-precision float scans in float32 on the CPU#3907v-code01 wants to merge 1 commit into
v-code01 wants to merge 1 commit into
Conversation
The CPU scan kept its running value in the output dtype and read it back from the output buffer each step (`op(*(output - 1), *input)`). For float16 / bfloat16 this means a long cumulative sum stops growing once the running value's ULP exceeds the increment: a float16 `cumsum` of ones saturates at 2048 and a bfloat16 one at 256, so e.g. `mx.cumsum(mx.ones(5000, mx.float16))` returns 2048 on the CPU while the GPU (whose blocked scan stays accurate) returns 5000. numpy has the same saturation, but the two MLX backends disagreeing on the same call is the real problem. Carry the scan accumulator in a separate `AccT` type and only narrow to the output dtype on store, mirroring the existing bool->int32 accumulation. `AccT` is float32 for float16/bfloat16 and the output type for everything else, so every other dtype is bit-identical (verified against numpy) and only the low-precision float paths change. This resolves the TODO in the file and makes the CPU match the GPU and the true value. Adds a regression test (float16/bfloat16 cumsum of ones past the stall point, contiguous and strided), pinned to the CPU stream.
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.
Problem
The CPU scan keeps its running value in the output dtype and reads it back from the output buffer each step (
op(*(output - 1), *input)). Forfloat16/bfloat16a long cumulative sum stops growing once the running value's ULP exceeds the increment:The float16 sum saturates at 2048 and bfloat16 at 256, regardless of length. The GPU's blocked scan stays accurate, so the same call returns different results on the two backends. (numpy has the same saturation summing in-dtype, but the CPU/GPU divergence within MLX is the real issue.)
The file already anticipated this:
Fix
Carry the scan accumulator in a separate
AccTtype and narrow to the output dtype only on store, mirroring the existingbool -> int32accumulation.AccTisfloat32forfloat16/bfloat16and the output type for every other dtype, so all non-low-precision dtypes are bit-identical and only the float16/bfloat16 paths change. The contiguous and strided scans both carry the accumulator (the strided path uses one accumulator per lane).After the fix the CPU matches the GPU and the true value:
Tests
Extended
test_scanswith a float16/bfloat16cumsum-of-ones check past the stall point (sizes 4096/5000/20000, contiguous and strided), pinned to the CPU stream. It fails on the current CPU backend (stalls at 2048) and passes with the fix. Fulltest_ops.py(146 tests / 309 subtests) and the autograd cumprod-grad test stay green;cumsum/cumprodfor int and float32 remain bit-identical to numpy.Verified with a CPU-only build (
MLX_BUILD_METAL=OFF) on an M4.