[PATCH v3 0/5] bswap.h: Rework ntohl handling

[PATCH v3 0/5] bswap.h: Rework ntohl handling

[Sebastian Andrzej Siewior]

Hi,

this series continues the rework of the bswap32/64()/ nothl() handling. 

I've been looking at recent compiler and they manage to recognize the
manual shifting and use an optimize opcode if available. The ntohl
version provided by glibc already provides an "optimized" version which
makes an optimisation in git almost not needed.
One of the motivation behind overwriting/ providing an optimized
version was to provide a macro instead of using a function call. One
libc that is still providing ntohl as a function call is musl.

While ntohl() is provided by the libc, the ntohll() is not. I found it
only on Windows provided by winsock.h.

I haven't touched the put/get_be*() macros. gcc & clang are both smart
enough to swap the content accordingly and perform a single store/ load.
Only the msvc seems to strugle here and performs multiple bytes stores/
loads and shifts.

v2…v3 https://lore.kernel.org/all/20250611221444.1567638-1-sebastian@breakpoint.cc/
  - Fixed typos in the patch description

Sebastian Andrzej Siewior (5):
  bswap.h: Add support for __BYTE_ORDER__
  bswap.h: Define GIT_LITTLE_ENDIAN on msvc as little endian
  bswap.h: Always overwrite ntohl/ ntohll macros
  bswap.h: Remove optimized x86 version of bswap32/64
  bswap.h: Provide a built-in based version of bswap32/64 if possible

 compat/bswap.h | 114 +++++++++++++++++++++----------------------------
 1 file changed, 48 insertions(+), 66 deletions(-)

-- 
2.50.0

[PATCH v3 1/5] bswap.h: Add support for __BYTE_ORDER__

[Sebastian Andrzej Siewior]

The __BYTE_ORDER__ define is provided by gcc (since ~v4.6), clang
(since ~v3.2) and icc (since ~16.0.3).

The __BYTE_ORDER and BYTE_ORDER macros are libc specific and are not
available on all supported platforms such as mingw.

Add support for the __BYTE_ORDER__ macro as a fallback.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 compat/bswap.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/compat/bswap.h b/compat/bswap.h
index b34054f2bd728..0a457542dd76a 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -116,6 +116,12 @@ static inline uint64_t git_bswap64(uint64_t x)
 # define GIT_LITTLE_ENDIAN LITTLE_ENDIAN
 # define GIT_BIG_ENDIAN BIG_ENDIAN
 
+#elif defined(__BYTE_ORDER__) && defined(__ORDER_LITTLE_ENDIAN__) && defined(__ORDER_BIG_ENDIAN__)
+
+# define GIT_BYTE_ORDER __BYTE_ORDER__
+# define GIT_LITTLE_ENDIAN __ORDER_LITTLE_ENDIAN__
+# define GIT_BIG_ENDIAN __ORDER_BIG_ENDIAN__
+
 #else
 
 # define GIT_BIG_ENDIAN 4321
-- 
2.50.0

[PATCH v3 2/5] bswap.h: Define GIT_LITTLE_ENDIAN on msvc as little endian

[Sebastian Andrzej Siewior]

The Microsoft Visual C++ (MSVC) compiler (as of Visual Studio 2022
version 17.13.6) does not define __BYTE_ORDER__ and its C-library does
not define __BYTE_ORDER. The compiler is supported only on arm64 and x86
which are all little endian.

Define GIT_BYTE_ORDER on msvc as little endian to avoid further checks.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 compat/bswap.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/compat/bswap.h b/compat/bswap.h
index 0a457542dd76a..fd604d9f7b74b 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -81,6 +81,10 @@ static inline uint64_t git_bswap64(uint64_t x)
 #define bswap32(x) _byteswap_ulong(x)
 #define bswap64(x) _byteswap_uint64(x)
 
+#define GIT_LITTLE_ENDIAN 1234
+#define GIT_BIG_ENDIAN 4321
+#define GIT_BYTE_ORDER GIT_LITTLE_ENDIAN
+
 #endif
 
 #if defined(bswap32)
@@ -122,7 +126,7 @@ static inline uint64_t git_bswap64(uint64_t x)
 # define GIT_LITTLE_ENDIAN __ORDER_LITTLE_ENDIAN__
 # define GIT_BIG_ENDIAN __ORDER_BIG_ENDIAN__
 
-#else
+#elif !defined(GIT_BYTE_ORDER)
 
 # define GIT_BIG_ENDIAN 4321
 # define GIT_LITTLE_ENDIAN 1234
-- 
2.50.0

[PATCH v3 3/5] bswap.h: Always overwrite ntohl/ ntohll macros

[Sebastian Andrzej Siewior]

The ntohl and htonl macros are redefined because the provided macros were
not always optimal. Sometimes it was a function call, sometimes it was a
macro which did the shifting. Using the 'bswap' opcode on x86 provides
probably better performance than performing the shifting.
These macros are only overwritten on x86 if the "optimized" version is
available.

The ntohll and htonll macros are not available on every platform (at
least glibc does not provide them) which means they need to be defined
once the endianness of the system is determined.

In order to get a more symmetrical setup, redfine the macros once the
endianness of the system has been determined.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 compat/bswap.h | 54 ++++++++++++++++++++++++--------------------------
 1 file changed, 26 insertions(+), 28 deletions(-)

diff --git a/compat/bswap.h b/compat/bswap.h
index fd604d9f7b74b..aeef304f671f5 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -87,27 +87,6 @@ static inline uint64_t git_bswap64(uint64_t x)
 
 #endif
 
-#if defined(bswap32)
-
-#undef ntohl
-#undef htonl
-#define ntohl(x) bswap32(x)
-#define htonl(x) bswap32(x)
-
-#endif
-
-#if defined(bswap64)
-
-#undef ntohll
-#undef htonll
-#define ntohll(x) bswap64(x)
-#define htonll(x) bswap64(x)
-
-#else
-
-#undef ntohll
-#undef htonll
-
 #if defined(__BYTE_ORDER) && defined(__LITTLE_ENDIAN) && defined(__BIG_ENDIAN)
 
 # define GIT_BYTE_ORDER __BYTE_ORDER
@@ -145,14 +124,33 @@ static inline uint64_t git_bswap64(uint64_t x)
 
 #endif
 
-#if GIT_BYTE_ORDER == GIT_BIG_ENDIAN
-# define ntohll(n) (n)
-# define htonll(n) (n)
-#else
-# define ntohll(n) default_bswap64(n)
-# define htonll(n) default_bswap64(n)
-#endif
+#undef ntohl
+#undef htonl
+#undef ntohll
+#undef htonll
 
+#if GIT_BYTE_ORDER == GIT_BIG_ENDIAN
+# define ntohl(x) (x)
+# define htonl(x) (x)
+# define ntohll(x) (x)
+# define htonll(x) (x)
+#else
+
+# if defined(bswap32)
+#  define ntohl(x) bswap32(x)
+#  define htonl(x) bswap32(x)
+# else
+#  define ntohl(x) default_swab32(x)
+#  define htonl(x) default_swab32(x)
+# endif
+
+# if defined(bswap64)
+#  define ntohll(x) bswap64(x)
+#  define htonll(x) bswap64(x)
+# else
+#  define ntohll(x) default_bswap64(x)
+#  define htonll(x) default_bswap64(x)
+# endif
 #endif
 
 static inline uint16_t get_be16(const void *ptr)
-- 
2.50.0

[PATCH v3 4/5] bswap.h: Remove optimized x86 version of bswap32/64

[Sebastian Andrzej Siewior]

On x86 the bswap32/64 macro is implemented based on the x86 opcode which
performs the required shifting in just one opcode.
The other CPUs fallback to the generic shifting as implemented by
default_swab32() and default_bswap64() if needed.

I've been looking at how good a compiler is at recognizing the default
shift and emitting an optimized operation:
- x86, arm64 msvc v19.20
  default_swab32() optimized
  default_bswap64() shifts
  _byteswap_uint64() optimized

- x86, arm64 msvc v19.37
  default_swab32() optimized
  default_bswap64() optimized
  _byteswap_uint64() optimized

- arm64, gcc-4.9.4: optimized
- x86-64, gcc-4.4.7: shifts
- x86-64, gcc-4.5.3: optimized
- x86-64, clang-3.0: optimized

Given that gcc-4.5 and clang-3.0 are fairly old, any recent compiler
should recognize the shift.

Remove the optimized x86 version and rely on the compiler.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 compat/bswap.h | 41 +----------------------------------------
 1 file changed, 1 insertion(+), 40 deletions(-)

diff --git a/compat/bswap.h b/compat/bswap.h
index aeef304f671f5..ed00f6d1d53f3 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -35,46 +35,7 @@ static inline uint64_t default_bswap64(uint64_t val)
 #undef bswap32
 #undef bswap64
 
-#if defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))
-
-#define bswap32 git_bswap32
-static inline uint32_t git_bswap32(uint32_t x)
-{
-	uint32_t result;
-	if (__builtin_constant_p(x))
-		result = default_swab32(x);
-	else
-		__asm__("bswap %0" : "=r" (result) : "0" (x));
-	return result;
-}
-
-#define bswap64 git_bswap64
-#if defined(__x86_64__)
-static inline uint64_t git_bswap64(uint64_t x)
-{
-	uint64_t result;
-	if (__builtin_constant_p(x))
-		result = default_bswap64(x);
-	else
-		__asm__("bswap %q0" : "=r" (result) : "0" (x));
-	return result;
-}
-#else
-static inline uint64_t git_bswap64(uint64_t x)
-{
-	union { uint64_t i64; uint32_t i32[2]; } tmp, result;
-	if (__builtin_constant_p(x))
-		result.i64 = default_bswap64(x);
-	else {
-		tmp.i64 = x;
-		result.i32[0] = git_bswap32(tmp.i32[1]);
-		result.i32[1] = git_bswap32(tmp.i32[0]);
-	}
-	return result.i64;
-}
-#endif
-
-#elif defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64) || defined(_M_ARM64))
+#if defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64) || defined(_M_ARM64))
 
 #include <stdlib.h>
 
-- 
2.50.0

[PATCH v3 5/5] bswap.h: Provide a built-in based version of bswap32/64 if possible

[Sebastian Andrzej Siewior]

The compiler is in general able to recognize the endian shift and
replace it with an optimized opcode if possible. On certain
architectures such as RiscV or MIPS the situation can get complicated.
They don't provide an optimized opcode and masking the "higher" bits may
required loading a constant which needs shifting. This causes the
compiler to emit a lot of instructions for the operation.

The provided builtin directive on these architecture calls a function
which does the operation instead of emitting the code for operation.

Bring back the change from commit 6547d1c9 (bswap.h: add support for
built-in bswap functions, 2025-04-23). The bswap32/64 macro can now be
defined unconditionally so it won't regress on big endian architectures.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 compat/bswap.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/compat/bswap.h b/compat/bswap.h
index ed00f6d1d53f3..28635ebc690e3 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -32,6 +32,14 @@ static inline uint64_t default_bswap64(uint64_t val)
 		((val & (uint64_t)0xff00000000000000ULL) >> 56));
 }
 
+/*
+ * __has_builtin is available since Clang 10 and GCC 10.
+ * Below is a fallback for older compilers.
+ */
+#ifndef __has_builtin
+# define __has_builtin(x) 0
+#endif
+
 #undef bswap32
 #undef bswap64
 
@@ -46,6 +54,11 @@ static inline uint64_t default_bswap64(uint64_t val)
 #define GIT_BIG_ENDIAN 4321
 #define GIT_BYTE_ORDER GIT_LITTLE_ENDIAN
 
+#elif __has_builtin(__builtin_bswap32) && __has_builtin(__builtin_bswap64)
+
+#define bswap32(x) __builtin_bswap32((x))
+#define bswap64(x) __builtin_bswap64((x))
+
 #endif
 
 #if defined(__BYTE_ORDER) && defined(__LITTLE_ENDIAN) && defined(__BIG_ENDIAN)
-- 
2.50.0

Re: [PATCH v3 0/5] bswap.h: Rework ntohl handling

[Junio C Hamano]

Sebastian Andrzej Siewior <sebastian@breakpoint.cc> writes:

> this series continues the rework of the bswap32/64()/ nothl() handling. 
>
> I've been looking at recent compiler and they manage to recognize the
> manual shifting and use an optimize opcode if available. The ntohl
> version provided by glibc already provides an "optimized" version which
> makes an optimisation in git almost not needed.
> One of the motivation behind overwriting/ providing an optimized
> version was to provide a macro instead of using a function call. One
> libc that is still providing ntohl as a function call is musl.
>
> While ntohl() is provided by the libc, the ntohll() is not. I found it
> only on Windows provided by winsock.h.
>
> I haven't touched the put/get_be*() macros. gcc & clang are both smart
> enough to swap the content accordingly and perform a single store/ load.
> Only the msvc seems to strugle here and performs multiple bytes stores/
> loads and shifts.
>
> v2…v3 https://lore.kernel.org/all/20250611221444.1567638-1-sebastian@breakpoint.cc/
>   - Fixed typos in the patch description

Thanks for updating the proposed log messages with typofixes.  I
understand the patch text has no changes?

Cc'ed those who gave comments on the previous round.  How does this
version look to you folks?

Thanks.

Re: [PATCH v3 0/5] bswap.h: Rework ntohl handling

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

On 2025-07-15 at 21:52:01, Junio C Hamano wrote:
> Thanks for updating the proposed log messages with typofixes.  I
> understand the patch text has no changes?
> 
> Cc'ed those who gave comments on the previous round.  How does this
> version look to you folks?

This seems fine to me.

I might go a little farther and simply say that recognizing the pattern
and emitting the most efficient code, whether that's a single
instruction or an optimized pattern of instructions, is a
quality-of-implementation issue in the compiler's peephole optimizer[0]
and that we don't really need to use the builtin functions or provide
special cases for MSVC still, but I realize that is not a very popular
opinion and I think this series is fine without that.

[0] Byte swapping, population count, and rotations are, in my view, the
most frequently open-coded functions and thus every compiler with a
peephole optimizer should recognize and handle them.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH v3 0/5] bswap.h: Rework ntohl handling

[Sebastian Andrzej Siewior]

On 2025-07-15 14:52:01 [-0700], Junio C Hamano wrote:
> Sebastian Andrzej Siewior <sebastian@breakpoint.cc> writes:
> 
> > this series continues the rework of the bswap32/64()/ nothl() handling. 
> >
> > I've been looking at recent compiler and they manage to recognize the
> > manual shifting and use an optimize opcode if available. The ntohl
> > version provided by glibc already provides an "optimized" version which
> > makes an optimisation in git almost not needed.
> > One of the motivation behind overwriting/ providing an optimized
> > version was to provide a macro instead of using a function call. One
> > libc that is still providing ntohl as a function call is musl.
> >
> > While ntohl() is provided by the libc, the ntohll() is not. I found it
> > only on Windows provided by winsock.h.
> >
> > I haven't touched the put/get_be*() macros. gcc & clang are both smart
> > enough to swap the content accordingly and perform a single store/ load.
> > Only the msvc seems to strugle here and performs multiple bytes stores/
> > loads and shifts.
> >
> > v2…v3 https://lore.kernel.org/all/20250611221444.1567638-1-sebastian@breakpoint.cc/
> >   - Fixed typos in the patch description
> 
> Thanks for updating the proposed log messages with typofixes.  I
> understand the patch text has no changes?

The patches iteself remain unchanged, only the patch description of a
few got updated.
There was only one comment regarding using endian(3). At the same time I
got the feeling to first get this through and then we can think how to
continue further. glibc optimizes ntohl already so git might not have
to. I only found musl that has a function call. I didn't look on *BSD.
It just something is needed for ntohll().

> Thanks.

Sebastian