Replace thrust::raw_pointer_cast with cuda::std::to_address#7439
Replace thrust::raw_pointer_cast with cuda::std::to_address#7439miscco wants to merge 6 commits intoNVIDIA:mainfrom
thrust::raw_pointer_cast with cuda::std::to_address#7439Conversation
c7fab2a to
0403ea8
Compare
bernhardmgruber
left a comment
There was a problem hiding this comment.
LGTM, just some questions.
This comment has been minimized.
This comment has been minimized.
138f219 to
1a5c52d
Compare
This comment has been minimized.
This comment has been minimized.
thrust/thrust/detail/pointer.h
Outdated
|
|
||
| // Specialize pointer traits for everything that has the raw_pointer alias | ||
| template <typename Pointer> | ||
| struct ::cuda::std::pointer_traits<Pointer, ::cuda::std::void_t<typename Pointer::raw_pointer>> |
There was a problem hiding this comment.
Consider specializing std::pointer_traits as well here?
There was a problem hiding this comment.
I am a bit on the fence, because the standard does not know about execution spaces, but with that I am fine
There was a problem hiding this comment.
I think it would be useful for downstream users. In my experience it is common for downstream projects to use thrust fancy pointers, but not yet cuda::std:: utilities. I myself have (carefully) rolled my own specializations for std::pointer_traits for thrust fancy pointers, simply to have generic code that needs to introspect pointers Just Work(TM).
thrust/thrust/device_ptr.h
Outdated
|
|
||
| // Specialize pointer traits for everything that has the raw_pointer alias | ||
| template <class T> | ||
| struct ::cuda::std::pointer_traits<THRUST_NS_QUALIFIER::device_ptr<T>> |
There was a problem hiding this comment.
Same here for std::pointer_traits?
There was a problem hiding this comment.
I definitely do not want to do that because that is a device pointer and the std traits have no dealing with that
There was a problem hiding this comment.
I'm not sure I follow, what would be the danger here?
There was a problem hiding this comment.
device only sequences should work with cuda::std:: types and not std::types
1a5c52d to
6597b5f
Compare
6597b5f to
b9668c9
Compare
|
I have the feeling this PR massively grew in size after I approved. Why was this necessary and why can't we ship the changes added later in a separate PR? |
|
Actually it is much smaller. It introduces It then replaces The issue is that we use |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
5e21068 to
da7abeb
Compare
thrust::raw_pointer_cast with cuda::std::to_address
da7abeb to
eeda680
Compare
|
Not to reviewers. all relevant product code changes are in f87d697 The rest is only search & replace |
eeda680 to
fc0439a
Compare
🥳 CI Workflow Results🟩 Finished in 3h 14m: Pass: 100%/398 | Total: 7d 06h | Max: 3h 03m | Hits: 90%/511025See results here. |
bernhardmgruber
left a comment
There was a problem hiding this comment.
If there are no uses of thrust::raw_pointer_cast remaining, can we deprecate it?
| #include <thrust/partition.h> | ||
|
|
||
| #include "thrust/detail/raw_pointer_cast.h" | ||
| #include "cuda/std/memory" |
There was a problem hiding this comment.
| #include "cuda/std/memory" | |
| #include <cuda/std/memory> |
This replaces
thrust::raw_pointer_castwith the standard conformingcuda::std::to_addressWe enable usage of that by specializing
cuda::std::pointer_traitsfor the thrust smart pointers.NOTE: I did not yet deprecate
thrust::raw_pointer_castbecause we should give users, e.g rapids a way to update and its not yet available. I plan on deprecating it in the 3.4 time frame