Add multi-GPU sort - #9982
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
9fe9a67 to
84caef1
Compare
|
/ok to test |
1 similar comment
|
/ok to test |
This comment has been minimized.
This comment has been minimized.
|
/ok to test |
This comment has been minimized.
This comment has been minimized.
3058e3c to
5884e03
Compare
|
/ok to test |
5 similar comments
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
c28dfe8 to
0066415
Compare
|
/ok to test |
This comment has been minimized.
This comment has been minimized.
b018e98 to
9fd6abb
Compare
|
/ok to test |
2 similar comments
|
/ok to test |
|
/ok to test |
This comment has been minimized.
This comment has been minimized.
0c31886 to
4c65b39
Compare
|
/ok to test |
1 similar comment
|
/ok to test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
libcudacxx/include/cuda/__runtime/types.h (1)
39-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Repeat the exact preprocessor condition in both branch comments.
The
#elseand#endifcomments do not repeat_CCCL_CTK_AT_LEAST(12, 2), making the branch harder to audit. Use the exact condition text in both comments.Based on learnings, annotated preprocessor branches must repeat the corresponding condition verbatim.
Source: Learnings
cudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/local_setup.h (1)
55-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion:
@tparam _Traitsno longer exists; the template parameters are_Tp,_Env,_BinaryOp(plus the deduced range parameters). Stale leftover from the traits-based design.Source: Coding guidelines
cudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/merge_k_way.h (1)
66-100: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winsuggestion: the loop bound is
__displs.size()but the body indexes__counts[__i], and__totalassumes__displsis the exclusive scan of__counts. Assert the contract so a caller mismatch fails loudly instead of reading out of bounds.Proposed assert
+ _CCCL_ASSERT(__counts.size() == __displs.size(), "counts and displs must have equal length"); const auto __total = __counts.back() + __displs.back();cudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/rebalance.h (1)
284-288: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: the transfers being awaited here are D2H (counts/displacements into host vectors), not HtoD — the comment reads backwards.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ded6089e-0fc1-4912-9a0c-c1d481dd2b66
📒 Files selected for processing (18)
cudax/include/cuda/experimental/__multi_gpu/algorithm/common.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/bucket_count_fn.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/buffer.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/data_exchange.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/execute.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/histogramming.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/ideal_rank_fn.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/local_setup.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/merge_k_way.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/rebalance.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/sorter.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/sort.hcudax/test/multi_gpu/algorithms/CMakeLists.txtcudax/test/multi_gpu/algorithms/sort/CMakeLists.txtcudax/test/multi_gpu/algorithms/sort/range_basic.culibcudacxx/include/cuda/__algorithm/copy.hlibcudacxx/include/cuda/__runtime/types.hlibcudacxx/include/cuda/std/__random/linear_congruential_engine.h
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 487ff05f-fd57-4ba5-beaf-c8377929028b
📒 Files selected for processing (10)
cudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/bucket_count_fn.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/data_exchange.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/execute.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/histogramming.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/ideal_rank_fn.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/local_setup.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/merge_k_way.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/rebalance.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/sorter.hcudax/include/cuda/experimental/__multi_gpu/algorithm/sort/sort.h
🚧 Files skipped from review as they are similar to previous changes (8)
- cudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/bucket_count_fn.h
- cudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/ideal_rank_fn.h
- cudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/execute.h
- cudax/include/cuda/experimental/__multi_gpu/algorithm/sort/sort.h
- cudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/rebalance.h
- cudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/local_setup.h
- cudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/data_exchange.h
- cudax/include/cuda/experimental/__multi_gpu/algorithm/sort/hss/histogramming.h
There was a problem hiding this comment.
Note this whole beast will be replaced by #10221
| { | ||
| __range.resize(__size); | ||
| } | ||
| } |
There was a problem hiding this comment.
This probably stays after adding the new resizable buffer, but I don't know how to handle user-provided buffers that cannot resize using one of these methods. We might consider just using a temporary buffer instead? But that probably balloons the already large temporary memory requirement for this algorithm.
| __CUDAX_MULTI_GPU_DISPATCH( | ||
| __comm.logical_device(), | ||
| CUB_NS_QUALIFIER::DeviceMergeSort::SortKeys, | ||
| ::cuda::std::to_address(::cuda::std::ranges::begin(__input)), | ||
| ::cuda::std::ranges::size(__input), | ||
| __cmp, | ||
| __env); |
There was a problem hiding this comment.
Could consider using radix sort here but... it's a bit painful to set that up (need to allocate a double buffer etc). I leave this for a follow-up
| template <class _CommRange, class _EnvRange, class _InputRange> | ||
| _CCCL_HOST_API void _HSSSorter<_Tp, _Env, _BinaryOp>::__rebalance_to_original_counts( | ||
| const __local_setup_result_type& __setup, _CommRange&& __comms, _EnvRange&& __envs, _InputRange&& __local_inputs) | ||
| { |
There was a problem hiding this comment.
This function is incredibly painful. Basically, we have just done a whole bunch of work sorting the array but as a side effect have ended up with pretty balanced distributions across all ranks. Since sort()s contract is that whatever size comes in must also come out, we now need to undo that and unbalance all the ranks.
This, in my opinion can probably be done much more intelligently than I do here, it's pretty brute force.
| template <template <class> class _Buffer, class _Tp> | ||
| struct _PerCommSplitters | ||
| { | ||
| _Buffer<_Bracket<_Tp>> __Ls; | ||
| _Buffer<_Bracket<_Tp>> __Us; | ||
| _Buffer<_Tp> __probes; | ||
| }; |
There was a problem hiding this comment.
The point of pulling these out is so that they depend only on as many template parameters as needed. However, _Buffer depends on the memory resource which depends on the environment so in reality they maybe doesn't save that much.
The point of fully generic _Buffer here also (they are all some flave of cuda::buffer under the hood) is to respect the various properties that come from the user's memory resource, in case later algorithms take advantage of these.
But maybe this is overkill, and we can just collapse these down to
cuda::buffer<_Bracket<_Tp>, cuda::mr::device_accessible> __LsBecause keeping them fully generic does have compile-time impact.
😬 CI Workflow Results🟥 Finished in 6h 30m: Pass: 98%/165 | Total: 2d 12h | Max: 6h 00m | Hits: 99%/382531See results here. |
e64cb0a to
b14f491
Compare
6c26cb6 to
87185a9
Compare
1 participant
Description
Needs #10221
Checklist