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

[Siarhei Siamashka]

On Mon, 4 Jun 2018 10:04:15 -0700

__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

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
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
---
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.

[Adam Jackson]

I think we're starting to be well overdue for an 0.36 release, but I'd
like to take the opportunity to suggest moving to fdo's gitlab as we do
https://gitlab.freedesktop.org/ajax/pixman/-/jobs/986

Agreed.

Filed an infrastructure ticket to track this:

https://gitlab.freedesktop.org/freedesktop/freedesktop/issues/31

I would like to make 0.36 pass the test suite with clang, so if you
have any time or interest I'd appreciate a second set of eyes. I'll
filed https://bugs.freedesktop.org/show_bug.cgi?id=106818 so we can
track it.

I can at least try to provoke some more interesting data.

I also need to take some time to look into the Loongson3 patch. If
you're not in a particular hurry, it would be nice to get that in.

patchwork doesn't know about that one, neither do I see anything that
sounds like a Loongson3 patch in bugzilla. Where is this? (Presumably
we should stop this thread and simply track this on the gitlab ticket.)

- ajax