Repository navigation
fix(ops): floor_divide floors for negative signed integers - #32
Merged
Merged
Conversation
floor_divide on integers uses the Divide primitive. The CPU and Metal Divide functors truncated toward zero, so -7 // 2 gave -3. NumPy and Python give -4. For signed integers the functors now subtract 1 when the remainder is not zero and the signs are different. A zero divisor keeps the old x / y result. Unsigned and float division do not change. Add C++ and Python tests for int8 to int64 with all sign combinations, exact multiples, scalar, strided and compiled inputs, and checks that unsigned and float results do not change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The signed-integer CPU Divide computed x / y before it checked y != 0. C++ does not define integer division by 0, so the compiler could remove the check. It also always computed q - 1, which overflows for min // 1. Now the functor divides by 1 where y == 0 or where min is divided by -1, returns 0 for x // 0 and min for min // -1 (the values arm64 gave before), and subtracts 1 only in the lanes that need it. Add tests with 40 elements, so the 16-lane int8 and int16 SIMD path runs, and CPU tests for x // 0 and min // -1. State the upstream ml-explore#4515 risk, the CUDA functor and the mlx-swift generated copies in fork.yaml. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.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.
Proposed changes
The defect
floor_divideon integer arrays rounds toward zero. It must round down.-7 // 2gives -3. NumPy (np.floor_divide) and Python (//) give -4. A test in the Swift wrapper found this.mlx/ops.cpp:3142-3155(floor_divide): for float types it returnsfloor(divide(a, b)), which is correct. For integer types (and bool) it makes aDivideprimitive on the integer inputs.mlx/backend/cpu/binary_ops.h:28onmain:DEFAULT_BINARY_OP(Divide, operator/). This is C++/, which truncates toward zero.mlx/backend/metal/kernels/binary_ops.h:43-48onmain:Dividereturnsx / y. Metal integer/also truncates toward zero.__floordiv__,__rfloordiv__and__ifloordiv__(python/src/array.cpp:691-719) callmx::floor_divide. They have the same defect.What happened before this PR:
floor(a / b). These do not use the integerDividepath.Integer
Divideexists only forfloor_divide.dividepromotes integers to float first (mlx/ops.cpp,at_least_float).Divide::vmapalready maps an integerDividetofloor_divide(mlx/primitives.cpp:1885).divmodwas already correct on all backends: the CPUDivModcode and the Metal and CUDAFloorDividefunctors floor the quotient.Upstream status
Upstream has the same defect up to 2026-09-28. Issue ml-explore#4450 reported it. Upstream fixed it in ml-explore#4515 (commit
0a8e18e0). That fix is inops.cpp. It keeps the truncatingDivideand adds 7 more graph nodes (remainder, twoless, twonot_equal,logical_and,astype,subtract) to subtract 1. Upstream did not keep truncation on purpose. It calls truncation a bug.This PR does not port ml-explore#4515 as it is, for two reasons:
-7 // 0gives 0 on the CPU (arm64sdivreturns 0). With Fix floor_divide for integers ml-explore/mlx#4515,remainder(-7, 0)is -7, which is not 0, and the signs are different, so the result becomes -1.floor_divide. The fix in the functor adds none.If a later upstream sync ports ml-explore#4515 on top of these flooring kernels,
floor_dividesubtracts 1 twice (-7 // 2gives -5). The new tests catch this.The fix, per backend
For signed integers:
q = a / b(truncated). If the remainder is not zero and the signs ofaandbare different, thenq = q - 1.mlx/backend/cpu/binary_ops.h):Divideis now its own struct. It does the step above for signed integerT, for SIMD widths N > 1 and for N = 1. C++ does not definex / 0ormin / -1, so the functor divides by 1 in those lanes (d = select(y == 0 || (x == min && y == -1), 1, y)). Then it selects 0 wherey == 0. It subtractsselect(mask, 1, 0), so it does not computeq - 1in lanes that do not need it (INT_MIN // 1does not overflow). All other types still usex / y. The CPU compile path (compiled_cpu) uses the same functor.mlx/backend/metal/kernels/binary_ops.h):Dividehas two overloads, in the same form as the existingFloorDivideandRemainder. The signed-integer overload does the step above. The other overload (unsigned, bool, float, half, bfloat16, complex) still returnsx / y. Fused kernels frommx.compileuse the same header.cu::Divide(mlx/backend/cuda/device/binary_ops.cuh:36-41) still returnsx / y. On CUDA, integerfloor_dividestill truncates. The CUDA file already has a correctFloorDividefunctor, whichDivModuses. This PR does not touch CUDA, so the CUDA build does not change. No workflow in this fork builds CUDA.Division by zero does not change. On
mainthe CPU computesx / 0with C++/(DEFAULT_BINARY_OP(Divide, operator/),Simd<T, 1>and the clang vector/for N > 1). C++ does not define this, but arm64sdivandudivreturn 0, somaingives 0 for signed and unsigned integers on the CPU. The new signed-integer code does not divide by 0: it returns 0 explicitly, so the result does not depend on the compiler. Unsigned integers still usex / y, as onmain. On Metal, the Metal Shading Language does not define integer division by zero. The value comes from the GPU hardware and this PR does not change it. For floats, the result isfloor(±inf)or NaN, as before. (Note:divmodalready gives -1 for-7 divmod 0on the CPU. This PR does not changedivmod.)The overflow case
INT_MIN // -1does not change. Onmainthe CPU givesINT_MIN(arm64sdivwraps; int8 and int16 promote tointand convert back). The new CPU code returnsINT_MINexplicitly and does not divide. On Metal this case is not defined and is not tested.Call-site risk
The behavior changes only for signed integers with operands of different signs and a remainder that is not zero.
This repository
mlx/primitives.cpp:1885,Divide::vmap: usesfloor_dividefor integerDivide. It now gives the same result as the unbatched op.python/mlx/optimizers/schedulers.py:58:step // step_size. The step count is not negative. No change.//inpython/mlxare on Python ints (shapes, sizes), not arrays.mlx-swift (
Source/, read only, at0f4fe40)Source/MLX/Ops+Array.swift:752(floorDivide(_:_:stream:)) andSource/MLX/MLXArray+Ops.swift:1913(MLXArray.floorDivide(_:stream:)) are wrappers aroundmlx_floor_divide. They pass on the new behavior. The other hits are documentation. A test in the wrapper found this defect, so code in the wrapper that expects -4 now passes.mlx-swift-lm (
Libraries/, read only, ate22fc82b)MLXLLM/Models/Qwen4ExpCompactQSA.swift:35floorDivide(positions + 1, 4)positionsfromint32Range(offset, offset + width), withprecondition(offset >= 0)MLXLLM/Models/Qwen4ExpQSA.swift:479floorDivide(int32Range(queryStart, keyTokens) + 1, ratio)queryStart = keyTokens - queryTokens(not negative),ratiois the compress ratio (positive)MLXLLM/Models/Qwen4ExpQSA.swift:883same expressionMLXLLM/Models/Qwen4ExpQSA.swift:995floorDivide(tokInt, ratio)ratiopositiveMLXLLM/Models/Qwen4ExpQSA.swift:999floorDivide(pos + 1, ratio)posfromint32Range(queryStart, keyTokens)MLXLMCommon/SwitchLayers.swift:250order.floorDivide(m)orderfromargSort(uint32),m = indices.dim(-1)No call site found can get negative operands, so none depends on the old truncation. A related case: the DeepseekV4 pool mask used true division where the reference uses floor division. That is fixed separately and does not use
floor_divide.Downstream: mlx-swift
mlx-swift keeps generated copies of both headers:
Source/Cmlx/mlx-generated/binary_ops.cpp,Source/Cmlx/mlx-generated/metal/binary_ops.handSource/Cmlx/mlx-generated/compiled_preamble.cpp. At the mlx-swift pin bump,tools/update-mlx.shmust run. If it does not, the Swift Metal path still truncates while the CPU floors.Test plan
tests/ops_tests.cpp,test floor_divide with negative operands): onDevice::cpu, and onDevice::gpuwhen Metal is available. For int8, int16, int32 and int64: all four sign combinations, exact multiples, zero dividends, and quotients between -1 and 0. Also a scalar divisor, transposed (strided) inputs, a match withdivmod(a, b)[0], the int8 limits (-128 // 3 == -43,127 // -2 == -64), unsigned types (no change) and float32 (no change). A 40-element case for each signed width, so the SIMD path runs (int8 and int16 have 16 lanes).test floor_divide integer edge cases on the CPU(CPU only):floor_divide({-7, 7, 0}, 0) == {0, 0, 0}, a 33-elementx // 0 == 0, and a 33-elementmin // -1 == minfor each signed width.python/tests/test_ops.py,test_floor_divide_negative_operands): the same cases on the default device, checked againstnp.floor_divideand Python//, throughmx.floor_divide,//,__rfloordiv__(-7 // y),divmod, transposed inputs and anmx.compilefused kernel (u // v + 1). Also unsigned, float16 and float32. The same 40-element case, and onmx.cputhe samex // 0andmin // -1cases. Metal does not define these two cases, so they are not tested on the GPU.main: onmain,Dividetruncates, so-7 // 2gives -3, and the tests expect -4. Upstream's test for Fix floor_divide for integers ml-explore/mlx#4515 failed in the same way before its fix.metal-build-and-test, macos-26) on head34b31e90: run 36943701359 passed: 292 C++ test cases with 3945 assertions, and 876 Python tests with 52 skipped. Build and Test run 36943701360 and Fork Diff Check run 36943701354 also passed./usr/bin/python3 scripts/check_forkdiff.py --upstream-ref refs/remotes/upstream/mainon the head commit: 173 of 173 changed files described.fork.yamlhas a new section, "Integer floor_divide rounds down".The Python tests use only the default device. The CPU pass in #30 runs the Python suite with
DEVICE=cpu. After this PR and #30 merge, that pass runs these same cases on the CPU. The C++ test already runs the CPU and the GPU in one pass.CONTRIBUTING checklist
Formatting:
uvx pre-commit run --fileson the changed files (clang-format, black, isort).Tests: new C++ and Python tests, listed above.
Benchmarks: no benchmark. The change adds a compare and a subtract to the signed-integer
Divideonly. Float and unsigned paths do not change. No local build was done (the laptop disk is shared). CI is the build and test authority.☐ I understand it is strictly prohibited to use AI to write PR description
AI usage disclosure: this change and this description were written by Claude (Opus 5.5) under David Tai's direction and review.
🤖 Generated with Claude Code