Repository navigation
C2H/CUB: Add checked buffer utilities, stream-aware test support, and migrate segmented-scan tests - #10898
C2H/CUB: Add checked buffer utilities, stream-aware test support, and migrate segmented-scan tests#10898oleksandr-pavlyk wants to merge 31 commits into
Conversation
📝 SummarySummary by CodeRabbit
WalkthroughThis change adds checked CUDA memory resources, stream-aware C2H generators, shared CUDA test utilities, and environment-aware launch wrappers. Segmented scan tests migrate from Thrust vectors to CUDA buffers with explicit streams, asynchronous transfers, host-backed expectations, and shared matchers. ChangesCUDA buffer migration
Assessment against linked issues
Out-of-scope changes
Suggested reviewers: Merge Risk: 🔵 Low · up to This test-infrastructure change moves segmented scans to checked CUDA buffers and explicit streams. Remaining concerns are bounded validation and edge-case robustness issues, so the change is mergeable with owner follow-up rather than showing a demonstrated production failure. Comment |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@coderabbitai Regarding classification of this change as Medium Merge Risk, I think the stream-ordering risk is overstated here. The PR does use Runtime streams for buffer/copy operations, but the validation paths are not reading segmented scan outputs from an unordered stream. The common launch helper synchronizes before returning for host, device-side, and graph launch modes. The env launch helper does the same. For direct default-stream validation, the shared segmented-scan test helper explicitly fences with For generated inputs, C2H uses So I do not think “some scans and validation operations still use independently synchronized streams” is enough to justify moderate merge risk unless there is a specific call site where a scan launch can race a later validation copy. Could you point to that call site? On annotations: C2H header APIs added for the checked device memory resource are annotated. Host-side test utilities are intentionally not annotated with |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
files=(
cub/test/catch2_test_device_segmented_scan_utils.cuh
cub/test/catch2_test_device_segmented_scan.cu
cub/test/catch2_test_device_segmented_scan_api.cu
cub/test/catch2_test_device_segmented_scan_env.cu
cub/test/catch2_test_device_segmented_scan_env_api.cu
cub/test/catch2_test_device_segmented_scan_multi_segment.cu
cub/test/catch2_test_device_segmented_scan_noncommutative.cu
c2h/include/c2h/checked_allocator.cuh
c2h/include/c2h/generators.h
)
for file in "${files[@]}"; do
if [[ -f "$file" ]]; then
echo "===== OUTLINE: $file ====="
ast-grep outline "$file" --items all 2>/dev/null || true
echo "===== ORDERING OPERATIONS: $file ====="
rg -n -C 4 \
'cuda(DeviceSynchronize|StreamSynchronize)|\.sync\(|copy_(to_host|to_device)|copy_bytes|DeviceSegmented(Scan|Reduce)|device_policy|thrust::cuda::par|_CCCL_HOST_API' \
"$file" || true
fi
doneLength of output: 63108 🏁 Script executed: #!/bin/bash
set -euo pipefail
for name in catch2_test_launch_helper.h catch2_test_env_launch_helper.h; do
echo "===== CANDIDATES: $name ====="
fd -a -t f "^${name}$" . -x sh -c '
echo "----- $1 -----"
ast-grep outline "$1" --items all 2>/dev/null || true
rg -n -C 8 "sync|Synchronize|stream|DECLARE_LAUNCH_WRAPPER|launch" "$1" || true
' sh {}
done
echo "===== STREAM SETUP AND WRAPPER INVOCATIONS ====="
sed -n '140,210p' cub/test/catch2_test_device_segmented_scan.cu
sed -n '120,210p' cub/test/catch2_test_device_segmented_scan_multi_segment.cu
sed -n '90,165p' cub/test/catch2_test_device_segmented_scan_noncommutative.cu
echo "===== C2H DEVICE POLICY ====="
rg -n -C 6 'device_policy|par_nosync|thrust::cuda::par' c2h/include cub/test -g '*.{h,hpp,cuh,cu}' || true
echo "===== ENV VALIDATION AFTER CUSTOM ENV LAUNCH ====="
sed -n '315,365p' cub/test/catch2_test_device_segmented_scan_env_api.cuLength of output: 194760 suggestion:
In
I withdraw the stream-ordering and annotation concerns. They do not justify Medium Merge Risk for this PR. ✏️ Learnings added
You are interacting with an AI system. |
This comment has been minimized.
This comment has been minimized.
|
Could we also extend some of this work to cudax? I ended up reinventing a lot of this under |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2c30052e-6501-4bea-b4fe-e6ab7c193076
📒 Files selected for processing (7)
c2h/include/c2h/buffer_generators.cuhc2h/include/c2h/catch2_seed.hc2h/include/c2h/detail/checked_memory.cuhc2h/include/c2h/detail/generators.cuhcub/test/catch2_test_device_segmented_scan.cucub/test/catch2_test_env_launch_helper.hcub/test/catch2_test_launch_helper.h
🚧 Files skipped from review as they are similar to previous changes (4)
- cub/test/catch2_test_launch_helper.h
- c2h/include/c2h/buffer_generators.cuh
- cub/test/catch2_test_device_segmented_scan.cu
- c2h/include/c2h/detail/checked_memory.cuh
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
oleksandr-pavlyk
left a comment
There was a problem hiding this comment.
Commented in CodeRabbit Change Stack
This comment has been minimized.
This comment has been minimized.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
* Fix outstanding encountered clang-tidy misc-const-correctness Nothing systematic here, but fix errors encounted outside of changeset of my PR in #10898 * Fix one more issue https://github.com/NVIDIA/cccl/actions/runs/34384989863/job/102579044195?pr=10898
…s-use-cuda-buffer
This comment has been minimized.
This comment has been minimized.
…s-use-cuda-buffer # Conflicts: # cub/test/catch2_test_env_launch_helper.h # cub/test/catch2_test_launch_helper.h
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cub/test/catch2_test_launch_helper.h (1)
622-626: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: this check is conditional here and at line 454, but line 497 makes
expected_allocation_sizemandatory forTEST_LAUNCH == 1. A test that omits the property therefore still passes for launcher ids 0 and 2, and only fails to compile for id 1. Make the property mandatory in all three paths so the allocation-size check always runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aed4e980-0af5-4022-9cc1-2d2558cc512e
📒 Files selected for processing (4)
cub/test/catch2_test_device_for_env.cucub/test/catch2_test_device_segmented_scan_env.cucub/test/catch2_test_launch_helper.hdocs/cub/developer/test_overview.rst
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
| pass any type supported by ``cuda::get_stream`` as the wrapper's first argument: | ||
|
|
||
| .. code-block:: c++ | ||
|
|
||
| cub_reduce_sum(cuda::get_stream(stream), d_in, d_out, n); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
important: the prose and the example disagree. Line 386 tells the reader to pass a type supported by cuda::get_stream, but line 390 passes cuda::get_stream(stream), which is the already-extracted stream_ref. Show the stream-like object directly so readers do not copy the extraction call.
- cub_reduce_sum(cuda::get_stream(stream), d_in, d_out, n);
+ cub_reduce_sum(stream, d_in, d_out, n);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pass any type supported by ``cuda::get_stream`` as the wrapper's first argument: | |
| .. code-block:: c++ | |
| cub_reduce_sum(cuda::get_stream(stream), d_in, d_out, n); | |
| pass any type supported by ``cuda::get_stream`` as the wrapper's first argument: | |
| .. code-block:: c++ | |
| cub_reduce_sum(stream, d_in, d_out, n); |
🥳 CI Workflow Results🟩 Finished in 2h 45m: Pass: 100%/163 | Total: 5d 06h | Max: 2h 45m | Hits: 60%/241704See results here. |
…#11314) * Fix outstanding encountered clang-tidy misc-const-correctness Nothing systematic here, but fix errors encounted outside of changeset of my PR in NVIDIA#10898 * Fix one more issue https://github.com/NVIDIA/cccl/actions/runs/34384989863/job/102579044195?pr=10898
Porting one missing change from NVIDIA#10898, to protect from overflow in integral addition of `bytes + padding`. Added a regression test using size_t::max() as bytes.
…#11314) * Fix outstanding encountered clang-tidy misc-const-correctness Nothing systematic here, but fix errors encounted outside of changeset of my PR in NVIDIA#10898 * Fix one more issue https://github.com/NVIDIA/cccl/actions/runs/34384989863/job/102579044195?pr=10898
|
This PR has been split into smaller PRs, most of which have been merged. Closing this PR now |
Description
This PR modernizes several C2H/CUB test utilities around
cuda::bufferand stream-aware test execution, then applies those utilities to the segmented-scan tests.The main motivation is to replace direct test use of
c2h::device_vector/c2h::host_vectorin new code paths with buffer-based helpers backed by checked C2H memory resources. This keeps the allocation checks that C2H relies on for constrained devices, while making the segmented-scan tests use the same runtime buffer abstractions that CUB APIs increasingly exercise.Main Changes
Adds checked C2H memory resources for
cuda::bufferNew helpers in
<c2h/checked_memory_resource.cuh>provide buffer construction utilities such asc2h::make_device_bufferandc2h::make_host_buffer.c2h::make_device_bufferallocates device-accessible storage through a checked memory resource, preserving the pre-allocation device-memory checks previously available throughc2h::device_vectorandc2h::checked_cuda_allocator.c2h::make_host_bufferallocates host-accessible storage through a checked host resource. On integrated-memory systems, the host allocation path checks available device/system memory before using new/delete-style allocation, matching the intent of the previousc2h::host_vectorchecked allocation path.Keeps the checked-memory-resource layer independent of Thrust containers
<c2h/checked_memory_resource.cuh>is intended to provide the allocation resources andcuda::bufferfactory helpers without depending on Thrust vector headers. This lets tests use checked buffer allocation without also pulling inthrust::device_vector/thrust::host_vector.Adds buffer-oriented C2H generator helpers
The C2H generator utilities are extended so tests can directly create and populate checked buffers instead of constructing a vector first and then extracting raw pointers.
The generator-facing helpers live with the C2H generator support, primarily
<c2h/generators.h>with CUDA implementation support in<c2h/detail/generators.cuh>. These generator utilities may still depend on Thrust because the existing random-data generation implementation uses Thrust/CUDA algorithm machinery. This is separate from the lower-level checked-memory-resource header, which should remain usable without Thrust vector dependencies.Stream-taking overloads are added for the buffer generator helpers. Those overloads enqueue their work on the caller-provided stream, so buffer population can participate in the same stream ordering as the test launch being exercised.
Adds stream-aware launch-wrapper overloads
The CUB Catch2 launch wrappers now support overloads that take a CUDA stream as the first argument. This lets tests opt into stream-ordered setup and algorithm execution while still reusing the existing launch-mode matrix controlled by
TEST_LAUNCH.The stream behavior differs by launch mode:
TEST_LAUNCH == 0: the stream is forwarded to the wrapped host-side CUB API calls, including both the temporary-storage sizing call and the real algorithm invocation. Synchronization is scoped to that stream.TEST_LAUNCH == 1: the stream-aware overload cannot forward the host stream into the device-side CUB call because a hostcudaStream_tis not valid inside the CDP-launched code. The helper synchronizes the provided stream with the host-side staging work, then uses the existing device-side launch path. The wrapped CUB call from device code receives the device-side/null stream behavior rather than the host stream.TEST_LAUNCH == 2: the stream is used for CUDA graph capture and graph execution. The wrapped host-side CUB API is invoked during capture with the provided stream, and the instantiated graph is launched on that stream.Migrates segmented-scan tests to the new helpers
catch2_test_device_segmented_scan.cuis updated to use C2H checked buffers and the buffer-producing generator helpers for inputs, offsets, outputs, and host-side reference data.The tests that are parameterized with
TEST_LAUNCH_PARAMuse the stream-aware launch wrappers so the same segmented-scan coverage continues to run across direct host launch, CDP launch, and CUDA graph launch modes.Reviewer Notes
The PR is broader than a local cleanup in
catch2_test_device_segmented_scan.cu. It adds reusable C2H buffer allocation/generation infrastructure and stream-aware launch-wrapper entry points, then uses segmented scan as the first substantial migration target.closes #10897
Previous edition of PR description
Segmented Scan tests to use CCCL Runtime buffers, instead of Thrust containers
Introduced
c2h::make_device_bufferutility that uses addedc2h::checked_device_memory_resource, acuda::mr resourcethat backscuda::device_bufferwith C2H’s checked cudaMalloc path.Uses
auto buffer = c2h::make_device_buffer<T>(stream, device, count, cuda::no_init);This replaces uses of
c2h::device_vectorin tests.Similarly,
c2h::host_vectoris replaced withc2h::host_bufferthat usesoperator new/operator delete-based device-bound synchronous memory resource.Checklist