[libc++] std::byteswap support for _BitInt(N)#196512
Conversation
Replace the static_assert(sizeof(_Tp) == 0) failure for sizeof > 16 with a byte-reversal loop. std::byteswap now works for _BitInt(N) with N > 128 and any future wider integer type. The loop reads the value 8 bits at a time via right-shift + cast through unsigned char, and writes the bytes back in reverse order. Left-shift and bitwise-OR on signed integral types are well-defined modulo 2^width in C++23. For sizeof 1, 2, 4, 8, and 16 the existing __builtin_bswap* paths are unchanged. Part of the [_BitInt(N) libc++ effort](https://discourse.llvm.org/t/bitint-n-support-in-libc-investigations-possible-improvements-looking-for-guidance/90063). Assisted-by: Claude (Anthropic) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
For _BitInt(N) where N is not a multiple of CHAR_BIT, byteswap of the
storage representation moves padding bits into significant positions and
produces a value whose meaning is unspecified. Reject those cases with a
static_assert that fires when the type's value bits do not fill the
entire object representation.
The size-1 case is exempt from the check because no bytes move and no
padding gets relocated. bool, _BitInt(7), and similar types stay
identity.
Suggested by philnik in the libc++ _BitInt review thread ("byteswap
with padding: should definitely be rejected").
Assisted-by: Claude (Anthropic)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Two test files for the changes in the prior two commits. byteswap.bitint.pass.cpp covers byte-aligned _BitInt(N) widths from 8 up to __BITINT_MAXWIDTH__: builtins for sizeof <= 16, the new generic loop for sizeof > 16. The size-1 case includes _BitInt(7) (identity, despite the padding bit) so the asymmetry with _BitInt(13) is pinned. The wide case has a ramp pattern with a distinct byte at every position, which surfaces any off-by-one in the loop indexing, plus low-byte and high-byte spot checks. Wide-width tests gate on __BITINT_MAXWIDTH__ so non-x86 / non-RISC-V64 targets compile a smaller subset. byteswap.bitint.verify.cpp pins the static_assert added by the previous commit. Covers _BitInt(13), _BitInt(17), _BitInt(33), _BitInt(65) on all targets, plus _BitInt(129) and _BitInt(255) where the platform supports them. The expected-error-re pattern matches both Clang's "static assertion" and GCC's "static_assert" wordings. Assisted-by: Claude (Anthropic) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
6e678d4 to
acf2941
Compare
|
@llvm/pr-subscribers-libcxx Author: Xavier Roche (xroche) ChangesAdd a byte-reversal loop fallback for Reject Part of the _BitInt(N) libc++ effort. Assisted-by: Claude (Anthropic) Full diff: https://github.com/llvm/llvm-project/pull/196512.diff 5 Files Affected:
diff --git a/libcxx/include/__bit/byteswap.h b/libcxx/include/__bit/byteswap.h
index 7ce7e069b4142..2754265080a34 100644
--- a/libcxx/include/__bit/byteswap.h
+++ b/libcxx/include/__bit/byteswap.h
@@ -12,7 +12,10 @@
#include <__concepts/arithmetic.h>
#include <__config>
+#include <__cstddef/size_t.h>
+#include <climits>
#include <cstdint>
+#include <limits>
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header
@@ -25,24 +28,48 @@ _LIBCPP_BEGIN_NAMESPACE_STD
template <integral _Tp>
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr _Tp byteswap(_Tp __val) noexcept {
if constexpr (sizeof(_Tp) == 1) {
+ // Identity for size-1 types: no bytes move and no padding gets shuffled
+ // into significant positions. bool, char, and _BitInt(N <= CHAR_BIT)
+ // all land here.
return __val;
- } else if constexpr (sizeof(_Tp) == 2) {
- return __builtin_bswap16(__val);
- } else if constexpr (sizeof(_Tp) == 4) {
- return __builtin_bswap32(__val);
- } else if constexpr (sizeof(_Tp) == 8) {
- return __builtin_bswap64(__val);
+ } else {
+ // Reject types whose value bits do not fill the entire object
+ // representation (e.g. _BitInt(13) has 3 padding bits in 2 bytes of
+ // storage). The byte-level builtins below would swap those padding
+ // bits into significant positions, and the resulting value's meaning
+ // is unspecified. The size-1 case above is exempt because no bytes
+ // move.
+ static_assert(numeric_limits<_Tp>::digits + numeric_limits<_Tp>::is_signed == sizeof(_Tp) * CHAR_BIT,
+ "std::byteswap requires a type whose value bits fill the entire "
+ "object representation; types like _BitInt(N) where N is not a "
+ "multiple of CHAR_BIT have padding bits and are rejected");
+ if constexpr (sizeof(_Tp) == 2) {
+ return __builtin_bswap16(__val);
+ } else if constexpr (sizeof(_Tp) == 4) {
+ return __builtin_bswap32(__val);
+ } else if constexpr (sizeof(_Tp) == 8) {
+ return __builtin_bswap64(__val);
# if _LIBCPP_HAS_INT128
- } else if constexpr (sizeof(_Tp) == 16) {
+ } else if constexpr (sizeof(_Tp) == 16) {
# if __has_builtin(__builtin_bswap128)
- return __builtin_bswap128(__val);
+ return __builtin_bswap128(__val);
# else
- return (static_cast<_Tp>(byteswap(static_cast<uint64_t>(__val))) << 64) |
- static_cast<_Tp>(byteswap(static_cast<uint64_t>(__val >> 64)));
+ return (static_cast<_Tp>(byteswap(static_cast<uint64_t>(__val))) << 64) |
+ static_cast<_Tp>(byteswap(static_cast<uint64_t>(__val >> 64)));
# endif // __has_builtin(__builtin_bswap128)
# endif // _LIBCPP_HAS_INT128
- } else {
- static_assert(sizeof(_Tp) == 0, "byteswap is unimplemented for integral types of this size");
+ } else {
+ // Generic byte-reversal for wide integer types (e.g. _BitInt(N) with
+ // N > 128). Reads the value 8 bits at a time and writes the bytes
+ // back in reverse order. Left-shift on signed integral types is
+ // well-defined modulo 2^width since C++20.
+ _Tp __result = 0;
+ for (size_t __i = 0; __i < sizeof(_Tp); ++__i) {
+ __result |= static_cast<_Tp>(static_cast<unsigned char>(__val >> (__i * CHAR_BIT)))
+ << ((sizeof(_Tp) - 1 - __i) * CHAR_BIT);
+ }
+ return __result;
+ }
}
}
diff --git a/libcxx/test/libcxx/transitive_includes/cxx23.csv b/libcxx/test/libcxx/transitive_includes/cxx23.csv
index c5cc61f06678c..073f698786117 100644
--- a/libcxx/test/libcxx/transitive_includes/cxx23.csv
+++ b/libcxx/test/libcxx/transitive_includes/cxx23.csv
@@ -42,6 +42,7 @@ barrier ctime
barrier limits
barrier ratio
barrier version
+bit climits
bit cstdint
bit limits
bit version
diff --git a/libcxx/test/libcxx/transitive_includes/cxx26.csv b/libcxx/test/libcxx/transitive_includes/cxx26.csv
index 253cf64703076..5b4ba7918ae96 100644
--- a/libcxx/test/libcxx/transitive_includes/cxx26.csv
+++ b/libcxx/test/libcxx/transitive_includes/cxx26.csv
@@ -40,6 +40,7 @@ barrier ctime
barrier limits
barrier ratio
barrier version
+bit climits
bit cstdint
bit limits
bit version
diff --git a/libcxx/test/std/numerics/bit/byteswap.bitint.pass.cpp b/libcxx/test/std/numerics/bit/byteswap.bitint.pass.cpp
new file mode 100644
index 0000000000000..ce432c1432c15
--- /dev/null
+++ b/libcxx/test/std/numerics/bit/byteswap.bitint.pass.cpp
@@ -0,0 +1,131 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+
+// <bit>
+
+// std::byteswap for _BitInt(N).
+//
+// Byte-aligned widths (N % CHAR_BIT == 0) work via the existing builtins
+// for sizeof <= 16 and via the new generic loop for sizeof > 16. Non-byte-
+// aligned widths are rejected by static_assert; that case is covered in
+// byteswap.bitint.verify.cpp.
+
+#include <bit>
+#include <cassert>
+#include <cstdint>
+
+#include "test_macros.h"
+
+#if TEST_HAS_EXTENSION(bit_int)
+
+template <class T>
+constexpr void test_roundtrip(T v) {
+ assert(std::byteswap(std::byteswap(v)) == v);
+ ASSERT_SAME_TYPE(decltype(std::byteswap(v)), T);
+ ASSERT_NOEXCEPT(std::byteswap(v));
+}
+
+constexpr bool test() {
+ // sizeof == 1: identity. The size-1 branch returns the input unchanged
+ // and the padding-bit static_assert is bypassed -- no bytes move and no
+ // padding gets shuffled into significant positions, so non-byte-aligned
+ // widths up to CHAR_BIT (e.g. _BitInt(7)) are also identity.
+ assert(std::byteswap(static_cast<unsigned _BitInt(8)>(0xAB)) == static_cast<unsigned _BitInt(8)>(0xAB));
+ test_roundtrip<unsigned _BitInt(8)>(0xAB);
+ test_roundtrip<signed _BitInt(8)>(0x12);
+ // _BitInt(7) signed has a padding bit but stays identity at sizeof == 1.
+ assert(std::byteswap(static_cast<signed _BitInt(7)>(42)) == static_cast<signed _BitInt(7)>(42));
+ assert(std::byteswap(static_cast<unsigned _BitInt(7)>(42)) == static_cast<unsigned _BitInt(7)>(42));
+
+ // sizeof == 2: __builtin_bswap16
+ assert(std::byteswap(static_cast<unsigned _BitInt(16)>(0xCDEF)) == static_cast<unsigned _BitInt(16)>(0xEFCD));
+ test_roundtrip<unsigned _BitInt(16)>(0xCDEF);
+ test_roundtrip<signed _BitInt(16)>(0x1234);
+
+ // sizeof == 4: __builtin_bswap32
+ assert(std::byteswap(static_cast<unsigned _BitInt(32)>(0x01234567U)) ==
+ static_cast<unsigned _BitInt(32)>(0x67452301U));
+ test_roundtrip<unsigned _BitInt(32)>(0x01234567U);
+ test_roundtrip<signed _BitInt(32)>(0x01234567);
+
+ // sizeof == 8: __builtin_bswap64
+ assert(std::byteswap(static_cast<unsigned _BitInt(64)>(0x0123456789ABCDEFULL)) ==
+ static_cast<unsigned _BitInt(64)>(0xEFCDAB8967452301ULL));
+ test_roundtrip<unsigned _BitInt(64)>(0x0123456789ABCDEFULL);
+ test_roundtrip<signed _BitInt(64)>(0x0123456789ABCDEFLL);
+
+# if __BITINT_MAXWIDTH__ >= 128
+ // sizeof == 16: __builtin_bswap128 (or 2x bswap64 fallback). Same path
+ // as the existing __int128_t / __uint128_t coverage in byteswap.pass.cpp.
+ unsigned _BitInt(128) v128 =
+ (static_cast<unsigned _BitInt(128)>(0x0123456789ABCDEFULL) << 64) |
+ static_cast<unsigned _BitInt(128)>(0x13579BDF02468ACEULL);
+ test_roundtrip<unsigned _BitInt(128)>(v128);
+ test_roundtrip<signed _BitInt(128)>(static_cast<signed _BitInt(128)>(v128));
+# endif
+
+# if __BITINT_MAXWIDTH__ >= 256
+ // sizeof == 32: hits the new generic loop fallback.
+ unsigned _BitInt(256) v256 =
+ (static_cast<unsigned _BitInt(256)>(0xDEADBEEFCAFEBABEULL) << 128) |
+ (static_cast<unsigned _BitInt(256)>(0x1234567890ABCDEFULL) << 64) |
+ static_cast<unsigned _BitInt(256)>(0xFEDCBA9876543210ULL);
+ test_roundtrip<unsigned _BitInt(256)>(v256);
+ test_roundtrip<signed _BitInt(256)>(static_cast<signed _BitInt(256)>(v256));
+
+ // Spot check for the wide loop: low byte of input must end up as the
+ // high byte of the output, and high byte of input as the low byte.
+ unsigned _BitInt(256) lo_only = 0xAB;
+ auto lo_swapped = std::byteswap(lo_only);
+ assert(static_cast<unsigned char>(lo_swapped >> ((sizeof(lo_swapped) - 1) * CHAR_BIT)) == 0xAB);
+ unsigned _BitInt(256) hi_only =
+ static_cast<unsigned _BitInt(256)>(0xCD) << ((sizeof(unsigned _BitInt(256)) - 1) * CHAR_BIT);
+ auto hi_swapped = std::byteswap(hi_only);
+ assert(static_cast<unsigned char>(hi_swapped) == 0xCD);
+
+ // Mid-value test: distinct byte at every position so an off-by-one in
+ // the loop indexing surfaces directly. Build 0x00010203...1F at bytes
+ // 0..31 then verify byteswap reverses the byte sequence.
+ unsigned _BitInt(256) ramp = 0;
+ for (int __i = 0; __i < 32; ++__i)
+ ramp |= static_cast<unsigned _BitInt(256)>(__i) << (__i * CHAR_BIT);
+ auto ramp_swapped = std::byteswap(ramp);
+ for (int __i = 0; __i < 32; ++__i)
+ assert(static_cast<unsigned char>(ramp_swapped >> ((31 - __i) * CHAR_BIT)) == __i);
+# endif
+
+# if __BITINT_MAXWIDTH__ >= 1024
+ // Larger width still in the generic loop.
+ unsigned _BitInt(1024) v1024 = static_cast<unsigned _BitInt(1024)>(0xAB) << ((128 - 1) * CHAR_BIT);
+ test_roundtrip<unsigned _BitInt(1024)>(v1024);
+# endif
+
+# if __BITINT_MAXWIDTH__ >= 4096
+ // Largest width tested. Picked to cover the upper end of what the
+ // dev-branch experiments exercised; values larger than this take a
+ // long time to constexpr-evaluate without adding much coverage.
+ unsigned _BitInt(4096) v4096 = static_cast<unsigned _BitInt(4096)>(0xAB) << ((512 - 1) * CHAR_BIT);
+ test_roundtrip<unsigned _BitInt(4096)>(v4096);
+# endif
+
+ return true;
+}
+
+int main(int, char**) {
+ test();
+ static_assert(test());
+ return 0;
+}
+
+#else
+
+int main(int, char**) { return 0; }
+
+#endif
diff --git a/libcxx/test/std/numerics/bit/byteswap.bitint.verify.cpp b/libcxx/test/std/numerics/bit/byteswap.bitint.verify.cpp
new file mode 100644
index 0000000000000..7ff60a9c94463
--- /dev/null
+++ b/libcxx/test/std/numerics/bit/byteswap.bitint.verify.cpp
@@ -0,0 +1,78 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+
+// <bit>
+
+// std::byteswap rejects _BitInt(N) where N is not a multiple of CHAR_BIT.
+//
+// The byte-level builtins (and the generic loop fallback) treat the
+// storage representation as the value, so for a type with padding bits
+// they would shuffle padding into significant positions and produce a
+// value whose semantic meaning is unspecified. The static_assert added
+// in [libc++] Reject byteswap of types with padding bits pins the
+// rejection so the diagnostic does not regress silently into a wrong
+// value.
+
+#include <bit>
+
+#include "test_macros.h"
+
+#if TEST_HAS_EXTENSION(bit_int)
+
+void f_unsigned_13() {
+ unsigned _BitInt(13) v = 0;
+ // expected-error-re@*:* {{{{(static assertion|static_assert)}} failed{{.*}}"std::byteswap requires{{.*}}"}}
+ (void)std::byteswap(v);
+}
+
+void f_signed_13() {
+ signed _BitInt(13) v = 0;
+ // expected-error-re@*:* {{{{(static assertion|static_assert)}} failed{{.*}}"std::byteswap requires{{.*}}"}}
+ (void)std::byteswap(v);
+}
+
+void f_unsigned_17() {
+ unsigned _BitInt(17) v = 0;
+ // expected-error-re@*:* {{{{(static assertion|static_assert)}} failed{{.*}}"std::byteswap requires{{.*}}"}}
+ (void)std::byteswap(v);
+}
+
+void f_signed_33() {
+ signed _BitInt(33) v = 0;
+ // expected-error-re@*:* {{{{(static assertion|static_assert)}} failed{{.*}}"std::byteswap requires{{.*}}"}}
+ (void)std::byteswap(v);
+}
+
+void f_unsigned_65() {
+ unsigned _BitInt(65) v = 0;
+ // expected-error-re@*:* {{{{(static assertion|static_assert)}} failed{{.*}}"std::byteswap requires{{.*}}"}}
+ (void)std::byteswap(v);
+}
+
+# if __BITINT_MAXWIDTH__ >= 129
+// _BitInt(129) is wider than __int128 and only available where
+// __BITINT_MAXWIDTH__ supports it (x86 / RISC-V 64). The wide-type
+// generic loop also relies on the rejection; cover it here.
+void f_unsigned_129() {
+ unsigned _BitInt(129) v = 0;
+ // expected-error-re@*:* {{{{(static assertion|static_assert)}} failed{{.*}}"std::byteswap requires{{.*}}"}}
+ (void)std::byteswap(v);
+}
+
+void f_signed_255() {
+ signed _BitInt(255) v = 0;
+ // expected-error-re@*:* {{{{(static assertion|static_assert)}} failed{{.*}}"std::byteswap requires{{.*}}"}}
+ (void)std::byteswap(v);
+}
+# endif
+
+#else
+// expected-no-diagnostics
+#endif
|
philnik777
left a comment
There was a problem hiding this comment.
Can't we just defer to __builtin_bswapg?
Address philnik's review on PR llvm#196512. The fast path is now a single call to __builtin_bswapg, which Clang added in November 2025 (PR llvm#162433). The builtin handles every standard integer type plus _BitInt(N) where N is a multiple of 16 (or N == 8 for the size-1 identity), and Clang itself diagnoses non-multiple-of-16 _BitInt with a clear "must be a multiple of 16 bits for byte swapping" message. That replaces the static_assert and the size-1 / wide-loop chain we shipped earlier. The previous implementation stays as a fallback under __has_builtin(__builtin_bswapg) for compilers that predate Clang 22. libc++ supports Clang 21+, and __builtin_bswapg is only in Clang 22+, so the fallback is reachable today on Clang 21. If we drop Clang 21 support (a separate libc++ policy decision) the fallback can go. Side effects: - The new <__cstddef/size_t.h> / <climits> / <limits> includes only apply on the fallback path now and are guarded behind the same __has_builtin check. - The bit climits transitive-include row added in the previous round goes away on Clang 22+; reverted in cxx23.csv and cxx26.csv. The fallback path still pulls climits in transitively but Clang 21 builds aren't measured by the in-tree CI. - byteswap.bitint.verify.cpp's expected-error-re regex now matches either spelling: the static_assert message on the fallback path and the "_BitInt type ... must be a multiple of 16 bits" message on the bswapg path. Verified locally: clang-20 fallback fires the static_assert, in-tree clang-23 bswapg fires Clang's diagnostic. Verified locally on Clang 23 (bswapg path) and Clang 20 (fallback path) with the full bit suite (18/18) and the transitive-includes test (114/114) passing on both. Cross-arch QEMU on 12 targets all green. Assisted-by: Claude (Anthropic) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Done! However, I kept the original implementation as a fallback as the minimum compiler version for libc++ is Clang 21 - should we bump it along the PR or wait to remove the conditionals later ? |
…guard The __has_builtin(__builtin_bswapg) guard in __bit/byteswap.h had the fallback path's three extra includes (<__cstddef/size_t.h>, <climits>, <limits>) inside the guard. That made the transitive include set differ between Clang 22+ (no climits in transitive) and Clang 21 / GCC (climits in transitive), so the single transitive_includes CSV could only match one. AIX-64-bit and android-x86-NDK CI use the fallback path and surfaced the mismatch. Move the three includes outside the guard so both paths share the same transitive set, and add the bit climits row back to cxx23.csv and cxx26.csv. Cost on the bswapg path is negligible (those headers are already in <bit>'s transitive set via other paths). Verified locally: in-tree clang (Clang 23, bswapg path) and g++-13 (targeted fallback compile) both accept the result. transitive_includes test 114/114 passes, bit suite 18/18 passes. Assisted-by: Claude (Anthropic) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Neither. We should just disable the tests for compilers that are too old. There is very little value in an implementation that we'll drop in a few months for a new, relatively experimental feature. |
Some Clang 22 builds have a constexpr-eval bug for __builtin_bswapg on 1-bit operands (i.e. bool), fixed in commit f541056 after 22.1.0 was cut. CI surfaced this as the existing byteswap.pass.cpp:100 static_assert(test()) failing on the stage2 (clang-22) configuration: test() calls byteswap on bool, and bool flows through bswapg without the size-1 short circuit. Move the size-1 identity case before the bswapg dispatch so size-1 types (bool, char, _BitInt(N <= CHAR_BIT)) never reach the bswapg constexpr path. Defensive against the bug regardless of Clang patch level. The fast path's general behaviour is unchanged for sizeof > 1. Verified locally with the in-tree clang (Clang 23, bswapg path) and clang-20 (fallback path); bit suite 18/18, the existing byteswap.pass.cpp still passes. Assisted-by: Claude (Anthropic) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…swapg Per philnik's review feedback on PR llvm#196512: "We should just disable the tests for compilers that are too old. There is very little value in an implementation that we'll drop in a few months for a new, relatively experimental feature." This drops the hand-rolled padding-bit static_assert and the wide generic-loop fallback from the previous commits. The bswapg path on Clang 22+ delivers the same behavior (rejection diagnostic on non-multiple-of-16 widths, support for arbitrary multiples of 16), and `// UNSUPPORTED: clang-21` on the new tests keeps the older compiler off the path entirely. Side effect: byteswap.h no longer pulls in `<climits>`, `<limits>`, or `<__cstddef/size_t.h>`, which the transitive_includes CSVs record. Assisted-by: Claude (Anthropic) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address review feedback on PR llvm#196512: 1. SFINAE-friendly: add a `requires` clause that admits sizeof == 1 types and integral types whose width is a multiple of 16. `requires { std::byteswap(_BitInt(13)()); }` now resolves to false instead of triggering a hard error from __builtin_bswapg's sema check. 2. Drop the size-1 special-case workaround comment that framed the identity branch as a workaround for the Clang 22.1.x bswapg-bool constexpr-eval bug. The size-1 dispatch is structural: bswapg rejects _BitInt(7), _BitInt(13), etc. for "not a multiple of 16 bits" regardless of Clang version, so the SFINAE constraint admits sizeof == 1 types and the function body routes them to the identity branch. The Clang 22.1.x bool bug is masked by this dispatch as a side effect, but the dispatch itself isn't a workaround. 3. Merge _BitInt(N) test cases from byteswap.bitint.pass.cpp into byteswap.pass.cpp, replace the .verify.cpp's expected-error checks with `static_assert(!has_byteswap<_BitInt(13)>)` and friends, and delete the two now-redundant .bitint test files. Assisted-by: Claude (Anthropic) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
philnik777
left a comment
There was a problem hiding this comment.
Mostly looks good. I'd like to consider the design question a bi5 though. If we don't find a good use-case I think we should reject for now.
| #if _LIBCPP_STD_VER >= 23 | ||
|
|
||
| template <integral _Tp> | ||
| requires(sizeof(_Tp) == 1 || (numeric_limits<_Tp>::digits + numeric_limits<_Tp>::is_signed) % 16 == 0) |
There was a problem hiding this comment.
I'm wondering whether we should accept _BitInts that are between 1 and 7. Supporting bool feels weird already, but that's required by the standard. Any thoughts on this?
There was a problem hiding this comment.
After digging,
- The pros: would be standard conformant (Constraints: T models integral.; Mandates: T does not have padding bits.)
- The cons: as far as I can see, libstdc++ and MSVC accept them
References:
gcc/libstdc++-v3/include/std/bit, lines 135-150 (master branch):
template<integral _Tp>
[[nodiscard]]
constexpr _Tp
byteswap(_Tp __value) noexcept
{
if constexpr (sizeof(_Tp) == 1)
return __value;
...
} STL/stl/inc/bit, lines 103-122 (main branch):
template <class _Ty>
requires is_integral_v<_Ty>
_NODISCARD constexpr _Ty byteswap(const _Ty _Val) noexcept {
if constexpr (sizeof(_Ty) == 1) {
return _Val;
} else if constexpr (sizeof(_Ty) == 2) {Unsure which path would be the best ? We could add a non-conformance warning comment also but hmmm.
There was a problem hiding this comment.
Neither of them have _BitInt support, so that's not relevant. Their integral/is_integral_v constraints don't allow for it AFAIK. I think I'd rather be conservative and disallow 1-7 for now. We can always allow for them later if there is a compelling use-case.
Actually, we have to reject bool according to the wording, since it does contain padding bits AFAIK. Given that we currently accept it I wouldn't want to change that in this PR, but I do think we should reject 1-7 _BitInts and maybe file an LWG issue about this. After an answer there we can update the constraints here.
IoW, let's go back to integral as the only constraint and add a static_assert to reject padding bits in anything other than bool.
There was a problem hiding this comment.
Actually, we have to reject
boolaccording to the wording, since it does contain padding bits AFAIK.
Hmm, IIUC [basic.fundamental]/10 indicates that bool has padding bits iff its underlying type (which is unsigned char on most platforms) has. I agree on that we should submit an LWG issue for this as bool wasn't discussed in P1272R4.
I strongly think that the "constraint" should be numeric_limits<_Tp>::digits + numeric_limits<_Tp>::is_signed == sizeof(_Tp) * 8 (or we may spell the magic number 8 as CHAR_BIT or numeric_limits<unsigned char>::digits).
@xroche I think you need to verify that byteswap doesn't accept _BitInt(48) (or _BitInt(80) or something like). As sizeof(_BitInt(48)) == 8 on known platforms, i.e. it has 64 bits in its object representation where 16 of them are padding bits.
I wonder whether it would be better to use static_assert here as the standard wording uses Mandates.
There was a problem hiding this comment.
Actually, we have to reject
boolaccording to the wording, since it does contain padding bits AFAIK.Hmm, IIUC [basic.fundamental]/10 indicates that
boolhas padding bits iff its underlying type (which isunsigned charon most platforms) has.
Where did you get that from?
I strongly think that the "constraint" should be
numeric_limits<_Tp>::digits + numeric_limits<_Tp>::is_signed == sizeof(_Tp) * 8(or we may spell the magic number 8 asCHAR_BITornumeric_limits<unsigned char>::digits).
Why? The standard clearly says that the constraint is on integral types, while the mandate is for non-padding bytes.
@xroche I think you need to verify that
byteswapdoesn't accept_BitInt(48)(or_BitInt(80)or something like). Assizeof(_BitInt(48)) == 8on known platforms, i.e. it has 64 bits in its object representation where 16 of them are padding bits.
I guess we'd need to do that to be strictly conforming. IMO we should file an LWG issue about this though, since Allowing byteswaps on any (at least multiple of 16 bits) _BitInt seems rather useful to me.
There was a problem hiding this comment.
Actually, we have to reject
boolaccording to the wording, since it does contain padding bits AFAIK.Hmm, IIUC [basic.fundamental]/10 indicates that
boolhas padding bits iff its underlying type (which isunsigned charon most platforms) has.Where did you get that from?
The standard wording currently says:
Type
boolis a distinct type that has the same object representation, value representation, and alignment requirements as an implementation-defined unsigned integer type.
To me, "same object representation, value representation" should imply same count of bits in object representation and value representation.
I strongly think that the "constraint" should be
numeric_limits<_Tp>::digits + numeric_limits<_Tp>::is_signed == sizeof(_Tp) * 8(or we may spell the magic number 8 asCHAR_BITornumeric_limits<unsigned char>::digits).Why? The standard clearly says that the constraint is on integral types, while the mandate is for non-padding bytes.
Sorry for confusing. I said "constraint" because currently this PR places this prohibition in the requires-clause.
There was a problem hiding this comment.
Actually, we have to reject
boolaccording to the wording, since it does contain padding bits AFAIK.Hmm, IIUC [basic.fundamental]/10 indicates that
boolhas padding bits iff its underlying type (which isunsigned charon most platforms) has.Where did you get that from?
The standard wording currently says:
Type
boolis a distinct type that has the same object representation, value representation, and alignment requirements as an implementation-defined unsigned integer type.To me, "same object representation, value representation" should imply same count of bits in object representation and value representation.
Sure, but that doesn't actually tell be the underlying type. "unsigned integer type" includes extended ones, so it could just as well be unsigned _BitInt(1) (which it is arguably more likely to be, given that 0b0 and 0b1 are the only valid bit patterns according to any implementation I'm aware of, which clearly doesn't require more than one bit).
There was a problem hiding this comment.
Just pushed an update with both pieces of feedback. The function is constrained on integral only; the non-padding-bits check is now a static_assert in the body, matching [bit.byteswap]/Mandates.
The static_assert also now uses digits + is_signed == sizeof(T) * CHAR_BIT rather than a modulo of 16. This catches frederick's _BitInt(48) case, plus _BitInt(80), _BitInt(192), and similar widths where the value bits are a multiple of 16 but the storage rounds up to the next power of two with padding at the top. The previous form silently admitted those (oops)
"bool" is carved out by name to keep its grandfathered behavior, until the LWG issue resolves whether it should be admitted in principle...
I split the tests (feedback welcome if this is not desirable):
- Positive _BitInt cases stay in byteswap.pass.cpp.
- A new byteswap.verify.cpp next to it covers all rejected shapes
There was a problem hiding this comment.
LWG4583 (for unusual integer types) is submitted.
There was a problem hiding this comment.
Added a short comment before the assert referencing LWG4583 😸
Not a use-case, but [my understanding] is the Mandate exists to prevent padding bits from being shuffled into value-bit positions; at sizeof == 1 no bytes move, so a padding bit can never land where a value bit was, and the result is always the input. That's why bool might be admitted despite the strict Mandate reading, and _BitInt(N<8) could fall in exactly the same category ? |
Address review feedback from philnik777 and frederick-vs-ja on PR llvm#196512: 1. Drop the `requires` clause and constrain only on `integral`, per `[bit.byteswap]/Constraints`. The previous `requires` form conflated the Constraints clause (SFINAE-friendly) with the Mandates clause (hard error per `[structure.specifications]/3.2`). 2. Enforce the Mandate "T does not have padding bits" via a `static_assert` inside the function body. The predicate is `digits + is_signed == sizeof(T) * CHAR_BIT`, which catches the `_BitInt(48)`, `_BitInt(80)`, `_BitInt(192)`, etc. cases where the value bit width is a multiple of 16 but the storage size rounds up to the next power of two with padding at the top. 3. `bool` is carved out by name (`is_same_v<remove_cv_t<T>, bool>`) to preserve the grandfathered behavior. Whether bool's padding-bit status should make it ill-formed is a wording question for LWG. Tests are split into two files: - `byteswap.pass.cpp` keeps the positive `_BitInt` cases where `digits + is_signed == sizeof * CHAR_BIT` holds (`_BitInt(8/16/32/ 64/128/256/...)`). - `byteswap.verify.cpp` (re-added next to `byteswap.pass.cpp`) covers the rejected shapes: sub-byte (`_BitInt(1/7)`), non-byte-aligned (`_BitInt(13/17/33/65)`), and byte-aligned-with-storage-padding (`_BitInt(24/40/48/56/80/96/112/192)`). Assisted-by: Claude (Anthropic) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
LWG 4583 (https://cplusplus.github.io/LWG/issue4583), submitted while this PR was under review and motivated by it, proposes relaxing the padding-bit Mandate to allow byte-aligned padding (e.g. _BitInt(48) where the 2 high storage bytes are entirely padding). The rejection here is strict per the current standard wording and intentionally remains so until LWG 4583 resolves. A brief comment in the source points future readers at the issue. No functional change. Assisted-by: Claude (Anthropic) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
philnik777
left a comment
There was a problem hiding this comment.
Basically looks good. Just a small comment.
| if constexpr (sizeof(_Tp) == 1) { | ||
| return __val; |
There was a problem hiding this comment.
Why do we need this if we have __builtin_bswapg?
There was a problem hiding this comment.
For clang-21 compatibility; otherwise we'll break byteswap(__int128) which is currently working
I can remove the support for clang-21, but this will be a regression 🤔
There was a problem hiding this comment.
How is this related to __int128?
There was a problem hiding this comment.
I misread the code; it indeed breaks on clang 21/22 but because of constexpr-eval bug in __builtin_bswapg(bool) (fix f541056, on main only, not in 22 branch)
Basically this is to prevent static_assert(std::byteswap(true) == true) to fail
|
Thanks for the reviews! I'll let you merge @philnik777 |
|
@philnik777 I realize I didn't document anything in the release notes related to the _BitInt works. Is this something worth doing as a follow-up ? |
|
It's a bit unfortunate that LWG4583 is rejected as "NAD, paper needed". |
The silver lining is that the reply wasn't "wrong, this is intentional" or "won't fix" but more "please provide a paper" <shrug> Do you plan to work on the paper ? We probably need to re-admit _BitInt to std::byteswap (P3666R4 8.2.9 currently excludes it) and relax the padding-bit Mandate so _BitInt(48), _BitInt(80) are accepted, but <sigh> |
P3666R4 ("Bit-precise integers", LEWG-approved at Brno 2026-06) takes the
position that libc++ should not support _BitInt(N) in <bit>, <charconv>,
<format>, std::hash, std::cmp_*, <numeric>'s gcd/lcm/midpoint/saturation.
Type traits, numeric_limits and <cmath> conversion paths stay supported.
libc++ already ships experimental _BitInt support in several gated areas
(PR llvm#196512 for byteswap, partial coverage from llvm#185027). Add
_LIBCPP_ENABLE_BITINT_EXTENSIONS as the user-facing opt-in macro and
_LIBCPP_HAS_BITINT_EXTENSIONS as the internal 0/1 flag. Off by default
to match P3666's stance. Opt in to keep the experimental coverage.
The gate flows through a helper trait __admits_bitint_extension_v in
<__type_traits/is_bit_precise_integer.h>. integer_traits.h folds the
trait into __is_signed_integer_v / __is_unsigned_integer_v, which
cascades to every consumer of __signed_integer / __unsigned_integer
(bit_log2, cmp_*, saturation, format_arg_store, mdspan). byteswap,
hash, to_chars, from_chars, gcd, lcm, midpoint use is_integral or
std::integral directly, so each picks up the gate via an explicit
constraint.
Existing positive _BitInt tests opt back in via
ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_BITINT_EXTENSIONS.
New libcxx-internal tests under libcxx/test/libcxx/bitint/:
- extensions_disabled.verify.cpp: under default, every gated facility
rejects _BitInt across signedness, widths 1/8/13/32/64/128 and cv
variants. Standard integers and traits stay accepted.
- extensions_enabled.compile.pass.cpp: under the macro, SFINAE-friendly
detection idioms report the same facilities as callable.
Discussion: https://discourse.llvm.org/t/implementing-p3666r4-bit-precise-integers-in-libc/91070
Follow-up surfaces not gated here: std::atomic<_BitInt(N)>'s integral
specialization is a larger change worth a separate PR; std::random's
seed_seq integer constructor is lower priority.
Assisted-by: Claude (Anthropic)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
P3666R4 ("Bit-precise integers", LEWG-approved at Brno 2026-06) takes the
position that libc++ should not support _BitInt(N) in <bit>, <charconv>,
<format>, std::hash, std::cmp_*, <numeric>'s gcd/lcm/midpoint/saturation.
Type traits, numeric_limits and <cmath> conversion paths stay supported.
libc++ already ships experimental _BitInt support in several gated areas
(PR llvm#196512 for byteswap, partial coverage from llvm#185027). Add
_LIBCPP_ENABLE_BITINT_EXTENSIONS as the user-facing opt-in macro and
_LIBCPP_HAS_BITINT_EXTENSIONS as the internal 0/1 flag. Off by default
to match P3666's stance. Opt in to keep the experimental coverage.
The gate flows through a helper trait __admits_bitint_extension_v in
<__type_traits/is_bit_precise_integer.h>. integer_traits.h folds the
trait into __is_signed_integer_v / __is_unsigned_integer_v, which
cascades to every consumer of __signed_integer / __unsigned_integer
(bit_log2, cmp_*, saturation, format_arg_store, mdspan). byteswap,
hash, to_chars, from_chars, gcd, lcm, midpoint use is_integral or
std::integral directly, so each picks up the gate via an explicit
constraint.
Existing positive _BitInt tests opt back in via
ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_BITINT_EXTENSIONS.
New libcxx-internal tests under libcxx/test/libcxx/bitint/:
- extensions_disabled.verify.cpp: under default, every gated facility
rejects _BitInt across signedness, widths 1/8/13/32/64/128 and cv
variants. Standard integers and traits stay accepted.
- extensions_enabled.compile.pass.cpp: under the macro, SFINAE-friendly
detection idioms report the same facilities as callable.
Discussion: https://discourse.llvm.org/t/implementing-p3666r4-bit-precise-integers-in-libc/91070
Follow-up surfaces not gated here: std::atomic<_BitInt(N)>'s integral
specialization is a larger change worth a separate PR; std::random's
seed_seq integer constructor is lower priority.
Assisted-by: Claude (Anthropic)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
P3666R4 ("Bit-precise integers", LEWG-approved at Brno 2026-06) takes the
position that libc++ should not support _BitInt(N) in <bit>, <charconv>,
<format>, std::hash, std::cmp_*, <numeric>'s gcd/lcm/midpoint/saturation.
Type traits, numeric_limits and <cmath> conversion paths stay supported.
libc++ already ships experimental _BitInt support in several gated areas
(PR llvm#196512 for byteswap, partial coverage from llvm#185027). Add
_LIBCPP_ENABLE_BITINT_EXTENSIONS as the user-facing opt-in macro and
_LIBCPP_HAS_BITINT_EXTENSIONS as the internal 0/1 flag. Off by default
to match P3666's stance. Opt in to keep the experimental coverage.
The gate flows through a helper trait __admits_bitint_extension_v in
<__type_traits/is_bit_precise_integer.h>. integer_traits.h folds the
trait into __is_signed_integer_v / __is_unsigned_integer_v, which
cascades to every consumer of __signed_integer / __unsigned_integer
(bit_log2, cmp_*, saturation, format_arg_store, mdspan). byteswap,
hash, to_chars, from_chars, gcd, lcm, midpoint use is_integral or
std::integral directly, so each picks up the gate via an explicit
constraint.
Existing positive _BitInt tests opt back in via
ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_BITINT_EXTENSIONS.
New libcxx-internal tests under libcxx/test/libcxx/bitint/:
- extensions_disabled.verify.cpp: under default, every gated facility
rejects _BitInt across signedness, widths 1/8/13/32/64/128 and cv
variants. Standard integers and traits stay accepted.
- extensions_enabled.compile.pass.cpp: under the macro, SFINAE-friendly
detection idioms report the same facilities as callable.
Discussion: https://discourse.llvm.org/t/implementing-p3666r4-bit-precise-integers-in-libc/91070
Follow-up surfaces not gated here: std::atomic<_BitInt(N)>'s integral
specialization is a larger change worth a separate PR; std::random's
seed_seq integer constructor is lower priority.
Assisted-by: Claude (Anthropic)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The byteswap padding-bit test added in llvm#196512 used unsigned _BitInt(96), which llvm#205295 had to skip on 32-bit x86: the i386 System V ABI aligns _BitInt to 4 bytes and packs _BitInt(96) into exactly 12 bytes with no padding, so std::byteswap accepts it and the expected diagnostic never fires. The skip used !defined(__i386__) as a stand-in because the object layout that drives the result is not visible to the preprocessor. Use unsigned _BitInt(72) instead. Its 9 value bytes round up to 12 on 32-bit x86 and 16 everywhere else, so the type always carries padding and std::byteswap always rejects it. The case now runs on every target, including 32-bit x86, with no target check. Assisted-by: Claude (Anthropic) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add a byte-reversal loop fallback for
std::byteswapwhensizeof(T) > 16,so the function works for
_BitInt(N)withN > 128and any future widerinteger type. Without it, those calls hit
static_assert(sizeof(_Tp) == 0)and fail to compile.
Reject
_BitInt(N)whereNis not a multiple ofCHAR_BIT. The existing__builtin_bswap{16,32,64,128}paths swap the storage representationincluding padding bits, and the resulting value's meaning is unspecified.
A new
static_assertcatches that case and reports it. Size-1 types areexempt from the check, since no bytes move there.
Part of the _BitInt(N) libc++ effort.
Assisted-by: Claude (Anthropic)