Repository navigation
Conversation
📝 SummarySummary by CodeRabbit
WalkthroughInitializer-list buffer APIs now distinguish lvalue and rvalue sources. Lvalue copies remain stream-ordered; rvalue copies complete source accesses before returning, using CUDA 13 copy attributes or stream synchronization on earlier toolkits. The change covers constructors, buffer factories, pool factories, documentation, and tests. ChangesInitializer-list buffer lifetimes
Suggested reviewers: Priority: ➖ Normal Change: Bug fix Merge Risk: 🟠 High · up to Buffer initialization can fail on CUDA 13 when using the legacy default stream. Add a supported copy path for that stream before merging.
Comment ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
|
![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:
34dabf5d-0409-490a-b694-67fa7f74483e
📒 Files selected for processing (4)
docs/libcudacxx/runtime/buffer.rstlibcudacxx/include/cuda/__container/buffer.hlibcudacxx/include/cuda/__container/make_buffer_with_pool.hlibcudacxx/test/libcudacxx/cuda/containers/buffer/constructor.cu
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
| ::cuda::__driver::__memcpyAsyncWithAttributes( | ||
| __dest, ::cuda::std::to_address(__first), sizeof(_Tp) * __count, __buf_.stream().get(), __attributes); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
rg -n -C 8 '__memcpyAsyncWithAttributes|cuMemcpyBatchAsync|cuMemcpyWithAttributesAsync' libcudacxx/include/cuda/__driver/driver_api.hRepository: NVIDIA/cccl
Length of output: 1652
important: Add a supported path for the legacy NULL stream.
On CUDA 13 and later, __memcpyAsyncWithAttributes calls cuMemcpyBatchAsync with the supplied stream. This replaces the previous copy path for non-empty copies, including lvalue sources. If stream_ref contains the legacy NULL stream, the driver API rejects that stream. Preserve a fallback that supports the legacy NULL stream before using this batch-copy path.
😬 CI Workflow Results🟥 Finished in 3h 09m: Pass: 99%/222 | Total: 5d 18h | Max: 2h 53m | Hits: 79%/763488See results here. AI failure analysis1. cuda::buffer attributed memcpy fails in segmented-reduce default tests · 1 jobExplanation: The PR routes every CUDA 13 `buffer::__copy_cross` operation through `cuMemcpyBatchAsync`; both failures occur while constructing a pinned expected buffer from a host vector and throw from that attributed-copy wrapper. This indicates the new path supplies an invalid or unsupported attribute/memory combination, rather than a segmented-reduce computation failure. Evidence: Copy this prompt into a coding agentJobs: 2. libcudacxx buffer constructor tests fail clang-tidy const-correctness · 1 jobExplanation: The newly added named-initializer-list tests declare variables that are never modified, and clang-tidy treats `misc-const-correctness` findings as errors. The log also reports the same issue for the pinned-buffer `input` declaration at line 773. Evidence: Copy this prompt into a coding agentJobs: 3. cuOpt fails against RMM 26.12 stream API · 1 jobExplanation: The optional third-party build checks out cuOpt main while resolving an RMM 26.12 development API, but cuOpt still references removed `rmm::cuda_stream_default` and `cuda_stream::view()` interfaces. The PR does not modify these APIs, so this is an external moving-dependency incompatibility rather than a failure caused by the buffer changes. Evidence: Copy this prompt into a coding agentJobs: |
Jacobfaib
left a comment
Jacobfaib
left a comment
There was a problem hiding this comment.
We should generalize using borrowed_range instead of treating initializer_list specially
| // Initializer lists must use the overloads that account for the source lifetime. | ||
| return ::cuda::std::ranges::__container_compatible_range<_Range, _Tp> | ||
| && !::cuda::std::is_same_v<::cuda::std::decay_t<_Range>, ::cuda::std::initializer_list<_Tp>>; |
There was a problem hiding this comment.
We can generalize this
| // Initializer lists must use the overloads that account for the source lifetime. | |
| return ::cuda::std::ranges::__container_compatible_range<_Range, _Tp> | |
| && !::cuda::std::is_same_v<::cuda::std::decay_t<_Range>, ::cuda::std::initializer_list<_Tp>>; | |
| return ::cuda::std::ranges::__container_compatible_range<_Range, _Tp> | |
| && ::cuda::std::ranges::borrowed_range<_Range>; |
(https://cppreference.com/cpp/ranges/borrowed_range)
buffer(..., std::vector<int>{1, 2, 3}) is just as big of an oopsie as buffer(..., {1, 2, 3}).
| _CCCL_HOST_API buffer(::cuda::stream_ref __stream, | ||
| _Resource&& __resource, | ||
| ::cuda::std::initializer_list<_Tp> __ilist, | ||
| const ::cuda::std::initializer_list<_Tp>& __ilist, |
There was a problem hiding this comment.
| const ::cuda::std::initializer_list<_Tp>& __ilist, | |
| ::cuda::std::initializer_list<_Tp> __ilist, |
We should not treat const initializer_list<_Tp> any differently. The user can just as well write
{
auto ilist = {1, 2, 3};
buffer(..., ilist);
}and we would be none the wiser.
There was a problem hiding this comment.
I'd argue this is just user's responsibility to either synchronize at the end of the block or extend the lifetime of .ilist as appropriate
| @@ -325,6 +339,33 @@ class buffer | |||
| this->__copy_cross(__ilist.begin(), __ilist.end(), __unwrapped_begin(), __buf_.size()); | |||
There was a problem hiding this comment.
| this->__copy_cross(__ilist.begin(), __ilist.end(), __unwrapped_begin(), __buf_.size()); | |
| this->__copy_cross(__ilist.begin(), __ilist.end(), __unwrapped_begin(), __buf_.size(), /*__ephemeral=*/true); |
…opies finish This could be reverted once #11958 lands
Jacobfaib
left a comment
Jacobfaib
left a comment
There was a problem hiding this comment.
Please open a follow-up issue to address borrowed ranges. Then we can get rid of the initializer_list overload entirely and just rely on ranges.
| __buffer_compatible_env<_Env>) | ||
| buffer<_Tp, _FirstProperty, _RestProperties...> | ||
| make_buffer(stream_ref __stream, _Resource&& __mr, ::cuda::std::initializer_list<_Tp> __ilist, const _Env& __env = {}) | ||
| _CCCL_HOST_API buffer<_Tp, _FirstProperty, _RestProperties...> make_buffer( |
There was a problem hiding this comment.
| _CCCL_HOST_API buffer<_Tp, _FirstProperty, _RestProperties...> make_buffer( | |
| [[nodiscard]] _CCCL_HOST_API buffer<_Tp, _FirstProperty, _RestProperties...> make_buffer( |
(and elsewhere)
Description
Constructing
cuda::bufferor calling its factories with an rvalue initializer list could leave an asynchronous copy reading backing storage after its lifetime ended. Preserve the list's value category through the constructors and factories, and complete source accesses before returning for rvalue lists.On CUDA 13, use batched memcpy with
CU_MEMCPY_SRC_ACCESS_ORDER_DURING_API_CALLfor rvalue lists and stream-ordered source access for other copies. On CUDA 12, synchronize the stream after copying a non-empty rvalue list