Use __builtin_bswapg in cuda::std::byteswap when available - #9785
Conversation
| if constexpr (sizeof(_Integer) > 1) | ||
| { | ||
| #if defined(_CCCL_BUILTIN_BSWAPG) | ||
| return static_cast<_Integer>(_CCCL_BUILTIN_BSWAPG(::cuda::std::__to_unsigned_like(__val))); |
There was a problem hiding this comment.
__builtin_bswapg is supposed to support any integer, do we still need the cast to unsigned?
Also on this note, we should add support for the sized builtins as well, __builtin_bswapg was only added in clang 22 (not sure if gcc even has it to begin with). Before that, __builtin_bswap{16, 32, 64} and possibly __builtin_bswap128 have existed for longer.
|
/ok to test |
@Jacobfaib, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test bb4ef02 |
This comment has been minimized.
This comment has been minimized.
|
Will do — I'll verify the unsigned cast is redundant and add the sized builtin fallback. Thanks for the review. |
bb4ef02 to
198d99a
Compare
|
/ok to test 198d99a |
| #if defined(_CCCL_BUILTIN_BSWAPG) | ||
| return static_cast<_Integer>(_CCCL_BUILTIN_BSWAPG(__val)); | ||
| #else // ^^^ _CCCL_BUILTIN_BSWAPG ^^^ / vvv !_CCCL_BUILTIN_BSWAPG vvv | ||
| if constexpr (sizeof(_Integer) == 2) |
There was a problem hiding this comment.
Don't hardcode the size here, I would say sizeof(_Integer) == sizeof(::cuda::std::uint16_t) since that is what we ultimately care about in the end.
| if constexpr (sizeof(_Integer) == 2) | ||
| { | ||
| # if defined(_CCCL_BUILTIN_BSWAP16) | ||
| return static_cast<_Integer>(_CCCL_BUILTIN_BSWAP16(static_cast<uint16_t>(__val))); |
There was a problem hiding this comment.
| return static_cast<_Integer>(_CCCL_BUILTIN_BSWAP16(static_cast<uint16_t>(__val))); | |
| return static_cast<_Integer>(_CCCL_BUILTIN_BSWAP16(static_cast<::cuda::std::uint16_t>(__val))); |
Unfortunately we must fully qualify all types and calls like this. It's not so strictly required for types, but better safe than sorry
|
Addressed both follow-up comments in 708003e: replaced the hardcoded size checks with comparisons against the corresponding fully qualified fixed-width types, and fully qualified the cast types in all three sized-builtin branches. Local pre-commit and the targeted |
Signed-off-by: Yuchen Fan <functionhx@gmail.com>
708003e to
98b083b
Compare
|
/ok to test 98b083b |
|
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 (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesByteswap builtin integration
Assessment against linked issues
Comment |
🥳 CI Workflow Results🟩 Finished in 1h 52m: Pass: 100%/120 | Total: 1d 21h | Max: 1h 19m | Hits: 100%/356615See results here. |
|
Thanks for the patch! |
Fixes #7778.