[libcxx-commits] [PATCH] D114074: [libc++] Implement P1272R4 (std::byteswap)
Nikolas Klauser via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Nov 17 12:16:23 PST 2021
philnik added inline comments.
================
Comment at: libcxx/include/__bit/byteswap.h:25
+template <integral _Tp>
+_LIBCPP_HIDE_FROM_ABI constexpr _Tp byteswap(_Tp __val) noexcept {
+ switch (sizeof(_Tp)) {
----------------
Quuxplusone wrote:
> Mordante wrote:
> > We conditionally support `__uint128_t`. This type won't work with the current implementation. I would prefer to support that type, or constrain function where `sizeof(_Tp) <= 8`. The availability is guarded by `_LIBCPP_HAS_NO_INT128`.
> >
> > Note that GCC already has a `__builtin_bswap128`, so maybe it's interesting to also add this to clang. I wouldn't mind looking into that myself if you don't want to.
> IMO:
> - Definitely add `_LIBCPP_UNREACHABLE();` at the end of this function.
> - If both Clang and GCC support `__builtin_bswap128` then we should just do that for `case 16`.
> - Otherwise, we should `case 16: return _Tp(byteswap(uint64_t(__val))) | (_Tp(byteswap(uint64_t(__val >> 64))) << 64); // TODO: __builtin_bswap128 when Clang supports it`.
Right now I'll just do the two 8 byte swaps. I think I will try to add `__builtin_bswap128` in a few days, but it can't be used right now anyways if I understand correctly. Would libc++15 be the earliest revision where this could be changed, assuming clang14 gets `__builtin_bswap128`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114074/new/
https://reviews.llvm.org/D114074
More information about the libcxx-commits
mailing list