Page MenuHomePhabricator

[X86][clang] Emit diagnostic for float and double when we have features -x87 and -sse on 64-bits
ClosedPublic

Authored by pengfei on Nov 30 2021, 1:57 AM.

Diff Detail

Event Timeline

pengfei requested review of this revision.Nov 30 2021, 1:57 AM
pengfei created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2021, 1:57 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
pengfei updated this revision to Diff 390632.Nov 30 2021, 2:10 AM

Fix a lit fail.

asavonic accepted this revision.Dec 7 2021, 5:17 AM

LGTM. The new diagnostic is not generic, but it is probably fine as long as x86_64 is the only target that needs this.

This revision is now accepted and ready to land.Dec 7 2021, 5:17 AM

Thanks @asavonic for the review!

phosek added a subscriber: phosek.Dec 9 2021, 4:41 PM

This change broke our kernel build. I believe that the issue is that the compiler doesn't consider function attributes.

This can be trivially reproduced as:

$ cat >t.c <<EOF
#include <x86intrin.h>
EOF
$ ./bin/clang -c t.c -o t.o -mno-sse
In file included from t.c:1:
In file included from /src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/x86intrin.h:13:
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/ia32intrin.h:251:1: error: SSE register return with SSE disabled
_castu32_f32(unsigned int __A) {
^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/ia32intrin.h:251:1: note: '_castu32_f32' defined here
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/ia32intrin.h:266:1: error: SSE register return with SSE disabled
_castu64_f64(unsigned long long __A) {
^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/ia32intrin.h:266:1: note: '_castu64_f64' defined here
In file included from t.c:1:
In file included from /src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/x86intrin.h:15:
In file included from /src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/immintrin.h:26:
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/xmmintrin.h:1607:1: error: SSE register return with SSE disabled
_mm_cvtss_f32(__m128 __a)
^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/xmmintrin.h:1607:1: note: '_mm_cvtss_f32' defined here
In file included from t.c:1:
In file included from /src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/x86intrin.h:15:
In file included from /src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/immintrin.h:31:
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/emmintrin.h:1566:1: error: SSE register return with SSE disabled
_mm_cvtsd_f64(__m128d __a)
^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/emmintrin.h:1566:1: note: '_mm_cvtsd_f64' defined here
In file included from t.c:1:
In file included from /src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/x86intrin.h:15:
In file included from /src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/immintrin.h:66:
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avxintrin.h:2257:1: error: SSE register return with SSE disabled
_mm256_cvtsd_f64(__m256d __a)
^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avxintrin.h:2257:1: note: '_mm256_cvtsd_f64' defined here
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avxintrin.h:2290:1: error: SSE register return with SSE disabled
_mm256_cvtss_f32(__m256 __a)
^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avxintrin.h:2290:1: note: '_mm256_cvtss_f32' defined here
In file included from t.c:1:
In file included from /src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/x86intrin.h:15:
In file included from /src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/immintrin.h:76:
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/f16cintrin.h:39:1: error: SSE register return with SSE disabled
_cvtsh_ss(unsigned short __a)
^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/f16cintrin.h:39:1: note: '_cvtsh_ss' defined here
In file included from t.c:1:
In file included from /src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/x86intrin.h:15:
In file included from /src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/immintrin.h:104:
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:4090:1: error: SSE register return with SSE disabled
_mm512_cvtsd_f64(__m512d __a)
^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:4090:1: note: '_mm512_cvtsd_f64' defined here
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:4096:1: error: SSE register return with SSE disabled
_mm512_cvtss_f32(__m512 __a)
^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:4096:1: note: '_mm512_cvtss_f32' defined here
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9358:10: error: SSE register return with SSE disabled
  return __builtin_ia32_reduce_fadd_pd512(-0.0, __W);
         ^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9358:10: note: '__builtin_ia32_reduce_fadd_pd512' defined here
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9357:48: error: SSE register return with SSE disabled
static __inline__ double __DEFAULT_FN_ATTRS512 _mm512_reduce_add_pd(__m512d __W) {
                                               ^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9357:48: note: '_mm512_reduce_add_pd' defined here
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9362:10: error: SSE register return with SSE disabled
  return __builtin_ia32_reduce_fmul_pd512(1.0, __W);
         ^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9362:10: note: '__builtin_ia32_reduce_fmul_pd512' defined here
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9361:48: error: SSE register return with SSE disabled
static __inline__ double __DEFAULT_FN_ATTRS512 _mm512_reduce_mul_pd(__m512d __W) {
                                               ^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9361:48: note: '_mm512_reduce_mul_pd' defined here
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9366:1: error: SSE register return with SSE disabled
_mm512_mask_reduce_add_pd(__mmask8 __M, __m512d __W) {
^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9366:1: note: '_mm512_mask_reduce_add_pd' defined here
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9372:1: error: SSE register return with SSE disabled
_mm512_mask_reduce_mul_pd(__mmask8 __M, __m512d __W) {
^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9372:1: note: '_mm512_mask_reduce_mul_pd' defined here
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9423:10: error: SSE register return with SSE disabled
  return __builtin_ia32_reduce_fadd_ps512(-0.0f, __W);
         ^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9423:10: note: '__builtin_ia32_reduce_fadd_ps512' defined here
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9422:1: error: SSE register return with SSE disabled
_mm512_reduce_add_ps(__m512 __W) {
^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9422:1: note: '_mm512_reduce_add_ps' defined here
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9428:10: error: SSE register return with SSE disabled
  return __builtin_ia32_reduce_fmul_ps512(1.0f, __W);
         ^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9428:10: note: '__builtin_ia32_reduce_fmul_ps512' defined here
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9427:1: error: SSE register return with SSE disabled
_mm512_reduce_mul_ps(__m512 __W) {
^
/src/clang-llvm/llvm-build/fuchsia/lib/clang/14.0.0/include/avx512fintrin.h:9427:1: note: '_mm512_reduce_mul_ps' defined here
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.

Would it be possible to revert the change?

phosek added a comment.Dec 9 2021, 4:47 PM

Note that GCC had the same issue in the past https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80298 which was later addressed.

Hi @phosek , thanks for reporting it.
I would consider it as a misuse of the header file when SSE is not usable, especially we now have the specific header "x86gprintrin.h" for such scenarios. Can you help to try if changing to "x86gprintrin.h" works?

phosek added a comment.Dec 9 2021, 5:25 PM

Hi @phosek , thanks for reporting it.
I would consider it as a misuse of the header file when SSE is not usable, especially we now have the specific header "x86gprintrin.h" for such scenarios. Can you help to try if changing to "x86gprintrin.h" works?

It's a normal practice to compile with -mno-{sse,avx} and then use SSE/AVX intrinsics explicitly inside runtime cpuid conditionals. The function calling the intrinsic might be marked with __attribute__((target("avx"))), but the source file's ambient -mo-{sse,avx} will be in force when #include <immintrin.h> is done. This change breaks that practice which is likely going to affect existing codebases.

phosek added a comment.Dec 9 2021, 5:28 PM

Hi @phosek , thanks for reporting it.
I would consider it as a misuse of the header file when SSE is not usable, especially we now have the specific header "x86gprintrin.h" for such scenarios. Can you help to try if changing to "x86gprintrin.h" works?

It's a normal practice to compile with -mno-{sse,avx} and then use SSE/AVX intrinsics explicitly inside runtime cpuid conditionals. The function calling the intrinsic might be marked with __attribute__((target("avx"))), but the source file's ambient -mo-{sse,avx} will be in force when #include <immintrin.h> is done. This change breaks that practice which is likely going to affect existing codebases.

I'd also point out that other compilers like GCC don't report the same error so this change makes Clang's behavior inconsistent.

Oh, that makes sense to me. Reverted and I will investigate it. Thanks!