[X86] Fix wrong handle with "-mno-x87"

[LiuChen3]

Summary

When the '-mno-x87' command line option is used, it should prevent all generation
of x87 instructions. Currently there are several circumstances under which it does not.
For example: https://godbolt.org/z/qnPaW7dd6

This patch adds "HasX87" predicates to each X87 instructions and when 80-bit
float pointer is supposed to pass or return on the x87 stack, reporting
an error if X87 is not supported.

[craig.topper]

What about cases that use x87 to do float/double conversions? I think it’s i64 conversions in 32-bit mode. But maybe other cases. Does gcc disable those with -mno-80387?

[LiuChen3]

In D100091#2675968, @craig.topper wrote:

What about cases that use x87 to do float/double conversions? I think it’s i64 conversions in 32-bit mode. But maybe other cases. Does gcc disable those with -mno-80387?

You are right! GCC can do the conversions: https://godbolt.org/z/87ez838oc .
With this patch, llvm will fail in ISEL. I want to make another patch to do this.

[craig.topper]

In D100091#2675996, @LiuChen3 wrote:

In D100091#2675968, @craig.topper wrote:

What about cases that use x87 to do float/double conversions? I think it’s i64 conversions in 32-bit mode. But maybe other cases. Does gcc disable those with -mno-80387?

You are right! GCC can do the conversions: https://godbolt.org/z/87ez838oc .
With this patch, llvm will fail in ISEL. I want to make another patch to do this.

I'm more concerned about these cases that don't use long double and aren't missed constant folding. https://godbolt.org/z/qGv54ef34

[LiuChen3]

I'm more concerned about these cases that don't use long double and aren't missed constant folding. https://godbolt.org/z/qGv54ef34

I see. This also break the ABI. Even when returning fp80 on 32bit target, gcc does not report an error. Can llvm report an error just like on a 64bit target?
Should we discuss the ABI issue on llvm-dev?

[LiuChen3]

In D100091#2677235, @craig.topper wrote:

In D100091#2675996, @LiuChen3 wrote:

In D100091#2675968, @craig.topper wrote:

What about cases that use x87 to do float/double conversions? I think it’s i64 conversions in 32-bit mode. But maybe other cases. Does gcc disable those with -mno-80387?

You are right! GCC can do the conversions: https://godbolt.org/z/87ez838oc .
With this patch, llvm will fail in ISEL. I want to make another patch to do this.

I'm more concerned about these cases that don't use long double and aren't missed constant folding. https://godbolt.org/z/qGv54ef34

It seems not easy to handle this. I want fix a bug in this patch. For this part, It looks more like a feature. We want to do that in another patch. What's your opinion here?

[craig.topper]

In D100091#2707381, @LiuChen3 wrote:

In D100091#2677235, @craig.topper wrote:

In D100091#2675996, @LiuChen3 wrote:

In D100091#2675968, @craig.topper wrote:

What about cases that use x87 to do float/double conversions? I think it’s i64 conversions in 32-bit mode. But maybe other cases. Does gcc disable those with -mno-80387?

You are right! GCC can do the conversions: https://godbolt.org/z/87ez838oc .
With this patch, llvm will fail in ISEL. I want to make another patch to do this.

I'm more concerned about these cases that don't use long double and aren't missed constant folding. https://godbolt.org/z/qGv54ef34

It seems not easy to handle this. I want fix a bug in this patch. For this part, It looks more like a feature. We want to do that in another patch. What's your opinion here?

What is the usage model you’re trying to enable? This has been broken for a long time. Why is it important now?

[craig.topper]

Does adding HasX87 predicates to the instructions cause the conversions in my example to fail?

[LiuChen3]

In D100091#2707521, @craig.topper wrote:

Does adding HasX87 predicates to the instructions cause the conversions in my example to fail?

Yes, it is. But this fail appeared before isel because of breaking ABI.

[LiuChen3]

What is the usage model you’re trying to enable? This has been broken for a long time. Why is it important now?

I don't know any specific case for now. This was found by @andrew.w.kaylor. I will check with him.

[andrew.w.kaylor]

In D100091#2707536, @LiuChen3 wrote:

What is the usage model you’re trying to enable? This has been broken for a long time. Why is it important now?

I don't know any specific case for now. This was found by @andrew.w.kaylor. I will check with him.

We have libraries that for performance reasons want to be able to avoid saving and restoring the x87 state and to safely do this, they need to rely on the fact that we won't generate x87 instructions if the compiler is told not to. That's the general motivation. This particular case is an easy contrived case that I came up with to show that the no-x87 option isn't always being respected. I don't think this case matters for our library because it's essentially user error. In theory, the front end code that sets up the ABI conformance could detect this case and report an error. Of course, that means relying on every front end to the right thing, so I think it would be better to (also?) reject this in the backend.

Basically, my ultimate goal here is to achieve a robust solution where "no-x87" means no x87 in all circumstances.

[craig.topper]

In D100091#2712957, @andrew.w.kaylor wrote:

In D100091#2707536, @LiuChen3 wrote:

What is the usage model you’re trying to enable? This has been broken for a long time. Why is it important now?

I don't know any specific case for now. This was found by @andrew.w.kaylor. I will check with him.

We have libraries that for performance reasons want to be able to avoid saving and restoring the x87 state and to safely do this, they need to rely on the fact that we won't generate x87 instructions if the compiler is told not to. That's the general motivation. This particular case is an easy contrived case that I came up with to show that the no-x87 option isn't always being respected. I don't think this case matters for our library because it's essentially user error. In theory, the front end code that sets up the ABI conformance could detect this case and report an error. Of course, that means relying on every front end to the right thing, so I think it would be better to (also?) reject this in the backend.

Basically, my ultimate goal here is to achieve a robust solution where "no-x87" means no x87 in all circumstances.

Thanks. Is the compiler still allowed to use the xmm registers? Is this needed for 32-bit mode or just 64-bit? Trying to understand how many problems need to be fixed.

[pengfei]

llvm/lib/Target/X86/X86ISelLowering.cpp

2737–2740

We don't need it after D112143.

3248–3249

ditto.

llvm/test/CodeGen/X86/nox87-error.ll

1

ditto.

llvm/lib/Target/X86/X86ISelLowering.cpp

if (!Subtarget.hasSSE1() && X86::FR32XRegClass.contains(VA.getLocReg())) {
       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) {
+      // When returning a value via  an X87 register, report an error if X87 is
+      // disabled.
+      if (!Subtarget.hasX87())
+        report_fatal_error("x87 register return with x87 disabled");
       // 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()))
         ValToCopy = DAG.getNode(ISD::FP_EXTEND, dl, MVT::f80, ValToCopy);
       RetVals.push_back(std::make_pair(VA.getLocReg(), ValToCopy));
       // Don't emit a copytoreg.
       continue;
     }
for (unsigned I = 0, InsIndex = 0, E = RVLocs.size(); I != E;
     }
 
     // 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())
-        report_fatal_error("X87 register return with X87 disabled");
+        report_fatal_error("x87 register return with x87 disabled");
       CopyVT = MVT::f80;
       RoundAfterCopy = (CopyVT != VA.getLocVT());
     }
 
     SDValue Val;
     if (VA.needsCustom()) {
       assert(VA.getValVT() == MVT::v64i1 &&
              "Currently the only custom case is when we split v64i1 to 2 regs");
bool ExtendedInMem =
       VA.isExtInLoc() && VA.getValVT().getScalarType() == MVT::i1 &&
       VA.getValVT().getSizeInBits() != VA.getLocVT().getSizeInBits();
 
   if (VA.getLocInfo() == CCValAssign::Indirect || ExtendedInMem)
     ValVT = VA.getLocVT();
   else
     ValVT = VA.getValVT();
 
+  if (ValVT == MVT::f80 && !Subtarget.hasX87())
+    report_fatal_error("x87 register return with x87 disabled");
+
   // FIXME: For now, all byval parameter objects are marked mutable. This can be
   // changed with more analysis.
   // In case of tail call optimization mark all arguments mutable. Since they
   // could be overwritten by lowering of arguments in case of a tail call.
   if (Flags.isByVal()) {
     unsigned Bytes = Flags.getByValSize();
     if (Bytes == 0) Bytes = 1; // Don't create zero-sized stack objects.
 

llvm/lib/Target/X86/X86InstrFormats.td

class Ii32PCRel<bits<8> o, Format f, dag outs, dag ins, string asm,
   let CodeSize = 3;
 }
 
 // FPStack Instruction Templates:
 // FPI - Floating Point Instruction template.
 class FPI<bits<8> o, Format F, dag outs, dag ins, string asm>
   : I<o, F, outs, ins, asm, []> {
   let Defs = [FPSW];
+  let Predicates = [HasX87];
 }
 
 // FpI_ - Floating Point Pseudo Instruction template. Not Predicated.
 class FpI_<dag outs, dag ins, FPFormat fp, list<dag> pattern>
   : PseudoI<outs, ins, pattern> {
   let FPForm = fp;
   let Defs = [FPSW];
+  let Predicates = [HasX87];
 }
 
 // Templates for instructions that use a 16- or 32-bit segmented address as
 //  their only operand: lcall (FAR CALL) and ljmp (FAR JMP)
 //
 //   Iseg16 - 16-bit segment selector, 16-bit offset
 //   Iseg32 - 16-bit segment selector, 32-bit offset
 

llvm/lib/Target/X86/X86InstrInfo.td

 def NoVLX_Or_NoVNNI : Predicate<"!Subtarget->hasVLX() || !Subtarget->hasVNNI()">;
 
 def HasBITALG    : Predicate<"Subtarget->hasBITALG()">;
 def HasPOPCNT    : Predicate<"Subtarget->hasPOPCNT()">;
 def HasAES       : Predicate<"Subtarget->hasAES()">;
 def HasVAES      : Predicate<"Subtarget->hasVAES()">;
 def NoVLX_Or_NoVAES : Predicate<"!Subtarget->hasVLX() || !Subtarget->hasVAES()">;
 def HasFXSR      : Predicate<"Subtarget->hasFXSR()">;
+def HasX87       : Predicate<"Subtarget->hasX87()">;
 def HasXSAVE     : Predicate<"Subtarget->hasXSAVE()">;
 def HasXSAVEOPT  : Predicate<"Subtarget->hasXSAVEOPT()">;
 def HasXSAVEC    : Predicate<"Subtarget->hasXSAVEC()">;
 def HasXSAVES    : Predicate<"Subtarget->hasXSAVES()">;
 def HasPCLMUL    : Predicate<"Subtarget->hasPCLMUL()">;
 def NoVLX_Or_NoVPCLMULQDQ :
                     Predicate<"!Subtarget->hasVLX() || !Subtarget->hasVPCLMULQDQ()">;
 def HasVPCLMULQDQ : Predicate<"Subtarget->hasVPCLMULQDQ()">;

llvm/test/CodeGen/X86/nox87-error.ll

+; RUN: not --crash llc < %s -mattr=-x87 -o /dev/null 2>&1 | FileCheck %s --check-prefix NOX87
+; RUN: llc < %s | FileCheck %s
+
+; NOX87: LLVM ERROR: x87 register return with x87 disabled
+
+; CHECK-LABEL: f1:
+; CHECK:       fldt    8(%rsp)
+; CHECK-NEXT:  fstpt   -24(%rsp)
+; CHECK-NEXT:  retq
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define dso_local void @f1(x86_fp80 %a) #0 {
+entry:
+  %a.addr = alloca x86_fp80, align 16
+  store x86_fp80 %a, x86_fp80* %a.addr, align 16
+  ret void
+}
+