[CUDA][Improvement] Fast path for gather #3794
Open
nastya236 wants to merge 1 commit into
Open
Conversation
zcbenz
reviewed
Jul 5, 2026
Comment on lines
+138
to
+139
| static_cast<uint32_t>(inputs[1].size()), | ||
| (n_vec + block_dim - 1) / block_dim, |
Collaborator
There was a problem hiding this comment.
Have you tried swapping grid.x and grid.y? It seems that currently the blocks would read in this sequence:
block0: src[0, 0:1024]
block1: src[1, 0:1024]
block2: src[2, 0:1024]
...
block8: src[0, 1024:2048]
block9: src[1, 1024:2048]
we might get better coalesce by swapping:
block0: src[0, 0:1024]
block1: src[0, 1024:2048]
block2: src[0, 2048:4096]
...
block8: src[1, 0:1024]
block9: src[1, 1024:2048]
For benchmarking you can use env var MLX_USE_CUDA_GRAPHS=0 to remove the overhead of cuda graph, there would still be overhead of lazy evaluation though.
Comment on lines
+133
to
+137
| uint32_t n_vec = (slice_size + vec_size - 1) / vec_size; | ||
| uint32_t max_block_dim = 256; | ||
| uint32_t block_dim = n_vec < max_block_dim ? n_vec : max_block_dim; | ||
| dim3 block(block_dim, 1, 1); | ||
| dim3 grid( |
Collaborator
There was a problem hiding this comment.
Some nitpickings:
Suggested change
| uint32_t n_vec = (slice_size + vec_size - 1) / vec_size; | |
| uint32_t max_block_dim = 256; | |
| uint32_t block_dim = n_vec < max_block_dim ? n_vec : max_block_dim; | |
| dim3 block(block_dim, 1, 1); | |
| dim3 grid( | |
| uint32_t n_vec = cuda::ceil_div(slice_size, vec_size); | |
| uint32_t max_block_dim = 256; | |
| uint32_t block_dim = std::min(n_vec, max_block_dim); | |
| dim3 block_dims(block_dim, 1, 1); | |
| dim3 num_blocks( |
We don't have standards on what names to call block and grid dims but we should use the same names in the same file. (We did not use the more common grid_dims as name because it means a different thing in Metal unfortunately.)
|
|
||
| if (nidx == 1 && axes_[0] == 0 && src.flags().row_contiguous && | ||
| inputs[1].flags().row_contiguous && slice_size == src.strides()[0] && | ||
| inputs[1].size() <= static_cast<size_t>(INT32_MAX)) { |
Collaborator
There was a problem hiding this comment.
It seems that the kernel is capable of handling the large size?
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.
Fast path for the case when gathered axis = 0, input is contiguous and slice size is a full stride. It is useful for gather before gather_mm and scatter after (for MoEs).
Some numbers on B200:
input shape is (B*L, D), output shape is (T, D)
Before:
1.5219 ms, 705.5 GB/s
After:
0.2272 ms 4725.9 GB/s
Torch:
0.1864 ms 5759.1 GB/s
I think the remaining ~0.05 ms is due to kernel launch overhead rather than kernel execution itself. That said, if you spot anything that could be optimized further to reduce it, I'm happy to address it.