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:
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Add striped vectorized loading cub/cub/block/block_load.cuh |
Adds the algorithm enum, loader helpers, unguarded dispatch, and guarded-load routing. |
Use vectorized loads in CUB algorithms cub/cub/agent/agent_find.cuh, cub/cub/agent/agent_reduce.cuh, cub/cub/device/dispatch/kernels/kernel_transform.cuh |
Find, reduce, and transform use the loading helpers in vectorized paths. Reduce derives its vector type from the effective load width. |
Test striped vectorized loads cub/test/catch2_test_block_load.cu, cub/test/catch2_test_enum_formatting.cu |
Tests cover striped-vectorized indexing, counting iterators, alignment, conversion cases, and enum formatting. |
HyperLogLog Vectorized Access
| Layer / File(s) | Summary |
|---|---|
Update HyperLogLog vectorized access cudax/include/cuda/experimental/__cuco/detail/hyperloglog/hyperloglog_impl.cuh, cudax/include/cuda/experimental/__cuco/detail/hyperloglog/kernels.cuh |
Alignment calculation uses the maximum vector-access limit. The kernel uses byte-vectorized storage and element aliases. |
Possibly related issues
- NVIDIA/cccl#12170 — Requests the striped vectorized load algorithm added here.
Priority:
Change: Feature
Merge Risk: 
Moderate · previous review bac3d
-
Aligned custom types can take a load path that bypasses their normal per-element copy behavior. Restrict that path before merging; convertible input types are supported.
Comment @coderabbitai help to get the list of available commands.
This comment has been minimized.
This comment has been minimized.
fbusato
left a comment
There was a problem hiding this comment.
this makes me think that we are keeping reinventing the wheel, as we could use cuda::std::simd directly
| _CCCL_PRAGMA_UNROLL_FULL() | ||
| for (int i = 0; i < ItemsPerThread / VecSize; i++) | ||
| { | ||
| InternalLoadDirectBlockedVectorized<Modifier>( | ||
| i * ThreadsPerBlock, thread_src_ptr, reinterpret_cast<T(&)[VecSize]>(dst_items[i * VecSize])); | ||
| } |
There was a problem hiding this comment.
(I didn't compile it)
| _CCCL_PRAGMA_UNROLL_FULL() | |
| for (int i = 0; i < ItemsPerThread / VecSize; i++) | |
| { | |
| InternalLoadDirectBlockedVectorized<Modifier>( | |
| i * ThreadsPerBlock, thread_src_ptr, reinterpret_cast<T(&)[VecSize]>(dst_items[i * VecSize])); | |
| } | |
| constexpr auto num_vecs = ItemsPerThread / VecSize; | |
| auto& dst_vecs = reinterpret_cast<T(*)[VecSize][num_vecs]>(dst_items); | |
| _CCCL_PRAGMA_UNROLL_FULL() | |
| for (int i = 0; i < ItemsPerThread / VecSize; i++) | |
| { | |
| InternalLoadDirectBlockedVectorized<Modifier>(i * ThreadsPerBlock, thread_src_ptr, dst_vecs[i]); | |
| } |
There was a problem hiding this comment.
Not really an improvement honestly
This comment has been minimized.
This comment has been minimized.
478dea5 to
144aee2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cccl/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
69290ac0-8724-4216-b330-2dd31a170789
📒 Files selected for processing: 4
cub/cub/agent/agent_find.cuhcub/cub/agent/agent_reduce.cuhcub/cub/block/block_load.cuhcub/cub/device/dispatch/kernels/kernel_transform.cuh
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
| if constexpr (THRUST_NS_QUALIFIER::is_contiguous_iterator_v<RandomAccessIterator> && ::cuda::__is_vectorizable<T>()) | ||
| { | ||
| const T* block_src_ptr = ::cuda::std::to_address(block_src_it); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
important: Keep convertible input types on the item-by-item path. If BlockLoad<float, ..., BLOCK_LOAD_STRIPED_VECTORIZED>::Load receives an int*, the contiguous branch tries to assign that pointer to const float* and fails to compile. BlockLoad documents that input values need only be convertible to T. Require the iterator value type to be T before selecting vector loads; otherwise use the existing item-by-item path. As per path instructions: “Focus on algorithm correctness, temporary-storage protocol, dispatch/policy selection, stream behavior, CUDA error handling, synchronization, memory access safety, performance regressions, and test coverage.”
Source: Path instructions
This comment has been minimized.
This comment has been minimized.
144aee2 to
25c09c7
Compare
25c09c7 to
26bf61c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
9fe40f6 to
3037b89
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3037b89 to
b1c36ca
Compare
b1c36ca to
382a0a7
Compare
This comment has been minimized.
This comment has been minimized.
382a0a7 to
3dd5238
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3dd5238 to
fbda65e
Compare
fbda65e to
9903519
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
9903519 to
22238ee
Compare
Load vectors of `cuda::__as_vectorized_storage_t` instead of `cuda::std::array` and access the elements through `__alias`. The widest vector access is taken from `cuda::__max_vector_access`, which is 16 bytes before CTK 13.0, instead of being hard coded to 32 bytes.
This adds machinery for striped vectorized loads to `CUB`. There are multiple places were we need to load vectors in a striped arrangement, for example in `DeviceReduce`, `DeviceFind` and the vectorized `DeviceTransform` kernel. Having this open coded complicates maintenance and makes it harder to reason about the code. In the future we want to add iterator unwrapping to extend the vectorized loads to non-contiguous inputs. `LoadDirectStripedVectorized` loads a striped arrangement of vectors of `VecSize` consecutive items: thread `t` loads the vectors `t`, `t + ThreadsPerBlock`, and so on. Like `LoadDirectBlockedVectorized`, it loads `__as_vectorized_storage_t` vectors of the value type of the input if the iterator is contiguous, that type is vectorizable and the input is aligned, and otherwise loads the same arrangement item by item. The items are converted to `T` when they are copied out of the vectors. `InternalLoadDirectStripedVectorized` only performs the vectorized loads and asserts that the input is aligned. `BLOCK_LOAD_STRIPED_VECTORIZED` uses vectors of the width that `BLOCK_LOAD_VECTORIZE` loads. Guarded loads use the arrangement of `BLOCK_LOAD_STRIPED`. `DeviceReduce`, `DeviceFind` and the vectorized `DeviceTransform` kernel load their full tiles with `InternalLoadDirectStripedVectorized`, and `DeviceTransform` loads non-contiguous inputs with `LoadDirectStripedVectorized`. The `VectorT` of `AgentReduce` is now sized by `vec_size` instead of `VECTOR_LOAD_LENGTH`, so policies with fewer items per thread than `VECTOR_LOAD_LENGTH` no longer load vectors wider than their items.
22238ee to
1ec9ece
Compare
🔬 Benchmark SASS comparisonHow to request a benchmark run
Targets with a SASS change
|
🥳 CI Workflow Results🟩 Finished in 3h 17m: Pass: 100%/254 | Total: 11d 09h | Max: 3h 16m | Hits: 24%/1260566See results here. AI failure analysis1. CUB striped-vectorized-load SASS changes lack required approval · 1 jobExplanation: The SASS comparison completed and detected changes from the PR's CUB vectorized-load modifications, but the approval job found no matching approval artifact for the current head SHA and intentionally failed. The top-level CI job failed only because this approval job failed, so it is omitted as a derivative gate failure. Evidence: Copy this prompt into a coding agentJobs: |
This adds machinery for striped vectorized loads to CUB. There are multiple places were we need to load vectors in a striped arrangement, for example in DeviceReduce, DeviceFind and the vectorized DeviceTransform kernel.
Having this open coded complicates maintenance and makes it harder to reason about the code.
In the future we want to add iterator unwrapping to extend the vectorized loads to non-contiguous inputs.
LoadDirectStripedVectorized loads a striped arrangement of vectors of VecSize consecutive items: thread t loads
the vectors t, t + ThreadsPerBlock, and so on. Like LoadDirectBlockedVectorized, it loads __as_bytes_vectorized_t
vectors if the iterator is contiguous, the type is vectorizable and the input is aligned, and otherwise loads the same
arrangement item by item. InternalLoadDirectStripedVectorized only performs the vectorized loads and asserts that the
input is aligned.
BLOCK_LOAD_STRIPED_VECTORIZED uses vectors of the width that BLOCK_LOAD_VECTORIZE loads. Guarded loads use the
arrangement of BLOCK_LOAD_STRIPED.
DeviceReduce, DeviceFind and the vectorized DeviceTransform kernel load their full tiles with
InternalLoadDirectStripedVectorized, and DeviceTransform loads non-contiguous inputs with
LoadDirectStripedVectorized. The VectorT of AgentReduce is now sized by vec_size instead of VECTOR_LOAD_LENGTH,
so policies with fewer items per thread than VECTOR_LOAD_LENGTH no longer load vectors wider than their items.
Stack created with GitHub Stacks CLI • Give Feedback 💬