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

[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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171006/661dbddf/attachment.sig>