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