[Mesa-dev] [PATCH] Replace byte-swapping code with builtins in pack.c

[Jochen Rollwagen]

Am 04.10.2017 um 05:59 schrieb Matt Turner:
> On Tue, Oct 3, 2017 at 11:01 AM, Jochen Rollwagen <joro-2013 at t-online.de> wrote:
>>  From 4cebe50a9bade6717923e104c954f3fad75f71bb Mon Sep 17 00:00:00 2001
>> From: Jochen Rollwagen <joro-2013 at 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-2013 at 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

  #define SWAP2BYTE(VALUE)            \
     {                        \
@@ -249,7 +255,7 @@ _mesa_pack_bitmap( GLint width, GLint height, const 
GLubyte
*source,
        bytes[1] = bytes[2];            \
        bytes[2] = tmp;                \
     }
-
+#endif

  static void
  extract_uint_indexes(GLuint n, GLuint indexes[],
-- 
1.7.9.5

Cheers

Jochen
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Simplify-byte-swapping-code-in-pack.c-on-Linux-syste.patch
Type: text/x-patch
Size: 1092 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171005/9d61e005/attachment-0001.bin>