[libcu++] Add __float128 support for cuda::std::fabs#9895
Conversation
b886cfb to
895d465
Compare
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesAdds guarded __float128 support
Suggested labels: Suggested reviewers: Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libcudacxx/include/cuda/std/__floating_point/cast.h (1)
868-948: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion:
__float128→half/bf16/fp8/fp6/fp4 branches duplicate thefloat-source dispatch table instead of delegating.Every other non-float source branch in this function (e.g.
__half,__nv_bfloat16, the fp8/fp6/fp4 sources) reaches these target types via::cuda::std::__fp_cast<_To>(::cuda::std::__fp_cast<float>(__v)), delegating to the existingfloat-source dispatch. The new__float128branch instead re-implements each target's raw intrinsic call (::__float2half,::__float2bfloat16,::__nv_cvt_float_to_fp8, etc.) directly onstatic_cast<float>(__v), duplicating logic that already lives in thefloat-source branch above.Suggested simplification
-# if _CCCL_HAS_NVFP16() - else if constexpr (is_same_v<_To, __half>) - { - return ::__float2half(static_cast<float>(__v)); - } -# endif // _CCCL_HAS_NVFP16() -# if _CCCL_HAS_NVBF16() - else if constexpr (is_same_v<_To, __nv_bfloat16>) - { - return ::__float2bfloat16(static_cast<float>(__v)); - } -# endif // _CCCL_HAS_NVBF16() -# if _CCCL_HAS_NVFP8_E4M3() - else if constexpr (is_same_v<_To, __nv_fp8_e4m3>) - { - return ::cuda::std::__fp_from_storage<__nv_fp8_e4m3>( - ::__nv_cvt_float_to_fp8(static_cast<float>(__v), __NV_NOSAT, __NV_E4M3)); - } -# endif // _CCCL_HAS_NVFP8_E4M3() - ... (fp8_e5m2, fp8_e8m0, fp6_e2m3, fp6_e3m2, fp4_e2m1 likewise) + else if constexpr (is_same_v<_To, __float128>) + { + return __v; + } + else + { + return ::cuda::std::__fp_cast<_To>(static_cast<float>(__v)); + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cc8d5b4c-51a7-4a37-bc84-f3e137d37303
📒 Files selected for processing (4)
libcudacxx/include/cuda/std/__cmath/abs.hlibcudacxx/include/cuda/std/__floating_point/cast.hlibcudacxx/test/libcudacxx/libcxx/numerics/floating.point/cast.pass.cpplibcudacxx/test/libcudacxx/std/numerics/c.math/fp_abs.pass.cpp
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: aec86913-01e8-4ce7-a021-277d2f9d2421
📒 Files selected for processing (4)
libcudacxx/include/cuda/std/__cmath/abs.hlibcudacxx/include/cuda/std/__floating_point/cast.hlibcudacxx/test/libcudacxx/libcxx/numerics/floating.point/cast.pass.cpplibcudacxx/test/libcudacxx/std/numerics/c.math/fp_abs.pass.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- libcudacxx/test/libcudacxx/libcxx/numerics/floating.point/cast.pass.cpp
- libcudacxx/test/libcudacxx/std/numerics/c.math/fp_abs.pass.cpp
- libcudacxx/include/cuda/std/__floating_point/cast.h
895d465 to
a9199a1
Compare
🥳 CI Workflow Results🟩 Finished in 2h 11m: Pass: 100%/120 | Total: 3d 15h | Max: 2h 06m | Hits: 43%/1154054See results here. |
fbusato
left a comment
There was a problem hiding this comment.
approved. very minor concern if the future the builtin will be supported on device code, but the SASS differs from the intrinsic. (I would prioritize the intrinsic)
Fixes #9903