Skip to content

Commit

Permalink
Merge bitcoin#29263: serialization: c++20 endian/byteswap/clz moderni…
Browse files Browse the repository at this point in the history
…zation

86b7f28 serialization: use internal endian conversion functions (Cory Fields)
432b18c serialization: detect byteswap builtins without autoconf tests (Cory Fields)
297367b crypto: replace CountBits with std::bit_width (Cory Fields)
52f9bba crypto: replace non-standard CLZ builtins with c++20's bit_width (Cory Fields)

Pull request description:

  This replaces bitcoin#28674, bitcoin#29036, and bitcoin#29057. Now ready for testing and review.

  Replaces platform-specific endian and byteswap functions. This is especially useful for kernel, as it means that our deep serialization code no longer requires bitcoin-config.h.

  I apologize for the size of the last commit, but it's hard to avoid making those changes at once.

  All platforms now use our internal functions rather than libc or platform-specific ones, with the exception of MSVC.

  Sadly, benchmarking showed that not all compilers are capable of detecting and optimizing byteswap functions, so compiler builtins are instead used where possible. However, they're now detected via macros rather than autoconf checks.

  This[ matches how libc++ implements std::byteswap for c++23](https://github.com/llvm/llvm-project/blob/main/libcxx/include/__bit/byteswap.h#L26).

  I suggest we move/rename `compat/endian.h`, but I left that out of this PR to avoid bikeshedding.

  bitcoin#29057 pointed out some irregularities in benchmarks. After messing with various compilers and configs for a few weeks with these changes, I'm of the opinion that we can't win on every platform every time, so we should take the code that makes sense going forward. That said, if any real-world slowdowns are caused here, we should obviously investigate.

ACKs for top commit:
  maflcko:
    ACK 86b7f28 📘
  fanquake:
    ACK 86b7f28 - we can finish pruning out the __builtin_clz* checks/usage once the minisketch code has been updated. This is more good cleanup pre-CMake & for the kernal.

Tree-SHA512: 715a32ec190c70505ffbce70bfe81fc7b6aa33e376b60292e801f60cf17025aabfcab4e8c53ebb2e28ffc5cf4c20b74fe3dd8548371ad772085c13aec8b7970e
  • Loading branch information
fanquake committed Mar 1, 2024
2 parents ae4165f + 86b7f28 commit 8da62a1
Show file tree
Hide file tree
Showing 11 changed files with 118 additions and 324 deletions.
14 changes: 1 addition & 13 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -958,7 +958,7 @@ if test "$TARGET_OS" = "darwin"; then
AX_CHECK_LINK_FLAG([-Wl,-fixup_chains], [HARDENED_LDFLAGS="$HARDENED_LDFLAGS -Wl,-fixup_chains"], [], [$LDFLAG_WERROR])
fi

AC_CHECK_HEADERS([endian.h sys/endian.h byteswap.h sys/select.h sys/prctl.h sys/sysctl.h vm/vm_param.h sys/vmmeter.h sys/resources.h])
AC_CHECK_HEADERS([sys/select.h sys/prctl.h sys/sysctl.h vm/vm_param.h sys/vmmeter.h sys/resources.h])

AC_CHECK_DECLS([getifaddrs, freeifaddrs],[CHECK_SOCKET],,
[#include <sys/types.h>
Expand All @@ -973,18 +973,6 @@ AC_CHECK_DECLS([pipe2])

AC_CHECK_FUNCS([timingsafe_bcmp])

AC_CHECK_DECLS([le16toh, le32toh, le64toh, htole16, htole32, htole64, be16toh, be32toh, be64toh, htobe16, htobe32, htobe64],,,
[#if HAVE_ENDIAN_H
#include <endian.h>
#elif HAVE_SYS_ENDIAN_H
#include <sys/endian.h>
#endif])

AC_CHECK_DECLS([bswap_16, bswap_32, bswap_64],,,
[#if HAVE_BYTESWAP_H
#include <byteswap.h>
#endif])

AC_MSG_CHECKING([for __builtin_clzl])
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ ]], [[
(void) __builtin_clzl(0);
Expand Down
66 changes: 43 additions & 23 deletions src/compat/byteswap.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,44 +5,66 @@
#ifndef BITCOIN_COMPAT_BYTESWAP_H
#define BITCOIN_COMPAT_BYTESWAP_H

#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#include <cstdint>
#ifdef _MSC_VER
#include <cstdlib>
#endif

#include <cstdint>

#if defined(HAVE_BYTESWAP_H)
#include <byteswap.h>
#endif
// All internal_bswap_* functions can be replaced with std::byteswap once we
// require c++23. Both libstdc++ and libc++ implement std::byteswap via these
// builtins.

#if defined(MAC_OSX)
#ifndef DISABLE_BUILTIN_BSWAPS
# if defined __has_builtin
# if __has_builtin(__builtin_bswap16)
# define bitcoin_builtin_bswap16(x) __builtin_bswap16(x)
# endif
# if __has_builtin(__builtin_bswap32)
# define bitcoin_builtin_bswap32(x) __builtin_bswap32(x)
# endif
# if __has_builtin(__builtin_bswap64)
# define bitcoin_builtin_bswap64(x) __builtin_bswap64(x)
# endif
# elif defined(_MSC_VER)
# define bitcoin_builtin_bswap16(x) _byteswap_ushort(x)
# define bitcoin_builtin_bswap32(x) _byteswap_ulong(x)
# define bitcoin_builtin_bswap64(x) _byteswap_uint64(x)
# endif
#endif

#include <libkern/OSByteOrder.h>
#define bswap_16(x) OSSwapInt16(x)
#define bswap_32(x) OSSwapInt32(x)
#define bswap_64(x) OSSwapInt64(x)
// MSVC's _byteswap_* functions are not constexpr

#ifndef _MSC_VER
#define BSWAP_CONSTEXPR constexpr
#else
// Non-MacOS / non-Darwin
#define BSWAP_CONSTEXPR
#endif

#if HAVE_DECL_BSWAP_16 == 0
inline uint16_t bswap_16(uint16_t x)
inline BSWAP_CONSTEXPR uint16_t internal_bswap_16(uint16_t x)
{
#ifdef bitcoin_builtin_bswap16
return bitcoin_builtin_bswap16(x);
#else
return (x >> 8) | (x << 8);
#endif
}
#endif // HAVE_DECL_BSWAP16 == 0

#if HAVE_DECL_BSWAP_32 == 0
inline uint32_t bswap_32(uint32_t x)
inline BSWAP_CONSTEXPR uint32_t internal_bswap_32(uint32_t x)
{
#ifdef bitcoin_builtin_bswap32
return bitcoin_builtin_bswap32(x);
#else
return (((x & 0xff000000U) >> 24) | ((x & 0x00ff0000U) >> 8) |
((x & 0x0000ff00U) << 8) | ((x & 0x000000ffU) << 24));
#endif
}
#endif // HAVE_DECL_BSWAP32 == 0

#if HAVE_DECL_BSWAP_64 == 0
inline uint64_t bswap_64(uint64_t x)
inline BSWAP_CONSTEXPR uint64_t internal_bswap_64(uint64_t x)
{
#ifdef bitcoin_builtin_bswap64
return bitcoin_builtin_bswap64(x);
#else
return (((x & 0xff00000000000000ull) >> 56)
| ((x & 0x00ff000000000000ull) >> 40)
| ((x & 0x0000ff0000000000ull) >> 24)
Expand All @@ -51,9 +73,7 @@ inline uint64_t bswap_64(uint64_t x)
| ((x & 0x0000000000ff0000ull) << 24)
| ((x & 0x000000000000ff00ull) << 40)
| ((x & 0x00000000000000ffull) << 56));
#endif
}
#endif // HAVE_DECL_BSWAP64 == 0

#endif // defined(MAC_OSX)

#endif // BITCOIN_COMPAT_BYTESWAP_H
Loading

0 comments on commit 8da62a1

Please sign in to comment.