feat: unify is_even#9120
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a centralized ChangesCentralized is_even predicate
Suggested labels
Suggested reviewers
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5aed0315-6ddb-43dd-8a4f-12869d69cf33
📒 Files selected for processing (42)
c2h/include/c2h/operator.cuhcub/test/catch2_test_device_select_api.cucub/test/catch2_test_device_select_flagged_if.culibcudacxx/benchmarks/bench/copy_if/basic.culibcudacxx/benchmarks/bench/remove_copy_if/basic.culibcudacxx/benchmarks/bench/remove_if/basic.culibcudacxx/test/libcudacxx/std/algorithms/alg.modifying/alg.partitions/pstl_partition.culibcudacxx/test/libcudacxx/std/algorithms/alg.modifying/alg.partitions/pstl_partition_copy.culibcudacxx/test/libcudacxx/std/algorithms/alg.modifying/alg.partitions/pstl_stable_partition.culibcudacxx/test/libcudacxx/std/numerics/simd/simd.loadstore/partial_load.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.loadstore/partial_store.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.loadstore/unchecked_load.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.loadstore/unchecked_store.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.mask.class/compound_assign.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.mask.class/conversion.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.mask.class/ctor.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.mask.class/subscript.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.mask.class/unary.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.reductions/all_of.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.reductions/any_of.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.reductions/none_of.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.reductions/reduce.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.reductions/reduce_count.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.reductions/reduce_max.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.reductions/reduce_max_index.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.reductions/reduce_min.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.reductions/reduce_min_index.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd.vec.class/ctor.pass.cpplibcudacxx/test/libcudacxx/std/numerics/simd/simd_test_utils.hthrust/benchmarks/bench/copy_if/basic.cuthrust/benchmarks/bench/remove_copy_if/basic.cuthrust/benchmarks/bench/remove_if/basic.cuthrust/testing/copy.cuthrust/testing/cuda/copy_if.cuthrust/testing/cuda/is_partitioned.cuthrust/testing/cuda/partition.cuthrust/testing/cuda/partition_point.cuthrust/testing/cuda/remove.cuthrust/testing/is_partitioned.cuthrust/testing/partition.cuthrust/testing/partition_point.cuthrust/testing/remove.cu
| Mask all_true(true); | ||
| Mask all_false(false); | ||
| Mask alternating(is_even{}); | ||
| Mask alternating(c2h::is_even); |
There was a problem hiding this comment.
important: Line 43 declares alternating as mutable but never modifies it; make it const to satisfy the project constness rule.
As per coding guidelines "All non-const variables must use const if they are not modified".
| Mask all_true(true); | ||
| Mask all_false(false); | ||
| Mask mixed(is_even{}); | ||
| Mask mixed(c2h::is_even); |
There was a problem hiding this comment.
important: Lines 45, 63, 90, and 117 create mask variables that are never mutated; declare each as const Mask ... to comply with constness requirements.
As per coding guidelines "All non-const variables must use const if they are not modified".
Also applies to: 63-63, 90-90, 117-117
| if constexpr (N > 1) | ||
| { | ||
| Mask even(is_even{}); | ||
| Mask even(c2h::is_even); |
There was a problem hiding this comment.
important: Line 43 should use const Mask even(c2h::is_even); since even is not modified.
As per coding guidelines "All non-const variables must use const if they are not modified".
| using Mask = typename Vec::mask_type; | ||
| Vec vec = make_iota_vec<T, N>(); | ||
| Mask even(is_even{}); | ||
| Mask even(c2h::is_even); |
There was a problem hiding this comment.
important: Line 135 creates even as non-const but never mutates it; declare it const.
As per coding guidelines "All non-const variables must use const if they are not modified".
| state.exec(nvbench::exec_tag::gpu | nvbench::exec_tag::no_batch | nvbench::exec_tag::sync, | ||
| [&](nvbench::launch& launch) { | ||
| thrust::remove_if(policy(alloc, launch), in.begin(), in.end(), is_even{}); | ||
| thrust::remove_if(policy(alloc, launch), in.begin(), in.end(), c2h::is_even); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact snippet around the reported line
FILE="thrust/benchmarks/bench/remove_if/basic.cu"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "== File: $FILE =="
# Show enough context around line 30
nl -ba "$FILE" | sed -n '1,120p' | sed -n '20,60p'
echo
echo "== Search for remove_if call patterns in this file =="
rg -n "thrust::remove_if\(|::thrust::remove_if\(" "$FILE" || true
echo
echo "== Check for any using directives that might affect qualification =="
rg -n "using\s+namespace\s+thrust|using\s+thrust::|namespace\s+thrust" "$FILE" || true
echo
echo "== Check local aliases for remove_if =="
rg -n "remove_if" "$FILE" || trueRepository: NVIDIA/cccl
Length of output: 172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="thrust/benchmarks/bench/remove_if/basic.cu"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "== File: $FILE =="
# Print lines 20-60 with line numbers using awk (nl not available)
awk 'NR>=20 && NR<=60 {printf "%5d\t%s\n", NR, $0}' "$FILE"
echo
echo "== Search for remove_if call patterns in this file =="
rg -n "thrust::remove_if\(|::thrust::remove_if\(" "$FILE" || true
echo
echo "== Check for any using directives that might affect qualification =="
rg -n "using\s+namespace\s+thrust|using\s+thrust::|namespace\s+thrust" "$FILE" || true
echo
echo "== Check for local aliases for remove_if =="
rg -n "remove_if" "$FILE" || trueRepository: NVIDIA/cccl
Length of output: 1274
suggestion: Qualify the free function call from the global namespace. File thrust/benchmarks/bench/remove_if/basic.cu:30 uses thrust::remove_if(...); update to ::thrust::remove_if(...) per the global qualification requirement.
Diff
- thrust::remove_if(policy(alloc, launch), in.begin(), in.end(), c2h::is_even);
+ ::thrust::remove_if(policy(alloc, launch), in.begin(), in.end(), c2h::is_even);📝 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.
| thrust::remove_if(policy(alloc, launch), in.begin(), in.end(), c2h::is_even); | |
| ::thrust::remove_if(policy(alloc, launch), in.begin(), in.end(), c2h::is_even); |
| h_new_end = thrust::copy_if(h_data.begin(), h_data.end(), h_result.begin(), c2h::is_even); | ||
|
|
||
| copy_if_kernel<<<1, 1>>>( | ||
| exec, d_data.begin(), d_data.end(), d_result.begin(), is_even<int>(), d_new_end_vec.begin()); | ||
| exec, d_data.begin(), d_data.end(), d_result.begin(), c2h::is_even, d_new_end_vec.begin()); |
There was a problem hiding this comment.
important: Line 192 and Line 195 use the non-stencil copy_if overload inside TestCopyIfStencilDevice, so this test does not validate stencil behavior even though stencil inputs are prepared; wire these calls to the stencil overload (and apply the same fix to the non-bool block) to avoid missing stencil regressions.
| iterator ref = thrust::stable_partition(v.begin(), v.end(), c2h::is_even); | ||
|
|
||
| thrust::device_vector<iterator> result(1); | ||
| partition_point_kernel<<<1, 1>>>(exec, v.begin(), v.end(), is_even<int>(), result.begin()); | ||
| partition_point_kernel<<<1, 1>>>(exec, v.begin(), v.end(), c2h::is_even, result.begin()); |
There was a problem hiding this comment.
important: add a direct include for c2h::is_even in this file (#include <c2h/operator.cuh>). Line 22 and Line 25 currently depend on transitive includes, which is fragile and can fail under include-order changes. As per coding guidelines: "Files must include all headers related to the symbols they are using".
| std::ptrdiff_t n_true = thrust::count_if(h_data.begin(), h_data.end(), c2h::is_even); | ||
| std::ptrdiff_t n_false = n - n_true; |
There was a problem hiding this comment.
important: compute the true-count from the stencil range, not the data range. Line 312 uses h_data, but this test’s partition_copy classification uses h_stencil (Line 323 and Line 327). That can mis-size h_true_results / h_false_results and cause out-of-bounds writes when stencil parity differs from data parity.
|
Most issues that the AI pointed out was already there, but if we gotta fix them, tell me. |
1 participant
This is a continuation of #7455.
This PR focuses on unifying
is_evenfunctor across CCCL testing and benchmarking environment.Quick Summary of Changed
is_evenlogic toc2h/include/c2h/operator.cuhand enclosed it in c2h namespace.Note
This does not unify
cub/*files, because they usescustom_type. If we have to unify them, then I'll.(Comment from @bernhardmgruber in #7455 specified duplicates in cub/*)
Also, this is an issue from my end, my system has integrated M2 chip and testing CCCL on it, is either impossible or painfully slow. So, changes can contain few errors.