[PATCH] [Sema][X86] Don't allow floating-point return types when SSE is disabled

[thegameg]

Summary

The following program hits a fatal_error in the X86 backend, when the
program is compiled with -mno-sse or -mno-sse2, which is understandable
due to the calling convention:

float f() { return 0.5f; };

since the error occurs in the backend, there are stack traces and bug
report messages that are generated.

This patch allows the compiler to avoid crashing and check in advance if
the code can be generated properly.

• include/clang/Basic/DiagnosticSemaKinds.td: Add the error.
• lib/Sema/SemaChecking.cpp: Check if the function returns a floating-point type.

[joerg]

I think this is the absolutely wrong place to put such logic. It really can not be anywhere but the backend.

[thegameg]

In D27304#610697, @joerg wrote:

I think this is the absolutely wrong place to put such logic. It really can not be anywhere but the backend.

I am aware of this. But the way the backend informs the Diagnostics looks like a crash, and asks for a bug report.

Are there any proper ways to inform the frontend about backend errors?

[hfinkel]

In D27304#610944, @thegameg wrote:

In D27304#610697, @joerg wrote:

I think this is the absolutely wrong place to put such logic. It really can not be anywhere but the backend.

I am aware of this. But the way the backend informs the Diagnostics looks like a crash, and asks for a bug report.

Are there any proper ways to inform the frontend about backend errors?

Yes. Look at how we report inline asm errors, etc. grep for DiagnosticInfoStackSize (used in PEI) or DiagnosticInfoResourceLimit (used by the AMDGPU backend).

[ahatanak]

In D27304#610944, @thegameg wrote:

In D27304#610697, @joerg wrote:

I think this is the absolutely wrong place to put such logic. It really can not be anywhere but the backend.

I am aware of this. But the way the backend informs the Diagnostics looks like a crash, and asks for a bug report.

Are there any proper ways to inform the frontend about backend errors?

Instead of checking the return type in SemaChecking.cpp, is defining a virtual function in TargetInfo and overriding it in X86TargetInfo an option?

[efriedma]

The logic to compute whether a calling convention uses SSE registers does exist in clang/lib/CodeGen/TargetInfo.cpp, so it might be possible to reuse that... but I'm not sure that's better than just detecting it in the backend.

[joerg]

The frontend is the wrong place as it doesn't even know if the register is ever going to be used. E.g. if it is a static function, all instances could be inlined away.

[RKSimon]

https://llvm.org/bugs/show_bug.cgi?id=30426#c3

On PR30426 the proposal was that we should just not accept x86_64 triples with no-sse/no-sse2 at the command line parsing level - we just have no way to guess what the coder was expecting and performing the check later on is just delaying the (almost) inevitable.

Also, please remember to include context in the diff.

[joerg]

Rejecting -mno-sse2 for x86_64 is even worse. Dynamic linkers e.g. in FreeBSD and NetBSD depend on that. They also don't contain floating point code, but that's a separate question. Similar constraints exist for the kernels.

[mkuper]

If there are real-life usecases, then GCC-compatible behavior is probably better than just rejecting it outright, so I'm retracting my comment from PR30426.

(I would probably have liked x86_64 with -mno-sse2 and no -msoft-float to be an error, but I would also like a pony, so...)

[thegameg]

LLVM patch here: https://reviews.llvm.org/D27522

include/clang/Basic/DiagnosticSemaKinds.td

   "this builtin is only available on x86-64 targets">;
 def err_x86_builtin_invalid_rounding : Error<
   "invalid rounding argument">;
+def err_x86_sse_register : Error<
+  "SSE register return with SSE disabled">;
 
 def err_builtin_longjmp_unsupported : Error<
   "__builtin_longjmp is not supported for the current target">;

lib/Sema/SemaChecking.cpp

         CheckArgumentWithTypeTag(I, Args.data());
     }
   }
+
+  // Don't allow calls returning floating point scalars when
+  // target == x86_64 && SSE disabled.
+  if (Context.getTargetInfo().getTriple().getArch() == llvm::Triple::x86_64 &&
+      (!Context.getTargetInfo().hasFeature("sse") ||
+       !Context.getTargetInfo().hasFeature("sse2"))) {
+    if (auto *ResultType = Proto->getReturnType().getTypePtrOrNull()) {
+      if (ResultType->isRealFloatingType())
+        Diag(Loc, diag::err_x86_sse_register);
+    }
+  }
 }
 
 /// CheckConstructorCall - Check a constructor call for correctness and safety