Am 06.10.2017 um 18:40 schrieb Dylan Baker:
Quoting Erik Faye-Lund (2017-10-06 00:31:20)
On Thu, Oct 5, 2017 at 8:59 PM, Jochen Rollwagen <joro-2...@t-online.de> wrote:
Am 04.10.2017 um 05:59 schrieb Matt Turner:
On Tue, Oct 3, 2017 at 11:01 AM, Jochen Rollwagen <joro-2...@t-online.de>
wrote:
  From 4cebe50a9bade6717923e104c954f3fad75f71bb Mon Sep 17 00:00:00 2001
From: Jochen Rollwagen <joro-2...@t-online.de>
Date: Tue, 3 Oct 2017 19:54:10 +0200
Subject: [PATCH] Replace byte-swapping code with builtins in pack.c

This patch replaces some code for byte-swapping in pack.c with the
builtin
functions allowing the compiler to do its optimization magic
---
   src/mesa/main/pack.c |   22 ++--------------------
   1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/src/mesa/main/pack.c b/src/mesa/main/pack.c
index 94a6d28..9bfde39 100644
--- a/src/mesa/main/pack.c
+++ b/src/mesa/main/pack.c
@@ -230,26 +230,8 @@ _mesa_pack_bitmap( GLint width, GLint height, const
GLubyte
   *source,
      }
   }

-
-#define SWAP2BYTE(VALUE)            \
-   {                        \
-      GLubyte *bytes = (GLubyte *) &(VALUE);    \
-      GLubyte tmp = bytes[0];            \
-      bytes[0] = bytes[1];            \
-      bytes[1] = tmp;                \
-   }
-
-#define SWAP4BYTE(VALUE)            \
-   {                        \
-      GLubyte *bytes = (GLubyte *) &(VALUE);    \
-      GLubyte tmp = bytes[0];            \
-      bytes[0] = bytes[3];            \
-      bytes[3] = tmp;                \
-      tmp = bytes[1];                \
-      bytes[1] = bytes[2];            \
-      bytes[2] = tmp;                \
-   }
-
+#define SWAP2BYTE(VALUE) __builtin_bswap16(VALUE)
+#define SWAP4BYTE(VALUE) __builtin_bswap32(VALUE)
In my experience it's much simpler to just write these as

         return ((x & 0xff) << 8) | ((x >> 8) & 0xff);

and

         return  ((x & 0xff) << 24) |
                 ((x & 0xff00) << 8) |
                 ((x & 0xff0000) >> 8) |
                 ((x >> 24) & 0xff);

and not have to deal with compiler intrinsics. Compilers will
recognize these patterns and use the appropriate instructions (rol for
2-bytes and bswap for 4-bytes).

You should be able to count the numbers of those instructions before
and after such a patch to confirm it's working as expected.
Fair enough. I've attached a new patch that optimizes the code on linux
systems only where there is byteswap.h containing (hopefully optimal)
functions. The other systems may be dealt with by a followup patch if
desired.

 From 327012410f75f77010b658ce9643a229c45bc308 Mon Sep 17 00:00:00 2001
From: Jochen Rollwagen <joro-2...@t-online.de>
Date: Thu, 5 Oct 2017 20:48:46 +0200
Subject: [PATCH] Simplify byte swapping code in pack.c on Linux systems

This patch replaces the code for byte swapping in pack.c with the function
from
byteswap.h on Linux systems
---
  src/mesa/main/pack.c |    8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/mesa/main/pack.c b/src/mesa/main/pack.c
index 94a6d28..d8ab267 100644
--- a/src/mesa/main/pack.c
+++ b/src/mesa/main/pack.c
@@ -230,6 +230,12 @@ _mesa_pack_bitmap( GLint width, GLint height, const
GLubyte
  *source,
     }
  }

+#ifdef __linux__
+#include <byteswap.h>
+
+#define SWAP2BYTE(VALUE) bswap_16(VALUE)
+#define SWAP4BYTE(VALUE) bswap_32(VALUE)
+#else
Another alternative would be to use:

AX_GCC_BUILTIN / HAVE___BUILTIN_BSWAP{16,32}

...to check explicitly for the builtins, like we do for other
builtins. We already do this for __builtin_bswap32 and
__builtin_bswap64, so it would seem like a pretty straight-forward
extension of that pattern.
Not withstanding Matt and Nicolai's points:

We already check for bswap builtins in configure (and meson), and would be the
right way to guard this since this isn't really linux specific, it's gcc/clang
specific, and I *think* the BSD's would benefit as well.

Dylan

Here's the latest version of the patch adding a check for __builtin_bswap16 in configure and using the bswap's in pack.c if present. Additionally, the new patch is heavily inspired by code from drivers/dri/i965/intel_tiled_memcpy.c :-)

However, although i changed configure.ac the new check doesn't seem to fire when configure'ing ? Since i'm not an autotools-expert i'm somewhat lost here.

As for
>From 1866d03a2231e65a8460f7acaa3b92af9c882e55 Mon Sep 17 00:00:00 2001
From: Jochen Rollwagen <joro-2...@t-online.de>
Date: Sun, 8 Oct 2017 18:22:17 +0200
Subject: [PATCH] Check for __builtin_bswap16 in configure and use
 __builtin_bswap16/32 in pack.c if present

This patch adds the builtin function __builtin_bswap16 as a check in configure.ac and uses the builtin byte swapping functions in pack.c if defined
---
 configure.ac         |    1 +
 src/mesa/main/pack.c |   41 ++++++++++++++++++++++-------------------
 2 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/configure.ac b/configure.ac
index 903a397..79fcd8b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -221,6 +221,7 @@ if test "x$SUNCC" = xyes; then
 fi
 
 dnl Check for compiler builtins
+AX_GCC_BUILTIN([__builtin_bswap16])
 AX_GCC_BUILTIN([__builtin_bswap32])
 AX_GCC_BUILTIN([__builtin_bswap64])
 AX_GCC_BUILTIN([__builtin_clz])
diff --git a/src/mesa/main/pack.c b/src/mesa/main/pack.c
index 94a6d28..a4a683a 100644
--- a/src/mesa/main/pack.c
+++ b/src/mesa/main/pack.c
@@ -230,26 +230,29 @@ _mesa_pack_bitmap( GLint width, GLint height, const GLubyte *source,
    }
 }
 
+static inline uint32_t
+SWAP4BYTE(uint32_t n)
+{
+#if defined(HAVE___BUILTIN_BSWAP32)
+   return __builtin_bswap32(n);
+#else
+   return (n >> 24) |
+          ((n >> 8) & 0x0000ff00) |
+          ((n << 8) & 0x00ff0000) |
+          (n << 24);
+#endif
+}
 
-#define SWAP2BYTE(VALUE)			\
-   {						\
-      GLubyte *bytes = (GLubyte *) &(VALUE);	\
-      GLubyte tmp = bytes[0];			\
-      bytes[0] = bytes[1];			\
-      bytes[1] = tmp;				\
-   }
-
-#define SWAP4BYTE(VALUE)			\
-   {						\
-      GLubyte *bytes = (GLubyte *) &(VALUE);	\
-      GLubyte tmp = bytes[0];			\
-      bytes[0] = bytes[3];			\
-      bytes[3] = tmp;				\
-      tmp = bytes[1];				\
-      bytes[1] = bytes[2];			\
-      bytes[2] = tmp;				\
-   }
-
+static inline uint16_t
+SWAP2BYTE(uint16_t n)
+{
+#if defined(HAVE___BUILTIN_BSWAP16)
+   return __builtin_bswap16(n);
+#else
+   return ((n >> 8) & 0x00ff) |
+          ((n << 8) & 0xff00);
+#endif
+}
 
 static void
 extract_uint_indexes(GLuint n, GLuint indexes[],
-- 
1.7.9.5

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to