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

[Nicolai Hähnle]

On 05.10.2017 20:59, Jochen Rollwagen wrote:
> 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.

This doesn't really address Matt's point that compilers are good at 
pattern matching byte swaps already.

Unless you can actually show with comparisons of the assembly and/or 
benchmarks that this results in better code, your patch makes the code 
base slightly worse because you've now added two different code paths 
where there was previously only one.

Cheers,
Nicolai


> 
>  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[],
> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.