[libcxx-commits] [PATCH] D114074: [libc++] Implement P1272R4 (std::byteswap)

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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)) {
----------------
philnik wrote:
> 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`?
> Would libc++15 be the earliest revision where this could be changed, assuming clang14 gets `__builtin_bswap128`?

You could make it conditional immediately (`#if __has_builtin(__builtin_bswap128)` — and in fact perhaps that should be part of this PR!), but yeah, my understanding of the new-as-of-last-year policy is that libc++14 needs to work with both Clang14 and Clang13; libc++15 needs to work with both Clang15 and Clang14; etc.


================
Comment at: libcxx/test/std/numerics/bit/byteswap.pass.cpp:46-47
+#ifdef TEST_128BIT_INT
+  test_num<__uint128_t>(make_128bit_int<__uint128_t>(0x0123456789ABCDEF, 0xFEDCBA9876543210),
+                        make_128bit_int<__uint128_t>(0x1032547698BADCFE, 0xEFCDAB8967452301));
+#endif
----------------
I'd feel better if the test case weren't constructed such that "nybble-swap each byte" was an acceptable implementation. :)
How about making the input `(0x0123456789ABCDEF, 0x13579BDF02468ACE)`?

FWIW, I also find the templated `make_128bit_int` quite verbose. I see that you're making it a template so that you don't have to hide it under an ifdef; but still, how about just this, right above the `return true`?
```
#ifndef _LIBCPP_HAS_NO_INT128
  auto before = __uint128_t(0x0123456789ABCDEF) << 64 | 0x13579BDF02468ACE;
  auto after = __uint128_t(0xCE8A4602DF9B5713) << 64 | 0xEFCDAB8967452301);
  test_num<__uint128_t>(before, after);
  test_num<__int128_t>(__int128_t(before), __int128_t(after));
#endif
```


================
Comment at: libcxx/test/std/numerics/bit/byteswap.pass.cpp:73
+  }
+#endif
+
----------------
Please add similar tests for `short, int, long, long long` and their unsigned counterparts.
(These can repeat the input data for `int{16,32,64}_t` AFAIC. We just want to make sure that we have covered whichever of `long` and `long long` is not `int64_t`.)


================
Comment at: libcxx/test/support/test_macros.h:207-209
+#if defined(_LIBCPP_VERSION) && !defined(_LIBCPP_HAS_NO_INT128)
+#  define TEST_128BIT_INT
+#endif
----------------
I bet this is overkill. Are we worried that some non-libc++ vendor might fail to define `byteswap` for `__int128_t`?

If we're messing with test macros related to 128-bit support, we might want to revisit libcxx/test/libcxx/input.output/filesystems/convert_file_time.pass.cpp too, I'm not sure.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114074/new/

https://reviews.llvm.org/D114074