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

[rnk]

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 clang to avoid crashing and report a proper diagnostic.

No tests are included in this patch, since it was already tested in llvm's codegen test suite.

[RKSimon]

Please can you add the context to the diff?

[thegameg]

In D27522#615861, @RKSimon wrote:

Please can you add the context to the diff?

I'm sorry, I'm not sure I understand what you mean by "context". Should I include the comments in the .patch? Should I add the comments added in the revision as comments in the code?

[RKSimon]

In D27522#615894, @thegameg wrote:

In D27522#615861, @RKSimon wrote:

Please can you add the context to the diff?

I'm sorry, I'm not sure I understand what you mean by "context". Should I include the comments in the .patch? Should I add the comments added in the revision as comments in the code?

http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Use svn diff --diff-cmd=diff -x -U999999 to create patches that include surrounding context so its easier to work out how the diff fits into the rest of the source.

Cheers, Simon

[thegameg]

More context added, thanks RKSimon.

[joerg]

There are two issues here. First of all, it is missing a test case. The second issue is that I somewhat don't like the local diagnose_fatal_error definition, I wonder if SDAG should provide such a helper in general.

[thegameg]

In D27522#630412, @joerg wrote:

There are two issues here. First of all, it is missing a test case.

There are already tests on this, but now it shows up in clang as just a diagnostic instead of a crash.

nosse-error1.ll
nosse-error2.ll

The second issue is that I somewhat don't like the local diagnose_fatal_error definition, I wonder if SDAG should provide such a helper in general.

I see. Should I just inline the code for each call?

Also, should FastISel be handling this too ?

[rnk]

I'll clean this up and land it with a clang-side test. We get too many issues about this.

[rnk]

• clean up, add test

cfe/trunk/test/CodeGen/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}}
+  return 1.4;
+}
+extern double g;
+void f2(void) { // expected-error {{SSE register return with SSE disabled}}
+  g = f1();
+}
+void take_double(double);
+void pass_double(void) {
+  // FIXME: Still asserts.
+  //take_double(1.5);
+}

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

 #include "llvm/CodeGen/MachineJumpTableInfo.h"
 #include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/WinEHFuncInfo.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/CallingConv.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GlobalAlias.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCExpr.h"

 
 static cl::opt<int> ExperimentalPrefLoopAlignment(
     "x86-experimental-pref-loop-alignment", cl::init(4),
     cl::desc("Sets the preferable loop alignment for experiments "
              "(the last x86-experimental-pref-loop-alignment bits"
              " of the loop header PC will be 0)."),
     cl::Hidden);
 
+/// Call this when the user attempts to do something unsupported, like
+/// returning a double without SSE2 enabled on x86_64. This is not fatal, unlike
+/// report_fatal_error, so calling code should attempt to recover without
+/// crashing.
+static void errorUnsupported(SelectionDAG &DAG, const SDLoc &dl,
+                             const char *Msg) {
+  MachineFunction &MF = DAG.getMachineFunction();
+  DAG.getContext()->diagnose(
+      DiagnosticInfoUnsupported(*MF.getFunction(), Msg, dl.getDebugLoc()));
+}
+
 X86TargetLowering::X86TargetLowering(const X86TargetMachine &TM,
                                      const X86Subtarget &STI)
     : TargetLowering(TM), Subtarget(STI) {
   bool UseX87 = !Subtarget.useSoftFloat() && Subtarget.hasX87();
   X86ScalarSSEf64 = Subtarget.hasSSE2();
   X86ScalarSSEf32 = Subtarget.hasSSE1();
   MVT PtrVT = MVT::getIntegerVT(8 * TM.getPointerSize());
 
for (unsigned I = 0, OutsIndex = 0, E = RVLocs.size(); I != E;
 
     assert(VA.getLocInfo() != CCValAssign::FPExt &&
            "Unexpected FP-extend for return value.");
 
     // If this is x86-64, and we disabled SSE, we can't return FP values,
     // or SSE or MMX vectors.
     if ((ValVT == MVT::f32 || ValVT == MVT::f64 ||
          VA.getLocReg() == X86::XMM0 || VA.getLocReg() == X86::XMM1) &&
-          (Subtarget.is64Bit() && !Subtarget.hasSSE1())) {
-      report_fatal_error("SSE register return with SSE disabled");
-    }
+      errorUnsupported(DAG, dl, "SSE register return with SSE disabled");
+      VA.convertToReg(X86::FP0); // Set reg to FP0, avoid hitting asserts.
+    } else if (ValVT == MVT::f64 &&
+               (Subtarget.is64Bit() && !Subtarget.hasSSE2())) {
       // Likewise we can't return F64 values with SSE1 only.  gcc does so, but
       // llvm-gcc has never done it right and no one has noticed, so this
       // should be OK for now.
-    if (ValVT == MVT::f64 &&
-        (Subtarget.is64Bit() && !Subtarget.hasSSE2()))
-      report_fatal_error("SSE2 register return with SSE2 disabled");
+      errorUnsupported(DAG, dl, "SSE2 register return with SSE2 disabled");
+      VA.convertToReg(X86::FP0); // Set reg to FP0, avoid hitting asserts.
+    }
 
     // Returns in ST0/ST1 are handled specially: these are pushed as operands to
     // the RET instruction and handled by the FP Stackifier.
     if (VA.getLocReg() == X86::FP0 ||
         VA.getLocReg() == X86::FP1) {
       // If this is a copy from an xmm register to ST(0), use an FPExtend to
       // change the value to the FP stack register class.
       if (isScalarFPTypeInSSEReg(VA.getValVT()))
if (RegMask) {
       for (MCSubRegIterator SubRegs(VA.getLocReg(), TRI, /*IncludeSelf=*/true);
            SubRegs.isValid(); ++SubRegs)
         RegMask[*SubRegs / 32] &= ~(1u << (*SubRegs % 32));
     }
 
     // If this is x86-64, and we disabled SSE, we can't return FP values
     if ((CopyVT == MVT::f32 || CopyVT == MVT::f64 || CopyVT == MVT::f128) &&
         ((Is64Bit || Ins[InsIndex].Flags.isInReg()) && !Subtarget.hasSSE1())) {
-      report_fatal_error("SSE register return with SSE disabled");
+      errorUnsupported(DAG, dl, "SSE register return with SSE disabled");
+      VA.convertToReg(X86::FP0); // Set reg to FP0, avoid hitting asserts.
     }
 
     // If we prefer to use the value in xmm registers, copy it out as f80 and
     // use a truncate to move it from fp stack reg to xmm reg.
     bool RoundAfterCopy = false;
     if ((VA.getLocReg() == X86::FP0 || VA.getLocReg() == X86::FP1) &&
         isScalarFPTypeInSSEReg(VA.getValVT())) {
       if (!Subtarget.hasX87())