cuda::simd Min/Max + ReLU - #11192
Conversation
📝 SummarySummary by CodeRabbit
WalkthroughChangesThe PR adds signed-integer SIMD SIMD min/max ReLU
Merge Risk: 🟡 Moderate · up to This change adds signed SIMD min/max ReLU APIs and DPX code generation. Functional and code-generation coverage still has gaps that could allow incorrect min_relu behavior or instruction selection to go undetected, so these issues should be resolved before merge. 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: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 79f5365c-19f7-4ad0-9661-c151da7a6c69
📒 Files selected for processing (10)
docs/libcudacxx/extended_api/simd.rstdocs/libcudacxx/extended_api/simd/min_max_relu.rstlibcudacxx/include/cuda/__simd/min_max_relu.hlibcudacxx/include/cuda/__simd/simd_intrinsics.hlibcudacxx/include/cuda/__simd/simd_intrinsics_array.hlibcudacxx/include/cuda/simdlibcudacxx/include/cuda/std/__internal/features.hlibcudacxx/include/cuda/std/__simd/basic_vec.hlibcudacxx/test/libcudacxx/std/numerics/simd/simd.non_std/min_max_relu.pass.cpplibcudacxx/test/simd_codegen/min_max/min_max_relu.cu
Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review.
This comment has been minimized.
This comment has been minimized.
| [[nodiscard]] _CCCL_HOST_DEVICE_API constexpr ::cuda::std::simd::basic_vec<_Tp, _Abi> max_relu( | ||
| const ::cuda::std::simd::basic_vec<_Tp, _Abi>& __lhs, const ::cuda::std::simd::basic_vec<_Tp, _Abi>& __rhs) noexcept | ||
| { | ||
| #if _CCCL_HAS_SIMD_MIN_MAX_RELU() |
There was a problem hiding this comment.
This nested preprocessor soup is pretty hard to follow :(, can we maybe isolate some of the steps into helper functions?
There was a problem hiding this comment.
I tried some alternatives, but they are not great, e.g. force to add a fallback implementation when it is not needed. The main problem is that the code is based on NV_IF_TARGET fall through, so I think there are no good alternatives.
a7e9744 to
732a09e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
libcudacxx/include/cuda/__simd/simd_intrinsics.h (1)
388-388: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: the
#endifannotation reverses the operand order of the guard at Line 348. Repeat the condition verbatim.-# endif // _CCCL_HAS_SIMD_8BIT_PTX() && _CCCL_HAS_SIMD_MIN_MAX_RELU() +# endif // _CCCL_HAS_SIMD_MIN_MAX_RELU() && _CCCL_HAS_SIMD_8BIT_PTX()Based on learnings: for every
#else/#endifcomment, the comment must repeat the exact verbatim condition text from the corresponding#if.Source: Learnings
libcudacxx/include/cuda/__simd/simd_intrinsics_array.h (2)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: the new
__dp4a_8bit_x4and__dp2a_16bit_x2_8bit_x4templates use::cuda::std::is_unsigned_v, but onlyis_integral.his added. Include the precise header foris_unsigned_v.# include <cuda/std/__type_traits/is_integral.h> +# include <cuda/std/__type_traits/is_unsigned.h>As per coding guidelines: "Include all headers needed by the symbols being used; do not rely on transitive includes." and "Use the most precise available header".
Source: Coding guidelines
228-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: all six new ReLU wrappers declare
__result_uwithout an initializer insideconstexprfunctions. An uninitialized local prevents constant evaluation, and the surrounding code uses uniform initialization (_SimdStorage __result{},__unsigned_storage_t __result_u{}inlibcudacxx/include/cuda/__simd/saturating_add.h). Add{}at each site.
libcudacxx/include/cuda/__simd/simd_intrinsics_array.h#L228-L228: change to__array_u32_t<_Np> __result_u{};in__vmax_relu_8bit_x4.libcudacxx/include/cuda/__simd/simd_intrinsics_array.h#L241-L241: change to__array_u32_t<_Np> __result_u{};in__vmin_relu_8bit_x4.libcudacxx/include/cuda/__simd/simd_intrinsics_array.h#L256-L256: change to__array_u32_t<_Np> __result_u{};in__vmax_relu_16bit_x2.libcudacxx/include/cuda/__simd/simd_intrinsics_array.h#L269-L269: change to__array_u32_t<_Np> __result_u{};in__vmin_relu_16bit_x2.libcudacxx/include/cuda/__simd/simd_intrinsics_array.h#L284-L284: change to__array_u32_t<_Np> __result_u{};in__vmax3_relu_16bit_x2.libcudacxx/include/cuda/__simd/simd_intrinsics_array.h#L299-L299: change to__array_u32_t<_Np> __result_u{};in__vmin3_relu_16bit_x2.As per coding guidelines: "Use uniform initialization for class constructors and compile-time conversions".
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 15576ff3-676d-4817-831e-858af753069e
📒 Files selected for processing (6)
docs/libcudacxx/extended_api/simd.rstlibcudacxx/include/cuda/__simd/simd_intrinsics.hlibcudacxx/include/cuda/__simd/simd_intrinsics_array.hlibcudacxx/include/cuda/simdlibcudacxx/include/cuda/std/__internal/features.hlibcudacxx/include/cuda/std/__simd/basic_vec.h
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.
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: 79f451d8-6b19-44e7-8d88-9b4c8726d247
📒 Files selected for processing (5)
docs/libcudacxx/extended_api/simd/min_max_relu.rstlibcudacxx/include/cuda/__simd/min_max_relu.hlibcudacxx/test/simd_codegen/CMakeLists.txtlibcudacxx/test/simd_codegen/min_max/min_max_relu.culibcudacxx/test/simd_codegen/min_max/min_max_relu_i8x4.cu
💤 Files with no reviewable changes (1)
- libcudacxx/test/simd_codegen/min_max/min_max_relu.cu
🚧 Files skipped from review as they are similar to previous changes (2)
- libcudacxx/include/cuda/__simd/min_max_relu.h
- docs/libcudacxx/extended_api/simd/min_max_relu.rst
Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review.
| ; SM107f: {{.*VIMNMX\.S8x4\.RELU.*PT.*}} | ||
| ; SM107f: {{.*VIMNMX\.S8x4\.RELU.*PT.*}} | ||
| ; SM120f: {{.*VIMNMX\.S8x4\.RELU.*PT.*}} | ||
| ; SM120f: {{.*VIMNMX\.S8x4\.RELU.*PT.*}} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
important: Make the min_relu checks distinguish PT from !PT. The pattern .*PT.* also matches !PT, so an incorrect VIMNMX.S8x4.RELU ... !PT instruction can pass the min checks. Require a whitespace-delimited PT token or explicitly exclude !PT.
Also applies to: 52-54
😬 CI Workflow Results🟥 Finished in 4h 29m: Pass: 98%/195 | Total: 4d 07h | Max: 4h 28m | Hits: 53%/1384558See results here. AI failure analysis1. libcudacxx SIMD three-input max_relu assertion on H100 · 2 jobsExplanation: Both available primary-job logs show the identical H100/SM90 device assertion, while the test reaches the device phase after its host checks. The PR introduces this test and routes three-input 32-bit `max_relu` through lane-wise `__vimax3_s32_relu`; the logs do not report the failing lane or actual value, so they localize the defect to that optimized device path without proving whether the underlying cause is the intrinsic, compiler lowering, or its use here. Evidence: Copy this prompt into a coding agentJobs: |
2 participants
Description
The PR adds the functionalities to generate the Dynamic Programming eXtension (DPX) instructions related to
min/max+ ReLU withcuda::std::simdvectors.The instructions are available on SM90+ for
s32,s16x2+ SM120/SM107 fors8x4.