Move cudax::copy(mdspan) to libcu++ - #11321
Conversation
📝 SummarySummary by CodeRabbit
WalkthroughChangesMdspan copy migration
Priority: ➖ Normal Merge Risk: 🔵 Low · up to Device-only consumers can detect mdspan copy support and then fail to find the advertised execution-policy overload. Gate the feature macro with the hosted availability condition before merge. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libcudacxx/include/cuda/__mdspan/__copy/mdspan_d2d.h (1)
146-146: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winimportant: Match the mdspan copy constraint to assignability. The PSTL overload requires assignability between the source and destination
device_mdspan::referencetypes, then callscuda::copy, whose element-type convertibility assertion can reject that valid pair. Use the reference types in the direct API and add compile-only coverage.- static_assert(::cuda::std::is_convertible_v<_TpIn, _TpOut>, "TpIn must be convertible to TpOut"); + using __src_reference = typename decltype(__src)::reference; + using __dst_reference = typename decltype(__dst)::reference; + static_assert(::cuda::std::is_assignable_v<__dst_reference, __src_reference>, + "destination mdspan reference must be assignable from source mdspan reference");
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ee25ae36-9e66-4c43-9e55-d5098a15e73f
📒 Files selected for processing (44)
cudax/benchmarks/bench/CMakeLists.txtcudax/benchmarks/bench/copy/CMakeLists.txtcudax/include/cuda/experimental/__copy_bytes/mdspan_d2h_h2d.cuhcudax/include/cuda/experimental/__copy_bytes/memcpy_batch_tiles.cuhcudax/include/cuda/experimental/__copy_bytes/print_raw_tensor.cuhcudax/include/cuda/experimental/__copy_bytes/types.cuhcudax/include/cuda/experimental/__fill_bytes/fill_bytes_mdspan.cuhcudax/include/cuda/experimental/__fill_bytes/fill_bytes_mdspan_utils.cuhcudax/include/cuda/experimental/copy.cuhcudax/test/CMakeLists.txtdocs/libcudacxx/extended_api/mdspan.rstdocs/libcudacxx/extended_api/mdspan/copy.rstlibcudacxx/benchmarks/bench/copy/copy_bench.culibcudacxx/include/cuda/__mdspan/__copy/abs_integer.hlibcudacxx/include/cuda/__mdspan/__copy/copy_contiguous.hlibcudacxx/include/cuda/__mdspan/__copy/copy_dst_contiguous.hlibcudacxx/include/cuda/__mdspan/__copy/copy_optimized.hlibcudacxx/include/cuda/__mdspan/__copy/copy_shared_memory.hlibcudacxx/include/cuda/__mdspan/__copy/copy_shared_memory_utils.hlibcudacxx/include/cuda/__mdspan/__copy/dispatch_by_vector.hlibcudacxx/include/cuda/__mdspan/__copy/mdspan_d2d.hlibcudacxx/include/cuda/__mdspan/__copy/mdspan_to_raw_tensor.hlibcudacxx/include/cuda/__mdspan/__copy/simplify_paired.hlibcudacxx/include/cuda/__mdspan/__copy/tensor_copy_utils.hlibcudacxx/include/cuda/__mdspan/__copy/tensor_iterator.hlibcudacxx/include/cuda/__mdspan/__copy/tensor_query.hlibcudacxx/include/cuda/__mdspan/__copy/types.hlibcudacxx/include/cuda/__mdspan/__copy/vector_access.hlibcudacxx/include/cuda/mdspanlibcudacxx/include/cuda/std/__pstl/cuda/mdspan/copy.hlibcudacxx/include/cuda/std/__pstl/dispatch.hlibcudacxx/include/cuda/std/__pstl/mdspan/copy.hlibcudacxx/include/cuda/std/mdspanlibcudacxx/include/cuda/std/versionlibcudacxx/test/libcudacxx/cuda/containers/views/mdspan/copy/copy.culibcudacxx/test/libcudacxx/cuda/containers/views/mdspan/copy/copy_common.cuhlibcudacxx/test/libcudacxx/cuda/containers/views/mdspan/copy/copy_edge_cases.culibcudacxx/test/libcudacxx/cuda/containers/views/mdspan/copy/copy_llm.culibcudacxx/test/libcudacxx/cuda/containers/views/mdspan/copy/copy_nvmath.culibcudacxx/test/libcudacxx/cuda/containers/views/mdspan/copy/copy_nvmath_transpose.culibcudacxx/test/libcudacxx/cuda/containers/views/mdspan/copy/copy_pstl.culibcudacxx/test/libcudacxx/cuda/containers/views/mdspan/copy/copy_shared_memory.culibcudacxx/test/libcudacxx/cuda/containers/views/mdspan/copy/copy_vectorize.culibcudacxx/test/libcudacxx/cuda/containers/views/mdspan/copy/copy_vectorize_5d.cu
💤 Files with no reviewable changes (5)
- cudax/benchmarks/bench/CMakeLists.txt
- cudax/include/cuda/experimental/copy.cuh
- cudax/benchmarks/bench/copy/CMakeLists.txt
- cudax/test/CMakeLists.txt
- cudax/include/cuda/experimental/__copy_bytes/types.cuh
Included review availability: 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.
| ::cuda::std::array<_ExtentT, _MaxRank> __extents; | ||
| ::cuda::std::array<_StrideT, _MaxRank> __strides; |
There was a problem hiding this comment.
Question: Could those also be
| ::cuda::std::array<_ExtentT, _MaxRank> __extents; | |
| ::cuda::std::array<_StrideT, _MaxRank> __strides; | |
| _ExtentT __extents[_MaxRank]; | |
| _StrideT __strides[_MaxRank]; |
There was a problem hiding this comment.
unfortunately no, we need array for passing by-value
davebayer
left a comment
There was a problem hiding this comment.
Do we really need 2 versions of the same function? For parallel algorithms, I believe we also allow the asynchronous version, when executed with gpu execution policy with a stream attached even though it violates the standard. CC @miscco
|
We currently do not support asynchronous PSTL algorithms. Even if we provide a stream we always synchronize at the end |
bernhardmgruber
left a comment
There was a problem hiding this comment.
Thrust change is fine
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libcudacxx/include/cuda/std/version (1)
89-89: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winimportant: Gate
__cccl_lib_mdspan_copywith_CCCL_HOSTED().
<cuda/std/version>defines the macro in device compilation, but<cuda/std/__pstl/mdspan/copy.h>declares the execution-policycuda::std::copyoverload only inside_CCCL_HOSTED(). A device-only consumer can use the macro to select that overload and then fail during overload lookup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 475f5d91-29d4-454c-9342-ddb1464666d9
📒 Files selected for processing (4)
libcudacxx/include/cuda/__mdspan/__copy/tensor_query.hlibcudacxx/include/cuda/std/__algorithm/inplace_merge.hlibcudacxx/include/cuda/std/__algorithm/stable_sort.hlibcudacxx/include/cuda/std/__pstl/mdspan/copy.h
🚧 Files skipped from review as they are similar to previous changes (1)
- libcudacxx/include/cuda/__mdspan/__copy/tensor_query.h
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.
🥳 CI Workflow Results🟩 Finished in 3h 19m: Pass: 100%/290 | Total: 10d 18h | Max: 3h 18m | Hits: 44%/1591894See results here. |
Description
Move the implementations, tests, benchmarks from
cudax::copy(mdspan)to libcu++ followingCopy and fill for mdspanP3242R4.The PR mostly moves the files from
cuda/experimentaltolibcudacxx/cuda/. There are a few new files underlibcudacxx/include/cuda/std.The PR also introduces the PSTL entry point in
<cuda/std/mdspan>.while the asynchronous version is in
<cuda/mdspan>Directly call from
cub::DeviceCopywill be in a follow-up PR