[CUB] Fix DeviceAdjacentDifference for cuda::device_buffer iterators#9861
[CUB] Fix DeviceAdjacentDifference for cuda::device_buffer iterators#9861Noperi0r wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesAdjacent Difference Iterator Compatibility
Assessment against linked issues
Suggested reviewers: Comment |
There was a problem hiding this comment.
Important: Please move the new test into the file catch2_test_device_adjacent_difference_substract_left.cu. The _api.cu files cover tested API examples for the documentation.
There was a problem hiding this comment.
Moved the regression test to
catch2_test_device_adjacent_difference_substract_left.cu
and restored catch2_test_device_adjacent_difference_api.cu to its previous scope.
The relocated focused test, target build, and targeted pre-commit checks pass locally.
|
|
||
| cuda::stream stream{cuda::devices[0]}; | ||
| auto input = cuda::make_device_buffer<type>(stream, cuda::devices[0], {2, 5, 9, 14, 20}); | ||
| c2h::device_vector<type> output(input.size()); |
There was a problem hiding this comment.
| c2h::device_vector<type> output(input.size()); | |
| c2h::device_vector<type> output(input.size(), thrust::no_init); |
There was a problem hiding this comment.
Applied thrust::no_init to the output allocation in the relocated test.
| == cub::DeviceAdjacentDifference::SubtractLeftCopy( | ||
| temporary_storage, temporary_bytes, input.begin(), output_it, input.size(), cuda::std::minus<>{}, stream.get())); | ||
|
|
||
| c2h::device_vector<std::uint8_t> temporary_buffer(temporary_bytes); |
There was a problem hiding this comment.
| c2h::device_vector<std::uint8_t> temporary_buffer(temporary_bytes); | |
| c2h::device_vector<std::uint8_t> temporary_buffer(temporary_bytes, thrust::no_init); |
There was a problem hiding this comment.
Applied thrust::no_init to the temporary-storage allocation in the relocated test.
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: de06a381-b09a-4138-9843-f913ccbe3dac
📒 Files selected for processing (1)
cub/test/catch2_test_device_adjacent_difference_substract_left.cu
|
Addressed the review feedback in the latest commits:
The target build, focused regression test, and targeted pre-commit checks pass locally. |
| void* temporary_storage = nullptr; | ||
| std::size_t temporary_bytes = 0; | ||
| REQUIRE( | ||
| cudaSuccess | ||
| == cub::DeviceAdjacentDifference::SubtractLeftCopy( | ||
| temporary_storage, temporary_bytes, input.begin(), output_it, input.size(), cuda::std::minus<>{}, stream.get())); | ||
|
|
||
| c2h::device_vector<std::uint8_t> temporary_buffer(temporary_bytes, thrust::no_init); | ||
| temporary_storage = thrust::raw_pointer_cast(temporary_buffer.data()); | ||
| REQUIRE( | ||
| cudaSuccess | ||
| == cub::DeviceAdjacentDifference::SubtractLeftCopy( | ||
| temporary_storage, temporary_bytes, input.begin(), output_it, input.size(), cuda::std::minus<>{}, stream.get())); |
There was a problem hiding this comment.
Important: Just use the launch wrapper adjacent_difference_subtract_left instead of handling the temporary storage allocation yourself.
There was a problem hiding this comment.
Updated the regression test to use adjacent_difference_subtract_left_copy, since this test covers SubtractLeftCopy. The launch wrapper now handles the temporary storage internally.
The focused target build, regression test, and targeted pre-commit checks pass locally.
|
|
||
| const c2h::host_vector<type> expected{2, 3, 4, 5, 6}; | ||
| REQUIRE(output == expected); |
There was a problem hiding this comment.
Important: I think we do not to sync when we use a custom stream:
| const c2h::host_vector<type> expected{2, 3, 4, 5, 6}; | |
| REQUIRE(output == expected); | |
| stream.sync(); | |
| const c2h::host_vector<type> expected{2, 3, 4, 5, 6}; | |
| REQUIRE(output == expected); |
Description
Closes #9859
cub::DeviceAdjacentDifference::SubtractLeftCopyfails to compile withcuda::device_buffer<T>::iteratorbecauseAgentDifferenceselectsLoadItthroughtry_make_cache_modified_iterator_tbut constructs it directly withLoadIt(input_it).This change constructs
load_itthroughtry_make_cache_modified_iterator, matching the existing type-selection path. It also adds a regression test that exercises both the temporary-storage query and execution calls withcuda::device_buffer<int>::iterator.Validation:
lid_0suite: 6/6 passedcompute-sanitizer --tool memcheck: 0 errorsChecklist