On Mon, 4 Jun 2018 10:04:15 -0700 Matt Turner <[email protected]> wrote:
> From: Vladimir Smirnov <[email protected]> > > __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 <[email protected]> > Reviewed-by: Matt Turner <[email protected]> > --- > 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 _______________________________________________ Pixman mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/pixman
