Repository navigation
Fix [QA Agent] Cooperative reduction - Warp - #11645
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 SummarySummary by CodeRabbit
WalkthroughLarge-integer warp bitwise reductions now support signed integral types. The implementation reduces unsigned chunks and casts the result back to the input type. Full and partial reduction tests cover signed and unsigned integer types. ChangesSigned Bitwise Reductions
Priority: ⬇️ Low Change: Bug fix Merge Risk: 🔵 Low · up to The signed-wide reduction tests miss low-word inputs, and supported non-GCC test configurations skip them. The reduction implementation has no demonstrated runtime failure, but these gaps leave important behavior less protected against regressions. Comment ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
|
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cub/test/warp/catch2_test_warp_reduce.cu (1)
332-352: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winsuggestion: Generate nonzero low chunks for signed-wide bitwise tests.
The default
c2h::genmaps the fullint64_tand__int128_tranges throughfloat. The generated values therefore have zero low 32-bit chunks. Both tests can pass if the implementation skips the low chunk.Use explicit bounds for signed wide types in both bitwise tests:
- c2h::gen(C2H_SEED(10), d_in); + if constexpr (cuda::std::is_signed_v<T> && sizeof(T) > sizeof(unsigned)) + { + c2h::gen(C2H_SEED(10), d_in, -(T{1} << 32), T{1} << 32); + } + else + { + c2h::gen(C2H_SEED(10), d_in); + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: NVIDIA/cccl/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ce66d928-a498-478e-9036-c186a17675bc
📒 Files selected for processing (1)
cub/test/warp/catch2_test_warp_reduce.cu
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: NVIDIA/cccl/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3b473db7-d827-484e-a6bd-2dfbde6d53be
📒 Files selected for processing (2)
cub/cub/warp/specializations/warp_redux.cuhcub/test/warp/catch2_test_warp_reduce.cu
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
| } | ||
|
|
||
| CUB_TEST("WarpReduce::Reduce, unsigned bitwise types", | ||
| #if _CCCL_COMPILER(GCC, >=, 8) // gcc 7 internal compiler error in test code only |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
rg -n "_CCCL_COMPILER|GCC 7|catch2_test_warp_reduce|warp_reduce" libcudacxx/include/cuda/std/__cccl/compiler.h cub/test .github cmake
sed -n '350,470p' cub/test/warp/catch2_test_warp_reduce.cuRepository: NVIDIA/cccl
Length of output: 35949
🏁 Script executed:
set -eu
printf '%s\n' '--- compiler macro semantics ---'
sed -n '1,115p' libcudacxx/include/cuda/std/__cccl/compiler.h
printf '%s\n' '--- warp test configuration references ---'
rg -n -C 3 'catch2_test_warp_reduce|warp_reduce.*(test|target)|CUB.*(GCC|Clang|MSVC|NVHPC)|NVHPC|Clang|MSVC|GCC 7|compiler' cub/test/CMakeLists.txt cub/test/warp .github cmake --glob '*.cmake' --glob '*.yml' --glob '*.yaml' --glob 'CMakeLists.txt' | head -n 300
printf '%s\n' '--- relevant test file setup and type lists ---'
sed -n '1,180p' cub/test/warp/catch2_test_warp_reduce.cuRepository: NVIDIA/cccl
Length of output: 33792
🏁 Script executed:
set -eu
printf '%s\n' '--- CUB test target and source registration ---'
sed -n '1,230p' cub/test/CMakeLists.txt
printf '%s\n' '--- warp test references outside the first scope ---'
rg -n -C 3 'warp_reduce|CUB_TEST|CMAKE_CXX_COMPILER_ID|CMAKE_CUDA_COMPILER_ID|host:' .github cmake ci cub --glob '*.yml' --glob '*.yaml' --glob '*.cmake' --glob 'CMakeLists.txt' --glob '*.json' | head -n 500Repository: NVIDIA/cccl
Length of output: 21357
important: Exclude only GCC versions below 8.
_CCCL_COMPILER(GCC, >=, 8) evaluates false for GCC below 8 and for inactive compilers. This removes both bitwise reduction tests for Clang, MSVC, and NVHPC, although the generic CUB test configuration builds this test source for those compilers.
Use !_CCCL_COMPILER(GCC, <, 8) at both guards and update the matching #endif annotations. The negated comparison evaluates true for inactive compilers.
Suggested fix
-#if _CCCL_COMPILER(GCC, >=, 8) // gcc 7 internal compiler error in test code only
+#if !_CCCL_COMPILER(GCC, <, 8) // gcc 7 internal compiler error in test code only
...
-#endif // _CCCL_COMPILER(GCC, >=, 8)
+#endif // !_CCCL_COMPILER(GCC, <, 8)
...
-#if _CCCL_COMPILER(GCC, >=, 8) // gcc 7 internal compiler error in test code only
+#if !_CCCL_COMPILER(GCC, <, 8) // gcc 7 internal compiler error in test code only
...
-#endif // _CCCL_COMPILER(GCC, >=, 8)
+#endif // !_CCCL_COMPILER(GCC, <, 8)📝 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.
| #if _CCCL_COMPILER(GCC, >=, 8) // gcc 7 internal compiler error in test code only | |
| #if !_CCCL_COMPILER(GCC, <, 8) // gcc 7 internal compiler error in test code only |
🥳 CI Workflow Results🟩 Finished in 4h 48m: Pass: 100%/202 | Total: 14d 22h | Max: 4h 02m | Hits: 31%/1771588See results here. AI failure analysis1. RAPIDS consumers fail against removed RMM stream-view API · 4 jobsExplanation: All four jobs use floating RAPIDS main revisions, but the selected RMM 26.12 headers no longer provide `rmm::cuda_stream_view`, its header, or `cuda_stream::view()`, while cuVS, cuOpt, and rapidsMPF still consume those APIs. The PR diff only changes CUB warp reduction, so the evidence indicates RAPIDS dependency skew rather than a regression in the proposed CUB changes. Evidence: Copy this prompt into a coding agentJobs: |
Description
Fix Cooperative reduction - Warp of
Issues not implemented:
Nit: the docs say "integral types of any size". This is the only one relevant. Instead of updating the documentation as suggested, the implementation extends bitwise operations to all signed types (for better perf).