[X86][SSE] Add _mm_undefined_* intrinsics

[RKSimon]

Summary

Adds missing SSE/AVX 'undefined' intrinsics (PR24040):

_mm_undefined_pd + _mm256_undefined_pd
_mm_undefined_ps + _mm256_undefined_ps
_mm_undefined_si128 + _mm256_undefined_si256

[mkuper]

Thanks, Simon!
I've wanted to add the _undefined intrinsics for a while now, but never got to it.

Anyway, this sort of implementation somewhat worries me.
Yes, I know that the gcc intrinsics do something very similar.
And I also know that in practice we'll get an undef value, nothing worse (assuming reading an uninitialized automatic variable is undefined behavior to begin with - which really depends on the spec interpretation :-) ).
And I know this isn't likely to change anytime soon.

Still, relying on what may be undefined behavior in the header files worries me, and I'd rather not have it implemented like that.
I was thinking about adding a __builtin_undef which explicitly resolves to an undef value.
Does that make sense to you?

test/CodeGen/sse-undefined.c

1

Perhaps a more explicit test?

[majnemer]

In D12052#225194, @mkuper wrote:

Thanks, Simon!
I've wanted to add the _undefined intrinsics for a while now, but never got to it.

Anyway, this sort of implementation somewhat worries me.
Yes, I know that the gcc intrinsics do something very similar.
And I also know that in practice we'll get an undef value, nothing worse (assuming reading an uninitialized automatic variable is undefined behavior to begin with - which really depends on the spec interpretation :-) ).
And I know this isn't likely to change anytime soon.

Still, relying on what may be undefined behavior in the header files worries me, and I'd rather not have it implemented like that.
I was thinking about adding a __builtin_undef which explicitly resolves to an undef value.
Does that make sense to you?

__builtin_undef seems like a pretty big hammer and does not sound trivial to implement. Not all types that can be emitted have an undef representation. For example, x86_mmx doesn't have an undef representation because there can be no constants of that type. I'd recommend a more narrow implementation technique unless we really need a more general one.

[RKSimon]

Yes using that uninitialized value has worried me as well. I originally set it to zero (and considered using __ LINE __ or __ COUNTER __) but both introduce defined behaviour that I could see causing all sorts of problems further down the line in debug vs release builds. How undefined do we want our undefined to be? ;-)

I can create builtin_ia32_undef64mmx / builtin_ia32_undef128 / builtin_ia32_undef256 / builtin_ia32_undef512 if nobody can think of a better alternative?

[cfe-commits]

I’m not sure how much people actually use these, but the AVX-512 versions of these, at least, can be very useful internally to implement AVX-512 intrinsics.
For AVX-512, we use the same GCC builtin for all 3 versions of the intrinsic (pass-through masked, set to zero masked, and unmasked). This is the same implementation that’s used in GCC, and is fairly clean, since the only difference is in the desired pass-through values (actual value, zero, or undef).

However, since we don’t actually have the undef intrinsics right now, we put a zero in the unmasked version as well, which is definitely a pessimization.
The plan is to change them to use undef once the undef intrinsics are implemented.

From: Eric Christopher [mailto:[e-mail]]
Sent: Monday, August 17, 2015 21:33
To: [e-mail]; [e-mail]; [e-mail]; Kuperstein, Michael M
Cc: [e-mail]; Badouh, Asaf; [e-mail]; Richard Smith
Subject: Re: [PATCH] D12052: [X86][SSE] Add _mm_undefined_* intrinsics

• msg-9058-576.txt159 BDownload

[RKSimon]

Added ia32 builtin undef intrinsics (I didn't bother with the mmx as I can't find any evidence of an undefined intrinsic for it). Added the avx512 intrinsics referenced in the intel intrinsics guide.

Technically there's nothing stopping us making these builtin more general (non x86 specific) - I don't know if people want us to go that way though?

I'll make the tests more explicit once we're happy that this is the way to go.

[mkuper]

I think this is slightly less elegant than having a generic builtin, but I'm just fine with it, especially if David/Eric prefer it to the generic version.

[mkuper]

Actually, thinking about it a bit more, a generic builtin most probably won't be more elegant.
LGTM.

lib/Headers/avxintrin.h

 
 static __inline void __DEFAULT_FN_ATTRS
 _mm256_stream_ps(float *__p, __m256 __a)
 {
   __builtin_ia32_movntps256(__p, (__v8sf)__a);
 }
 
 /* Create vectors */
+static __inline__ __m256d __DEFAULT_FN_ATTRS
+_mm256_undefined_pd()
+{
+  __m256d __u;
+  return __builtin_shufflevector(__u, __u, -1, -1, -1, -1);
+}
+
+static __inline__ __m256 __DEFAULT_FN_ATTRS
+_mm256_undefined_ps()
+{
+  __m256 __u;
+  return __builtin_shufflevector(__u, __u, -1, -1, -1, -1, -1, -1, -1, -1);
+}
+
+static __inline__ __m256i __DEFAULT_FN_ATTRS
+_mm256_undefined_si256()
+{
+  __m256i __u;
+  return __builtin_shufflevector(__u, __u, -1, -1, -1, -1);
+}
+
 static __inline __m256d __DEFAULT_FN_ATTRS
 _mm256_set_pd(double __a, double __b, double __c, double __d)
 {
   return (__m256d){ __d, __c, __b, __a };
 }
 
 static __inline __m256 __DEFAULT_FN_ATTRS
 _mm256_set_ps(float __a, float __b, float __c, float __d,

lib/Headers/emmintrin.h

_mm_loadl_pd(__m128d __a, double const *__dp)
   struct __mm_loadl_pd_struct {
     double __u;
   } __attribute__((__packed__, __may_alias__));
   double __u = ((struct __mm_loadl_pd_struct*)__dp)->__u;
   return (__m128d){ __u, __a[1] };
 }
 
 static __inline__ __m128d __DEFAULT_FN_ATTRS
+_mm_undefined_pd()
+{
+  __m128d __u;
+  return __builtin_shufflevector(__u, __u, -1, -1);
+}
+
+static __inline__ __m128d __DEFAULT_FN_ATTRS
 _mm_set_sd(double __w)
 {
   return (__m128d){ __w, 0 };
 }
 
 static __inline__ __m128d __DEFAULT_FN_ATTRS
 _mm_set1_pd(double __w)
 {

 {
   struct __mm_loadl_epi64_struct {
     long long __u;
   } __attribute__((__packed__, __may_alias__));
   return (__m128i) { ((struct __mm_loadl_epi64_struct*)__p)->__u, 0};
 }
 
 static __inline__ __m128i __DEFAULT_FN_ATTRS
+_mm_undefined_si128()
+{
+  __m128i __u;
+  return __builtin_shufflevector(__u, __u, -1, -1);
+}
+
+static __inline__ __m128i __DEFAULT_FN_ATTRS
 _mm_set_epi64x(long long q1, long long q0)
 {
   return (__m128i){ q0, q1 };
 }
 
 static __inline__ __m128i __DEFAULT_FN_ATTRS
 _mm_set_epi64(__m64 q1, __m64 q0)
 {

lib/Headers/xmmintrin.h

 static __inline__ __m128 __DEFAULT_FN_ATTRS
 _mm_loadr_ps(const float *__p)
 {
   __m128 __a = _mm_load_ps(__p);
   return __builtin_shufflevector(__a, __a, 3, 2, 1, 0);
 }
 
 static __inline__ __m128 __DEFAULT_FN_ATTRS
+_mm_undefined_ps()
+{
+  __m128 __u;
+  return __builtin_shufflevector(__u, __u, -1, -1, -1, -1);
+}
+
+static __inline__ __m128 __DEFAULT_FN_ATTRS
 _mm_set_ss(float __w)
 {
   return (__m128){ __w, 0, 0, 0 };
 }
 
 static __inline__ __m128 __DEFAULT_FN_ATTRS
 _mm_set1_ps(float __w)
 {

test/CodeGen/sse-undefined.c

+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -o - | not grep "xmm"
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -o - | not grep "ymm"
+
+// Don't include mm_malloc.h, it's system specific.
+#define __MM_MALLOC_H
+
+#include <x86intrin.h>
+
+__m128 test_mm_undefined_ps() {
+  return _mm_undefined_ps();
+}
+
+__m128d test_mm_undefined_pd() {
+  return _mm_undefined_pd();
+}
+
+__m128i test_mm_undefined_si128() {
+  return _mm_undefined_si128();
+}
+
+__m256 test_mm256_undefined_ps() {
+  return _mm256_undefined_ps();
+}
+
+__m256d test_mm256_undefined_pd() {
+  return _mm256_undefined_pd();
+}
+
+__m256i test_mm256_undefined_si256() {
+  return _mm256_undefined_si256();
+}