This repository has been archived by the owner on Mar 21, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 757
Add unique_count algorithm #1619
Merged
Merged
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
8aecfe5
add unique_count algorithm
upsj 0c354e8
unique_count: weaken iterator requirements
upsj fca96ec
unique: improve template parameter naming
upsj 0b41e08
unique: test with ForwardIterator parameters
upsj 1532df8
improve forward_iterator_wrapper
upsj e37433c
unique_count: add missing cuda tests
upsj fac3657
use thrust iterator categories in iterator wrapper
upsj a865d53
Revert "use thrust iterator categories in iterator wrapper"
upsj 7bf9735
Revert "improve forward_iterator_wrapper"
upsj 57f8e5e
Revert "unique: test with ForwardIterator parameters"
upsj File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
#pragma once | ||
|
||
#include <thrust/iterator/iterator_traits.h> | ||
#include <thrust/iterator/iterator_categories.h> | ||
#include <type_traits> | ||
|
||
|
||
// Wraps an existing iterator into a forward iterator, | ||
// thus removing some of its functionality | ||
template <typename Iterator> | ||
struct forward_iterator_wrapper { | ||
// LegacyIterator requirements | ||
using iterator_system_tag = typename thrust::iterator_system<Iterator>::type; | ||
using reference = typename thrust::iterator_traits<Iterator>::reference; | ||
using pointer = typename thrust::iterator_traits<Iterator>::pointer; | ||
using value_type = typename thrust::iterator_traits<Iterator>::value_type; | ||
using difference_type = typename thrust::iterator_traits<Iterator>::difference_type; | ||
using iterator_category = typename std::conditional< | ||
std::is_convertible<iterator_system_tag, thrust::device_system_tag>::value, | ||
thrust::forward_device_iterator_tag, | ||
typename std::conditional< | ||
std::is_convertible<iterator_system_tag, thrust::host_system_tag>::value, | ||
thrust::forward_host_iterator_tag, | ||
std::forward_iterator_tag>::type>::type; | ||
using base_iterator_category = typename thrust::iterator_traits<Iterator>::iterator_category; | ||
static_assert( | ||
std::is_convertible<base_iterator_category, std::forward_iterator_tag>::value, | ||
"Cannot create forward_iterator_wrapper around an iterator that is not itself at least a forward iterator"); | ||
|
||
upsj marked this conversation as resolved.
Show resolved
Hide resolved
|
||
__host__ __device__ reference operator*() const { | ||
return *wrapped; | ||
} | ||
|
||
__host__ __device__ forward_iterator_wrapper& operator++() { | ||
++wrapped; | ||
return *this; | ||
} | ||
|
||
// LegacyInputIterator | ||
friend __host__ __device__ bool operator==(const forward_iterator_wrapper& a, const forward_iterator_wrapper& b) { | ||
return a.wrapped == b.wrapped; | ||
} | ||
|
||
friend __host__ __device__ bool operator!=(const forward_iterator_wrapper& a, const forward_iterator_wrapper& b) { | ||
return !(a == b); | ||
} | ||
|
||
__host__ __device__ forward_iterator_wrapper operator++(int) { | ||
auto cpy = *this; | ||
++(*this); | ||
return cpy; | ||
} | ||
|
||
template <typename It = Iterator> | ||
__host__ __device__ typename std::enable_if<std::is_pointer<It>::value, pointer>::type operator->() const { | ||
return wrapped; | ||
} | ||
|
||
template <typename It = Iterator> | ||
__host__ __device__ typename std::enable_if<!std::is_pointer<It>::value, pointer>::type operator->() const { | ||
return wrapped.operator->(); | ||
upsj marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
Iterator wrapped; | ||
}; | ||
|
||
|
||
template <typename Iterator> | ||
forward_iterator_wrapper<Iterator> make_forward_iterator_wrapper(Iterator it) { | ||
return {it}; | ||
} |
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unique_count
claims to work with forward iterators. There should be a test using forward iterators.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a nice way to wrap an iterator into a forward_iterator in Thrust? I wrote a small wrapper class and that seems to work and compile, but I suppose that problem has been solved elsewhere already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericniebler is right, we should be testing this. Unfortunately we lack robust testing for these sorts of things.
Thanks for adding the new testing infrastructure! Please include it in this PR, ideally in the testing framework so we can reuse it from other tests later 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test to unique_copy and unique. I am not 100% sure it does what we would expect - due to the missing iterator tag, it gets executed sequentially on the CPU using device references for access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean, forward iterators always dispatch to the CPU? @allisonvacanti can you comment on that? I mean, it seems reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounded odd to me, as I've never seen any logic in Thrust that would dispatch forward iterators to serial implementations. So I dug into it, and unfortunately this is due to a pretty nasty bug in Thrust's iterator traits.
The details are gory, but I've summarized them in a comment on #902.
Until that's fixed, I'm not comfortable merging this test using the forward_iterator_wrapper, since they only "do the right thing" because the iterator framework is broken.
I hate to churn on this PR even more, but I think we should remove the iterator wrappers for now and just test that the regular iterators work. We can re-introduce the wrapper tests as part of #55, after #902 is fixed and settled.
@ericniebler Can you review the two linked issues and see if you agree with my suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that forward iterators actually need to be dispatched to the sequential backend. They support multipass reading and should be usable in a parallel algorithm, so long as they're only copied and incremented. Is there something in the unique_count/count_if algorithms that would break them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main issue I see with parallel execution on forward iterators is that they introduce an essentially linear dependency chain that means either every thread
i
starts frombegin
and increments iti
times, or waits until one of its predecessorsj
is done and writes its iterator somewhere, to then increments iti - j
times. Both don't really seem useful for parallel execution to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will require more increments, but if the work-per-element is expensive compared to the cost of the iterator increment, it may still make sense to parallelize. I'd rather let the user make that call, since they can opt-in to sequential execution by passing in the sequential exeuction policy (
thrust::seq
).More importantly, the sequential implementation executes on CPU, and some types of device memory aren't accessible from the CPU's address space, so switching to
seq
really needs to be opt-in rather than default.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification, that makes sense! I was only thinking of simple but massively parallel cases.