Skip to content
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

[FEA]: Expose thrust::detail::contiguous_iterator_raw_pointer_cast #1711

Closed
1 task done
bernhardmgruber opened this issue May 6, 2024 · 5 comments · Fixed by #1717
Closed
1 task done

[FEA]: Expose thrust::detail::contiguous_iterator_raw_pointer_cast #1711

bernhardmgruber opened this issue May 6, 2024 · 5 comments · Fixed by #1717
Labels
feature request New feature or request.

Comments

@bernhardmgruber
Copy link
Contributor

Is this a duplicate?

Area

Thrust

Is your feature request related to a problem? Please describe.

I have found thrust::detail::contiguous_iterator_raw_pointer_cast quite useful to unwrap iterators to their underlying pointers. I use this to unwrap a zip iterator and check whether I can use a kernel operating on contiguous memory or not:

    cuda::std::apply([](auto... it) {
            if constexpr((thrust::is_contiguous_iterator<decltype(it)>::value && ...))
                fast_path(thrust::detail::contiguous_iterator_raw_pointer_cast(it)...);
            else
                generic(it...);
        }, zip_iterator.get_iterator_tuple());

The function contiguous_iterator_raw_pointer_cast seems to do exactly what I need, but it is declared in a detail namespace.

Describe the solution you'd like

Move contiguous_iterator_raw_pointer_cast out of the detail namespace and make it part of thrust's public API.

Describe alternatives you've considered

I can always circumvent this by using thrust::raw_pointer_cast(&*it) instead, but it would be nice if I could use the already implemented function.

Additional context

No response

@bernhardmgruber bernhardmgruber added the feature request New feature or request. label May 6, 2024
@bernhardmgruber
Copy link
Contributor Author

CC @ahendriksen

@miscco
Copy link
Collaborator

miscco commented May 6, 2024

I am not too happy about exposing this. It is a highly dangerous facility that should be used internally.

Is there any reason you cannot use / improve an existing thrust algorithm?

@bernhardmgruber
Copy link
Contributor Author

I am not too happy about exposing this. It is a highly dangerous facility that should be used internally.

I think it's less dangerous than me calling thrust::raw_pointer_cast(&*it) on an unknown iterator type, potentially forgetting to guard it with a if constexpr (thrust::is_contiguous_iterator_v<decltype(it)>).

Is there any reason you cannot use / improve an existing thrust algorithm?

@ahendriksen and I would like to upstream some improvements at some point, but for now work externally. Independently, other uses may also want to use that functionality to define their own algorithms.

bernhardmgruber added a commit to bernhardmgruber/cccl that referenced this issue May 7, 2024
Lifts the following functions into the public API and renames them:
* contiguous_iterator_raw_pointer_t -> unwrap_contiguous_iterator_t
* contiguous_iterator_raw_pointer_cast -> unwrap_contiguous_iterator
* try_unwrap_contiguous_iterator_return_t -> try_unwrap_contiguous_iterator_t
* try_unwrap_contiguous_iterator

Fixes: NVIDIA#1711
@bernhardmgruber
Copy link
Contributor Author

@alliepiper mentioned during our VC yesterday that she is fine with exposing the functionality since it's there for a while and seems to have stabilized.

bernhardmgruber added a commit to bernhardmgruber/cccl that referenced this issue May 7, 2024
Lifts the following functions into the public API and renames them:
* contiguous_iterator_raw_pointer_t -> unwrap_contiguous_iterator_t
* contiguous_iterator_raw_pointer_cast -> unwrap_contiguous_iterator
* try_unwrap_contiguous_iterator_return_t -> try_unwrap_contiguous_iterator_t
* try_unwrap_contiguous_iterator

Fixes: NVIDIA#1711

Co-authored-by: Michael Schellenberger Costa <[email protected]>
bernhardmgruber added a commit that referenced this issue May 7, 2024
Lifts the following functions into the public API and renames them:
* contiguous_iterator_raw_pointer_t -> unwrap_contiguous_iterator_t
* contiguous_iterator_raw_pointer_cast -> unwrap_contiguous_iterator
* try_unwrap_contiguous_iterator_return_t -> try_unwrap_contiguous_iterator_t
* try_unwrap_contiguous_iterator

Fixes: #1711

Co-authored-by: Michael Schellenberger Costa <[email protected]>
@alliepiper
Copy link
Collaborator

It is a highly dangerous facility that should be used internally.

Some more context:

Thrust provides a trait to identify contiguous iterators. It is defined for all contiguous thrust iterators and some common STL vector implementations, etc, and users are allowed to opt-in through some THRUST_PROCLAIM_... macros to declare that their iterator types are contiguous. Doing so allows them to enable both performance and compile-time optimizations by allowing these iterators to be converted to raw pointers.

The utilities exposed by this PR are safest solutions I can think of to solve the problem of converting generic iterators to raw pointers. If an input pointer type is not explicitly marked as contiguous, it is safely and by default returned unmodified. Only known contiguous iterators are decayed to a raw pointer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants