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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

LGTM after the open comments have been addressed.



================
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:
> 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.
I also wanted to suggest to add `#if __has_builtin(__builtin_bswap128)` so I think it would be great to make that part of this PR. GCC already supports it so that means the code path will have test coverage. After it's available in Clang it will use it out-of-the-box. The `#else` branch can just use the fallback you currently use. That way we're compatible with implementations that don't have `__builtin_bswap128`.


================
Comment at: libcxx/include/__bit/byteswap.h:25
+
+template <integral _Tp>
+_LIBCPP_HIDE_FROM_ABI constexpr _Tp byteswap(_Tp __val) noexcept {
----------------
I still think it would be good to constrain the template on the maximum supported `sizeof(_Tp)`. I prefer a compile-time error over run-time bug or error.


================
Comment at: libcxx/test/support/test_macros.h:207-209
+#if defined(_LIBCPP_VERSION) && !defined(_LIBCPP_HAS_NO_INT128)
+#  define TEST_128BIT_INT
+#endif
----------------
Quuxplusone wrote:
> 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.
+1. Currently we have 121 occurrences of `_LIBCPP_HAS_NO_INT128` in our test directory.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114074