[PATCH 0/4] qemu/bswap: Use compiler __builtin_bswap()

[PATCH 0/4] qemu/bswap: Use compiler __builtin_bswap()

[Philippe Mathieu-Daudé]

Implement Richard's suggestion to use __builtin_bswap().

const_XX() not updated. We could use le_bswap() definitions
but they are undefined, not sure yet what is the best way.
Meanwhile I figure this out, these patches are ready for review.

Philippe Mathieu-Daudé (4):
  qemu/bswap: Remove unused qemu_bswap_len()
  qemu/bswap: Replace bswapXX() by compiler __builtin_bswap()
  qemu/bswap: Replace bswapXXs() by compiler __builtin_bswap()
  qemu/bswap: Remove <byteswap.h> dependency

 configure            | 13 ---------
 include/qemu/bswap.h | 69 +++++++++-----------------------------------
 2 files changed, 13 insertions(+), 69 deletions(-)

-- 
2.26.2

[PATCH 1/4] qemu/bswap: Remove unused qemu_bswap_len()

[Philippe Mathieu-Daudé]

Last use of qemu_bswap_len() has been removed in commit
e5fd1eb05ec ("apb: add busA qdev property to PBM PCI bridge").

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/bswap.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 1d3e4c24e41..8b01c38040c 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -169,12 +169,6 @@ CPU_CONVERT(le, 16, uint16_t)
 CPU_CONVERT(le, 32, uint32_t)
 CPU_CONVERT(le, 64, uint64_t)
 
-/* len must be one of 1, 2, 4 */
-static inline uint32_t qemu_bswap_len(uint32_t value, int len)
-{
-    return bswap32(value) >> (32 - 8 * len);
-}
-
 /*
  * Same as cpu_to_le{16,32}, except that gcc will figure the result is
  * a compile-time constant if you pass in a constant.  So this can be
-- 
2.26.2

Re: [PATCH 1/4] qemu/bswap: Remove unused qemu_bswap_len()

[Richard Henderson]

On 9/23/20 3:02 AM, Philippe Mathieu-Daudé wrote:
> Last use of qemu_bswap_len() has been removed in commit
> e5fd1eb05ec ("apb: add busA qdev property to PBM PCI bridge").
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qemu/bswap.h | 6 ------
>  1 file changed, 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 2/4] qemu/bswap: Replace bswapXX() by compiler __builtin_bswap()

[Philippe Mathieu-Daudé]

Use the compiler built-in function to byte swap values,
as the compiler is clever and will fold constants.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/bswap.h | 30 ++++--------------------------
 1 file changed, 4 insertions(+), 26 deletions(-)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 8b01c38040c..d1a2f700f2a 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -27,32 +27,10 @@ static inline uint64_t bswap64(uint64_t x)
 {
     return bswap_64(x);
 }
-# else
-static inline uint16_t bswap16(uint16_t x)
-{
-    return (((x & 0x00ff) << 8) |
-            ((x & 0xff00) >> 8));
-}
-
-static inline uint32_t bswap32(uint32_t x)
-{
-    return (((x & 0x000000ffU) << 24) |
-            ((x & 0x0000ff00U) <<  8) |
-            ((x & 0x00ff0000U) >>  8) |
-            ((x & 0xff000000U) >> 24));
-}
-
-static inline uint64_t bswap64(uint64_t x)
-{
-    return (((x & 0x00000000000000ffULL) << 56) |
-            ((x & 0x000000000000ff00ULL) << 40) |
-            ((x & 0x0000000000ff0000ULL) << 24) |
-            ((x & 0x00000000ff000000ULL) <<  8) |
-            ((x & 0x000000ff00000000ULL) >>  8) |
-            ((x & 0x0000ff0000000000ULL) >> 24) |
-            ((x & 0x00ff000000000000ULL) >> 40) |
-            ((x & 0xff00000000000000ULL) >> 56));
-}
+#else
+# define bswap16(_x) __builtin_bswap16(_x)
+# define bswap32(_x) __builtin_bswap32(_x)
+# define bswap64(_x) __builtin_bswap64(_x)
 #endif /* ! CONFIG_MACHINE_BSWAP_H */
 
 static inline void bswap16s(uint16_t *s)
-- 
2.26.2

Re: [PATCH 2/4] qemu/bswap: Replace bswapXX() by compiler __builtin_bswap()

[Richard Henderson]

On 9/23/20 3:02 AM, Philippe Mathieu-Daudé wrote:
> Use the compiler built-in function to byte swap values,
> as the compiler is clever and will fold constants.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qemu/bswap.h | 30 ++++--------------------------
>  1 file changed, 4 insertions(+), 26 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 3/4] qemu/bswap: Replace bswapXXs() by compiler __builtin_bswap()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/bswap.h | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index d1a2f700f2a..6875e48d617 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -35,29 +35,31 @@ static inline uint64_t bswap64(uint64_t x)
 
 static inline void bswap16s(uint16_t *s)
 {
-    *s = bswap16(*s);
+    *s = __builtin_bswap16(*s);
 }
 
 static inline void bswap32s(uint32_t *s)
 {
-    *s = bswap32(*s);
+    *s = __builtin_bswap32(*s);
 }
 
 static inline void bswap64s(uint64_t *s)
 {
-    *s = bswap64(*s);
+    *s = __builtin_bswap64(*s);
 }
 
 #if defined(HOST_WORDS_BIGENDIAN)
 #define be_bswap(v, size) (v)
-#define le_bswap(v, size) glue(bswap, size)(v)
+#define le_bswap(v, size) glue(__builtin_bswap, size)(v)
 #define be_bswaps(v, size)
-#define le_bswaps(p, size) do { *p = glue(bswap, size)(*p); } while(0)
+#define le_bswaps(p, size) \
+            do { *p = glue(__builtin_bswap, size)(*p); } while (0)
 #else
 #define le_bswap(v, size) (v)
-#define be_bswap(v, size) glue(bswap, size)(v)
+#define be_bswap(v, size) glue(__builtin_bswap, size)(v)
 #define le_bswaps(v, size)
-#define be_bswaps(p, size) do { *p = glue(bswap, size)(*p); } while(0)
+#define be_bswaps(p, size) \
+            do { *p = glue(__builtin_bswap, size)(*p); } while (0)
 #endif
 
 /**
-- 
2.26.2

Re: [PATCH 3/4] qemu/bswap: Replace bswapXXs() by compiler __builtin_bswap()

[Richard Henderson]

On 9/23/20 3:02 AM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qemu/bswap.h | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 4/4] qemu/bswap: Remove <byteswap.h> dependency

[Philippe Mathieu-Daudé]

All our supported compilers provide the __builtin_bswap()
functions. Drop the <byteswap.h> dependency.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 configure            | 13 -------------
 include/qemu/bswap.h | 17 -----------------
 2 files changed, 30 deletions(-)

diff --git a/configure b/configure
index e8e8e984f24..bff787daea7 100755
--- a/configure
+++ b/configure
@@ -4886,16 +4886,6 @@ if test "$docs" != "no" ; then
   fi
 fi
 
-# Search for bswap_32 function
-byteswap_h=no
-cat > $TMPC << EOF
-#include <byteswap.h>
-int main(void) { return bswap_32(0); }
-EOF
-if compile_prog "" "" ; then
-  byteswap_h=yes
-fi
-
 # Search for bswap32 function
 bswap_h=no
 cat > $TMPC << EOF
@@ -6789,9 +6779,6 @@ fi
 if test "$st_atim" = "yes" ; then
   echo "HAVE_STRUCT_STAT_ST_ATIM=y" >> $config_host_mak
 fi
-if test "$byteswap_h" = "yes" ; then
-  echo "CONFIG_BYTESWAP_H=y" >> $config_host_mak
-fi
 if test "$bswap_h" = "yes" ; then
   echo "CONFIG_MACHINE_BSWAP_H=y" >> $config_host_mak
 fi
diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 6875e48d617..c192a6ad3e7 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -10,23 +10,6 @@
 # include <sys/endian.h>
 #elif defined(__HAIKU__)
 # include <endian.h>
-#elif defined(CONFIG_BYTESWAP_H)
-# include <byteswap.h>
-
-static inline uint16_t bswap16(uint16_t x)
-{
-    return bswap_16(x);
-}
-
-static inline uint32_t bswap32(uint32_t x)
-{
-    return bswap_32(x);
-}
-
-static inline uint64_t bswap64(uint64_t x)
-{
-    return bswap_64(x);
-}
 #else
 # define bswap16(_x) __builtin_bswap16(_x)
 # define bswap32(_x) __builtin_bswap32(_x)
-- 
2.26.2

Re: [PATCH 4/4] qemu/bswap: Remove <byteswap.h> dependency

[Peter Maydell]

On Wed, 23 Sep 2020 at 11:09, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> All our supported compilers provide the __builtin_bswap()
> functions. Drop the <byteswap.h> dependency.
>
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 6875e48d617..c192a6ad3e7 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -10,23 +10,6 @@
>  # include <sys/endian.h>
>  #elif defined(__HAIKU__)
>  # include <endian.h>
> -#elif defined(CONFIG_BYTESWAP_H)
> -# include <byteswap.h>
> -
> -static inline uint16_t bswap16(uint16_t x)
> -{
> -    return bswap_16(x);
> -}
> -
> -static inline uint32_t bswap32(uint32_t x)
> -{
> -    return bswap_32(x);
> -}
> -
> -static inline uint64_t bswap64(uint64_t x)
> -{
> -    return bswap_64(x);
> -}
>  #else
>  # define bswap16(_x) __builtin_bswap16(_x)
>  # define bswap32(_x) __builtin_bswap32(_x)

I suppose if we wanted to just use the __builtin_bswap*
implementation on all hosts (ie drop the special casing
of CONFIG_MACHINE_BSWAP_H/FreeBSD/Haiku) we'd need to
rename our macros to avoid potential conflicts with the
versions in the system headers there in case they were
pulled in via some other path ?

thanks
-- PMM

Re: [PATCH 4/4] qemu/bswap: Remove <byteswap.h> dependency

[Philippe Mathieu-Daudé]

On 9/23/20 12:16 PM, Peter Maydell wrote:
> On Wed, 23 Sep 2020 at 11:09, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> All our supported compilers provide the __builtin_bswap()
>> functions. Drop the <byteswap.h> dependency.
>>
>> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
>> index 6875e48d617..c192a6ad3e7 100644
>> --- a/include/qemu/bswap.h
>> +++ b/include/qemu/bswap.h
>> @@ -10,23 +10,6 @@
>>  # include <sys/endian.h>
>>  #elif defined(__HAIKU__)
>>  # include <endian.h>
>> -#elif defined(CONFIG_BYTESWAP_H)
>> -# include <byteswap.h>
>> -
>> -static inline uint16_t bswap16(uint16_t x)
>> -{
>> -    return bswap_16(x);
>> -}
>> -
>> -static inline uint32_t bswap32(uint32_t x)
>> -{
>> -    return bswap_32(x);
>> -}
>> -
>> -static inline uint64_t bswap64(uint64_t x)
>> -{
>> -    return bswap_64(x);
>> -}
>>  #else
>>  # define bswap16(_x) __builtin_bswap16(_x)
>>  # define bswap32(_x) __builtin_bswap32(_x)
> 
> I suppose if we wanted to just use the __builtin_bswap*
> implementation on all hosts (ie drop the special casing
> of CONFIG_MACHINE_BSWAP_H/FreeBSD/Haiku) we'd need to
> rename our macros to avoid potential conflicts with the
> versions in the system headers there in case they were
> pulled in via some other path ?

Yes, this is why I couldn't get ride of everything

After reading Paolo's suggestion to use qatomic*, I
am tempted to suggest qbswap* but I am still looking
for better alternatives...

> 
> thanks
> -- PMM
> 

Re: [PATCH 4/4] qemu/bswap: Remove <byteswap.h> dependency

[Richard Henderson]

On 9/23/20 3:30 AM, Philippe Mathieu-Daudé wrote:
> On 9/23/20 12:16 PM, Peter Maydell wrote:
>> I suppose if we wanted to just use the __builtin_bswap*
>> implementation on all hosts (ie drop the special casing
>> of CONFIG_MACHINE_BSWAP_H/FreeBSD/Haiku) we'd need to
>> rename our macros to avoid potential conflicts with the
>> versions in the system headers there in case they were
>> pulled in via some other path ?
> 
> Yes, this is why I couldn't get ride of everything
> 
> After reading Paolo's suggestion to use qatomic*, I
> am tempted to suggest qbswap* but I am still looking
> for better alternatives...

Hum, maybe.  It's pretty ugly.  We could just leave those alone and hope the
system version is decent enough.


r~

Re: [PATCH 4/4] qemu/bswap: Remove <byteswap.h> dependency

[Peter Maydell]

On Thu, 24 Sep 2020 at 20:30, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 9/23/20 3:30 AM, Philippe Mathieu-Daudé wrote:
> > On 9/23/20 12:16 PM, Peter Maydell wrote:
> >> I suppose if we wanted to just use the __builtin_bswap*
> >> implementation on all hosts (ie drop the special casing
> >> of CONFIG_MACHINE_BSWAP_H/FreeBSD/Haiku) we'd need to
> >> rename our macros to avoid potential conflicts with the
> >> versions in the system headers there in case they were
> >> pulled in via some other path ?
> >
> > Yes, this is why I couldn't get ride of everything
> >
> > After reading Paolo's suggestion to use qatomic*, I
> > am tempted to suggest qbswap* but I am still looking
> > for better alternatives...
>
> Hum, maybe.  It's pretty ugly.  We could just leave those alone and hope the
> system version is decent enough.

Mostly what I would like is to be able to be rid of the
configure machinery so we could just use a single
portable implementation.

thanks
-- PMM

Re: [PATCH 4/4] qemu/bswap: Remove <byteswap.h> dependency

[Philippe Mathieu-Daudé]

On 9/24/20 9:33 PM, Peter Maydell wrote:
> On Thu, 24 Sep 2020 at 20:30, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 9/23/20 3:30 AM, Philippe Mathieu-Daudé wrote:
>>> On 9/23/20 12:16 PM, Peter Maydell wrote:
>>>> I suppose if we wanted to just use the __builtin_bswap*
>>>> implementation on all hosts (ie drop the special casing
>>>> of CONFIG_MACHINE_BSWAP_H/FreeBSD/Haiku) we'd need to
>>>> rename our macros to avoid potential conflicts with the
>>>> versions in the system headers there in case they were
>>>> pulled in via some other path ?
>>>
>>> Yes, this is why I couldn't get ride of everything
>>>
>>> After reading Paolo's suggestion to use qatomic*, I
>>> am tempted to suggest qbswap* but I am still looking
>>> for better alternatives...
>>
>> Hum, maybe.  It's pretty ugly.  We could just leave those alone and hope the
>> system version is decent enough.
> 
> Mostly what I would like is to be able to be rid of the
> configure machinery so we could just use a single
> portable implementation.

Good news. Using the Haiku image provided by Alexander,
GCC is pretty recent and supports builtin atomic:

> uname -a
Haiku shredder 1 hrev54154+115 Jun 30 2020 15:20 x86_64 x86_64 Haiku
> gcc --version
gcc (2019_05_24) 8.3.0
Copyright (C) 2018 Free Software Foundation, Inc.

The <endian.h> include (/system/develop/headers/posix/endian.h)
only defines BYTE_ORDER.

Looking at FreeBSD, bswap*() are defined as macro:
https://github.com/freebsd/freebsd/blob/master/sys/sys/endian.h#L58
So we could eventually undefine them... but the manpage
describe them as functions [*]:

DESCRIPTION
    The bswap16(), bswap32(), and bswap64() functions return
    a byte order swapped integer.

So theoretically they could change the prototype still respecting
the documentation contract, and undefines wouldn't work anymore.

Would this be acceptable? Any clever idea?

[*] https://www.freebsd.org/cgi/man.cgi?query=bswap32&apropos=0&sektion=9

> 
> thanks
> -- PMM
> 

Re: [PATCH 4/4] qemu/bswap: Remove <byteswap.h> dependency

[Richard Henderson]

On 9/25/20 2:22 AM, Philippe Mathieu-Daudé wrote:
> Looking at FreeBSD, bswap*() are defined as macro:
> https://github.com/freebsd/freebsd/blob/master/sys/sys/endian.h#L58

That works for me.

> So theoretically they could change the prototype still respecting
> the documentation contract, and undefines wouldn't work anymore.

But we'd be #defining our own macros that would mean that the out-of-line
functions would never be used.  So it still works.


r~

Re: [PATCH 4/4] qemu/bswap: Remove <byteswap.h> dependency

[Richard Henderson]

On 9/23/20 3:02 AM, Philippe Mathieu-Daudé wrote:
> All our supported compilers provide the __builtin_bswap()
> functions. Drop the <byteswap.h> dependency.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  configure            | 13 -------------
>  include/qemu/bswap.h | 17 -----------------
>  2 files changed, 30 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~