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

[Chandler Carruth via cfe-commits]

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 at 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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151001/496024af/attachment.html>