[X86][SSE] _mm_store1_ps/_mm_store1_pd should require an aligned pointer

[RKSimon]

Summary

According to the gcc headers, intel intrinsics docs and msdn codegen the _mm_store1_ps/_mm_store1_pd (and their _mm_store_ps1/_mm_store_pd1 analogues) should require an aligned pointer - the clang headers are the only implementation I can find that assume non-aligned stores (by storing with _mm_storeu_ps/_mm_storeu_pd).

This patch raises the alignment requirements to match the other implementations by calling _mm_store_ps/_mm_store_pd instead.

I've also added the missing _mm_store_pd1 intrinsic (which maps to _mm_store1_pd like _mm_store_ps1 does to _mm_store1_ps).

As a followup I'll update the llvm fast-isel tests to match this codegen.

[craig.topper]

Can you double check gcc's xmmintrin.h again. I'm pretty sure _mm_store1_ps
is calling _mm_storeu_ps.

[RKSimon]

In D20617#439200, @craig.topper wrote:

Can you double check gcc's xmmintrin.h again. I'm pretty sure _mm_store1_ps
is calling _mm_storeu_ps.

Yes you're right - for gcc _mm_store1_pd is aligned (and there is a comment saying it must be), but _mm_store1_ps is unaligned. The intel intrinsics docs and msvc codegen both set both ps and pd versions to aligned store though.

If you wish I can just do the pd fixes - we are alone in doing a extract + 2*movsd - the rest all use shufpd+movapd

Suggestions for ps?

[craig.topper]

Given that its documented as being aligned. I'm ok with it. LGTM

[majnemer]

lib/Headers/emmintrin.h

598 ↗

(On Diff #58397)

You could use __attribute__((align_value(16))) no?

[RKSimon]

lib/Headers/emmintrin.h

598 ↗

(On Diff #58397)

Technically yes but AFAICT there are no other users of this approach in the headers - is it something that we should be encouraging do you think?

Craig - I think you wrote in a commit about dropping the unaligned intrinsics, is that how you'd do it?

cfe/trunk/lib/Headers/emmintrin.h

 {
   struct __mm_store_sd_struct {
     double __u;
   } __attribute__((__packed__, __may_alias__));
   ((struct __mm_store_sd_struct*)__dp)->__u = __a[0];
 }
 
 static __inline__ void __DEFAULT_FN_ATTRS
+_mm_store_pd(double *__dp, __m128d __a)
+{
+  *(__m128d*)__dp = __a;
+}
+
+static __inline__ void __DEFAULT_FN_ATTRS
 _mm_store1_pd(double *__dp, __m128d __a)
 {
-  struct __mm_store1_pd_struct {
-    double __u[2];
-  } __attribute__((__packed__, __may_alias__));
-  ((struct __mm_store1_pd_struct*)__dp)->__u[0] = __a[0];
-  ((struct __mm_store1_pd_struct*)__dp)->__u[1] = __a[0];
+  __a = __builtin_shufflevector((__v2df)__a, (__v2df)__a, 0, 0);
+  _mm_store_pd(__dp, __a);
 }
 
 static __inline__ void __DEFAULT_FN_ATTRS
-_mm_store_pd(double *__dp, __m128d __a)
+_mm_store_pd1(double *__dp, __m128d __a)
 {
-  *(__m128d *)__dp = __a;
+  return _mm_store1_pd(__dp, __a);
 }
 
 static __inline__ void __DEFAULT_FN_ATTRS
 _mm_storeu_pd(double *__dp, __m128d __a)
 {
   struct __storeu_pd {
     __m128d __v;
   } __attribute__((__packed__, __may_alias__));

cfe/trunk/lib/Headers/xmmintrin.h

 {
   struct __storeu_ps {
     __m128 __v;
   } __attribute__((__packed__, __may_alias__));
   ((struct __storeu_ps*)__p)->__v = __a;
 }
 
 static __inline__ void __DEFAULT_FN_ATTRS
-_mm_store1_ps(float *__p, __m128 __a)
+_mm_store_ps(float *__p, __m128 __a)
 {
-  __a = __builtin_shufflevector((__v4sf)__a, (__v4sf)__a, 0, 0, 0, 0);
-  _mm_storeu_ps(__p, __a);
+  *(__m128*)__p = __a;
 }
 
 static __inline__ void __DEFAULT_FN_ATTRS
-_mm_store_ps1(float *__p, __m128 __a)
+_mm_store1_ps(float *__p, __m128 __a)
 {
-    return _mm_store1_ps(__p, __a);
+  __a = __builtin_shufflevector((__v4sf)__a, (__v4sf)__a, 0, 0, 0, 0);
+  _mm_store_ps(__p, __a);
 }
 
 static __inline__ void __DEFAULT_FN_ATTRS
-_mm_store_ps(float *__p, __m128 __a)
+_mm_store_ps1(float *__p, __m128 __a)
 {
-  *(__m128 *)__p = __a;
+  return _mm_store1_ps(__p, __a);
 }
 
 static __inline__ void __DEFAULT_FN_ATTRS
 _mm_storer_ps(float *__p, __m128 __a)
 {
   __a = __builtin_shufflevector((__v4sf)__a, (__v4sf)__a, 3, 2, 1, 0);
   _mm_store_ps(__p, __a);
 }

cfe/trunk/test/CodeGen/sse2-builtins.c

 }
 
 void test_mm_store_pd(double* A, __m128d B) {
   // CHECK-LABEL: test_mm_store_pd
   // CHECK: store <2 x double> %{{.*}}, <2 x double>* %{{.*}}, align 16
   _mm_store_pd(A, B);
 }
 
+void test_mm_store_pd1(double* x, __m128d y) {
+  // CHECK-LABEL: test_mm_store_pd1
+  // CHECK: shufflevector <2 x double> %{{.*}}, <2 x double> %{{.*}}, <2 x i32> zeroinitializer
+  // CHECK: store <2 x double> %{{.*}}, <2 x double>* {{.*}}, align 16
+  _mm_store_pd1(x, y);
+}
+
 void test_mm_store_sd(double* A, __m128d B) {
   // CHECK-LABEL: test_mm_store_sd
   // CHECK: extractelement <2 x double> %{{.*}}, i32 0
   // CHECK: store double %{{.*}}, double* %{{.*}}, align 1{{$}}
   _mm_store_sd(A, B);
 }
 
 void test_mm_store_si128(__m128i* A, __m128i B) {
   // CHECK-LABEL: test_mm_store_si128
   // CHECK: store <2 x i64> %{{.*}}, <2 x i64>* %{{.*}}, align 16
   _mm_store_si128(A, B);
 }
 
 void test_mm_store1_pd(double* x, __m128d y) {
   // CHECK-LABEL: test_mm_store1_pd
-  // CHECK: extractelement <2 x double> %{{.*}}, i32 0
-  // CHECK: store {{.*}} double* {{.*}}, align 1{{$}}
-  // CHECK: store {{.*}} double* {{.*}}, align 1{{$}}
+  // CHECK: shufflevector <2 x double> %{{.*}}, <2 x double> %{{.*}}, <2 x i32> zeroinitializer
+  // CHECK: store <2 x double> %{{.*}}, <2 x double>* %{{.*}}, align 16
   _mm_store1_pd(x, y);
 }
 
 void test_mm_storeh_pd(double* A, __m128d B) {
   // CHECK-LABEL: test_mm_storeh_pd
   // CHECK: extractelement <2 x double> %{{.*}}, i32 1
   // CHECK: store double %{{.*}}, double* %{{.*}}, align 1
   _mm_storeh_pd(A, B);