[libcu++] Implement P3793R2 Better Shifting (without SIMD)#9993
[libcu++] Implement P3793R2 Better Shifting (without SIMD)#9993davebayer wants to merge 4 commits into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesAdds Integer shift utilities
Suggested reviewers: Comment |
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: 487bc115-c3d6-4a80-845a-898abd27190e
📒 Files selected for processing (6)
libcudacxx/include/cuda/std/__bit/shl.hlibcudacxx/include/cuda/std/__bit/shr.hlibcudacxx/include/cuda/std/bitlibcudacxx/test/libcudacxx/std/numerics/bit/bit.shift/shl.pass.cpplibcudacxx/test/libcudacxx/std/numerics/bit/bit.shift/shr.pass.cpplibcudacxx/test/support/test_macros.h
247d04d to
dfc3703
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libcudacxx/test/support/test_macros.h (1)
112-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Make the pointer object
const.ptris not reassigned; use a top-levelconstwhile retaining itsvolatilequalification for the assembly operand.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: edbeeff7-f119-4e50-a24a-968539124e5c
📒 Files selected for processing (1)
libcudacxx/test/support/test_macros.h
| [[nodiscard]] _CCCL_API constexpr _Tp shl(_Tp __v, _Shift __shift) noexcept | ||
| { | ||
| constexpr auto __width = static_cast<uint32_t>(__num_bits_v<_Tp>); | ||
| const auto __ushift = ::cuda::uabs(__shift); |
There was a problem hiding this comment.
what about we add an assertion for -min value instead?
|
|
||
| _CCCL_TEMPLATE(class _Tp, class _Shift) | ||
| _CCCL_REQUIRES(__cccl_is_integer_v<_Tp> _CCCL_AND __cccl_is_integer_v<_Shift>) | ||
| [[nodiscard]] _CCCL_API constexpr _Tp shl(_Tp __v, _Shift __shift) noexcept |
There was a problem hiding this comment.
| [[nodiscard]] _CCCL_API constexpr _Tp shl(_Tp __v, _Shift __shift) noexcept | |
| [[nodiscard]] _CCCL_API constexpr _Tp shl(const _Tp __v, const _Shift __shift) noexcept |
| } | ||
| return (__ushift < __width) ? (__v >> __ushift) : static_cast<_Tp>(::cuda::std::cmp_less(__v, 0) ? -1 : 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
| } | |
| } | |
| else { |
| _CCCL_REQUIRES(__cccl_is_integer_v<_Tp> _CCCL_AND __cccl_is_integer_v<_Shift>) | ||
| [[nodiscard]] _CCCL_API constexpr _Tp shr(_Tp __v, _Shift __shift) noexcept | ||
| { | ||
| constexpr auto __width = static_cast<uint32_t>(__num_bits_v<_Tp>); |
There was a problem hiding this comment.
| constexpr auto __width = static_cast<uint32_t>(__num_bits_v<_Tp>); | |
| constexpr auto __width = uint32_t{__num_bits_v<_Tp>}; |
|
|
||
| _CCCL_TEMPLATE(class _Tp, class _Shift) | ||
| _CCCL_REQUIRES(__cccl_is_integer_v<_Tp> _CCCL_AND __cccl_is_integer_v<_Shift>) | ||
| [[nodiscard]] _CCCL_API constexpr _Tp shr(_Tp __v, _Shift __shift) noexcept |
There was a problem hiding this comment.
it uses PTX so Tile is not supported
| [[nodiscard]] _CCCL_API constexpr _Tp shr(_Tp __v, _Shift __shift) noexcept | |
| [[nodiscard]] _CCCL_HOST_DEVICE_API constexpr _Tp shr(_Tp __v, _Shift __shift) noexcept |
| #include <cuda/std/__utility/cmp.h> | ||
| #include <cuda/std/cstdint> | ||
|
|
||
| #if _CCCL_CUDA_COMPILATION() |
There was a problem hiding this comment.
| #if _CCCL_CUDA_COMPILATION() | |
| #if _CCCL_CUDA_COMPILATION() && !_CCCL_TILE_COMPILATION() |
(or similar)
| // under the Apache License v2.0 with LLVM Exceptions. | ||
| // See https://llvm.org/LICENSE.txt for license information. | ||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
| // SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. |
There was a problem hiding this comment.
should we also update __cpp_lib_bitops?
| @@ -106,31 +106,23 @@ | |||
| # define TEST_NVRTC_VIRTUAL_DEFAULT_DTOR_ANNOTATION | |||
There was a problem hiding this comment.
question. why we need to modify this file?
|
|
||
| const S neg_shifts[] = {smin, S{-65}, S{-33}, S{-32}, S{-15}, S{-7}, S{-1}}; | ||
|
|
||
| _CCCL_PRAGMA_NOUNROLL() |
There was a problem hiding this comment.
I don't think make sense to use pragma unroll in tests
😬 CI Workflow Results🟥 Finished in 4h 21m: Pass: 89%/120 | Total: 5d 11h | Max: 3h 37m | Hits: 45%/2480803See results here. |
This PR implement P3793R2 Better Shifting. I've omitted the simd support, should be done in a separate PR.