[libcu++] Use cccl runtime in thrust tests pt 2#9633
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds pinned host/device buffer helpers and broadens ChangesCUDA Test Runtime and Algorithm Test Refactors
Suggested reviewers
Comment |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
thrust/testing/cuda/merge.cu (1)
30-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Use
constexprfor these compile-time values.
n,sizes, andnum_sizesare compile-time constants. As per coding guidelines, “All variables that can be evaluated at compile time must be declaredconstexpr.”- const size_t n = 10000; - const size_t sizes[] = {0, 1, n / 2, n, n + 1, 2 * n}; - const size_t num_sizes = sizeof(sizes) / sizeof(size_t); + constexpr size_t n = 10000; + constexpr size_t sizes[] = {0, 1, n / 2, n, n + 1, 2 * n}; + constexpr size_t num_sizes = sizeof(sizes) / sizeof(sizes[0]);Source: Coding guidelines
thrust/testing/cuda/merge_by_key.cu (1)
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Make the returned iterator pair
const.
endis not modified after initialization. As per coding guidelines, “All variables that are not modified must be declaredconst, including ... function return values.”- auto end = thrust::merge_by_key( + const auto end = thrust::merge_by_key(Source: Coding guidelines
thrust/testing/cuda/set_intersection.cu (1)
28-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Declare read-only input buffers const.
aandbare only used as inputs in both tests; useconst autofor these handles. As per coding guidelines, “All variables that are not modified must be declaredconst.”Also applies to: 63-64
Source: Coding guidelines
thrust/testing/cuda/set_intersection_by_key.cu (1)
24-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Declare read-only locals const.
end,a_key,b_key, anda_valare not modified after initialization; useconst auto. As per coding guidelines, “All variables that are not modified must be declaredconst.”Also applies to: 44-46, 91-93
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 81309f84-15ae-449c-add0-312e73ef848a
📒 Files selected for processing (5)
thrust/testing/cuda/cccl_runtime_test_helper.cuhthrust/testing/cuda/merge.cuthrust/testing/cuda/merge_by_key.cuthrust/testing/cuda/set_intersection.cuthrust/testing/cuda/set_intersection_by_key.cu
🥳 CI Workflow Results🟩 Finished in 1h 01m: Pass: 100%/70 | Total: 19h 47m | Max: 1h 01m | Hits: 92%/152517See results here. |
|
|
||
| for (cuda::std::size_t i = 0; i < size; ++i) | ||
| { | ||
| result[i] = static_cast<T>(unittest::generate_random_integer<RandomT>{}(static_cast<unsigned int>(first + i))); |
There was a problem hiding this comment.
Important: Isn't this extremely inefficient if size is large? This should be limited like the unittest::random_integers function
There was a problem hiding this comment.
What do you mean by limited? random_integers just passes the size to transform and doesn't cap it
| for (cuda::std::size_t i = 0; i < size; ++i) | ||
| { | ||
| result[i] = static_cast<T>(unittest::generate_random_sample<RandomT>{}(static_cast<unsigned int>(first + i))); |
3 participants
Continuing #9591 with second set of changes to Use CCCL Runtime in thrust tests