Skip to content

Compute float16 and bfloat16 floor divide in float32 - #40

Merged
davidtai merged 310 commits into
mainfrom
fix/half-floor-divide
Oct 24, 2026
Merged

davidtai merged 310 commits into
mainfrom
fix/half-floor-divide

Conversation

@davidtai

@davidtai davidtai commented Oct 23, 2026 •

Copy link
Copy Markdown

Proposed changes

This PR computes float16 and bfloat16 floor_divide, and the divmod quotient, in float32. The result is then rounded once to the output type. David asked for this as a separate PR after the review of PR #38.

The defect

Upstream computes these operations as floor(a / b) in the half type. The quotient is rounded to float16 or bfloat16 before the floor. When the true quotient is less than half an ulp below an integer n, it rounds to n, and the result is n instead of n - 1.

numpy's float16 divmod converts to float32 and uses npy_divmodf, which works from fmod, so it gives n - 1. Upstream's test_divmod draws unseeded float16 inputs from uniform(1, 100) and compares with numpy. My estimate is that it fails in about 4% of runs per device. It failed once in the GPU shard of PR #38 (run 39966062214), and a re-run passed.

The fix

A helper exact_floor_divide(float a, float b) uses the method of npy_divmodf:

  1. mod = fmod(a, b)
  2. div = (a - mod) / b
  3. If mod and b have different signs, subtract 1 from div.
  4. Floor div. Add 1 if div - floor(div) > 0.5. Keep the sign of zero. Return a / b when b == 0.

Where the fix applies:

File Change
mlx/backend/metal/kernels/binary_ops.h FloorDivide uses the helper for half and bfloat16_t. DivMod uses FloorDivide, so its quotient is fixed too.
mlx/backend/cpu/simd/base_simd.h The helper, and the scalar floor_divide for float16 and bfloat16.
mlx/backend/cpu/simd/accelerate_simd.h The SIMD floor_divide for float16 and bfloat16 (one lane at a time through the helper).
mlx/backend/cpu/binary.cpp The CPU DivMod computes the half-type quotient and remainder in float32 and rounds each once.

What does not change:

  • float32, float64 and integer paths keep their code and their speed. Only the half-type branches change.
  • Remainder does not change. fmod is exact, so the remainder agrees with the new quotient: a == q * b + r within the dtype rounding. The new test checks this.
  • The CUDA kernels do not change. No workflow in this fork builds CUDA.

Tests

  • test_divmod does not change, and it stays unseeded.
  • New: test_floor_divide_half_rounding_window in python/tests/test_ops.py.
    • It builds float16 and bfloat16 pairs whose true quotient is less than a quarter ulp below an integer, so the half-type quotient certainly rounds up.
    • It checks floor_divide and the divmod quotient against the floor of the exact quotient. For float16 it also checks against numpy's divmod, both the quotient and the remainder.
    • It checks that 0 <= r < b and q * b + r == a.
    • It runs on both the CPU and the GPU stream, so the GPU and CPU test passes each cover both devices.
  • Before the fix (head 7e044d98, test only, run 39971647170): python-gpu (0) and python-cpu (0) fail in all 4 subtests (float16 and bfloat16, on the CPU and the GPU). Every quotient is 1 too high, for example [5.0, 9.0, 10.0, ...] against [4.0, 8.0, 9.0, ...].
  • After the fix (head 790f01b2, run 39974672718): every check passes, including the new test and test_divmod on both passes. On attempt 1, python-gpu (0) passed all its tests but failed later in "Build Metal extension example" on a DNS error while fetching nanobind from PyPI. A re-run of that job passed.

fork.yaml

New section "half and bfloat16 floor divide in float32". It names the upstream behavior and why the fork changes it.

Draft upstream issue (not opened)

Title: floor_divide and divmod for float16/bfloat16 round the quotient before the floor

mx.floor_divide and the quotient of mx.divmod compute floor(a / b) in the input dtype for float16 and bfloat16. The division result is rounded to the half type first. If the true quotient is less than half an ulp below an integer n, it rounds to n, and the result is n instead of n - 1. numpy's float16 divmod computes in float32 from fmod (npy_divmodf) and returns n - 1.

Example: a = 3.001953125 (float16), b = 1.0009765625 (float16). The true quotient is about 2.99902, so floor is 2. In float16 the quotient rounds to 3.0, and mx.floor_divide(a, b) returns 3.

This makes python/tests/test_ops.py::test_divmod flaky. It draws unseeded float16 inputs from uniform(1, 100). The rounding window near a quotient q is about q / 2950 of the range, so about 0.07% of pairs hit it. With about 56 float16 pairs per call, the test fails in about 4% of runs.

Suggested fix: compute float16 and bfloat16 floor division in float32 with the npy_divmodf method (mod = fmod(a, b), div = (a - mod) / b, floor with the sign correction), then round once to the output dtype. This applies to the Metal FloorDivide (and DivMod), the CPU scalar and SIMD floor_divide, the CPU DivMod, and probably the CUDA FloorDivide.

Test plan

There are no local or ai-server test runs. A compile-only Release build on ai-server (-DMLX_BUILD_TESTS=ON -DMLX_METAL_JIT=OFF) of the fix finished with 0 errors, and I removed the folder afterwards. PR CI is the check:

  • Fork Tests: build-cpp, build-wheel, the C++ tests, python-gpu (0..1), python-cpu (0..2), and the aggregate metal-build-and-test
  • Build and Test: Check Lint, Linux Sanitizer (ASAN, UBSAN), Linux Fedora (x86_64, aarch64)
  • Fork Diff Check
  • CodeQL does not run on this PR, because its base is not main.

Checklist

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes (I ran the same hook versions on the changed files: clang-format 21.1.8, ruff 0.16.10.)
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the necessary documentation (if needed) (fork.yaml)
  • I understand it is strictly prohibited to use AI to write PR description
  • AI usage disclosure: An AI agent (Claude Opus 5.5 in Claude Code) wrote the fix, the test, the commits and this description. David directed the work. The fork inherits upstream's rule against AI-written PR text, so the box above is not ticked. David decides.

🤖 Generated with Claude Code

zcbenz and others added 30 commits August 23, 2026 00:37
Signed-off-by: Pradyot Ranjan <[email protected]>
Co-authored-by: Pradyot Ranjan <[email protected]>
Co-authored-by: Cheng <[email protected]>
davidtai and others added 17 commits October 22, 2026 23:02
The previous commit cleared C only in the gemms/cblas.cpp wrappers.
Four more call sites call cblas_sgemm directly with beta 0:
BlockMaskedMM and GatherMM (the CPU MoE gathered matmul) in
masked_mm.cpp, and the two explicit-gemm convolutions in conv.cpp.
They can give the same NaN * 0 from a reused output buffer.

Add one helper, clear_gemm_output in gemm.h, that fills the exact C
region of a call (rows by columns, with its ldc) with zeros. Use it at
all five call sites, the wrappers included. No other BLAS gemm or gemv
call exists in the CPU backend.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Split the fork-tests job into parallel jobs. build-wheel and build-cpp
build in parallel. build-cpp also runs the C++ tests on the GPU and on
the CPU. python-gpu (3 shards) and python-cpu (4 shards) run the Python
tests from the uploaded wheel. metal-build-and-test merges the coverage
profiles of all jobs, writes the same coverage report, and fails when
another job failed.

python_shards.py checks the shard map: each python/tests/test*.py file
must be in each pass exactly one time. Split files must partition their
test IDs. The last job fails when a test ran in two shards.

fork-build-cache restores and saves ccache on pull requests and on main,
and keeps a cache of the compiled Metal kernels through an xcrun wrapper
keyed on the preprocessed source, the arguments and the toolchain.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Split test_fast_sdpa and test_quantized into named tests and rest
entries in the CPU shards, so that no CPU shard has an entry longer than
test_gated_delta_sequential. Put the entries in longest-first order.

Save the ccache files and the Metal kernel cache only when the build had
misses.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
The full --version text of clang and metal names the install folder and
the OS, and it changed between three macos-26 runner images, so every
job started from an empty cache. Key ccache on the clang major.minor
version and fall back to a cache of another clang version; ccache keeps
compiler_check=content. Key the Metal kernel cache on the metal compiler
version and the kernels hash, and fall back only to a cache of the same
metal compiler. Each Metal entry key holds the first line of the metal
--version text and the SDK build.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Upstream 5dbe999 (ml-explore#4634) makes is_donatable() also require that MLX owns
the buffer, so the allocator never reuses a wrapped external buffer. The
fork's mlx-c buffer-info API reported `unique` from is_donatable(), and
mlx-swift-lm uses that as a "no other reference" proof for paged KV
segments, checkpoint memory accounting and MTP state, including wrapped
host pages, which now always read as not unique.

Add is_uniquely_referenced(): the same reference-count test without the
ownership test. is_donatable() and donation behavior do not change.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
test_eval.test_async_eval_error_in_synchronize (upstream ml-explore#4338) failed
on some runs of the fork's CPU test pass: mx.async_eval raised the
LU factorization error that the test expects only from
mx.synchronize.

Cause: StreamThread::enqueue throws the stream's stored error when it
runs on the main thread (ml-explore#3742). async_eval enqueues several tasks.
When the stream thread finishes the failing task before async_eval
enqueues the next one, that enqueue throws. The order depends on
thread timing, so the failure is intermittent. The fork does not
change this path.

async_eval now holds a scheduler::DeferStreamErrors guard. While it
lives, enqueue on that thread does not throw the stored error, and
synchronize throws it later. eval does not change.

Add a C++ test that stores the error before async_eval, so it fails
on every run without the fix.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
The hosted macOS runners are a shared pool. Pack the GPU pass into 2
shards and the CPU pass into 3 shards, balanced by the durations that
run 39902948153 measured. A run now uses 8 macOS jobs, and at most 6 run
at the same time (before: 10 jobs, 8 at the same time).

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
The preprocessed source of the NAX kernels names the Metal toolchain
mount folder, which has a random suffix on each runner. The 10 NAX
kernels missed the Metal cache in every warm build (41 of 51 hits).
Remove the suffix from the text that the key hashes, and keep the
toolchain version.

CPU shard 0 was the slowest shard in the cold and warm runs of b72420d
(651 s and 470 s of tests). Move test_quantized:rest to CPU shard 1
(221 s).

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
ci: build once and run the Metal and CPU test passes in parallel
Merge the 9 upstream commits after c6abf0d: ml-explore#4729, ml-explore#4730, ml-explore#4731,
ml-explore#4728, ml-explore#4736, ml-explore#4737, ml-explore#4724, ml-explore#4738 and ml-explore#4744. The merge has no
conflict. Every merged file gets exactly upstream's change.

ml-explore#4728 widens the affine qmv_wide input-vector tile from 5 to 8. The
fork has no qmv_wide change in this repository, and the GPT-OSS MXFP4
decode tail is an fp gather_qmv kernel that ml-explore#4728 does not touch.
Each input vector still accumulates in its own registers, so the
results do not change.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Set base.hash to the new merge-base after the merge of the 9 upstream
commits. No fork section changes: the merge has no conflict, and each
merged file gets exactly upstream's change.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Add test_floor_divide_half_rounding_window. It builds float16 and
bfloat16 pairs whose true quotient lies within a quarter ulp below an
integer n. In the half type that quotient rounds to n, so floor(a / b)
gives n, while the floor of the exact quotient (and numpy's float16
divmod) is n - 1. The test checks floor_divide and the divmod quotient
against numpy, and checks that the divmod remainder is in [0, b) and
that q * b + r == a, on the CPU and on the GPU.

This commit has only the test, so CI shows it fail before the fix.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
floor_divide and the divmod quotient for float16 and bfloat16 were
floor(a / b) in the half type. The quotient is rounded to the half
type first, so a true quotient less than half an ulp below an integer
n rounds to n, and the result is n, not n - 1. numpy's float16 divmod
computes in float32 from fmod and gives n - 1. This made upstream's
unseeded test_divmod fail in a few percent of runs.

Compute these operations for float16 and bfloat16 in float32 with the
method of numpy's npy_divmodf (mod = fmod(a, b), div = (a - mod) / b,
then a floor with the sign correction), and round once to the output
type. This applies to the Metal FloorDivide (DivMod uses it), the CPU
scalar and SIMD floor_divide, and the CPU DivMod. float32, float64 and
integer paths do not change.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Base automatically changed from sync/upstream-2026-10-22 to main October 23, 2026 19:15
Main's tree equals 83dbdb0, the base of this branch, so the merge
keeps this branch's tree.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@davidtai
davidtai merged commit c51f9f3 into main Oct 24, 2026
23 of 26 checks passed
@davidtai
davidtai deleted the fix/half-floor-divide branch October 24, 2026 11:42
jonathan308 added a commit to Layr-Labs/d-inference that referenced this pull request Oct 24, 2026
…x ff4bb35b)

libs/mlx-swift-lm moves efd38585 -> d4fd516e, mlx-swift-lm #356's head
after it merges SDK main 476cb7c0. Main gained #358, which pins
Layr-Labs/mlx-swift eedca444 (was 6c31f9f1). That mlx-swift pins mlx
ff4bb35b and mlx-c 004c735f, which bring two fixes:

- float16 and bfloat16 floor divide computed in float32
  (Layr-Labs/mlx#40; CPU and the Metal binary_ops kernel);
- a freed compiled closure erased from every thread's compile cache
  (Layr-Labs/mlx#43).

libs/mlx-swift moves to eedca444 and libs/mlx to ff4bb35b in lockstep,
so the metallib is rebuilt from the new kernels. No Swift API changed:
the SDK's own change is Package.swift only, and mlx-swift changes its
pins and regenerated sources.
jonathan308 added a commit that referenced this pull request Oct 26, 2026
Brings in main at f78f0fa (2026-10-24): the upstream ml-explore/mlx sync
to afc03c5 (#38, with its history restored by #42) and the fork fixes
#40 and #43-#46.

Conflicts and how they were resolved:
- mlx/distributed/jaccl/lib/jaccl/mesh.cpp: main's per-wire queue setup
  (upstream ml-explore#4607, a second cable for the mesh) with this branch's
  COMPLETION_QUEUE_DEPTH for every completion queue.
- mlx/distributed/jaccl/lib/jaccl/mesh_impl.h: main's stride parameters
  for all_gather and sum_scatter, with this branch's progress-guard calls
  and stage_send in every send path.
- mlx/distributed/jaccl/lib/jaccl/ring_impl.h: this branch keeps its
  guarded member dispatch_wires (pool-wire errors are reported, the group
  closes after all wires return). Main moved an unguarded copy into
  threadpool.h and called it as dispatch_wires(pool_, ...); the ring call
  sites call the member again. The all_gather slice code is main's
  (identical to this branch's backport of upstream ml-explore#4443).
- quantized_nax.h, fp_quantized_nax.h: main's kernels. Main already keeps
  the revert of the upstream ml-explore#4352 simdgroup skip (no sg_active) and adds
  the ragged-K tail bound of upstream ml-explore#4009.
- fork.yaml: main's file, plus the JACCL description and section. The
  upstream backports this branch carried (ml-explore#4443, ml-explore#4557, ml-explore#4558, the
  scoped_env helper of ml-explore#4474) are now upstream ancestry, so their "not fork
  work" section and its references are gone. The JACCL section now says
  that a mesh with more than one wire has one guard per wire.

Also:
- python/tests/ring_test_distributed.py: main's file. The auto-merge had
  added test_strict_init_after_fallback twice.
- tests/jaccl: the mesh all_gather and sum_scatter calls pass main's new
  stride argument (equal to the chunk size, as main's own
  all_reduce_scatter_gather does).

Tests: tests/jaccl (simulated verbs, ASan and UBSan): 10 of 10 passed.
check_forkdiff.py --upstream-ref upstream/main: 111 changed files, all
described, base afc03c5 is the merge-base. clang-format 21: no changes.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
jonathan308 added a commit that referenced this pull request Oct 26, 2026
…ectives branch

Brings in merge/33 at 187b6c6, which is main f78f0fa (the upstream
ml-explore/mlx sync to afc03c5 with its restored history, and the fork
fixes #40, #43-#46) merged into the JACCL send-frame and progress-guard
branch. This branch had merged main up to ad4745a; everything main added
since comes in here.

Conflicts and how they were resolved:
- mlx/fence.h, mlx/backend/{metal,cuda,no_gpu}/fence.cpp, mlx/transforms.cpp:
  main's value-based fence API (upstream ml-explore#4552: update returns a value,
  wait takes it) with this branch's fenced inputs and eager commit.
  Fence::wait_in_consumer now takes the value of the update it waits for,
  and FencedInput carries that value, so a fenced input never waits for a
  later update than its own. The Metal fence keeps the two-word buffer of
  the fused coherent update, now owned through main's allocator::Data.
- Metal fast fence wait: this branch orders the spin with
  wait_for_all_pending (the pass waits for every pass in flight). Upstream
  ml-explore#4689 gets the same order by putting the spin in a pass of its own
  (end_encoding() before it, end_encoding(true) after it). The merge keeps
  the spin in the current pass and adds a buffer barrier after
  wait_for_all_pending, so it also follows the earlier kernels of the same
  pass, as upstream's separate pass does (the same line that the next
  branch, fix/cluster-metal-fence-wait, adds). A wait adds no encoder
  boundary; tensor-parallel decode waits for a CPU-stream all-sum about
  twice a layer. end_encoding(bool wait_in_flight) stays and shares one
  loop with wait_for_all_pending.
- docs/src/usage/environment_variables.rst: both sides' entries
  (MLX_METAL_FUSED_FENCE_UPDATE, MLX_METAL_FENCED_INPUTS, MLX_CONV_WINOGRAD).
- fork.yaml: the merged JACCL branch's file plus this branch's sections
  (startup readiness, two-rank sealed all-sum, fused fence update, fenced
  inputs, cheaper custom-kernel launches). The fenced-input section now
  explains the fence-wait order against upstream ml-explore#4689 and the value-based
  waits. The "not fork work" backport section is gone (upstream ancestry).

Also adapted:
- mlx/distributed/jaccl/lib/jaccl/mesh.h: main's mesh has one MeshImpl per
  wire (upstream ml-explore#4607). The startup wait budget goes to every wire; the
  two-rank exchange of the streamed sealed sum runs on the first wire. The
  two-Mac cluster's device matrix has one device per peer, so it has one
  wire and nothing changes for it.

Unchanged by the merge: main's quantized, matmul and SDPA kernels and their
dispatch (this branch had no change there beyond what main already carried
since #37), main's revert of the upstream ml-explore#4352 simdgroup skip, and this
branch's revert of 4d91320 (main does not carry 4d91320; the footprint
policy keeps {page, page, 0, 0, page}).

Tests (Ultra A, M5 Ultra; Debug build with Metal, 8 jobs):
- tests/tests on the GPU: 310 of 316 cases passed. The 6 failures (linalg
  QR, inverse, Cholesky, pseudo-inverse, eigh, LU) fail the same way on
  main f78f0fa on this machine.
- tests/tests with DEVICE=cpu: 316 of 316 passed.
- ctest process_allocation_cap: 15 of 15 passed.
- check_forkdiff.py --upstream-ref upstream/main: 137 changed files, all
  described, base afc03c5 is the merge-base.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
jonathan308 added a commit that referenced this pull request Oct 26, 2026
…e fence-ordering branch

Brings in merge/35 at 2b5253a: main f78f0fa (the upstream ml-explore/mlx
sync to afc03c5 and the fork fixes #40, #43-#46) through the JACCL and
cluster-collectives branches, with their conflict resolutions. This
branch's tip is d11238f (32 x 32 steel tiles where kernels are
precompiled).

Conflicts and how they were resolved:
- mlx/transforms.cpp: this branch's event fences (a GPU stream never spins
  on another GPU stream, nor on CPU work that can wait for one) with main's
  value-based waits (upstream ml-explore#4552). FenceNeed records the value of each
  of its two updates (fast_value, event_value); every wait uses the value of
  the update it waits for, fast and event fences alike, and a fenced input
  gets the fast value. Waits look the fence up with at(), as main does.
- mlx/backend/{metal,cuda,no_gpu}/fence.cpp: Fence(Stream, bool fast) next to
  the value-based wait. The Metal fence-wait barrier was already in the
  merged collectives branch (the same line), so it merged cleanly.
- mlx/backend/metal/quantized.cpp (qmm): this branch's affine steel tiles
  (MLX_QMM_STEEL_TILE, 64 x 32 / 32 x 64 for prompts) beside main's FP4
  half-loader 64 x 64 tile (upstream ml-explore#4709). The modes do not overlap:
  wide affine tiles keep the "_bm<m>_bk<k>_bn<n>" name, the FP4 tile keeps
  main's precompiled "_bm_64_bn_64" name. The wide-tile special case is gone:
  main's affine_qmm_t has has_global_scale before BM, BK, BN, so every
  transposed qmm now passes (global scale, bm, bk, bn) in main's order. The
  old call would have instantiated affine_qmm_t<..., bm, bk, bn> into the
  wrong template parameters. d11238f's kernels_jit_compiled() gate is kept.
- mlx/ops.cpp: main's 1-bit affine_quantize branch, with this branch's
  caller stream on the scales select.
- mlx/distributed/sealed.cpp: this branch's __APPLE__ guard around the
  dispatch include.
- tests/CMakeLists.txt: metal_fence_chain beside main's GPU_TEST_SOURCES
  (renamed from METAL_TEST_SOURCES upstream), which now also lists
  d11238f's quantized_tile_tests.cpp.
- .github/workflows/fork-tests.yml: main's restructured workflow; the
  bounded Metal fence chain step now runs in the build-cpp job after the
  standalone allocation cap tests (main has no single build-and-test job
  any more).

fork.yaml merged without conflicts (this branch's sections inside the
merged file). The steel-tile section now also lists kernels.h,
jit_kernels.cpp and nojit_kernels.cpp: main dropped the "Upstream v0.32.2
sync" section that described them on this branch, and d11238f's
kernels_jit_compiled() lives there.

Tests (Ultra A, M5 Ultra; Debug build with Metal, 8 jobs):
- tests/tests on the GPU: 315 of 321 cases passed. The 6 failures (linalg
  QR, inverse, Cholesky, pseudo-inverse, eigh, LU) fail the same way on
  main f78f0fa on this machine: TF32 is on by default on a NAX GPU. With
  MLX_ENABLE_TF32=0: 321 of 321 (with quantized_tile_tests).
- tests/tests with DEVICE=cpu: 321 of 321 passed (with the sealed stream
  tests).
- ctest process_allocation_cap: 15 of 15; metal_fence_chain (fast synch,
  8/50/1024 ops a buffer): 3 of 3.
- check_forkdiff.py --upstream-ref upstream/main: 144 changed files, all
  described. clang-format 21: no changes.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.