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