Clamp out of range shift amounts - #4271
Conversation
A shift amount that is negative or at least the operand width is undefined in C++. The scalar path took the hardware answer and the vectorized path saturated, so left_shift on the same values returned different results depending on the length of the array. Clamp the amount so both paths agree and match numpy and pytorch.
CI showed the GPU masks the shift count against the operand width, so a CPU only fix left left_shift(1, 32) returning 1 there and 0 on the CPU. Apply the same clamp in both GPU kernels.
|
CI caught a real gap in this and I have pushed the fix. The macOS job failed on the test I added: That is the GPU, not the CPU. It is the same masking behaviour the CPU scalar path had, so a CPU only change left So the clamp now also lives in Worth saying plainly: I could not have found this locally, my build here is CPU only. The failing job is what told me what the GPU actually does, and it is the check on the two kernels I cannot compile. |
|
The GPU fix works. The test that was failing now passes on the Metal run: The three Test macOS is still red, but on a different and unrelated step. After the suite passes, the job dies downloading a wheel: That is the same PyPI 502 that is failing the job on my other PR, and my four other open PRs have this job green, so it is a transient registry problem rather than anything in this branch. A re-run should clear it. I cannot trigger one here. |
On the CPU,
left_shiftreturns a different answer for the same values depending on how long the array is:At n=7 a single array contains both answers: the vectorized body produces 0 and the scalar tail produces 1. A strided view takes the scalar path too, so
mx.left_shift(x[::2], 32)disagrees withmx.left_shift(x, 32).The cause is that a shift amount that is negative or at least the operand width is undefined in C++, and the two paths landed on different answers for it.
binary_ops.hmaps both ops straight onto the built in operators:The scalar overload gets the hardware behaviour, which on arm64 and x86 masks the count against the width, so
1 << 32becomes1 << 0. The vectorized overload goes through the Accelerate vector types, which saturate to 0 instead.numpy and pytorch agree with each other on every case, and mlx's vectorized path already matches them:
So the rule is: out of range means zero for a left shift, and the sign bit for a right shift. This clamps the amount to make that true on both paths. The mask keeps the shift itself in range so the operation is no longer undefined, and the
selectpicks the out of range answer.Verified against numpy over all 8 integer dtypes, array lengths 1, 3, 5, 7, 8, 16, 33, 64 and 257, every shift amount from 0 to width+2 plus
2*widthand8*widthand the negative amounts, and the type min, max, and assorted values. That is 39744 combinations, all matching, where main misses 2697 of them. Also checked 9999 element arrays of randomly mixed amounts, which exercises the vector body and the scalar tail in a single call.The extra select is not free but it is small, 8M elements on an M4:
This started as a CPU only change, because Metal and CUDA also use a bare
x << yand I have no GPU here to see what they actually returned. CI answered that: the macOS run failed on the new test withleft_shift mlx.core.uint32 n=1 1 by 32, which is the GPU masking the count against the width exactly like the CPU scalar path did. So the same clamp is now inmetal/kernels/binary_ops.handcuda/device/binary_ops.cuh, and all three backends agree with numpy and pytorch.The GPU side is the same shape as the CPU one, with the signed check behind
if constexprso the unsigned instantiations do not emit a tautological comparison under-DCMAKE_COMPILE_WARNING_AS_ERROR=ON. Shifts are only instantiated for the eight integer types, so there is no bool, float or complex path to worry about. I still cannot build Metal or CUDA locally, so CI is the check on those two.test_ops.py,test_array.py,test_reduce.py,test_nn.py,test_linalg.py,test_fft.py,test_compile.py,test_vmap.py,test_autograd.py,test_random.py,test_double.pyand the C++ suite (249 cases, 3350 assertions) pass. The added test fails on main withleft_shift mlx.core.uint32 n=1 1 by 32.I am a freshman working through this codebase and I used Claude Code alongside it. Every number above came from a run on this machine.