[Pixman] [PATCH] test: Adjust for clang's removal of __builtin_shuffle

[Siarhei Siamashka]

On Mon,  4 Jun 2018 10:04:15 -0700
Matt Turner <mattst88 at gmail.com> wrote:

> From: Vladimir Smirnov <civil at gentoo.org>
> 
> __builtin_shuffle was removed in clang 5.0.

Hi, your patch summary is not exactly accurate. Clang never
had __builtin_shuffle() intrinsic in the first place, as you can
also learn from the linked discussion in the cfe-dev mailing list.

What really happened is explained in the commit message of this patch:
    https://lists.freedesktop.org/archives/pixman/2017-September/004680.html

> 
> Build log says:
> test/utils-prng.c:207:27: error: use of unknown builtin '__builtin_shuffle' [-Wimplicit-function-declaration]
>             randdata.vb = __builtin_shuffle (randdata.vb, bswap_shufflemask);
>                           ^
> test/utils-prng.c:207:25: error: assigning to 'uint8x16' (vector of 16 'uint8_t' values) from incompatible type 'int'
>             randdata.vb = __builtin_shuffle (randdata.vb, bswap_shufflemask);
>                         ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 2 errors generated
> 
> Link to original discussion:
> http://lists.llvm.org/pipermail/cfe-dev/2017-August/055140.html
> 
> It's possible to build pixman if attached patch is applied. Basically
> patch adds check for __builtin_shuffle support and in case there is
> none, falls back to clang-specific __builtin_shufflevector that do the
> same but have different API.
> 
> Bugzilla: https://bugs.gentoo.org/646360
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104886
> Tested-by: Philip Chimento <philip.chimento at gmail.com>
> Reviewed-by: Matt Turner <mattst88 at gmail.com>
> ---
> I turned https://bugs.freedesktop.org/show_bug.cgi?id=104886#c2 into a
> Tested-by tag for Philip.
> 
> I also reversed the order of the preprocessor conditions in order to
> simplify it a bit (the !defined(__clang__) looked like a problem waiting
> to happen).
> 
> Unfortunately combiner-test, gradient-crash-test, and stress-test fail
> when built with clang for unrelated reasons.
> 
>  test/utils-prng.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/test/utils-prng.c b/test/utils-prng.c
> index c27b5be..0cf53dd 100644
> --- a/test/utils-prng.c
> +++ b/test/utils-prng.c
> @@ -199,12 +199,25 @@ randmemset_internal (prng_t                  *prng,
>          }
>          else
>          {
> +
> +#ifndef __has_builtin
> +#define __has_builtin(x) 0
> +#endif
> +
>  #ifdef HAVE_GCC_VECTOR_EXTENSIONS
> -            const uint8x16 bswap_shufflemask =
> +# if __has_builtin(__builtin_shufflevector)
> +            randdata.vb =
> +                __builtin_shufflevector (randdata.vb, randdata.vb,
> +                                          3,  2,  1,  0,  7,  6 , 5,  4,
> +                                         11, 10,  9,  8, 15, 14, 13, 12);
> +# else
> +            static const uint8x16 bswap_shufflemask =
>              {
>                  3, 2, 1, 0, 7, 6, 5, 4, 11, 10, 9, 8, 15, 14, 13, 12
>              };
>              randdata.vb = __builtin_shuffle (randdata.vb, bswap_shufflemask);
> +# endif
> +
>              store_rand_128_data (buf, &randdata, aligned);
>              buf += 16;
>  #else

This looks somewhat ugly, but it's okay if it works. I'm not
particularly happy about GCC and Clang developers deliberately
trying to make their compilers incompatible (a NIH syndrome?).

GCC's point of view (the GCC documentation):

   "Note that __builtin_shuffle is intentionally semantically compatible
    with the OpenCL shuffle and shuffle2 functions. "

Clang's point of view (the guy from the cfe-dev mailing list):

   "I don't think it was ever supported at all. You are not suppossed to
    use the builtins directly, but the frontend macros/functions from
    *imm.h. Of course, I wouldn't be surprised if GCC added a random
    non-standard builtin..."

The whole point of using vector extensions for optimizing the PRNG code
in pixman was that we automatically have fast vectorized code on all
platforms and don't need use SSE2, ARM NEON, Altivec/VMX or any other
platform dependent intrinsics in multiple ifdef blocks. So we get a
better performance for "make check" as long as the compiler is GCC.

While searching some information about __has_builtin() macro
support, I found this GCC feature request

    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66970

and also a funny issue reported at stackoverflow:

    https://stackoverflow.com/questions/42744425/clangs-has-builtin-doesnt-always-work

This has some potential to give us headache in the future, but the
worst that may happen would be just a pixman compilation failure or
a problem detected by the test suite. Then we would need to look at
this shit and add more checks/workarounds as needed.

I think that the older patch for this issue is somewhat cleaner
because it limits the use of vector extensions to just GCC rather
than trying to satisfy both GCC and Clang at the same time. But
with your patch we also get better performance for Clang, so
that's a good reason to prefer it.

-- 
Best regards,
Siarhei Siamashka