-
Notifications
You must be signed in to change notification settings - Fork 218
Allow mutation through a transform_iterator #2006
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ec3c5ae
to
6b6d930
Compare
🟩 CI finished in 5h 20m: Pass: 100%/250 | Total: 5d 02h | Avg: 29m 26s | Max: 1h 02m | Hits: 54%/248341
|
Project | |
---|---|
CCCL Infrastructure | |
libcu++ | |
CUB | |
+/- | Thrust |
CUDA Experimental | |
pycuda |
Modifications in project or dependencies?
Project | |
---|---|
CCCL Infrastructure | |
libcu++ | |
+/- | CUB |
+/- | Thrust |
CUDA Experimental | |
+/- | pycuda |
🏃 Runner counts (total jobs: 250)
# | Runner |
---|---|
178 | linux-amd64-cpu16 |
41 | linux-amd64-gpu-v100-latest-1 |
16 | linux-arm64-cpu16 |
15 | windows-amd64-cpu16 |
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.
- We should figure out and document the edge cases for how this feature behaves for cross-system vectors/references.
- A new thrust example would be a nice-to-have.
- The transform_iterator should document allowed and disallowed usages of this feature.
thrust/testing/transform_iterator.cu
Outdated
void TestTransformIteratorAsDestination() | ||
{ | ||
constexpr auto n = 10; | ||
thrust::host_vector<int> src(n, 1234); |
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.
Suggestion: Also test with device_vector
. The CUDA-backed device_vector
references/pointers/etc are significantly more complex than the host_vector
implementations.
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.
Yeah, that opened a can of worms
_CCCL_HOST_DEVICE typename super_t::reference dereference() const | ||
{ | ||
// TODO(bgruber): I am not sure this is the correct check here. There is also the trait | ||
// thrust::detail::is_wrapped_reference that sounds fitting. Only allowing to pass through l-value references |
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.
IIRC, the device_vector
references are proxy references that is_wrapped_reference
is designed to detect. We may need to handle these specially.
thrust/testing/transform_iterator.cu
Outdated
thrust::host_vector<int> src(n, 1234); | ||
thrust::host_vector<foo> dst(n, foo{1, 2}); | ||
|
||
thrust::copy(src.begin(), src.end(), thrust::make_transform_iterator(dst.begin(), access_x{})); |
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.
Question: What happens if this is attempted on cross-system host<->device vectors?
This feature requires some more changes to improve support for proxy references (we currently force a proxy reference to decay into its value type and pass that further). Fixing that breaks some "actors", whatever that is. I'll see when I have some time to give it another go. |
6b6d930
to
5115363
Compare
9785d2e
to
22f3939
Compare
22f3939
to
02f088a
Compare
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. |
02f088a
to
420edae
Compare
But only if the transform iterator's base iterator does not return a wrapped reference and is not a device_vector
420edae
to
bf58e85
Compare
It turned out too hard to retrofit this feature into |
But only if the transform iterator's base iterator returns a true l-value reference (and not a proxy reference).
Fixes: #792