Re: [Patch][i386] PR 70118: Fix ubsan warning on SSE2 loadl_epi64 and storel_epi64

Re: [Patch][i386] PR 70118: Fix ubsan warning on SSE2 loadl_epi64 and storel_epi64

[Uros Bizjak]

Hello!

> 2016-11-30  Allan Sandfeld Jensen  <allan.jensen@qt.io>
>
>        PR target/70118
>        * gcc/config/i386/mmintrin.h (__m64_u): New type
>        * gcc/config/i386/emmintrin.h (_mm_loadl_epi64, _mm_storel_epi64):
>        Make the allowed unaligned memory access explicit.

OK.

Thanks,
Uros.

Re: [Patch][i386] PR 70118: Fix ubsan warning on SSE2 loadl_epi64 and storel_epi64

[Allan Sandfeld Jensen]

On Tuesday 06 December 2016, Uros Bizjak wrote:
> Hello!
> 
> > 2016-11-30  Allan Sandfeld Jensen  <allan.jensen@qt.io>
> > 
> >        PR target/70118
> >        * gcc/config/i386/mmintrin.h (__m64_u): New type
> >        * gcc/config/i386/emmintrin.h (_mm_loadl_epi64, _mm_storel_epi64):
> >        Make the allowed unaligned memory access explicit.
> 
> OK.
> 
Thanks

Note I don't have write access.

Best regards
`Allan

Re: [Patch][i386] PR 70118: Fix ubsan warning on SSE2 loadl_epi64 and storel_epi64

[Uros Bizjak]

On Fri, Dec 9, 2016 at 11:49 PM, Allan Sandfeld Jensen
<linux@carewolf.com> wrote:
> On Tuesday 06 December 2016, Uros Bizjak wrote:
>> Hello!
>>
>> > 2016-11-30  Allan Sandfeld Jensen  <allan.jensen@qt.io>
>> >
>> >        PR target/70118
>> >        * gcc/config/i386/mmintrin.h (__m64_u): New type
>> >        * gcc/config/i386/emmintrin.h (_mm_loadl_epi64, _mm_storel_epi64):
>> >        Make the allowed unaligned memory access explicit.
>>
>> OK.
>>
> Thanks
>
> Note I don't have write access.

Committed to mainline SVN as r243527.

Uros.

Re: [Patch][i386] PR 70118: Fix ubsan warning on SSE2 loadl_epi64 and storel_epi64

[Allan Sandfeld Jensen]

[-- Attachment #1: Type: Text/Plain, Size: 315 bytes --]

Trying again, this time with changelog.

gcc/

2016-11-30  Allan Sandfeld Jensen  <allan.jensen@qt.io>

        PR target/70118
        * gcc/config/i386/mmintrin.h (__m64_u): New type
        * gcc/config/i386/emmintrin.h (_mm_loadl_epi64, _mm_storel_epi64):
	   Make the allowed unaligned memory access explicit.

[-- Attachment #2: unaligned_sse2_epi64.diff --]
[-- Type: text/x-patch, Size: 1653 bytes --]

Index: gcc/config/i386/emmintrin.h
===================================================================
--- gcc/config/i386/emmintrin.h	(revision 242892)
+++ gcc/config/i386/emmintrin.h	(working copy)
@@ -703,9 +703,9 @@
 }
 
 extern __inline __m128i __attribute__((__gnu_inline__, __always_inline__, __artificial__))
-_mm_loadl_epi64 (__m128i const *__P)
+_mm_loadl_epi64 (__m128i_u const *__P)
 {
-  return _mm_set_epi64 ((__m64)0LL, *(__m64 *)__P);
+  return _mm_set_epi64 ((__m64)0LL, *(__m64_u *)__P);
 }
 
 extern __inline void __attribute__((__gnu_inline__, __always_inline__, __artificial__))
@@ -721,9 +721,9 @@
 }
 
 extern __inline void __attribute__((__gnu_inline__, __always_inline__, __artificial__))
-_mm_storel_epi64 (__m128i *__P, __m128i __B)
+_mm_storel_epi64 (__m128i_u *__P, __m128i __B)
 {
-  *(long long *)__P = ((__v2di)__B)[0];
+  *(__m64_u *)__P = (__m64) ((__v2di)__B)[0];
 }
 
 extern __inline __m64 __attribute__((__gnu_inline__, __always_inline__, __artificial__))
Index: gcc/config/i386/mmintrin.h
===================================================================
--- gcc/config/i386/mmintrin.h	(revision 242892)
+++ gcc/config/i386/mmintrin.h	(working copy)
@@ -37,6 +37,9 @@
    vector types, and their scalar components.  */
 typedef int __m64 __attribute__ ((__vector_size__ (8), __may_alias__));
 
+/* Unaligned version of the same type  */
+typedef int __m64_u __attribute__ ((__vector_size__ (8), __may_alias__, __aligned__ (1)));
+
 /* Internal data types for implementing the intrinsics.  */
 typedef int __v2si __attribute__ ((__vector_size__ (8)));
 typedef short __v4hi __attribute__ ((__vector_size__ (8)));

Re: [Patch][i386] PR 70118: Fix ubsan warning on SSE2 loadl_epi64 and storel_epi64

[Allan Sandfeld Jensen]

[-- Attachment #1: Type: Text/Plain, Size: 855 bytes --]

On Sunday 27 November 2016, Marc Glisse wrote:
> On Sat, 26 Nov 2016, Allan Sandfeld Jensen wrote:
> > Use the recently introduced unaligned variant of __m128i and add a
> > similar __m64 and use those to make it clear these two intrinsics
> > require neither 128- bit nor 64-bit alignment.
> 
> Thanks for doing this. You'll want Uros or Kirill to review your patch.
> There are probably several more places that could do with an unaligned
> fix, but we don't have to find them all at once.
> First I found it strange to use __m64, but then it actually seems like a
> good call to use a type that is not just aligned(1) but also may_alias.
> 
> +  *(__m64_u *)__P = __m64(((__v2di)__B)[0]);
> 
> gcc complains about this syntax for me, it wants parentheses around
> __m64... Did it pass the testsuite for you?

Fixed, it now matches the move just below.

[-- Attachment #2: unaligned_sse2_epi64.diff --]
[-- Type: text/x-patch, Size: 1653 bytes --]

Index: gcc/config/i386/emmintrin.h
===================================================================
--- gcc/config/i386/emmintrin.h	(revision 242892)
+++ gcc/config/i386/emmintrin.h	(working copy)
@@ -703,9 +703,9 @@
 }
 
 extern __inline __m128i __attribute__((__gnu_inline__, __always_inline__, __artificial__))
-_mm_loadl_epi64 (__m128i const *__P)
+_mm_loadl_epi64 (__m128i_u const *__P)
 {
-  return _mm_set_epi64 ((__m64)0LL, *(__m64 *)__P);
+  return _mm_set_epi64 ((__m64)0LL, *(__m64_u *)__P);
 }
 
 extern __inline void __attribute__((__gnu_inline__, __always_inline__, __artificial__))
@@ -721,9 +721,9 @@
 }
 
 extern __inline void __attribute__((__gnu_inline__, __always_inline__, __artificial__))
-_mm_storel_epi64 (__m128i *__P, __m128i __B)
+_mm_storel_epi64 (__m128i_u *__P, __m128i __B)
 {
-  *(long long *)__P = ((__v2di)__B)[0];
+  *(__m64_u *)__P = (__m64) ((__v2di)__B)[0];
 }
 
 extern __inline __m64 __attribute__((__gnu_inline__, __always_inline__, __artificial__))
Index: gcc/config/i386/mmintrin.h
===================================================================
--- gcc/config/i386/mmintrin.h	(revision 242892)
+++ gcc/config/i386/mmintrin.h	(working copy)
@@ -37,6 +37,9 @@
    vector types, and their scalar components.  */
 typedef int __m64 __attribute__ ((__vector_size__ (8), __may_alias__));
 
+/* Unaligned version of the same type  */
+typedef int __m64_u __attribute__ ((__vector_size__ (8), __may_alias__, __aligned__ (1)));
+
 /* Internal data types for implementing the intrinsics.  */
 typedef int __v2si __attribute__ ((__vector_size__ (8)));
 typedef short __v4hi __attribute__ ((__vector_size__ (8)));

Re: [Patch][i386] PR 70118: Fix ubsan warning on SSE2 loadl_epi64 and storel_epi64

[Marc Glisse]

On Sat, 26 Nov 2016, Allan Sandfeld Jensen wrote:

> Use the recently introduced unaligned variant of __m128i and add a similar
> __m64 and use those to make it clear these two intrinsics require neither 128-
> bit nor 64-bit alignment.

Thanks for doing this. You'll want Uros or Kirill to review your patch.
There are probably several more places that could do with an unaligned 
fix, but we don't have to find them all at once.
First I found it strange to use __m64, but then it actually seems like a 
good call to use a type that is not just aligned(1) but also may_alias.

+  *(__m64_u *)__P = __m64(((__v2di)__B)[0]);

gcc complains about this syntax for me, it wants parentheses around 
__m64... Did it pass the testsuite for you?
On the other hand, this seems less complicated:

   *(__m64_u *)__P = *(__m64*)&__B;

I am now wondering if we are not using the __v2di-like types too much, in 
places where the lack of may_alias might be an issue... Or maybe I am 
afraid for no reason and even here the may_alias is unnecessary. Looking 
at dumps also makes me wonder if we could simplify 
view_convert_expr(bit_field_expr) to just bit_field_expr when it is the 
only use.

-- 
Marc Glisse

[Patch][i386] PR 70118: Fix ubsan warning on SSE2 loadl_epi64 and storel_epi64

[Allan Sandfeld Jensen]

[-- Attachment #1: Type: text/plain, Size: 189 bytes --]

Use the recently introduced unaligned variant of __m128i and add a similar 
__m64 and use those to make it clear these two intrinsics require neither 128-
bit nor 64-bit alignment.

`Allan

[-- Attachment #2: unaligned-sse_epi64.diff --]
[-- Type: text/x-patch, Size: 1652 bytes --]

Index: gcc/config/i386/emmintrin.h
===================================================================
--- gcc/config/i386/emmintrin.h	(revision 242753)
+++ gcc/config/i386/emmintrin.h	(working copy)
@@ -703,9 +703,9 @@
 }
 
 extern __inline __m128i __attribute__((__gnu_inline__, __always_inline__, __artificial__))
-_mm_loadl_epi64 (__m128i const *__P)
+_mm_loadl_epi64 (__m128i_u const *__P)
 {
-  return _mm_set_epi64 ((__m64)0LL, *(__m64 *)__P);
+  return _mm_set_epi64 ((__m64)0LL, *(__m64_u *)__P);
 }
 
 extern __inline void __attribute__((__gnu_inline__, __always_inline__, __artificial__))
@@ -721,9 +721,9 @@
 }
 
 extern __inline void __attribute__((__gnu_inline__, __always_inline__, __artificial__))
-_mm_storel_epi64 (__m128i *__P, __m128i __B)
+_mm_storel_epi64 (__m128i_u *__P, __m128i __B)
 {
-  *(long long *)__P = ((__v2di)__B)[0];
+  *(__m64_u *)__P = __m64(((__v2di)__B)[0]);
 }
 
 extern __inline __m64 __attribute__((__gnu_inline__, __always_inline__, __artificial__))
Index: gcc/config/i386/mmintrin.h
===================================================================
--- gcc/config/i386/mmintrin.h	(revision 242753)
+++ gcc/config/i386/mmintrin.h	(working copy)
@@ -37,6 +37,9 @@
    vector types, and their scalar components.  */
 typedef int __m64 __attribute__ ((__vector_size__ (8), __may_alias__));
 
+/* Unaligned version of the same type  */
+typedef int __m64_u __attribute__ ((__vector_size__ (8), __may_alias__, __aligned__ (1)));
+
 /* Internal data types for implementing the intrinsics.  */
 typedef int __v2si __attribute__ ((__vector_size__ (8)));
 typedef short __v4hi __attribute__ ((__vector_size__ (8)));