Repository navigation
[CUB] Fully support warp redux - #9441
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces warp-level REDUX reduction dispatch for SM80 and SM100f architectures via new ChangesWarp REDUX dispatch and consumers
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: 6
🧹 Nitpick comments (4)
cub/cub/warp/specializations/warp_redux.cuh (4)
1-2: 📐 Maintainability & Code Quality | 💤 Low valuesuggestion: SPDX license identifier should be
BSD-3-Clausefor full compliance.
65-73: 📐 Maintainability & Code Quality | 💤 Low valuesuggestion: Per coding guidelines, unmodified parameters and local variables should be
const.maskparameter andvaluelocal are not modified.template <typename T, typename ReductionOp> [[nodiscard]] _CCCL_DEVICE_API _CCCL_FORCEINLINE T -warp_redux_sm80(const T input, const ::cuda::std::uint32_t mask, ReductionOp) +warp_redux_sm80(const T input, const ::cuda::std::uint32_t mask, ReductionOp) noexcept { static_assert(is_warp_redux_op_supported_sm80<ReductionOp, T>, "Reduction operator not supported"); _CCCL_ASSERT(mask != 0, "Mask must not be 0"); - using promotion_t = ::cuda::std::conditional_t<::cuda::std::is_signed_v<T>, int, unsigned>; - const auto value = static_cast<promotion_t>(input); + using promotion_t = ::cuda::std::conditional_t<::cuda::std::is_signed_v<T>, int, unsigned>; + const promotion_t value = static_cast<promotion_t>(input);Source: Coding guidelines
110-121: 🚀 Performance & Scalability | 💤 Low valuesuggestion: The macro-generated PTX functions use
asm volatilebut the reduction is a pure read operation with no memory side effects. Considerasmwithoutvolatileto allow compiler optimization, or add a brief comment explaining whyvolatileis necessary (e.g., if required for synchronization semantics).
131-149: 📐 Maintainability & Code Quality | 💤 Low valuesuggestion: Per guidelines,
valueandresultlocals should beconstwhere possible.valueis not modified after initialization.template <typename T, typename ReductionOp> [[nodiscard]] _CCCL_DEVICE_API -_CCCL_FORCEINLINE T warp_redux_sm100af(const T input, const ::cuda::std::uint32_t mask, ReductionOp) +_CCCL_FORCEINLINE T warp_redux_sm100af(const T input, const ::cuda::std::uint32_t mask, ReductionOp) noexcept { static_assert(is_warp_redux_op_supported_sm100af<ReductionOp, T>, "Reduction operator not supported"); _CCCL_ASSERT(mask != 0, "Mask must not be 0"); const float value = ::cuda::std::__fp_cast<float>(input); - float result; + const float result = [&]() { if constexpr (is_cuda_minimum_v<ReductionOp, T>) { - result = cub::detail::redux_sm100af_min_ptx(value, mask); + return ::cub::detail::redux_sm100af_min_ptx(value, mask); } else { - result = cub::detail::redux_sm100af_max_ptx(value, mask); + return ::cub::detail::redux_sm100af_max_ptx(value, mask); } + }(); return ::cuda::std::__fp_cast<T>(result); }Alternatively, keep the current structure but add
constwhere applicable and fully qualifycub::detail::as::cub::detail::.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 66046a44-cb32-43c6-8ce3-c6b5f927aa61
📒 Files selected for processing (6)
cub/cub/detail/warpspeed/look_ahead.cuhcub/cub/thread/thread_operators.cuhcub/cub/warp/specializations/warp_reduce_batched_wspro.cuhcub/cub/warp/specializations/warp_reduce_shfl.cuhcub/cub/warp/specializations/warp_redux.cuhcub/test/warp/catch2_test_warp_reduce.cu
💤 Files with no reviewable changes (1)
- cub/cub/thread/thread_operators.cuh
| if constexpr (ALL_LANES_VALID && is_warp_redux_op_supported<ReductionOp, T>) | ||
| { | ||
| NV_IF_TARGET(NV_PROVIDES_SM_80, (return reduce_op_sync(input, member_mask, reduction_op);)) | ||
| if (const auto output = cub::detail::warp_redux(input, member_mask, reduction_op)) | ||
| { | ||
| return *output; | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
suggestion: Per coding guidelines, trait and function call should be fully qualified with :: prefix.
- if constexpr (ALL_LANES_VALID && is_warp_redux_op_supported<ReductionOp, T>)
+ if constexpr (ALL_LANES_VALID && ::cub::detail::is_warp_redux_op_supported<ReductionOp, T>)
{
- if (const auto output = cub::detail::warp_redux(input, member_mask, reduction_op))
+ if (const auto output = ::cub::detail::warp_redux(input, member_mask, reduction_op))
{
return *output;
}
}Source: Coding guidelines
| NV_IF_TARGET(NV_PROVIDES_SM_80, (return cub::detail::warp_redux_sm80(input, mask, reduction_op);)) | ||
| } | ||
| #if __cccl_ptx_isa >= 860 | ||
| else if constexpr (is_warp_redux_op_supported_sm100af<ReductionOp, T>) | ||
| { | ||
| NV_IF_TARGET(NV_HAS_FEATURE_SM_100a, (return cub::detail::warp_redux_sm100af(input, mask, reduction_op);)) | ||
| # if __cccl_ptx_isa >= 880 | ||
| NV_IF_TARGET(NV_HAS_FEATURE_SM_100f, (return cub::detail::warp_redux_sm100af(input, mask, reduction_op);)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
suggestion: Multiple call sites across warp_redux.cuh, look_ahead.cuh, warp_reduce_batched_wspro.cuh, and warp_reduce_shfl.cuh reference cub::detail:: functions/traits without the leading :: global namespace qualifier. Per coding guidelines, all free function calls and trait references must be fully qualified starting from global namespace (e.g., ::cub::detail::warp_redux(...)). This ensures ADL doesn't cause unexpected behavior and maintains consistency across the codebase.
Source: Coding guidelines
| else | ||
| { | ||
| _CCCL_UNREACHABLE(); | ||
| return T{}; |
There was a problem hiding this comment.
Should this be an optional as well? Its packed into one after return anyway
There was a problem hiding this comment.
it could be a good extension. For now, I would keep it as is because it used only one time
This comment has been minimized.
This comment has been minimized.
Co-authored-by: David Bayer <48736217+davebayer@users.noreply.github.com>
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: baabf63b-60f6-4346-ae5f-cf824bf3bffa
📒 Files selected for processing (1)
cub/cub/warp/specializations/warp_redux.cuh
This comment has been minimized.
This comment has been minimized.
Co-authored-by: David Bayer <48736217+davebayer@users.noreply.github.com>
|
pre-commit.ci autofix |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cub/cub/warp/specializations/warp_redux.cuh (1)
165-175:⚠️ Potential issue | 🔴 Critical | ⚡ Quick wincritical:
#endifat line 174 has no matching#if. The inner#if __cccl_ptx_isa >= 880at line 168 is closed by#endifat line 172. Line 174 is orphaned and will fail to compile.Add the missing guard:
+#if __cccl_ptx_isa >= 860 else if constexpr (is_warp_redux_op_supported_sm100f<ReductionOp, T>) { // Before PTX ISA 8.8, float reductions are only supported on sm100a. # if __cccl_ptx_isa >= 880 NV_IF_TARGET(NV_HAS_FEATURE_SM_100f, (return cub::detail::warp_redux_sm100f(input, mask, reduction_op);)) # else // ^^^ __cccl_ptx_isa >= 880 ^^^ / vvv __cccl_ptx_isa < 880 vvv NV_IF_TARGET(NV_HAS_FEATURE_SM_100a, (return cub::detail::warp_redux_sm100f(input, mask, reduction_op);)) # endif // ^^^ __cccl_ptx_isa < 880 ^^^ } `#endif` // __cccl_ptx_isa >= 860
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f1e5288b-be3b-4c37-8cdc-7d615b947197
📒 Files selected for processing (1)
cub/cub/warp/specializations/warp_redux.cuh
|
/ok to test d008108 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cub/cub/warp/specializations/warp_redux.cuh (3)
50-52:⚠️ Potential issue | 🔴 Critical | ⚡ Quick wincritical: Undefined symbol
is_warp_redux_op_supported_sm100f— the trait is defined asis_warp_redux_op_supported_sm100afon line 46. This will fail to compile.template <typename Op, typename T> inline constexpr bool is_warp_redux_op_supported = - is_warp_redux_op_supported_sm80<Op, T> || is_warp_redux_op_supported_sm100f<Op, T>; + is_warp_redux_op_supported_sm80<Op, T> || is_warp_redux_op_supported_sm100af<Op, T>;
123-141:⚠️ Potential issue | 🔴 Critical | ⚡ Quick wincritical: Line 127 uses undefined
is_warp_redux_op_supported_sm100f. Should beis_warp_redux_op_supported_sm100afto match the trait definition on line 46.- static_assert(is_warp_redux_op_supported_sm100f<ReductionOp, T>, "Reduction operator not supported"); + static_assert(is_warp_redux_op_supported_sm100af<ReductionOp, T>, "Reduction operator not supported");
148-167:⚠️ Potential issue | 🔴 Critical | ⚡ Quick wincritical: Lines 161 and 163 call
warp_redux_sm100fwhich is undefined — the function is defined aswarp_redux_sm100afon line 125. Also, per past review feedback, use::cub::detail::prefix.else if constexpr (is_warp_redux_op_supported_sm100af<ReductionOp, T>) { // Before PTX ISA 8.8, float reductions are only supported on sm100a. `#if` __cccl_ptx_isa >= 880 - NV_IF_TARGET(NV_HAS_FEATURE_SM_100f, (return cub::detail::warp_redux_sm100f(input, mask, reduction_op);)) + NV_IF_TARGET(NV_HAS_FEATURE_SM_100f, (return ::cub::detail::warp_redux_sm100af(input, mask, reduction_op);)) `#else` // ^^^ __cccl_ptx_isa >= 880 ^^^ / vvv __cccl_ptx_isa < 880 vvv - NV_IF_TARGET(NV_HAS_FEATURE_SM_100a, (return cub::detail::warp_redux_sm100f(input, mask, reduction_op);)) + NV_IF_TARGET(NV_HAS_FEATURE_SM_100a, (return ::cub::detail::warp_redux_sm100af(input, mask, reduction_op);)) `#endif` // ^^^ __cccl_ptx_isa < 880 ^^^ }Similarly, line 155 should use
::cub::detail::warp_redux_sm80.
🧹 Nitpick comments (1)
cub/cub/warp/specializations/warp_redux.cuh (1)
117-117: 💤 Low valuesuggestion: Typo — "corresposing" should be "corresponding".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f8eb5788-efc4-4720-af83-27e811e14f14
📒 Files selected for processing (1)
cub/cub/warp/specializations/warp_redux.cuh
|
SASS comparison: no diff for sum benchmarks (as expected). SM100af min/max float replaces 5 |
|
/ok to test |
This comment has been minimized.
This comment has been minimized.
|
/ok to test 2428754 |
This comment has been minimized.
This comment has been minimized.
|
/ok to test 8904168 |
🥳 CI Workflow Results🟩 Finished in 2h 41m: Pass: 100%/287 | Total: 11d 19h | Max: 2h 41m | Hits: 15%/1148788See results here. |
Description
I recently saw at least two PRs related to the warp reduction with specific dispatch for
redux. Considering that the current dispatch logic is quite messy and there are additional optimizations, I create this PR:reduxcode to a dedicated header.sm100a/fredux.sync.min/max.f32forfloat__halfand__nv_bfloat16min/max are also handled by promoted them tofloat.warp_reduxfunction returnscuda::std::optionalto greatly simplify the calling dispatch.reduxis valid.Todo: