Note that http://reviews.llvm.org/D13324 is the promised improved fix here, awaiting review. =] On Wed, Sep 30, 2015 at 7:23 PM Chandler Carruth via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> 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 > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
