[X86] replace vinsertf128 intrinsics with generic shuffles

[spatel]

Summary

This is the sibling patch for the LLVM half of this change:
http://reviews.llvm.org/D8086

We want to remove as much custom x86 shuffling nonsense as possible.

For the avxintrin.h header, it's not clear to me how we decide between macro implementations and inline function implementations. I thought macros were generally frowned upon, so I went with inlines.

[craig.topper]

Macros are needed when clang's integer constant evaluation is being used to create the inputs to the vector shuffle.

I believe with the inline version you've used the IR will contain two shuffles and the optimizers in the backend will have to reduce it.

I think I'd prefer the macro approach and only one shuffle with math on the immediates.

[spatel]

Ah - I had looked at a problem where we accept a variable when only a constant is acceptable:
http://llvm.org/bugs/show_bug.cgi?id=21713

Not sure if that's still a bug.

In any case, patch updated:

1. Use macros.
2. Shuffle mask elements generated individually via selects.
3. More test cases added to confirm the that the masks are good.

[RKSimon]

Thanks for working on this Sanjay!

lib/Headers/avxintrin.h

1160 ↗

(On Diff #21319)

The use of macros leaves these open to Wshadow warnings - what is the effect of casting them directly in the __builtin_shufflevector args?

1173 ↗

(On Diff #21319)

Ditto Wshadow

1182 ↗

(On Diff #21319)

Ditto Wshadow

1188 ↗

(On Diff #21319)

Similar to D8086 - shouldn't the immediate value be masked to just use the lsb? ((M)&1)

[spatel]

Updated to ignore upper bits of the immediate input.

[spatel]

In D8088#135378, @RKSimon wrote:

The use of macros leaves these open to Wshadow warnings -
what is the effect of casting them directly in the __builtin_shufflevector args?

I admit that I have no clue - I just copy pasted from the existing macros in this file.

I wasn't sure why those names had double-leading-underscores, but now that you mention shadows, I assume it's precisely to avoid that problem.

As an experiment, I removed the temp values by casting the inputs directly into the shuffles, and I don't see any warnings.

But I think if we want to make this change, then we should make it for all of the macros in this file in a follow-on commit. That way the code will be consistently implemented, and if we've missed some nuance of macro-fication, we can revert easily.

[andreadb]

In D8088#135479, @spatel wrote:

In D8088#135378, @RKSimon wrote:

The use of macros leaves these open to Wshadow warnings -
what is the effect of casting them directly in the __builtin_shufflevector args?

I admit that I have no clue - I just copy pasted from the existing macros in this file.

I wasn't sure why those names had double-leading-underscores, but now that you mention shadows, I assume it's precisely to avoid that problem.

The double-leading-underscores are not used to avoid the warnings. If you remove the extra underscores, you would still get the [-Wshadow] warnings.
In my experiments, you get those warnings only if you explicitly pass flags -Wshadow and -save-temps. Alternatively, you would only need to pass -Wshadow on a preprocessed file.

As an experiment, I removed the temp values by casting the inputs directly into the shuffles, and I don't see any warnings.

But I think if we want to make this change, then we should make it for all of the macros in this file in a follow-on commit. That way the code will be consistently implemented, and if we've missed some nuance of macro-fication, we can revert easily.

[spatel]

In D8088#135520, @andreadb wrote:

The double-leading-underscores are not used to avoid the warnings. If you remove the extra underscores, you would still get the [-Wshadow] warnings.

I may be misunderstanding, but I thought the double-leading-underscores were used to avoid interacting with the user's namespace. Double-leading-underscore is reserved for the compiler only?

[craig.topper]

I believe the Wshadow warnings occur when you pass a call to one of the macros as an argument to another macro. This causes the same variable name to occur in the inner and outer macro expansion. This occurs regardless of any underscores.

I'm guessing it requires save-temps to expose it because clang is probably smart enough to suppress the warning when macro expansion and compiling are done together. But with save-temps, compiling happens on the file written out by the preprocessor so clang doesn't know about the macro anymore.

This can prevented if you don't create intermediate variables inside the macro.

[spatel]

Updated patch to not use intermediate variables in the new macros.

Thanks everyone for the explanation. I must be missing a step because I was not seeing any warnings using a preprocessed file and -Wshadow. Please let me know if this version solves the problem.

[craig.topper]

LGTM

[spatel]

Thanks, Craig! Checked in at r231792.

cfe/trunk/include/clang/Basic/BuiltinsX86.def

 BUILTIN(__builtin_ia32_cvtps2dq256, "V8iV8f", "")
 BUILTIN(__builtin_ia32_cvtps2pd256, "V4dV4f", "")
 BUILTIN(__builtin_ia32_cvttpd2dq256, "V4iV4d", "")
 BUILTIN(__builtin_ia32_cvtpd2dq256, "V4iV4d", "")
 BUILTIN(__builtin_ia32_cvttps2dq256, "V8iV8f", "")
 BUILTIN(__builtin_ia32_vperm2f128_pd256, "V4dV4dV4dIc", "")
 BUILTIN(__builtin_ia32_vperm2f128_ps256, "V8fV8fV8fIc", "")
 BUILTIN(__builtin_ia32_vperm2f128_si256, "V8iV8iV8iIc", "")
-BUILTIN(__builtin_ia32_vinsertf128_pd256, "V4dV4dV2dIc", "")
-BUILTIN(__builtin_ia32_vinsertf128_ps256, "V8fV8fV4fIc", "")
-BUILTIN(__builtin_ia32_vinsertf128_si256, "V8iV8iV4iIc", "")
 BUILTIN(__builtin_ia32_sqrtpd256, "V4dV4d", "")
 BUILTIN(__builtin_ia32_sqrtps256, "V8fV8f", "")
 BUILTIN(__builtin_ia32_rsqrtps256, "V8fV8f", "")
 BUILTIN(__builtin_ia32_rcpps256, "V8fV8f", "")
 BUILTIN(__builtin_ia32_roundpd256, "V4dV4dIi", "")
 BUILTIN(__builtin_ia32_roundps256, "V8fV8fIi", "")
 BUILTIN(__builtin_ia32_vtestzpd, "iV2dV2d", "")
 BUILTIN(__builtin_ia32_vtestcpd, "iV2dV2d", "")

cfe/trunk/lib/Headers/avxintrin.h

 static __inline long long  __attribute__((__always_inline__, __nodebug__))
 _mm256_extract_epi64(__m256i __a, const int __imm)
 {
   __v4di __b = (__v4di)__a;
   return __b[__imm & 3];
 }
 #endif
 
-/* Vector insert */
-#define _mm256_insertf128_pd(V1, V2, O) __extension__ ({ \
-  __m256d __V1 = (V1); \
-  __m128d __V2 = (V2); \
-  (__m256d)__builtin_ia32_vinsertf128_pd256((__v4df)__V1, (__v2df)__V2, (O)); })
-
-#define _mm256_insertf128_ps(V1, V2, O) __extension__ ({ \
-  __m256 __V1 = (V1); \
-  __m128 __V2 = (V2); \
-  (__m256)__builtin_ia32_vinsertf128_ps256((__v8sf)__V1, (__v4sf)__V2, (O)); })
-
-#define _mm256_insertf128_si256(V1, V2, O) __extension__ ({ \
-  __m256i __V1 = (V1); \
-  __m128i __V2 = (V2); \
-  (__m256i)__builtin_ia32_vinsertf128_si256((__v8si)__V1, (__v4si)__V2, (O)); })
-
 static __inline __m256i __attribute__((__always_inline__, __nodebug__))
 _mm256_insert_epi32(__m256i __a, int __b, int const __imm)
 {
   __v8si __c = (__v8si)__a;
   __c[__imm & 7] = __b;
   return (__m256i)__c;
 }
 

 }
 
 static __inline __m256i __attribute__((__always_inline__, __nodebug__))
 _mm256_castsi128_si256(__m128i __a)
 {
   return __builtin_shufflevector(__a, __a, 0, 1, -1, -1);
 }
 
+/* 
+   Vector insert.
+   We use macros rather than inlines because we only want to accept
+   invocations where the immediate M is a constant expression.
+*/
+#define _mm256_insertf128_ps(V1, V2, M) __extension__ ({ \
+  (__m256)__builtin_shufflevector( \
+    (__v8sf)(V1), \
+    (__v8sf)_mm256_castps128_ps256((__m128)(V2)), \
+    (((M) & 1) ?  0 :  8), \
+    (((M) & 1) ?  1 :  9), \
+    (((M) & 1) ?  2 : 10), \
+    (((M) & 1) ?  3 : 11), \
+    (((M) & 1) ?  8 :  4), \
+    (((M) & 1) ?  9 :  5), \
+    (((M) & 1) ? 10 :  6), \
+    (((M) & 1) ? 11 :  7) );})
+
+#define _mm256_insertf128_pd(V1, V2, M) __extension__ ({ \
+  (__m256d)__builtin_shufflevector( \
+    (__v4df)(V1), \
+    (__v4df)_mm256_castpd128_pd256((__m128d)(V2)), \
+    (((M) & 1) ? 0 : 4), \
+    (((M) & 1) ? 1 : 5), \
+    (((M) & 1) ? 4 : 2), \
+    (((M) & 1) ? 5 : 3) );})
+
+#define _mm256_insertf128_si256(V1, V2, M) __extension__ ({ \
+  (__m256i)__builtin_shufflevector( \
+    (__v4di)(V1), \
+    (__v4di)_mm256_castsi128_si256((__m128i)(V2)), \
+    (((M) & 1) ? 0 : 4), \
+    (((M) & 1) ? 1 : 5), \
+    (((M) & 1) ? 4 : 2), \
+    (((M) & 1) ? 5 : 3) );})
+
 /* SIMD load ops (unaligned) */
 static __inline __m256 __attribute__((__always_inline__, __nodebug__))
 _mm256_loadu2_m128(float const *__addr_hi, float const *__addr_lo)
 {
   struct __loadu_ps {
     __m128 __v;
   } __attribute__((__packed__, __may_alias__));
 

cfe/trunk/lib/Sema/SemaChecking.cpp

bool Sema::CheckX86BuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall) {
   unsigned i = 0, l = 0, u = 0;
   switch (BuiltinID) {
   default: return false;
   case X86::BI_mm_prefetch: i = 1; l = 0; u = 3; break;
   case X86::BI__builtin_ia32_vextractf128_pd256:
   case X86::BI__builtin_ia32_vextractf128_ps256:
   case X86::BI__builtin_ia32_vextractf128_si256:
   case X86::BI__builtin_ia32_extract128i256: i = 1, l = 0, u = 1; break;
-  case X86::BI__builtin_ia32_vinsertf128_pd256:
-  case X86::BI__builtin_ia32_vinsertf128_ps256:
-  case X86::BI__builtin_ia32_vinsertf128_si256:
   case X86::BI__builtin_ia32_insert128i256: i = 2, l = 0; u = 1; break;
   case X86::BI__builtin_ia32_sha1rnds4: i = 2, l = 0; u = 3; break;
   case X86::BI__builtin_ia32_vpermil2pd:
   case X86::BI__builtin_ia32_vpermil2pd256:
   case X86::BI__builtin_ia32_vpermil2ps:
   case X86::BI__builtin_ia32_vpermil2ps256: i = 3, l = 0; u = 3; break;
   case X86::BI__builtin_ia32_cmpb128_mask:
   case X86::BI__builtin_ia32_cmpw128_mask:

cfe/trunk/test/CodeGen/avx-shuffle-builtins.c

test_mm256_broadcast_ss(float const *__a) {
   // CHECK: insertelement <8 x float> {{.*}}, i32 2
   // CHECK: insertelement <8 x float> {{.*}}, i32 3
   // CHECK: insertelement <8 x float> {{.*}}, i32 4
   // CHECK: insertelement <8 x float> {{.*}}, i32 5
   // CHECK: insertelement <8 x float> {{.*}}, i32 6
   // CHECK: insertelement <8 x float> {{.*}}, i32 7
   return _mm256_broadcast_ss(__a);
 }
+
+// Make sure we have the correct mask for each insertf128 case.
+
+__m256d test_mm256_insertf128_ps_0(__m256 a, __m128 b) {
+  // CHECK-LABEL: @test_mm256_insertf128_ps_0
+  // CHECK: shufflevector{{.*}}<i32 8, i32 9, i32 10, i32 11, i32 4, i32 5, i32 6, i32 7>
+  return _mm256_insertf128_ps(a, b, 0);
+}
+
+__m256d test_mm256_insertf128_pd_0(__m256d a, __m128d b) {
+  // CHECK-LABEL: @test_mm256_insertf128_pd_0
+  // CHECK: shufflevector{{.*}}<i32 4, i32 5, i32 2, i32 3>
+  return _mm256_insertf128_pd(a, b, 0);
+}
+
+__m256d test_mm256_insertf128_si256_0(__m256i a, __m128i b) {
+  // CHECK-LABEL: @test_mm256_insertf128_si256_0
+  // CHECK: shufflevector{{.*}}<i32 4, i32 5, i32 2, i32 3>
+  return _mm256_insertf128_si256(a, b, 0);
+}
+
+__m256d test_mm256_insertf128_ps_1(__m256 a, __m128 b) {
+  // CHECK-LABEL: @test_mm256_insertf128_ps_1
+  // CHECK: shufflevector{{.*}}<i32 0, i32 1, i32 2, i32 3, i32 8, i32 9, i32 10, i32 11>
+  return _mm256_insertf128_ps(a, b, 1);
+}
+
+__m256d test_mm256_insertf128_pd_1(__m256d a, __m128d b) {
+  // CHECK-LABEL: @test_mm256_insertf128_pd_1
+  // CHECK: shufflevector{{.*}}<i32 0, i32 1, i32 4, i32 5>
+  return _mm256_insertf128_pd(a, b, 1);
+}
+
+__m256d test_mm256_insertf128_si256_1(__m256i a, __m128i b) {
+  // CHECK-LABEL: @test_mm256_insertf128_si256_1
+  // CHECK: shufflevector{{.*}}<i32 0, i32 1, i32 4, i32 5>
+  return _mm256_insertf128_si256(a, b, 1);
+}
+

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

#endif
   tmp_V8i = __builtin_ia32_cvtps2dq256(tmp_V8f);
   tmp_V4d = __builtin_ia32_cvtps2pd256(tmp_V4f);
   tmp_V4i = __builtin_ia32_cvttpd2dq256(tmp_V4d);
   tmp_V4i = __builtin_ia32_cvtpd2dq256(tmp_V4d);
   tmp_V8i = __builtin_ia32_cvttps2dq256(tmp_V8f);
   tmp_V4d = __builtin_ia32_vperm2f128_pd256(tmp_V4d, tmp_V4d, 0x7);
   tmp_V8f = __builtin_ia32_vperm2f128_ps256(tmp_V8f, tmp_V8f, 0x7);
   tmp_V8i = __builtin_ia32_vperm2f128_si256(tmp_V8i, tmp_V8i, 0x7);
-  tmp_V4d = __builtin_ia32_vinsertf128_pd256(tmp_V4d, tmp_V2d, 0x1);
-  tmp_V8f = __builtin_ia32_vinsertf128_ps256(tmp_V8f, tmp_V4f, 0x1);
-  tmp_V8i = __builtin_ia32_vinsertf128_si256(tmp_V8i, tmp_V4i, 0x1);
   tmp_V4d = __builtin_ia32_sqrtpd256(tmp_V4d);
   tmp_V8f = __builtin_ia32_sqrtps256(tmp_V8f);
   tmp_V8f = __builtin_ia32_rsqrtps256(tmp_V8f);
   tmp_V8f = __builtin_ia32_rcpps256(tmp_V8f);
   tmp_V4d = __builtin_ia32_roundpd256(tmp_V4d, 0x1);
   tmp_V8f = __builtin_ia32_roundps256(tmp_V8f, 0x1);
   tmp_i = __builtin_ia32_vtestzpd(tmp_V2d, tmp_V2d);
   tmp_i = __builtin_ia32_vtestcpd(tmp_V2d, tmp_V2d);