Make tests build without relaxed constexpr #17691
Conversation
PointKernel
added
feature request
| @@ -201,7 +201,7 @@ struct host_span : public cudf::detail::span_base<T, Extent, host_span<T, Extent | |||
| /// @param data Pointer to the first element in the span | |||
| /// @param size The number of elements in the span | |||
| /// @param is_device_accessible Whether the data is device accessible (e.g. pinned memory) | |||
| constexpr host_span(T* data, std::size_t size, bool is_device_accessible) | |||
| CUDF_HOST_DEVICE constexpr host_span(T* data, std::size_t size, bool is_device_accessible) | |||
There was a problem hiding this comment.
This is not ideal but necessary because base_2dspan serves as a convenient base class, while host_2dspan and device_2dspan are essentially type aliases of the base. As a result, all member functions in the base must be marked as __host__ __device__. Consequently, the host span constructor invoked by these base member functions must also be __host__ __device__.
There was a problem hiding this comment.
I agree that this tradeoff is worthwhile, but could we add a comment indicating that this is the rationale? When I first started trying to remove --expt-relaxed-constexpr I considered going the opposite route and pushing a lot of logic down from base_2dspan rather than dealing with this.
When we move to C++20 we'll be able to use std::span (and presumably cuda::std::span) so hopefully this becomes a moot point then.
There was a problem hiding this comment.
could we add a comment indicating that this is the rationale?
Good point. Will do Done
Actually, cuda::std::span is backported to C++17, so it is already available for use. However, I’m not sure if it is mature enough to replace our custom device_span and host_span.
There was a problem hiding this comment.
Yeah I think we may as well wait for C++20 so we can use host_span properly.
| @@ -201,7 +201,7 @@ struct host_span : public cudf::detail::span_base<T, Extent, host_span<T, Extent | |||
| /// @param data Pointer to the first element in the span | |||
| /// @param size The number of elements in the span | |||
| /// @param is_device_accessible Whether the data is device accessible (e.g. pinned memory) | |||
| constexpr host_span(T* data, std::size_t size, bool is_device_accessible) | |||
| CUDF_HOST_DEVICE constexpr host_span(T* data, std::size_t size, bool is_device_accessible) | |||
There was a problem hiding this comment.
Yeah I think we may as well wait for C++20 so we can use host_span properly.
|
/merge |
Description
Contributes to #7795
This PR updates tests to build without depending on the relaxed constexpr build option.
Checklist