Create unique, sequence, and fill wrappers in cugraph to reduce binary size#5559
Create unique, sequence, and fill wrappers in cugraph to reduce binary size#5559ChuckHastings wants to merge 6 commits into
Conversation
…h::sequence, cugraph::unique, cugraph::fill
|
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. |
ChuckHastings
added
improvement
| T const& value) const | ||
| { | ||
| using value_t = detail::fill_iterator_value_t<ForwardIterator>; | ||
| if constexpr (detail::fill_supported_iterator_v<ForwardIterator>) { |
There was a problem hiding this comment.
Don't we need to check for exec_policy (i.e. detail::is_rmm_exec_policy_v)?
There was a problem hiding this comment.
We currently instantiate both of the exec policies that we currently use (rmm::exec_policy, rmm::exec_policy_nosync), so everything currently compiles and executes properly. If we were to try and use a different exec policy, the compilation would fail - so at least it wouldn't be a runtime error.
We could add a safety check in case we want things to keep compiling in those cases (if we somehow add a different exec policy). But if we do that I suspect we should do it for all of the functions in this file.
| RandomAccessIterator last) const | ||
| { | ||
| using value_t = detail::sort_iterator_value_t<RandomAccessIterator>; | ||
| if constexpr (detail::sort_supported_arithmetic_scalar_v<value_t> || |
There was a problem hiding this comment.
Why are we using sort_supported_arithmetic_scalar_v here but checking for iterator in other places? (e.g. fill_supported_iterator_v). And no need to check for exec_policy?
Will this work if RandomAccessIterator is not a pointer?
There was a problem hiding this comment.
Yes, this was an oversight in the original implementation. I will fix this.
…ighten guard around iterators, add nosync exec policy support for scan, saving more binary size
seunghwak
left a comment
seunghwak
left a comment
There was a problem hiding this comment.
LGTM besides few minor cosmetic issues
| * matching explicit instantiation (otherwise a call through @ref cugraph::sort_wrapper can fail at | ||
| * matching explicit instantiation (otherwise a call through @ref cugraph::sort can fail at | ||
| * link | ||
| * time). |
There was a problem hiding this comment.
Something very minor but "link" and "time" can go to the same line. No need to break line here.
| { | ||
| if constexpr ((detail::is_rmm_exec_policy_v<ExecutionPolicy> || | ||
| detail::is_rmm_exec_policy_nosync_v<ExecutionPolicy>) && | ||
| detail::sort_supported_iterator_v<RandomAccessIterator>) { |
There was a problem hiding this comment.
Isn't this a misnomer?
If sort & unique support same types of iterators, should we better define
using iterator_supported_iterator_v = sort_supported_iterator_v;
?
There was a problem hiding this comment.
Added additional using.
| namespace detail { | ||
|
|
||
| /** @brief Whether iterator type @p T has an out-of-line @ref sort_impl lexicographic sort. */ | ||
| /** @brief True for scalar value types dispatched to @ref sort_impl / @ref unique_impl. */ |
There was a problem hiding this comment.
Something minor but what's the rule in placing these type trait templates?
We have
sort related templates
scan related templates
rmm_exec_policy related templates
more scan related templates
scan_impl
sort_impl
unique_impl
more sort related templates
fill related templates
sequence related templates
(and they alternate few times)
fill_impl
sequence_impl
...
This looks inconsistent and makes it a bit difficult to navigate.
We may do something like
all type trait templates
all implementations
or
sort related templates
sort impl
unique related templates
unique impl
scan related templates
scan impl
...
and so on.
|
/merge |
3 participants
Next effort in binary size reduction.
This PR:
libcugraph_common.solibcugraph.solibcugraph_mg.so