[Mesa-dev] [PATCH] Check for __builtin_bswap16 in configure and use __builtin_bswap16/32 in pack.c if present

[Jochen Rollwagen]

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-2013 at 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-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
>> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Check-for-__builtin_bswap16-in-configure-and-use-__b.patch
Type: text/x-patch
Size: 2175 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171008/7baa13c6/attachment-0001.bin>