Patch to fix test for GCC vector extensions

Submitted by Rob Tsuk on Aug. 7, 2017, 2:52 p.m.

Details

Message ID 4A4006FD-E49D-4E9A-88A3-67071DA3DA0B@tsuk.com
State New
Headers show
Series "Patch to fix test for GCC vector extensions" ( rev: 1 ) in Pixman

Not browsing as part of any series.

Commit Message

Rob Tsuk Aug. 7, 2017, 2:52 p.m.
From 662b17d0d691b567a3343af3439f5fbefe989584 Mon Sep 17 00:00:00 2001
From: Rob Tsuk <robtsuk@google.com>
Date: Mon, 7 Aug 2017 07:11:10 -0700
Subject: [PATCH 1/1] Fix test for GCC vector extensions

The previous test was getting a false positive for
clang version 4.
---
 configure.ac | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/configure.ac b/configure.ac
index e833e45..cbebc82 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1101,7 +1101,7 @@  support_for_gcc_vector_extensions=no
 AC_MSG_CHECKING(for GCC vector extensions)
 AC_LINK_IFELSE([AC_LANG_SOURCE([[
 unsigned int __attribute__ ((vector_size(16))) e, a, b;
-int main (void) { e = a - ((b << 27) + (b >> (32 - 27))) + 1; return e[0]; }
+int main (void) { __builtin_shuffle(a,b);e = a - ((b << 27) + (b >> (32 - 27))) + 1; return e[0]; }
 ]])], support_for_gcc_vector_extensions=yes)
 
 if test x$support_for_gcc_vector_extensions = xyes; then

Comments

On Mon, 7 Aug 2017 07:52:29 -0700
Rob Tsuk <rob@tsuk.com> wrote:

> From 662b17d0d691b567a3343af3439f5fbefe989584 Mon Sep 17 00:00:00 2001
> From: Rob Tsuk <robtsuk@google.com>
> Date: Mon, 7 Aug 2017 07:11:10 -0700
> Subject: [PATCH 1/1] Fix test for GCC vector extensions
> 
> The previous test was getting a false positive for
> clang version 4.
> ---
>  configure.ac | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure.ac b/configure.ac
> index e833e45..cbebc82 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1101,7 +1101,7 @@ support_for_gcc_vector_extensions=no
>  AC_MSG_CHECKING(for GCC vector extensions)
>  AC_LINK_IFELSE([AC_LANG_SOURCE([[
>  unsigned int __attribute__ ((vector_size(16))) e, a, b;
> -int main (void) { e = a - ((b << 27) + (b >> (32 - 27))) + 1; return e[0]; }
> +int main (void) { __builtin_shuffle(a,b);e = a - ((b << 27) + (b >> (32 - 27))) + 1; return e[0]; }
>  ]])], support_for_gcc_vector_extensions=yes)
>  
>  if test x$support_for_gcc_vector_extensions = xyes; then

Thanks, the patch looks mostly good. We indeed have to add
the use of __builtin_shuffle() to the configure check code
snippet because it is used by pixman and Clang implements
this functionality in a different and incompatible way.

We did not see this problem with older versions of Clang
because it did not support the shift vector by scalar operation
in its vector extensions.

Too bad that GCC and Clang parted their ways and we can't have
a single unified implementation. The whole reason to use GCC
vector extension was that we could avoid platform-dependent
intrinsics in this code.

About the patch. It would be great to use the result of
__builtin_shuffle() in the intermediate calculations just in
case. See
   https://cgit.freedesktop.org/pixman/commit/?id=a566f627dbd6ea8f2cba70a446e62caaa2ecbd26
Your proposed modification to my patch makes sense. I’m not certain when I”ll have time to perform it.

> On Sep 17, 2017, at 6:40 AM, Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote:
> 
> On Mon, 7 Aug 2017 07:52:29 -0700
> Rob Tsuk <rob@tsuk.com <mailto:rob@tsuk.com>> wrote:
> 
>> From 662b17d0d691b567a3343af3439f5fbefe989584 Mon Sep 17 00:00:00 2001
>> From: Rob Tsuk <robtsuk@google.com>
>> Date: Mon, 7 Aug 2017 07:11:10 -0700
>> Subject: [PATCH 1/1] Fix test for GCC vector extensions
>> 
>> The previous test was getting a false positive for
>> clang version 4.
>> ---
>> configure.ac | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/configure.ac b/configure.ac
>> index e833e45..cbebc82 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -1101,7 +1101,7 @@ support_for_gcc_vector_extensions=no
>> AC_MSG_CHECKING(for GCC vector extensions)
>> AC_LINK_IFELSE([AC_LANG_SOURCE([[
>> unsigned int __attribute__ ((vector_size(16))) e, a, b;
>> -int main (void) { e = a - ((b << 27) + (b >> (32 - 27))) + 1; return e[0]; }
>> +int main (void) { __builtin_shuffle(a,b);e = a - ((b << 27) + (b >> (32 - 27))) + 1; return e[0]; }
>> ]])], support_for_gcc_vector_extensions=yes)
>> 
>> if test x$support_for_gcc_vector_extensions = xyes; then
> 
> Thanks, the patch looks mostly good. We indeed have to add
> the use of __builtin_shuffle() to the configure check code
> snippet because it is used by pixman and Clang implements
> this functionality in a different and incompatible way.
> 
> We did not see this problem with older versions of Clang
> because it did not support the shift vector by scalar operation
> in its vector extensions.
> 
> Too bad that GCC and Clang parted their ways and we can't have
> a single unified implementation. The whole reason to use GCC
> vector extension was that we could avoid platform-dependent
> intrinsics in this code.
> 
> About the patch. It would be great to use the result of
> __builtin_shuffle() in the intermediate calculations just in
> case. See
>   https://cgit.freedesktop.org/pixman/commit/?id=a566f627dbd6ea8f2cba70a446e62caaa2ecbd26 <https://cgit.freedesktop.org/pixman/commit/?id=a566f627dbd6ea8f2cba70a446e62caaa2ecbd26>
> 
> -- 
> Best regards,
> Siarhei Siamashka
On Mon, 18 Sep 2017 07:24:49 -0700
Rob Tsuk <rob@tsuk.com> wrote:

> > On Sep 17, 2017, at 6:40 AM, Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote:
> > 
> > On Mon, 7 Aug 2017 07:52:29 -0700
> > Rob Tsuk <rob@tsuk.com <mailto:rob@tsuk.com>> wrote:
> >   
> >> From 662b17d0d691b567a3343af3439f5fbefe989584 Mon Sep 17 00:00:00 2001
> >> From: Rob Tsuk <robtsuk@google.com>
> >> Date: Mon, 7 Aug 2017 07:11:10 -0700
> >> Subject: [PATCH 1/1] Fix test for GCC vector extensions
> >> 
> >> The previous test was getting a false positive for
> >> clang version 4.
> >> ---
> >> configure.ac | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/configure.ac b/configure.ac
> >> index e833e45..cbebc82 100644
> >> --- a/configure.ac
> >> +++ b/configure.ac
> >> @@ -1101,7 +1101,7 @@ support_for_gcc_vector_extensions=no
> >> AC_MSG_CHECKING(for GCC vector extensions)
> >> AC_LINK_IFELSE([AC_LANG_SOURCE([[
> >> unsigned int __attribute__ ((vector_size(16))) e, a, b;
> >> -int main (void) { e = a - ((b << 27) + (b >> (32 - 27))) + 1; return e[0]; }
> >> +int main (void) { __builtin_shuffle(a,b);e = a - ((b << 27) + (b >> (32 - 27))) + 1; return e[0]; }
> >> ]])], support_for_gcc_vector_extensions=yes)
> >> 
> >> if test x$support_for_gcc_vector_extensions = xyes; then  
> > 
> > Thanks, the patch looks mostly good. We indeed have to add
> > the use of __builtin_shuffle() to the configure check code
> > snippet because it is used by pixman and Clang implements
> > this functionality in a different and incompatible way.
> > 
> > We did not see this problem with older versions of Clang
> > because it did not support the shift vector by scalar operation
> > in its vector extensions.
> > 
> > Too bad that GCC and Clang parted their ways and we can't have
> > a single unified implementation. The whole reason to use GCC
> > vector extension was that we could avoid platform-dependent
> > intrinsics in this code.
> > 
> > About the patch. It would be great to use the result of
> > __builtin_shuffle() in the intermediate calculations just in
> > case. See
> >   https://cgit.freedesktop.org/pixman/commit/?id=a566f627dbd6ea8f2cba70a446e62caaa2ecbd26 <https://cgit.freedesktop.org/pixman/commit/?id=a566f627dbd6ea8f2cba70a446e62caaa2ecbd26>
>
> Your proposed modification to my patch makes sense. I’m not certain when I”ll have time to perform it.

It does not really require much time. I just did that for you:
https://lists.freedesktop.org/archives/pixman/2017-September/004680.html
Thanks! It had been so long since I’d submitted the patch that I’d forgotten on which machine I’d done the work and what I needed to do to get set up to modify it. I’m glad you found it easy to do for me.

> On Sep 18, 2017, at 9:54 AM, Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote:
> 
> On Mon, 18 Sep 2017 07:24:49 -0700
> Rob Tsuk <rob@tsuk.com> wrote:
> 
>>> On Sep 17, 2017, at 6:40 AM, Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote:
>>> 
>>> On Mon, 7 Aug 2017 07:52:29 -0700
>>> Rob Tsuk <rob@tsuk.com <mailto:rob@tsuk.com>> wrote:
>>> 
>>>> From 662b17d0d691b567a3343af3439f5fbefe989584 Mon Sep 17 00:00:00 2001
>>>> From: Rob Tsuk <robtsuk@google.com>
>>>> Date: Mon, 7 Aug 2017 07:11:10 -0700
>>>> Subject: [PATCH 1/1] Fix test for GCC vector extensions
>>>> 
>>>> The previous test was getting a false positive for
>>>> clang version 4.
>>>> ---
>>>> configure.ac | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 
>>>> diff --git a/configure.ac b/configure.ac
>>>> index e833e45..cbebc82 100644
>>>> --- a/configure.ac
>>>> +++ b/configure.ac
>>>> @@ -1101,7 +1101,7 @@ support_for_gcc_vector_extensions=no
>>>> AC_MSG_CHECKING(for GCC vector extensions)
>>>> AC_LINK_IFELSE([AC_LANG_SOURCE([[
>>>> unsigned int __attribute__ ((vector_size(16))) e, a, b;
>>>> -int main (void) { e = a - ((b << 27) + (b >> (32 - 27))) + 1; return e[0]; }
>>>> +int main (void) { __builtin_shuffle(a,b);e = a - ((b << 27) + (b >> (32 - 27))) + 1; return e[0]; }
>>>> ]])], support_for_gcc_vector_extensions=yes)
>>>> 
>>>> if test x$support_for_gcc_vector_extensions = xyes; then  
>>> 
>>> Thanks, the patch looks mostly good. We indeed have to add
>>> the use of __builtin_shuffle() to the configure check code
>>> snippet because it is used by pixman and Clang implements
>>> this functionality in a different and incompatible way.
>>> 
>>> We did not see this problem with older versions of Clang
>>> because it did not support the shift vector by scalar operation
>>> in its vector extensions.
>>> 
>>> Too bad that GCC and Clang parted their ways and we can't have
>>> a single unified implementation. The whole reason to use GCC
>>> vector extension was that we could avoid platform-dependent
>>> intrinsics in this code.
>>> 
>>> About the patch. It would be great to use the result of
>>> __builtin_shuffle() in the intermediate calculations just in
>>> case. See
>>>  https://cgit.freedesktop.org/pixman/commit/?id=a566f627dbd6ea8f2cba70a446e62caaa2ecbd26 <https://cgit.freedesktop.org/pixman/commit/?id=a566f627dbd6ea8f2cba70a446e62caaa2ecbd26>
>> 
>> Your proposed modification to my patch makes sense. I’m not certain when I”ll have time to perform it.
> 
> It does not really require much time. I just did that for you:
> https://lists.freedesktop.org/archives/pixman/2017-September/004680.html
> 
> -- 
> Best regards,
> Siarhei Siamashka