[PATCH 0/4] qemu/bswap: Let cpu_to_endian() functions handle constant expressions

[PATCH 0/4] qemu/bswap: Let cpu_to_endian() functions handle constant expressions

[Philippe Mathieu-Daudé]

Stefan Hajnoczi suggested we can use __builtin_constant_p()
in the cpu_to_ENDIAN()/ENDIAN_to_cpu() functions to check if
expression is constant, and if so use the const_ENDIAN() macros.

This series modifies cpu_to_ENDIAN() to use const_ENDIAN() when
possible.

Philippe Mathieu-Daudé (4):
  qemu/bswap: Move const_le() definitions around
  qemu/bswap: add const_be16() and const_be32()
  qemu/bswap: Add const_le64() and const_be64()
  qemu/bswap: Let cpu_to_endian() functions handle constant expressions

 include/qemu/bswap.h | 75 +++++++++++++++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 21 deletions(-)

-- 
2.26.2

[PATCH 1/4] qemu/bswap: Move const_le() definitions around

[Philippe Mathieu-Daudé]

As we want the cpu_to_endian() macros use the
const_le() macros, move them earlier, so they
are defined when we use them.

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

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 1d3e4c24e41..c3b4277342b 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -82,6 +82,25 @@ static inline void bswap64s(uint64_t *s)
 #define be_bswaps(p, size) do { *p = glue(bswap, size)(*p); } while(0)
 #endif
 
+/*
+ * Same as cpu_to_le{16,32} described below, except that gcc will
+ * figure the result is a compile-time constant if you pass in a constant.
+ * So this can be used to initialize static variables.
+ */
+#if defined(HOST_WORDS_BIGENDIAN)
+# define const_le32(_x)                          \
+    ((((_x) & 0x000000ffU) << 24) |              \
+     (((_x) & 0x0000ff00U) <<  8) |              \
+     (((_x) & 0x00ff0000U) >>  8) |              \
+     (((_x) & 0xff000000U) >> 24))
+# define const_le16(_x)                          \
+    ((((_x) & 0x00ff) << 8) |                    \
+     (((_x) & 0xff00) >> 8))
+#else
+# define const_le32(_x) (_x)
+# define const_le16(_x) (_x)
+#endif
+
 /**
  * Endianness conversion functions between host cpu and specified endianness.
  * (We list the complete set of prototypes produced by the macros below
@@ -175,25 +194,6 @@ 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
- * used to initialize static variables.
- */
-#if defined(HOST_WORDS_BIGENDIAN)
-# define const_le32(_x)                          \
-    ((((_x) & 0x000000ffU) << 24) |              \
-     (((_x) & 0x0000ff00U) <<  8) |              \
-     (((_x) & 0x00ff0000U) >>  8) |              \
-     (((_x) & 0xff000000U) >> 24))
-# define const_le16(_x)                          \
-    ((((_x) & 0x00ff) << 8) |                    \
-     (((_x) & 0xff00) >> 8))
-#else
-# define const_le32(_x) (_x)
-# define const_le16(_x) (_x)
-#endif
-
 /* Unions for reinterpreting between floats and integers.  */
 
 typedef union {
-- 
2.26.2

[PATCH 2/4] qemu/bswap: add const_be16() and const_be32()

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

We already have the const_le() macros for little endian.
Implement the big-endian equivalent.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/qemu/bswap.h | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index c3b4277342b..6885984e00c 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -83,7 +83,7 @@ static inline void bswap64s(uint64_t *s)
 #endif
 
 /*
- * Same as cpu_to_le{16,32} described below, except that gcc will
+ * Same as cpu_to_{be,le}{16,32} described below, except that gcc will
  * figure the result is a compile-time constant if you pass in a constant.
  * So this can be used to initialize static variables.
  */
@@ -96,11 +96,20 @@ static inline void bswap64s(uint64_t *s)
 # define const_le16(_x)                          \
     ((((_x) & 0x00ff) << 8) |                    \
      (((_x) & 0xff00) >> 8))
+# define const_be32(_x) (_x)
+# define const_be16(_x) (_x)
 #else
 # define const_le32(_x) (_x)
 # define const_le16(_x) (_x)
+# define const_be32(_x)                          \
+    ((((_x) & 0x000000ffU) << 24) |              \
+     (((_x) & 0x0000ff00U) <<  8) |              \
+     (((_x) & 0x00ff0000U) >>  8) |              \
+     (((_x) & 0xff000000U) >> 24))
+# define const_be16(_x)                          \
+    ((((_x) & 0x00ff) << 8) |                    \
+     (((_x) & 0xff00) >> 8))
 #endif
-
 /**
  * Endianness conversion functions between host cpu and specified endianness.
  * (We list the complete set of prototypes produced by the macros below
-- 
2.26.2

[PATCH 3/4] qemu/bswap: Add const_le64() and const_be64()

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

We already have the const_endian() macros for 16-bit and
32-bit values. Implement the 64-bit equivalent macros.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/qemu/bswap.h | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 6885984e00c..de256cea3ab 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -83,11 +83,20 @@ static inline void bswap64s(uint64_t *s)
 #endif
 
 /*
- * Same as cpu_to_{be,le}{16,32} described below, except that gcc will
+ * Same as cpu_to_{be,le}{16,32,64} described below, except that gcc will
  * figure the result is a compile-time constant if you pass in a constant.
  * So this can be used to initialize static variables.
  */
 #if defined(HOST_WORDS_BIGENDIAN)
+# define const_le64(_x)                          \
+    ((((_x) & 0x00000000000000ffU) << 56) |      \
+     (((_x) & 0x000000000000ff00U) << 40) |      \
+     (((_x) & 0x0000000000ff0000U) << 24) |      \
+     (((_x) & 0x00000000ff000000U) <<  8) |      \
+     (((_x) & 0x000000ff00000000U) >>  8) |      \
+     (((_x) & 0x0000ff0000000000U) >> 24) |      \
+     (((_x) & 0x00ff000000000000U) >> 40) |      \
+     (((_x) & 0xff00000000000000U) >> 56))
 # define const_le32(_x)                          \
     ((((_x) & 0x000000ffU) << 24) |              \
      (((_x) & 0x0000ff00U) <<  8) |              \
@@ -96,11 +105,22 @@ static inline void bswap64s(uint64_t *s)
 # define const_le16(_x)                          \
     ((((_x) & 0x00ff) << 8) |                    \
      (((_x) & 0xff00) >> 8))
+# define const_be64(_x) (_x)
 # define const_be32(_x) (_x)
 # define const_be16(_x) (_x)
 #else
+# define const_le64(_x) (_x)
 # define const_le32(_x) (_x)
 # define const_le16(_x) (_x)
+# define const_be64(_x)                          \
+    ((((_x) & 0x00000000000000ffU) << 56) |      \
+     (((_x) & 0x000000000000ff00U) << 40) |      \
+     (((_x) & 0x0000000000ff0000U) << 24) |      \
+     (((_x) & 0x00000000ff000000U) <<  8) |      \
+     (((_x) & 0x000000ff00000000U) >>  8) |      \
+     (((_x) & 0x0000ff0000000000U) >> 24) |      \
+     (((_x) & 0x00ff000000000000U) >> 40) |      \
+     (((_x) & 0xff00000000000000U) >> 56))
 # define const_be32(_x)                          \
     ((((_x) & 0x000000ffU) << 24) |              \
      (((_x) & 0x0000ff00U) <<  8) |              \
-- 
2.26.2

Re: [PATCH 3/4] qemu/bswap: Add const_le64() and const_be64()

[Richard Henderson]

On 9/17/20 9:31 AM, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> We already have the const_endian() macros for 16-bit and
> 32-bit values. Implement the 64-bit equivalent macros.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/qemu/bswap.h | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 6885984e00c..de256cea3ab 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -83,11 +83,20 @@ static inline void bswap64s(uint64_t *s)
>  #endif
>  
>  /*
> - * Same as cpu_to_{be,le}{16,32} described below, except that gcc will
> + * Same as cpu_to_{be,le}{16,32,64} described below, except that gcc will
>   * figure the result is a compile-time constant if you pass in a constant.
>   * So this can be used to initialize static variables.
>   */
>  #if defined(HOST_WORDS_BIGENDIAN)
> +# define const_le64(_x)                          \
> +    ((((_x) & 0x00000000000000ffU) << 56) |      \
> +     (((_x) & 0x000000000000ff00U) << 40) |      \
> +     (((_x) & 0x0000000000ff0000U) << 24) |      \
> +     (((_x) & 0x00000000ff000000U) <<  8) |      \
> +     (((_x) & 0x000000ff00000000U) >>  8) |      \
> +     (((_x) & 0x0000ff0000000000U) >> 24) |      \
> +     (((_x) & 0x00ff000000000000U) >> 40) |      \
> +     (((_x) & 0xff00000000000000U) >> 56))
>  # define const_le32(_x)                          \
>      ((((_x) & 0x000000ffU) << 24) |              \
>       (((_x) & 0x0000ff00U) <<  8) |              \
> @@ -96,11 +105,22 @@ static inline void bswap64s(uint64_t *s)
>  # define const_le16(_x)                          \
>      ((((_x) & 0x00ff) << 8) |                    \
>       (((_x) & 0xff00) >> 8))
> +# define const_be64(_x) (_x)
>  # define const_be32(_x) (_x)
>  # define const_be16(_x) (_x)
>  #else
> +# define const_le64(_x) (_x)
>  # define const_le32(_x) (_x)
>  # define const_le16(_x) (_x)
> +# define const_be64(_x)                          \
> +    ((((_x) & 0x00000000000000ffU) << 56) |      \
> +     (((_x) & 0x000000000000ff00U) << 40) |      \
> +     (((_x) & 0x0000000000ff0000U) << 24) |      \
> +     (((_x) & 0x00000000ff000000U) <<  8) |      \
> +     (((_x) & 0x000000ff00000000U) >>  8) |      \
> +     (((_x) & 0x0000ff0000000000U) >> 24) |      \
> +     (((_x) & 0x00ff000000000000U) >> 40) |      \
> +     (((_x) & 0xff00000000000000U) >> 56))
>  # define const_be32(_x)                          \
>      ((((_x) & 0x000000ffU) << 24) |              \
>       (((_x) & 0x0000ff00U) <<  8) |              \
> 

This duplication suggests that we define these as const_bswap64, and define
const_le64/be64 on top of that.  Similar for the other defines as you're moving
them.


r~

[PATCH 4/4] qemu/bswap: Let cpu_to_endian() functions handle constant expressions

[Philippe Mathieu-Daudé]

cpu_to_endian() and endian_to_cpu() can be extended to handle
constant expressions. That way the programmer doesn't need to
remember the const_X() API exists.

Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/bswap.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index de256cea3ab..8827e4760b9 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -191,12 +191,16 @@ static inline void bswap64s(uint64_t *s)
 #define CPU_CONVERT(endian, size, type)\
 static inline type endian ## size ## _to_cpu(type v)\
 {\
-    return glue(endian, _bswap)(v, size);\
+    return __builtin_constant_p(v) ?\
+           const_ ## endian ## size(v) :\
+           glue(endian, _bswap)(v, size);\
 }\
 \
 static inline type cpu_to_ ## endian ## size(type v)\
 {\
-    return glue(endian, _bswap)(v, size);\
+    return __builtin_constant_p(v) ?\
+           const_ ## endian ## size(v) :\
+           glue(endian, _bswap)(v, size);\
 }\
 \
 static inline void endian ## size ## _to_cpus(type *p)\
-- 
2.26.2

Re: [PATCH 4/4] qemu/bswap: Let cpu_to_endian() functions handle constant expressions

[Richard Henderson]

On 9/17/20 9:31 AM, Philippe Mathieu-Daudé wrote:
> cpu_to_endian() and endian_to_cpu() can be extended to handle
> constant expressions. That way the programmer doesn't need to
> remember the const_X() API exists.
> 
> Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qemu/bswap.h | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index de256cea3ab..8827e4760b9 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -191,12 +191,16 @@ static inline void bswap64s(uint64_t *s)
>  #define CPU_CONVERT(endian, size, type)\
>  static inline type endian ## size ## _to_cpu(type v)\
>  {\
> -    return glue(endian, _bswap)(v, size);\
> +    return __builtin_constant_p(v) ?\
> +           const_ ## endian ## size(v) :\
> +           glue(endian, _bswap)(v, size);\
>  }\
>  \
>  static inline type cpu_to_ ## endian ## size(type v)\
>  {\
> -    return glue(endian, _bswap)(v, size);\
> +    return __builtin_constant_p(v) ?\
> +           const_ ## endian ## size(v) :\
> +           glue(endian, _bswap)(v, size);\
>  }\
>  \
>  static inline void endian ## size ## _to_cpus(type *p)\
> 

How does this actually affect code generation?

If it does, then that's a mistake in our definition of bswap{16,32,64}(), which
should have been able to fold constants just fine.

Looking at all of that CONFIG_MACHINE_BSWAP_H stuff, I think we should just
ditch it all in favour of __builin_bswap*.  The final piece, __builtin_bswap16,
came in at ac868f29d7e8 in gcc-4.8.


r~