Add mx.searchsorted with CPU, Metal and CUDA kernels - #4035
Conversation
|
Thanks for the PR, just skimmed the implementation and I think it is a good start, it will take a while before we can do a formal review though. |
Closes ml-explore#1255. One thread per element of the values input, each running an independent binary search over the sorted sequence. Both bounds are the same descent with a different predicate, so left and right share a code path: left (lower bound) advances while lt(a[mid], v) right (upper bound) advances while !lt(v, a[mid]) Ordering follows sort rather than raw IEEE comparison. numpy and torch disagree here, numpy placing NaN last and torch walking past it, and MLX already implements numpy's rule in sort on all three backends. If searchsorted compared with a raw <, it would disagree with the op that produces its input, making searchsorted(sort(x), v) wrong wherever x contains a NaN. Each backend's implementation therefore sits beside its sort and calls that sort's existing comparator instead of defining a second one: nan_aware_less on CPU, LessThan on Metal and CUDA. The output is uint32 with the shape of the values input, matching argsort, argmax, argmin and argpartition. numpy returns int64; the deviation is deliberate and consistent with the rest of MLX. sorter, batched sorted sequences and an axis argument are all left out. Each is additive later, and none is needed for the numpy contract.
|
No rush on the review, thanks for taking a look. The two Windows CUDA failures are mine. I passed a tag dispatch value straight into a template argument, which nvcc accepts with gcc as the host compiler but not with MSVC, hence "expression must have a constant value". Actively working on the fix; it needs to use the The macOS failure looks unrelated, it is the distributed ring test. |
caf20c4 to
e9b37c3
Compare
An empty sequence has no buffer to bind, so dispatching the kernel left argument 0 unset and Metal's argument validation aborted the process: validateComputeFunctionArguments:1038: failed assertion `Compute Function(searchsorted_v_float32_left): missing Buffer binding at index 0 for a[0].' The existing guard only covered an empty output, which does not catch an empty sequence paired with non-empty values. Every value belongs at index 0 in that case, which is the answer the CPU path already reaches by walking a zero-length range, so fill the output and skip the dispatch. Covered by the empty-input case already in test_searchsorted; it aborted rather than failed, so it only surfaces where Metal validation is on.
|
Correction to what I said earlier: the macOS failure was not the distributed ring test, it An empty sorted sequence has no buffer to bind, so argument 0 was never set and Metal's Fixed by filling the output and skipping the dispatch: with nothing to compare against every Verified on an M5 Max with Fork CI is 22/22 green, and One unrelated thing I hit while checking, in case it is useful: |
| return {zeros(primals[0].shape(), uint32, stream())}; | ||
| } | ||
|
|
||
| bool SearchSorted::is_equivalent(const Primitive& other) const { |
There was a problem hiding this comment.
Can you move the definitions so they are in the same order as the class shows in the header?
| const std::vector<array>&) { | ||
| std::vector<array> vjps; | ||
| for (auto arg : argnums) { | ||
| vjps.push_back(zeros_like(primals[arg], stream())); |
There was a problem hiding this comment.
I assume vjp/jvp is not available for searchsorted args? It show throw error "Use stop_gradient on indices to stop gradients from being computed."
There was a problem hiding this comment.
searchsorted is piecewise constant, so zero is the true derivative rather than a placeholder, same as ArgSort and the comparison ops. The stop_gradient error belongs to gather/scatter, where indices are an input; here they are the output.
| using CTYPE = MLX_GET_TYPE(type_tag); | ||
| using T = cuda_type_t<CTYPE>; | ||
| dispatch_bool(right_, [&](auto right_tag) { | ||
| dispatch_bool(out.size() > INT32_MAX, [&](auto large) { |
There was a problem hiding this comment.
There won't be much threads occupied by the kernel so it is fine to always use int64_t as index.
There was a problem hiding this comment.
Done, always int64_t now, which drops the dispatch. Left the runtime flag on get_launch_args, since that one picks the grid shape rather than the index width.
| if (index >= size) { | ||
| return; | ||
| } | ||
| IdxT loc = contiguous |
There was a problem hiding this comment.
It should be very rare for the array and values to be non-contiguous, we should just assume they are contiguous in the kernel, and do a contiguous copy in host when necessary. This is also PyTorch does in their implementation.
There was a problem hiding this comment.
Done. contiguous_copy_gpu on the host when the values are not row-contiguous, and the kernel indexes directly. Dropped the shape/strides/ndim/contiguous params with it.
vmap, vjp, jvp, is_equivalent, output_shapes.
Always index with int64_t, which drops the large/small dispatch, and assume the values are contiguous in the kernel, copying on the host when they are not.
|
All four addressed. The CUDA change is compile-checked only on my side. The equivalent cases pass on CPU and Metal locally: transposed, broadcast, 0-d and empty inputs. |
Closes #1255.
Adds
mx.searchsortedas a primitive with hand written kernels for CPU, Metal and CUDA. This replaces #4014, which composed the same op out of existing primitives and was closed with "this is something that we want to have a kernel rather than a fallback". That was the right call: the composed form costs a dispatch per binary search step, and the numbers below show what that is worth.Design
One thread per element of
values, each running an independent binary search over the sorted sequence.sorted_sequenceis 1-D,valuesmay be any shape, and the output takes the shape ofvalues.Ordering follows
sort, not raw IEEE. This is the one decision worth calling out. numpy and torch disagree here: numpy uses a total order where NaN sorts last, torch uses raw comparisons and walks past NaNs. MLX already has numpy's rule insort, in three places that all implement the same predicate:nan_aware_less,mlx/backend/cpu/sort.cppLessThan,mlx/backend/metal/kernels/sort.hLessThan,mlx/backend/cuda/sort.cuIf
searchsortedused a raw<it would disagree with the very op that produces its input, sosearchsorted(sort(x), v)would be wrong for anyxcontaining a NaN. Each backend's implementation therefore lives beside its sort and calls that sort's existing comparator rather than defining a second one. That is also why the three files touched are the existingsort.cpp/sort.curather than new ones.The two bounds are the same descent with a different predicate, so both sides share one code path:
Output is
uint32, matchingargsort,argmax,argminandargpartition. numpy returnsint64; the deviation is deliberate and consistent with the rest of MLX.Numbers
M5 Max. Wall clock warmup, arms interleaved round by round, median of five 200 ms windows.
composedis the branchless form of the binary search sketched in #1255,linearis the(a[None,:] < v[:,None]).sum(1)workaround from the same thread.A single dispatch costs 0.150 ms on this machine, measured as a one element
abs. Every row up to and including 16777216 x 1024 sits at that floor, so those rows compare one launch against roughly a hundred rather than comparing search throughput:Past the floor, where the search itself dominates:
Treat the absolute times as approximate. Across sessions the dispatch floor on this machine moved between 0.131 ms and 0.150 ms depending on how warm it was, and every arm moved with it, so the ratios drifted by up to 8% (the largest row measured between 13.2x and 14.3x). The tables above are one internally consistent run rather than a mix. The shape of the result is not sensitive to that: the kernel is roughly an order of magnitude ahead once the search dominates, and the ordering never changes.
This also settles the open question from #1255, which was whether to dispatch between the two workarounds based on size:
There is no crossover worth dispatching on. The linear form never beats the kernel at any size measured, and it allocates
n*mso it runs out of memory at 16384 x 16384. A single kernel covers the whole range.Correctness
Validated against numpy over 459 checks: randomized sweeps, sizes straddling the threadgroup and work per thread boundaries, all dtypes, mixed dtype promotion, empty and 0-d inputs, values above and below the range, ties, infinities, NaN in either argument, and an exhaustive check for every n from 1 to 32 against every gap and every value.
The cases worth naming, because they are the ones a hand written kernel gets wrong and a composed one structurally cannot:
values. Transposed, sliced, reversed and broadcast views. The first version of this usedflags().contiguous, which only promises the buffer has no gaps and is therefore true for a transpose. The result was the right values in the wrong order. It needsflags().row_contiguous.sorted_sequence, including a reversed view, so the search has to honour a negative stride.Also checked directly: inserting each returned index into the sequence keeps it sorted under
mx.sort's own ordering, on the same device. That is the property the op actually owes, and it is stronger than agreeing with numpy.python/tests/test_ops.py::test_searchsortedcovers the same ground in the upstream suite. Full suite is green (802 tests).MLX_METAL_JIT=ONwas built and run, since the JIT preamble is only assembled at first use and a broken one compiles fine.The primitive is registered in
mlx/export.cppandtest_export_import.py::test_export_searchsortedround trips both sides. Without that entry everything else still passes and onlymx.export_functionfails, so it is worth an explicit test.The C++ suite has 6 pre-existing
linalg_testsfailures on this machine; they reproduce identically on a cleanorigin/mainbuild, so they are unrelated.Scope
Deliberately left out, all additive later:
sorter=. It issearchsorted(take(a, sorter), v)at the op level, and pushing it into the kernel costs a dependent load per step for everyone who does not pass it.sorted_sequence. torch supports it, numpy raises, and it changes the output shape rule. [WIP] Add mlx.core.searchsorted #2817 invented a third rule for this and shipped it unfinished.axis=. numpy has no such argument.Built and linked in three configurations, since the default build exercises none of the backend-absent paths: the default Metal build,
MLX_METAL_JIT=ON, and the-DMLX_BUILD_CPU=OFF -DBUILD_SHARED_LIBS=ON -DMLX_METAL_JIT=ONcombination the macOSjitjob uses.Known weak points
The CUDA kernel compiles but has never executed. I have no NVIDIA device, so I ran the GPU-less build on a fork with your own
setupandbuildactions, which fall back to-DMLX_CUDA_ARCHITECTURES=80 -DMLX_BUILD_TESTS=OFFwhen__nvcc_device_queryfinds no card. It builds clean on cuda-12.6, 12.9 and 13.0, with-DCMAKE_COMPILE_WARNING_AS_ERROR=ON, andlibmlx.solinks withSearchSorted::eval_gpupresent innmoutput, including thecomplex64instantiation. So nvcc and the linker are happy. What is still unverified is that it produces correct numbers on real hardware: no kernel launch has ever happened. That rests on the CUDA kernel being the same binary search over the sameLessThancomparator as the CPU and Metal paths, which is an inference from the shared algorithm rather than a measurement.The CUDA path does not specialise on
ndimthe waycopy_general.cuandternary.cudo (it does specialise on index width). Straightforward follow up once it is known to run correctly.Mixed dtypes are searched in
promote_types(sorted_sequence, values), which is MLX's rule and not numpy's. MLX keepsint32withfloat32infloat32where numpy widens tofloat64, so an integer sequence searched with float values is rounded first. That is consistent with every other MLX op rather than with numpy, and the docstring says so.sorted_sequenceis capped atUINT32_MAXelements, which the op rejects explicitly rather than truncating, since theuint32result cannot index past that anyway.Complex values with a NaN in the imaginary part only are ordered differently by CPU and GPU. That is pre-existing in
mx.sort, not new here:nan_aware_lessinmlx/backend/cpu/sort.cppcallsstd::isnanon acomplex64_t, which converts throughcomplex64_t::operator float()and therefore only inspects the real part, while the Metal and CUDA comparators test both parts.searchsortedinherits this rather than introducing it, and inherits it correctly: on each device it returns the indexsorton that same device would put the value at (0 on CPU, 2 on GPU for the case above). So the per-device contract holds exactly wheresortis self-inconsistent across devices, which is the behaviour this design intends. Fixing the CPU comparator is a one line change but it altersmx.sort's output for complex input, so it belongs in its own PR rather than bundled here. Happy to send that separately.Denormals differ between backends on Metal, which flushes them in comparisons.
mx.sortalready has this (np.sortputs1e-40last,mx.sorton GPU puts it first), sosearchsortedinherits it rather than introducing it. The test asserts consistency withmx.sortthere instead of with numpy.