r248980 - Patch over a really horrible bug in our vector builtins that showed up

[Chandler Carruth via cfe-commits]

Author: chandlerc
Date: Wed Sep 30 21:21:34 2015
New Revision: 248980

URL: http://llvm.org/viewvc/llvm-project?rev=248980&view=rev
Log:
Patch over a really horrible bug in our vector builtins that showed up
recently when we started using direct conversion to model sign
extension. The __v16qi type we use for SSE v16i8 vectors is defined in
terms of 'char' which may or may not be signed! This causes us to
generate pmovsx and pmovzx depending on the setting of -funsigned-char.

This patch just forms an explicitly signed type and uses that to
formulate the sign extension. While this gets the correct behavior
(which we now verify with the enhanced test) this is just the tip of the
ice berg. Now that I know what to look for, I have found errors of this
sort *throughout* our vector code. Fortunately, this is the only
specific place where I know of users actively having their code
miscompiled by Clang due to this, so I'm keeping the fix for those users
minimal and targeted.

I'll be sending a proper email for discussion of how to fix these
systematically, what the implications are, and just how widely broken
this is... From what I can tell, we have never shipped a correct set of
builtin headers for x86 when users rely on -funsigned-char. Oops.

Modified:
    cfe/trunk/lib/Headers/smmintrin.h
    cfe/trunk/test/CodeGen/sse41-builtins.c

Modified: cfe/trunk/lib/Headers/smmintrin.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/smmintrin.h?rev=248980&r1=248979&r2=248980&view=diff
==============================================================================
--- cfe/trunk/lib/Headers/smmintrin.h (original)
+++ cfe/trunk/lib/Headers/smmintrin.h Wed Sep 30 21:21:34 2015
@@ -286,19 +286,34 @@ _mm_cmpeq_epi64(__m128i __V1, __m128i __
 static __inline__ __m128i __DEFAULT_FN_ATTRS
 _mm_cvtepi8_epi16(__m128i __V)
 {
-  return (__m128i)__builtin_convertvector(__builtin_shufflevector((__v16qi)__V, (__v16qi)__V, 0, 1, 2, 3, 4, 5, 6, 7), __v8hi);
+  /* We need a local definitively signed typedef similar to __v16qi even in the
+   * presence of __UNSIGNED_CHAR__.
+   * FIXME: __v16qi should almost certainly be definitively signed.
+   */
+  typedef signed char __signed_v16qi __attribute__((__vector_size__(16)));
+  return (__m128i)__builtin_convertvector(__builtin_shufflevector((__signed_v16qi)__V, (__signed_v16qi)__V, 0, 1, 2, 3, 4, 5, 6, 7), __v8hi);
 }
 
 static __inline__ __m128i __DEFAULT_FN_ATTRS
 _mm_cvtepi8_epi32(__m128i __V)
 {
-  return (__m128i)__builtin_convertvector(__builtin_shufflevector((__v16qi)__V, (__v16qi)__V, 0, 1, 2, 3), __v4si);
+  /* We need a local definitively signed typedef similar to __v16qi even in the
+   * presence of __UNSIGNED_CHAR__.
+   * FIXME: __v16qi should almost certainly be definitively signed.
+   */
+  typedef signed char __signed_v16qi __attribute__((__vector_size__(16)));
+  return (__m128i)__builtin_convertvector(__builtin_shufflevector((__signed_v16qi)__V, (__signed_v16qi)__V, 0, 1, 2, 3), __v4si);
 }
 
 static __inline__ __m128i __DEFAULT_FN_ATTRS
 _mm_cvtepi8_epi64(__m128i __V)
 {
-  return (__m128i)__builtin_convertvector(__builtin_shufflevector((__v16qi)__V, (__v16qi)__V, 0, 1), __v2di);
+  /* We need a local definitively signed typedef similar to __v16qi even in the
+   * presence of __UNSIGNED_CHAR__.
+   * FIXME: __v16qi should almost certainly be definitively signed.
+   */
+  typedef signed char __signed_v16qi __attribute__((__vector_size__(16)));
+  return (__m128i)__builtin_convertvector(__builtin_shufflevector((__signed_v16qi)__V, (__signed_v16qi)__V, 0, 1), __v2di);
 }
 
 static __inline__ __m128i __DEFAULT_FN_ATTRS

Modified: cfe/trunk/test/CodeGen/sse41-builtins.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/sse41-builtins.c?rev=248980&r1=248979&r2=248980&view=diff
==============================================================================
--- cfe/trunk/test/CodeGen/sse41-builtins.c (original)
+++ cfe/trunk/test/CodeGen/sse41-builtins.c Wed Sep 30 21:21:34 2015
@@ -1,6 +1,8 @@
 // REQUIRES: x86-registered-target
 // RUN: %clang_cc1 %s -O0 -triple=x86_64-apple-darwin -target-feature +sse4.1 -emit-llvm -o - -Werror | FileCheck %s
+// RUN: %clang_cc1 %s -O0 -triple=x86_64-apple-darwin -target-feature +sse4.1 -fno-signed-char -emit-llvm -o - -Werror | FileCheck %s
 // RUN: %clang_cc1 %s -O0 -triple=x86_64-apple-darwin -target-feature +sse4.1 -S -o - -Werror | FileCheck %s --check-prefix=CHECK-ASM
+// RUN: %clang_cc1 %s -O0 -triple=x86_64-apple-darwin -target-feature +sse4.1 -fno-signed-char -S -o - -Werror | FileCheck %s --check-prefix=CHECK-ASM
 
 // Don't include mm_malloc.h, it's system specific.
 #define __MM_MALLOC_H