Repository navigation
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:
📝 Summary
Priority: Change: Refactor Merge Risk:
 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cccl/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
66ea645b-d550-4e8e-858a-714e153906f1
📒 Files selected for processing: 16
cub/cub/agent/agent_find.cuhcub/cub/agent/agent_histogram.cuhcub/cub/agent/agent_reduce.cuhcub/cub/block/block_load.cuhcub/cub/block/block_load_to_shared.cuhcub/cub/device/device_for.cuhcub/cub/warp/warp_load.cuhcudax/include/cuda/experimental/__cuco/detail/hyperloglog/hyperloglog_impl.cuhcudax/include/cuda/experimental/__cuco/detail/hyperloglog/kernels.cuhlibcudacxx/include/cuda/__mdspan/__copy/copy_contiguous.hlibcudacxx/include/cuda/__mdspan/__copy/dispatch_by_vector.hlibcudacxx/include/cuda/__mdspan/__copy/mdspan_d2d.hlibcudacxx/include/cuda/__mdspan/__copy/tensor_copy_utils.hlibcudacxx/include/cuda/__mdspan/__copy/vector_access.hlibcudacxx/include/cuda/__memory/as_bytes_vectorized.hlibcudacxx/test/libcudacxx/libcxx/memory/as_bytes_vectorized.compile.pass.cpp
💤 Files with no reviewable changes: 1
- libcudacxx/include/cuda/__mdspan/__copy/vector_access.h
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
| // TODO: replace with ::cuda::__vector_type | ||
| using __vector_type = ::cuda::std::array<__value_type, _VectorSize>; | ||
| using __value_type = typename _RefType::__value_type; | ||
| using __vector_type = ::cuda::__as_bytes_vectorized_t<__value_type, _VectorSize>; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
important: Preserve valid object access in the vectorized kernel.
For a vectorizable type such as float, this change reads value-type input through integer-word storage. Line 95 then calls __alias<__value_type>() on that storage. The supplied __alias implementation casts and launders the pointer, but launder does not create __value_type objects. These accesses can violate C++ aliasing and object-lifetime rules. Use a byte-copy operation into live value objects, and restrict this path to types for which that copy is valid.
Source: Path instructions
There was a problem hiding this comment.
This is something I have been wondering about, We could use start_lifetime_as but that is only available on newer compilers.
We probably want to extend the core machinery to also work with float / double here
@bernhardmgruber thoughts?
| #if _CCCL_CTK_AT_LEAST(13, 0) | ||
| inline constexpr size_t __max_vector_access = 32ull; | ||
| #else | ||
| inline constexpr size_t __max_vector_access = 16ull; | ||
| #endif // _CCCL_CTK_AT_LEAST(13, 0) |
There was a problem hiding this comment.
This should probably have a host fallback as well. I can imagine on host its probably whatever the size of the largest vector instruction is
| template <class _Tp, size_t _VecSize> | ||
| [[nodiscard]] _CCCL_API constexpr auto __as_bytes_vectorized_impl() noexcept | ||
| { | ||
| if constexpr (alignof(_Tp) <= __max_vector_access) |
There was a problem hiding this comment.
Can reuse __is_vectorizable() here instead
| //! bytes, grouped into the widest vector words that evenly divide the storage. It is aligned for the widest vector | ||
| //! access of its size, at most to `__max_vector_access` but at least to `alignof(_Tp)` | ||
| template <class _Tp, size_t _VecSize> | ||
| using __as_bytes_vectorized_t = decltype(::cuda::__as_bytes_vectorized_impl<_Tp, _VecSize>()); |
There was a problem hiding this comment.
| using __as_bytes_vectorized_t = decltype(::cuda::__as_bytes_vectorized_impl<_Tp, _VecSize>()); | |
| using __as_bytes_vectorized_t _CCCL_NODEBUG = decltype(::cuda::__as_bytes_vectorized_impl<_Tp, _VecSize>()); |
(and everywhere else here as well)
| constexpr size_t __words_per_vector = | ||
| ::cuda::std::min(size_t{1} << ::cuda::std::countr_zero(__num_words), __max_vector_access / sizeof(_Word)); |
There was a problem hiding this comment.
Does this need bit twiddling, can not we express this calculation using normal calculations especially since this is constexpr anyways? They are more readable :(
| } | ||
| else | ||
| { // Default case: use 32 bit words as the vectorized storage | ||
| return __bytes_vectorized_t<::cuda::std::uint32_t, alignof(_Tp), ::cuda::ceil_div(sizeof(_Tp), 4) * _VecSize>{}; |
There was a problem hiding this comment.
Is "4" supposed to be sizeof(uint32_t)?
| // Aligned to the widest vector access that evenly divides the storage size | ||
| static constexpr size_t __full_alignment = size_t{1} << ::cuda::std::countr_zero(sizeof(_VectorWord) * _NumVectors); | ||
| static constexpr size_t __alignment = | ||
| ::cuda::std::max(_MinAlignment, ::cuda::std::min(__full_alignment, __max_vector_access)); |
There was a problem hiding this comment.
Surely what you meant to write here was cuda::std::clamp()
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cccl/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
a96c5827-2e1f-47f6-a02a-175273b6b35e
📒 Files selected for processing: 2
libcudacxx/include/cuda/__memory/as_bytes_vectorized.hlibcudacxx/test/libcudacxx/libcxx/memory/as_bytes_vectorized.compile.pass.cpp
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
This PR causes massive SASS changes that we need to carefully inspect. Please split this PR into just adding the |
72ca3b5 to
8633c16
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cccl/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
df65420d-702e-4f57-94d0-67691e43875e
📒 Files selected for processing: 8
cub/cub/agent/agent_find.cuhcub/cub/agent/agent_histogram.cuhcub/cub/agent/agent_reduce.cuhcub/cub/block/block_load.cuhcub/cub/device/device_for.cuhcub/cub/warp/warp_load.cuhlibcudacxx/include/cuda/__memory/as_bytes_vectorized.hlibcudacxx/test/libcudacxx/libcxx/memory/as_bytes_vectorized.compile.pass.cpp
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
| //! @param[out] dst_items | ||
| //! destination to load data into | ||
| template <typename T, int ItemsPerThread> | ||
| template <CacheLoadModifier Modifier = LOAD_DEFAULT, typename T, int ItemsPerThread> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
important: Preserve the existing public template interface.
Existing calls such as LoadDirectBlockedVectorized<int, 4>(tid, ptr, items) now fail because the first template argument must be a CacheLoadModifier. The base revision exposes the type-first interface. (raw.githubusercontent.com)
Keep a forwarding overload with the original template parameters. Remove the default modifier from the modifier-first overload to avoid ambiguity.
| } | ||
| else | ||
| { | ||
| LoadDirectBlocked(linear_tid, block_src_ptr, dst_items); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
important: Preserve the cache modifier in scalar fallbacks.
With CUDA 13, T = int, ItemsPerThread = 8, and a pointer aligned to 16 but not 32 bytes, this fallback discards Modifier. The old implementation used two aligned 16-byte loads and retained the modifier. (raw.githubusercontent.com)
A LOAD_VOLATILE iterator therefore changes from volatile reads to ordinary reads. This violates the selected load semantics. Use ThreadLoad<Modifier> for each scalar load in every fallback. (raw.githubusercontent.com)
|
|
||
| // Type of the vector that is used to load the input data | ||
| using vector_t = typename CubVector<T, vec_size>::Type; | ||
| using vector_t = ::cuda::__as_bytes_vectorized_t<T, vec_size>; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
important: Keep non-trivially copyable types on the scalar path.
When ForEachCopyN receives a contiguous range of non-trivially copyable T, __for_each_n can select this vector type. The new load copies words, and __alias<T> passes those words to op without constructing copied T objects. The former generic CubVector contained T members. This can break a copy constructor that maintains object invariants. Select the byte-vector path only when T is trivially copyable; otherwise use the scalar wrapper. (github.com)
This comment has been minimized.
This comment has been minimized.
8633c16 to
2176a1e
Compare
2176a1e to
2640363
Compare
| #if _CCCL_CTK_AT_LEAST(13, 0) | ||
| inline constexpr size_t __max_vector_access = 32ull; | ||
| #else | ||
| inline constexpr size_t __max_vector_access = 16ull; | ||
| #endif // _CCCL_CTK_AT_LEAST(13, 0) |
There was a problem hiding this comment.
Important: We should gate this on PTX ISA 8.8 and not on the CTK version. I believe that's relevant for clang in CUDA mode? Not sure.
Also notice that 32-byte loads require sm100+. That does not mean we can't load a longlong4_a32 before sm100, but it will just be split into two 16-byte loads. This may impact the design though, as we don't need to aim for 32-byte loads before sm100. But we can also address this later and just aim for 32-byte loads when possible.
| using _Word = | ||
| ::cuda::std::conditional_t<::cuda::std::is_arithmetic_v<_Tp>, | ||
| _Tp, | ||
| ::cuda::std::__make_nbit_uint_t<::cuda::std::min(alignof(_Tp), size_t{8}) * CHAR_BIT>>; |
There was a problem hiding this comment.
Q: I don't understand where 8 comes from here. Why can't we have words larger than 8 bytes?
This comment has been minimized.
This comment has been minimized.
c1ff51d to
f82b645
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments: 1
libcudacxx/test/libcudacxx/libcxx/memory/as_bytes_vectorized.compile.pass.cpp: 1
305-305:Trivial · Functional Correctness · Quick win suggestion: Add runtime coverage for
__alias.This
.compile.pass.cpptest cannot provide runtime coverage. It also cannot provide constant-evaluation coverage for__alias, because neither overload isconstexpr. Add a separate.pass.cpptest that writes distinct values through__alias<_Word>(i)and__alias<_Tp>(i), reads them throughdata(), and covers both const and non-const overloads.The
uint64_tself-comparison is not useful. It reads the same element twice and does not test indexing or aliasing. The existingstatic_assertalready exercisesdata()during constant evaluation, but only for flat iteration over zero-initialized storage.
-
-
- Source mode: Auto
- Repository: NVIDIA/cccl
- Branch: main
- File: .coderabbit.yaml
- Commit: 036ecd2
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
6e62416b-e538-409a-b9ca-06e80a914737
-
libcudacxx/include/cuda/__memory/as_bytes_vectorized.hlibcudacxx/test/libcudacxx/libcxx/memory/as_bytes_vectorized.compile.pass.cpp
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.
-
| using _Word = | ||
| ::cuda::std::conditional_t<::cuda::std::is_arithmetic_v<_Tp>, | ||
| _Tp, | ||
| ::cuda::std::__make_nbit_uint_t<::cuda::std::min(alignof(_Tp), size_t{8}) * CHAR_BIT>>; |
There was a problem hiding this comment.
🔎 Supported by static analysis
-
Script executed:
sed -n '50,157p' libcudacxx/include/cuda/__memory/as_bytes_vectorized.h rg -n '__as_bytes_vectorized_t|__is_vectorizable' libcudacxx/include/cuda/__mdspan libcudacxx/test/libcudacxx/libcxx/memory/as_bytes_vectorized.compile.pass.cpp
Repository: NVIDIA/cccl
Length of output: 6546
Script executed:
set -e printf '%s\n' '--- test declarations and const cases ---' sed -n '1,115p' libcudacxx/test/libcudacxx/libcxx/memory/as_bytes_vectorized.compile.pass.cpp sed -n '260,315p' libcudacxx/test/libcudacxx/libcxx/memory/as_bytes_vectorized.compile.pass.cpp printf '%s\n' '--- all repository references ---' rg -n -F --glob '*.{h,hpp,cpp,cu,cc,cxx}' \ -e '__as_bytes_vectorized_t' \ -e '__as_bytes_vectorized_impl' \ -e '__is_vectorizable' \ -e '.__alias(' \ libcudacxx cub || test "$?" -eq 1 printf '%s\n' '--- changed-file includes and nearby declarations ---' sed -n '1,60p' libcudacxx/include/cuda/__memory/as_bytes_vectorized.h
Repository: NVIDIA/cccl
Length of output: 17955
suggestion: Strip cv-qualifiers from _Tp before selecting _Word.
__is_vectorizable<const int>() accepts this specialization. _WordType therefore becomes const int, so the storage is read-only. If a caller instantiates __alias<int>, the mutable overload attempts to cast const int* to int* and is ill-formed. Existing tests do not instantiate that member, but the supported specialization permits it.
Suggested fix
-
+#include <cuda/std/__type_traits/remove_cv.h> ... using _Word = ::cuda::std::conditional_t<::cuda::std::is_arithmetic_v<_Tp>, - _Tp, + ::cuda::std::remove_cv_t<_Tp>, ::cuda::std::__make_nbit_uint_t<::cuda::std::min(alignof(_Tp), size_t{8}) * CHAR_BIT>>;
This comment has been minimized.
This comment has been minimized.
f82b645 to
48965c3
Compare
We use vectorized loads in a lot of places in CUB. However, the machinery used was not really suited to current improvements, such as wider vectorized loads and also alignment issues. This unifies that machinery in a single type that ensures that we can use the widest possible vector load for a given array of T Integers and floating-point values are stored as themselves, other types as unsigned integer words of their alignment. The storage is a single array of words, so a pointer to the first word can be iterated over the whole storage, which rebasing an iterator onto the storage relies on. `__alias` returns the object at a given index, words directly so that it returns the live word without laundering, other types through launder. I went to great length to ensure that there are no SASS differences between the old and new machinery and also that we do not rely on the cuda vector types to keep compile times down. The users are switched over to it in the following commits.
Reshape tensors for vectorized access to `cuda::__as_bytes_vectorized_t<char, N>`, which has the same size and alignment as `__vector_access<N>` for the power of two widths used here, and remove the now unused helper. `__max_vector_access` is now provided by `as_bytes_vectorized.h`, so it is removed from `vector_access.h` as well. The copy headers now include what they previously got transitively through `vector_access.h`.
48965c3 to
ae65cdf
Compare
😬 CI Workflow Results🟥 Finished in 1h 50m: Pass: 99%/228 | Total: 6d 16h | Max: 1h 49m | Hits: 89%/735386See results here. AI failure analysis1. libcu++ NVRTC GPU execution reports launch failures and illegal instruction on T4 · 1 jobExplanation: Configuration and compilation succeeded, but six unrelated tests failed during NVRTC GPU execution: four reported CUDA launch failures, one reported an illegal instruction, and one crashed with exit code -11. The PR changes are confined to mdspan vectorization internals and a compile-only test, and the equivalent C++20 NVRTC job succeeded, so the evidence favors a transient or poisoned GPU context rather than a demonstrated PR regression; serial reproduction is needed to identify any initiating test. Evidence: Copy this prompt into a coding agentJobs: |
We use vectorized loads in a lot of places in CUB. However, the machinery used was not really suited to current improvements, such as wider vectorized loads and also alignment issues.
This unifies that machinery in a single type that ensures that we can use the widest possible vector load for a given array of T
I went to great length to ensure that there are no SASS differences between the old and new machinery and also that we do not rely on the cuda vector types to keep compile times down.
Stack created with GitHub Stacks CLI • Give Feedback 💬