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

[pengfei]

Summary

A follow up of D114162.

[pengfei]

Fix a lit fail.

[asavonic]

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

[pengfei]

Thanks @asavonic for the review!

[phosek]

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]

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

[pengfei]

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]

In D114782#3184317, @pengfei wrote:

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]

In D114782#3184406, @phosek wrote:

In D114782#3184317, @pengfei wrote:

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.

[pengfei]

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

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_omp_invariant_dependency : Error<
   "expected loop invariant expression">;
 def err_omp_invariant_or_linear_dependency : Error<
   "expected loop invariant expression or '<invariant1> * %0 + <invariant2>' kind of expression">;
 def err_omp_wrong_dependency_iterator_type : Error<
   "expected an integer or a pointer type of the outer loop counter '%0' for non-rectangular nests">;
 def err_target_unsupported_type
     : Error<"%0 requires %select{|%2 bit size}1 %3 %select{|return }4type support,"
             " but target '%5' does not support it">;
+def err_target_unsupported_type_without_sse : Error<
+  "SSE register return with SSE disabled">;
 def err_omp_lambda_capture_in_declare_target_not_to : Error<
   "variable captured in declare target region must appear in a to clause">;
 def err_omp_device_type_mismatch : Error<
   "'device_type(%0)' does not match previously specified 'device_type(%1)' for the same declaration">;
 def err_omp_wrong_device_function_call : Error<
   "function with 'device_type(%0)' is not available on %select{device|host}1">;
 def note_omp_marked_device_type_here : Note<"marked as 'device_type(%0)' here">;
 def warn_omp_declare_target_after_first_use : Warning<

clang/lib/Basic/Targets/X86.cpp

bool X86TargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
   SimdDefaultAlign =
       hasFeature("avx512f") ? 512 : hasFeature("avx") ? 256 : 128;
 
   // FIXME: We should allow long double type on 32-bits to match with GCC.
   // This requires backend to be able to lower f80 without x87 first.
   if (!HasX87 && LongDoubleFormat == &llvm::APFloat::x87DoubleExtended())
     HasLongDouble = false;
 
+  if (SSELevel < SSE1 && getTriple().getArch() == llvm::Triple::x86_64)
+    HasFPReturn = false;
+
   return true;
 }
 
 /// X86TargetInfo::getTargetDefines - Return the set of the X86-specific macro
 /// definitions for this particular subtarget.
 void X86TargetInfo::getTargetDefines(const LangOptions &Opts,
                                      MacroBuilder &Builder) const {
   // Inline assembly supports X86 flag outputs.

clang/lib/Sema/Sema.cpp

if (!TI.hasLongDoubleType() && UnqualTy == Context.LongDoubleTy) {
       }
       if (D)
         targetDiag(D->getLocation(), diag::note_defined_here, FD) << D;
     }
 
     bool IsDouble = UnqualTy == Context.DoubleTy;
     bool IsFloat = UnqualTy == Context.FloatTy;
     if (IsRetTy && !TI.hasFPReturn() && (IsDouble || IsFloat)) {
-      PartialDiagnostic PD = PDiag(diag::err_target_unsupported_type);
-      if (D)
-        PD << D;
-      else
-        PD << "expression";
-
-      if (Diag(Loc, PD, FD)
-          << false /*show bit size*/ << 0 << Ty << true /*return*/
-          << Context.getTargetInfo().getTriple().str()) {
+      if (Diag(Loc, diag::err_target_unsupported_type_without_sse)) {
         if (D)
           D->setInvalidDecl();
       }
       if (D)
         targetDiag(D->getLocation(), diag::note_defined_here, FD) << D;
     }
   };
 

clang/test/CodeGen/X86/x86_64-mno-sse.c

 // RUN: %clang_cc1 -triple x86_64-linux -target-feature -sse -target-feature -sse2 -S -o /dev/null -verify %s
 // REQUIRES: x86-registered-target
 
-double f1(void) { // expected-error {{SSE register return with SSE disabled}}
+// expected-error@+2{{SSE register return with SSE disabled}}
+// expected-note@+1{{'f1' defined here}}
+double f1(void) {
   return 1.4;
 }
 extern double g;
-void f2(void) { // expected-error {{SSE register return with SSE disabled}}
+void f2(void) {
   g = f1();
 }
 void take_double(double);
 void pass_double(void) {
-  // FIXME: Still asserts.
-  //take_double(1.5);
+  take_double(1.5);
 }

clang/test/Sema/x86_64-no-x87.cpp

-// RUN: %clang_cc1 -fsyntax-only -verify %s -triple x86_64-linux-gnu -target-feature -x87
+// RUN: %clang_cc1 -fsyntax-only -verify %s -triple x86_64-linux-gnu -target-feature -x87 -target-feature -sse -DERROR_LONGDOUBLE -DERROR_NOSSE
+// RUN: %clang_cc1 -fsyntax-only -verify %s -triple x86_64-linux-gnu -target-feature -x87 -DERROR_LONGDOUBLE
 // RUN: %clang_cc1 -fsyntax-only -verify %s -triple x86_64-linux-gnu -DNOERROR
 
 #ifdef NOERROR
 // expected-no-diagnostics
 #endif
 
 typedef long double long_double;
 
 // Declaration is fine, unless it is called or defined.
 double decl(long_double x, long_double y);
 
 template <typename T>
 T decl_ld_del(T);
 
 // No code is generated for deleted functions
 long_double decl_ld_del(long_double) = delete;
 double decl_ld_del(double) = delete;
 float decl_ld_del(float) = delete;
 
-#ifndef NOERROR
+#ifdef ERROR_LONGDOUBLE
 // expected-error@+4{{'def' requires  'long_double' (aka 'long double') type support, but target 'x86_64-unknown-linux-gnu' does not support it}}
 // expected-note@+3{{'def' defined here}}
 // expected-note@+2{{'x' defined here}}
 #endif
 int def(long_double x) {
-#ifndef NOERROR
+#ifdef ERROR_LONGDOUBLE
 // expected-error@+2{{'x' requires  'long_double' (aka 'long double') type support, but target 'x86_64-unknown-linux-gnu' does not support it}}
 #endif
   return (int)x;
 }
 
-#ifndef NOERROR
+#ifdef ERROR_LONGDOUBLE
 // expected-note@+3{{'ld_args' defined here}}
 // expected-note@+2{{'ld_args' defined here}}
 #endif
 int ld_args(long_double x, long_double y);
 
 int call1(float x, float y) {
-#ifndef NOERROR
+#ifdef ERROR_LONGDOUBLE
   // expected-error@+2 2{{'ld_args' requires  'long_double' (aka 'long double') type support, but target 'x86_64-unknown-linux-gnu' does not support it}}
 #endif
   return ld_args(x, y);
 }
 
-#ifndef NOERROR
+#ifdef ERROR_LONGDOUBLE
 // expected-note@+2{{'ld_ret' defined here}}
 #endif
 long_double ld_ret(double x, double y);
 
 int call2(float x, float y) {
-#ifndef NOERROR
+#ifdef ERROR_LONGDOUBLE
   // expected-error@+2{{'ld_ret' requires  'long_double' (aka 'long double') type support, but target 'x86_64-unknown-linux-gnu' does not support it}}
 #endif
   return (int)ld_ret(x, y);
 }
 
 int binop(double x, double y) {
-#ifndef NOERROR
+#ifdef ERROR_LONGDOUBLE
   // expected-error@+2 2{{expression requires  'long_double' (aka 'long double') type support, but target 'x86_64-unknown-linux-gnu' does not support it}}
 #endif
   double z = (long_double)x * (long_double)y;
   return (int)z;
 }
 
 void assign1(long_double *ret, double x) {
-#ifndef NOERROR
+#ifdef ERROR_LONGDOUBLE
   // expected-error@+2{{expression requires  'long_double' (aka 'long double') type support, but target 'x86_64-unknown-linux-gnu' does not support it}}
 #endif
   *ret = x;
 }
 
 struct st_long_double1 {
-#ifndef NOERROR
+#ifdef ERROR_LONGDOUBLE
   // expected-note@+2{{'ld' defined here}}
 #endif
   long_double ld;
 };
 
 struct st_long_double2 {
-#ifndef NOERROR
+#ifdef ERROR_LONGDOUBLE
   // expected-note@+2{{'ld' defined here}}
 #endif
   long_double ld;
 };
 
 struct st_long_double3 {
-#ifndef NOERROR
+#ifdef ERROR_LONGDOUBLE
   // expected-note@+2{{'ld' defined here}}
 #endif
   long_double ld;
 };
 
 void assign2() {
   struct st_long_double1 st;
-#ifndef NOERROR
+#ifdef ERROR_LONGDOUBLE
   // expected-error@+3{{expression requires  'long_double' (aka 'long double') type support, but target 'x86_64-unknown-linux-gnu' does not support it}}
   // expected-error@+2{{'ld' requires  'long_double' (aka 'long double') type support, but target 'x86_64-unknown-linux-gnu' does not support it}}
 #endif
   st.ld = 0.42;
 }
 
 void assign3() {
   struct st_long_double2 st;
-#ifndef NOERROR
+#ifdef ERROR_LONGDOUBLE
   // expected-error@+3{{expression requires  'long_double' (aka 'long double') type support, but target 'x86_64-unknown-linux-gnu' does not support it}}
   // expected-error@+2{{'ld' requires  'long_double' (aka 'long double') type support, but target 'x86_64-unknown-linux-gnu' does not support it}}
 #endif
   st.ld = 42;
 }
 
 void assign4(double d) {
   struct st_long_double3 st;
-#ifndef NOERROR
+#ifdef ERROR_LONGDOUBLE
   // expected-error@+3{{expression requires  'long_double' (aka 'long double') type support, but target 'x86_64-unknown-linux-gnu' does not support it}}
   // expected-error@+2{{'ld' requires  'long_double' (aka 'long double') type support, but target 'x86_64-unknown-linux-gnu' does not support it}}
 #endif
   st.ld = d;
 }
 
 void assign5() {
   // unused variable declaration is fine
   long_double ld = 0.42;
 }
 
 // Double and Float return type on x86_64 do not use x87 registers
+#ifdef ERROR_NOSSE
+  // expected-error@+3{{SSE register return with SSE disabled}}
+  // expected-note@+2{{'d_ret1' defined here}}
+#endif
 double d_ret1(float x) {
   return 0.0;
 }
 
+#ifdef ERROR_NOSSE
+  // expected-note@+2{{'d_ret2' defined here}}
+#endif
 double d_ret2(float x);
 
+#ifdef ERROR_NOSSE
+  // expected-error@+3{{SSE register return with SSE disabled}}
+#endif
 int d_ret3(float x) {
   return (int)d_ret2(x);
 }
 
+#ifdef ERROR_NOSSE
+  // expected-error@+3{{SSE register return with SSE disabled}}
+  // expected-note@+2{{'f_ret1' defined here}}
+#endif
 float f_ret1(float x) {
   return 0.0f;
 }
 
+#ifdef ERROR_NOSSE
+  // expected-note@+2{{'f_ret2' defined here}}
+#endif
 float f_ret2(float x);
 
+#ifdef ERROR_NOSSE
+  // expected-error@+3{{SSE register return with SSE disabled}}
+#endif
 int f_ret3(float x) {
   return (int)f_ret2(x);
 }