Repository navigation
Add fixed-size segment env overloads for DeviceSegmentedReduce - #8097
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. |
This comment has been minimized.
This comment has been minimized.
b728f61 to
5f48a59
Compare
This comment has been minimized.
This comment has been minimized.
bernhardmgruber
left a comment
There was a problem hiding this comment.
First pass through the header
8c9d411 to
047a10b
Compare
This comment has been minimized.
This comment has been minimized.
bernhardmgruber
left a comment
There was a problem hiding this comment.
I pushed a bit of refactoring so you can print and compare vectors of pairs.
| { | ||
| private: | ||
| template <typename InputIteratorT, typename OutputIteratorT> | ||
| CUB_RUNTIME_FUNCTION static cudaError_t fixed_size_arg_min_dispatch( |
There was a problem hiding this comment.
Great work! Now try to unify this function with fixed_size_arg_max_dispatch. There are two different tokens that matter ;)
| //! - Provides "run-to-run" determinism for pseudo-associative reduction | ||
| //! (e.g., addition of floating point types) on the same GPU device. | ||
| //! However, results for pseudo-associative reduction may be inconsistent | ||
| //! from one device to a another device of a different compute-capability | ||
| //! because CUB can employ different tile-sizing for different architectures. |
There was a problem hiding this comment.
Q: Why is this comment deleted? Doesn't the fixed-size segmented reduction provide run-to-run determinism? Was the comment wrong?
There was a problem hiding this comment.
this is not an env overload
| cudaStream_t custom_stream; | ||
| REQUIRE(cudaSuccess == cudaStreamCreate(&custom_stream)); |
There was a problem hiding this comment.
Important: Please use cuda::stream instead of manual resource management
This comment has been minimized.
This comment has been minimized.
3d76549 to
0acae2d
Compare
This comment has been minimized.
This comment has been minimized.
- Retain offset_t/SegmentSizeT comments across all fixed-size env overloads - Retain PR NVIDIA#414 breaking-change comments in ArgMin/ArgMax env overloads - Refactor fixed-size ArgMin/ArgMax into shared private helpers (fixed_size_arg_min_dispatch, fixed_size_arg_max_dispatch) to reduce duplication between env and non-env overloads
…) but outside of the literalinclude guards
0acae2d to
f82642c
Compare
This comment has been minimized.
This comment has been minimized.
bernhardmgruber
left a comment
There was a problem hiding this comment.
Two more things, otherwise LGTM
| // allow serializing only pairs with arithmetic members otherwise normal_iterator tests complain | ||
| enable_if_t<::cuda::std::is_arithmetic_v<T1>&& ::cuda::std::is_arithmetic_v<T2>, int> = 0> |
There was a problem hiding this comment.
I don't understand why this is necessary. If operator<< is called for a pair of iterators, and you can't serialize an iterator, then why does removing this overload allow the program to compile? what other operator<< is chosen instead?
Which test fails to compile if you remove the enable_if?
There was a problem hiding this comment.
ci was failing in libcudacxx/test/libcudacxx/std/algorithms/alg.modifying/alg.partitions/pstl_partition_copy.cu in L#67 before i add this sfinae
There was a problem hiding this comment.
Thx! I understand now what's happening. Catch2 was trying to find an operator<<(osteam, pair<some thrust iterators>). Before, it didn't fine one and just did not generate code for printing the value. With the above change (and no constraint) it found one, but then the implementation of operator<< did not compile.
The trick here is, as you noticed, to make the operator<< SFINAE friendly. That means, that the operator<< will SFINAE itself out if it knows that its implementation would not compile. This way, a library like Catch2 can detect whether it can use the operator<< (by testing whether a call to that operator<< would compile).
I refined the constraint to use:
template <typename T1,
typename T2,
::cuda::std::void_t<decltype(::cuda::std::declval<::std::ostream>()
<< ::cuda::std::declval<T1>() << ::cuda::std::declval<T2>())>* = nullptr>
::std::ostream& operator<<(::std::ostream& os, const pair<T1, T2>& pair)This tests whether T1 and T2 can we written to an ostream as part of the interface of operator<<. If they can't be written, the operator<< is SFINAE-ed out.
44d2941 to
cee2dba
Compare
cee2dba to
3c44079
Compare
This comment has been minimized.
This comment has been minimized.
🥳 CI Workflow Results🟩 Finished in 1h 10m: Pass: 100%/269 | Total: 2d 00h | Max: 45m 54s | Hits: 98%/177147See results here. |
…A#8097) * Add fixed-size segment env overloads for DeviceSegmentedReduce * Fix critical issues plus<> specilization * feedback address - Retain offset_t/SegmentSizeT comments across all fixed-size env overloads - Retain PR NVIDIA#414 breaking-change comments in ArgMin/ArgMax env overloads - Refactor fixed-size ArgMin/ArgMax into shared private helpers (fixed_size_arg_min_dispatch, fixed_size_arg_max_dispatch) to reduce duplication between env and non-env overloads * Refactor * More refactoring * Refactor: unify fixed-size ArgMin/ArgMax dispatch into a single function * Make using offset_t = int explicit * Substitute cudaStream_t with cuda::stream and use always stream.sync() but outside of the literalinclude guards * Guard Sum with sfinae * Sfinae operator<<(os,...) for cuda::std::pairs * Do not specialize std::plus<> --------- Co-authored-by: Bernhard Manfred Gruber <bernhardmgruber@gmail.com>
Split 2/2 for env
SegmentedReduceoperator<<(ostream, cub::KeyValuePair)#8335closes #7550