Rework constant_sequence taking a single auto NTTP to a variadic NTTP#9533
Rework constant_sequence taking a single auto NTTP to a variadic NTTP#9533codingwithmagga wants to merge 15 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
OverviewThis PR refactors Key ChangesCore implementation (
|
| Layer / File(s) | Summary |
|---|---|
__constant_sequence definition and make_constant_sequence factory libcudacxx/include/cuda/__argument/argument.h |
Adds compile-time type-traits headers and cuda::std::array include; replaces the single-NTTP class with __constant_sequence<T, T... Vs>; introduces array-element-expansion helpers __make_constant_sequence_impl/__make_constant_sequence to convert C-style or cuda::std::array into the value pack. |
Wrapper detection and __unwrap update libcudacxx/include/cuda/__argument/argument.h |
Updates __is_wrapper_v specialization to recognize __constant_sequence<T, Vs...>; replaces __unwrap to return cuda::std::array<remove_cvref_t<T>, sizeof...(Vs)> built from the pack. |
Bounds computation and traits specialization libcudacxx/include/cuda/__argument/argument.h |
Rewrites __constant_sequence_compute_lowest/__constant_sequence_compute_highest for pack inputs and empty packs; updates __traits_impl, __lowest_, and __highest_ to match the new representation. |
Test rewrites for new API libcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpp, libcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp, libcudacxx/test/libcudacxx/cuda/argument/usage_example.pass.cpp |
Removes TEST_HAS_CLASS_NTTP-guarded blocks; adds constexpr C-array and cuda::std::array fixtures; validates unwrap, traits, and bounds for direct __constant_sequence<int, 1, 2, 3> and __make_constant_sequence-built sequences; includes explicit empty-sequence bounds testing. |
Assessment against linked issues
| Objective | Addressed | Explanation |
|---|---|---|
Change __constant_sequence from auto V NTTP to variadic template <class T, T... Vs> form [#9272] |
✅ | |
Add __make_constant_sequence factory that accepts C-style array or cuda::std::array and expands elements into the value pack [#9272] |
✅ | |
Enable C++17-compatible construction of constant sequences (remove class-NTTP requirement) [#9272] |
✅ |
Out-of-scope changes
None
Possibly related PRs
- NVIDIA/cccl#9473: Changes
__type_lowest/__type_highest, which the new empty-pack bounds paths use.
Suggested reviewers
- gevtushenko
- jrhemstad
- miscco
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
libcudacxx/include/cuda/__argument/argument.h (1)
115-116: ⚡ Quick winsuggestion: These new non-public templates use short non-reserved parameter names (
T,Vs,Is). Align them with libcudacxx internal naming rules (reserved identifiers with_prefix, non-single-letter where possible) for consistency with this include path.
As per coding guidelines, "All non-public template parameters must be C++ reserved identifiers prefixed with_, avoiding single-letter names (e.g., use_Tpinstead of_T)."Also applies to: 152-153, 175-176, 688-689, 730-731, 802-803, 824-825, 894-895, 978-979, 1031-1032
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0879d251-db8c-482f-b2f3-61e4486be8df
📒 Files selected for processing (4)
libcudacxx/include/cuda/__argument/argument.hlibcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/usage_example.pass.cpp
| template <class T> | ||
| struct __is_builtin_array : ::cuda::std::false_type | ||
| {}; | ||
|
|
||
| template <class T, ::cuda::std::size_t N> | ||
| struct __is_builtin_array<T[N]> : ::cuda::std::true_type | ||
| {}; | ||
|
|
||
| template <class T> | ||
| struct __is_cuda_array : ::cuda::std::false_type | ||
| {}; | ||
|
|
||
| template <class T, ::cuda::std::size_t N> | ||
| struct __is_cuda_array<::cuda::std::array<T, N>> : ::cuda::std::true_type | ||
| {}; | ||
|
|
||
| template <class T> | ||
| struct __array_extent; | ||
|
|
||
| template <class T, ::cuda::std::size_t N> | ||
| struct __array_extent<T[N]> : ::cuda::std::integral_constant<::cuda::std::size_t, N>{}; | ||
|
|
||
| template <class T, ::cuda::std::size_t N> | ||
| struct __array_extent<::cuda::std::array<T, N>> : ::cuda::std::integral_constant<::cuda::std::size_t, N>{}; | ||
|
|
||
| template <class> | ||
| inline constexpr bool __always_false_v = false; |
There was a problem hiding this comment.
Critical: All this already exists and we should just reuse the existing traits
There was a problem hiding this comment.
Thanks for your review.
I replaced them with the existing traits.
| template <class T> | ||
| struct __is_cuda_array : ::cuda::std::false_type |
There was a problem hiding this comment.
Critical: Internal traits should never go through structs, but use inline variables. We also have this already defined __is_cuda_std_array_v
| {}; | ||
|
|
||
| template <class T, ::cuda::std::size_t N> | ||
| struct __is_builtin_array<T[N]> : ::cuda::std::true_type |
There was a problem hiding this comment.
Critical: is_bounded_array_v
| }; | ||
|
|
||
| template <class> | ||
| inline constexpr bool __always_false_v = false; |
There was a problem hiding this comment.
Use cuda::std::__always_false_v instead.
| template <class> | ||
| inline constexpr bool __always_false_v = false; | ||
|
|
||
| template <const auto& Arr, ::cuda::std::size_t... Is> |
There was a problem hiding this comment.
Template parameters must be made __ugly as well
| template <const auto& Arr, ::cuda::std::size_t... Is> | ||
| _CCCL_API constexpr auto __make_constant_sequence_impl(::cuda::std::index_sequence<Is...>) | ||
| { | ||
| using raw_array = ::cuda::std::remove_const_t<::cuda::std::remove_reference_t<decltype(Arr)>>; |
There was a problem hiding this comment.
| using raw_array = ::cuda::std::remove_const_t<::cuda::std::remove_reference_t<decltype(Arr)>>; | |
| using __raw_array = ::cuda::std::remove_cvref_t<decltype(Arr)>; |
| } | ||
| return __min; | ||
| } | ||
| return static_cast<_ElementType>(*::cuda::std::min_element(__first, __last)); |
There was a problem hiding this comment.
Why no longer min_element?
There was a problem hiding this comment.
Thanks for your review!
I think at one point during development I wasn't able to use it and replaced it here.
I found a way to use it again here making the code simpler.
Applied the same for the max computation too.
|
I have one additional question: During development I tried to run the tests where I did the changes (e.g. |
libcu++ uses the LLVM Infrastructure Tester short After you configured libcu++ you can directly invoke |
…nce_variadic_nttp
…on for constant_sequence
….com/codingwithmagga/cccl into 9272_constant_sequence_variadic_nttp
3 participants
Description
closes #9272
Refactors
__constant_sequenceto use a type + value-pack template (template <class T, T... Vs>) instead of a single class NTTP (template <auto _Value>). This removes the dependency on class non-type template parameters, making__constant_sequenceusable in C++17 without theTEST_HAS_CLASS_NTTPguards that previously gated most of its tests.A new factory function
make_constant_sequence<Arr>()is added to construct a__constant_sequencefrom either a C-style array or acuda::std::arraywith static storage duration, expanding the existing constexpr infrastructure (__unwrap,__lowest_,__highest_,__traits_impl) to match the new template signature.Changes:
__constant_sequence<T, Vs...>replaces__constant_sequence<_Value>;value_typeis now alwayscuda::std::array<T, sizeof...(Vs)>make_constant_sequence<Arr>()factory supports bothT[N]andcuda::std::array<T, N>inputs__constant_sequence_compute_lowest/highestreimplemented as constexpr loops over a pack-expanded local array, replacing thestd::min/max_elementcalls that required a runtime iterator__unwrapnow reconstructs the array from the pack:cuda::std::array<T, sizeof...(Vs)>{Vs...}TEST_HAS_CLASS_NTTPguards removed from the three affected test files; tests updated to usemake_constant_sequenceor direct__constant_sequence<T, Vs...>spellingChecklist