Page MenuHomePhabricator

[X86][clang] Enable floating-point type for -mno-x87 option on 32-bits
ClosedPublic

Authored by pengfei on Thu, Nov 18, 6:49 AM.

Details

Summary

We should match GCC's behavior which allows floating-point type for -mno-x87 option on 32-bits. https://godbolt.org/z/KrbhfWc9o

The previous block issues have partially been fixed by D112143.

Diff Detail

Event Timeline

pengfei requested review of this revision.Thu, Nov 18, 6:49 AM
pengfei created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptThu, Nov 18, 6:49 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
asavonic added inline comments.Fri, Nov 19, 1:11 AM
clang/lib/Basic/Targets/X86.cpp
385–388

I see that D112143 changed the ABI so that FP return values do not use x87 registers on i386. Therefore HasFPReturn flag can be removed.

However, operations with long double (x87 80-bit) should still be unsupported on both targets, because IIRC there is no SSE equivalent for them. GCC compiles them as soft-fp when -mno-x87 is set, but I haven't found 80-bit soft-fp implementation in LLVM.

long double baz(long double a, long double b) {
    return a + b;
}
baz:
   [...]
   call  __addxf3

For some reason GCC only does this for for i386 target, for x86_64 it just emits the diagnostic about disabled x87.

pengfei added inline comments.Fri, Nov 19, 1:35 AM
clang/lib/Basic/Targets/X86.cpp
385–388

Thanks for looking at this patch.
I don't think we need to exclude f80 particularly. IIUC, backend tries all possible ways to lower a given operation. Lowering to library is always the last choice. So the behavior is not confined to soft-fp.
It's true LLVM has problems with f80 lowering without x87. I commented it in D112143 and hope D100091 will fix them. We don't need to bother to change it again in future.

For some reason GCC only does this for for i386 target, for x86_64 it just emits the diagnostic about disabled x87.

I think the root reason is the difference in ABI. 32-bits ABI allows passing and returning f80 without x87 registers while 64-bits doesn't. So we have to and only need to disable it for x86_64.

asavonic added inline comments.Fri, Nov 19, 2:01 AM
clang/lib/Basic/Targets/X86.cpp
385–388

I don't think we need to exclude f80 particularly. IIUC, backend tries all possible ways to lower a given operation. Lowering to library is always the last choice. So the behavior is not confined to soft-fp.
It's true LLVM has problems with f80 lowering without x87. I commented it in D112143 and hope D100091 will fix them. We don't need to bother to change it again in future.

Right, but can LLVM lower any x87 80-bit fp operations other than return values?
If it cannot, then I think a source level diagnostic is a good thing to have. Otherwise the only handling we have is the codegen crash with "x87 register return with x87 disabled" and no source-level context.

pengfei updated this revision to Diff 389914.Thu, Nov 25, 10:18 PM

Emit diagnostic for long double for i386 target too.

pengfei added inline comments.Thu, Nov 25, 10:21 PM
clang/lib/Basic/Targets/X86.cpp
385–388

No. I checked we are able to lower with changes like below. But it requires enabling all operations with full test. So emitting diagnostic seems good for now.

--- a/llvm/lib/Target/X86/X86ISelLowering.cpp
+++ b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -729,6 +729,8 @@ X86TargetLowering::X86TargetLowering(const X86TargetMachine &TM,
     // FIXME: When the target is 64-bit, STRICT_FP_ROUND will be overwritten
     // as Custom.
     setOperationAction(ISD::STRICT_FP_ROUND, MVT::f80, Legal);
+  } else {
+    setOperationAction(ISD::FADD,        MVT::f80, LibCall);
   }

   // f128 uses xmm registers, but most operations require libcalls.


--- a/llvm/lib/Target/X86/X86InstrFormats.td
+++ b/llvm/lib/Target/X86/X86InstrFormats.td
@@ -472,6 +472,7 @@ class Ii32PCRel<bits<8> o, Format f, dag outs, dag ins, string asm,
 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.
@@ -479,6 +480,7 @@ 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


--- a/llvm/lib/Target/X86/X86InstrInfo.td
+++ b/llvm/lib/Target/X86/X86InstrInfo.td
@@ -929,6 +929,7 @@ 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()">;
asavonic accepted this revision.Mon, Nov 29, 12:27 AM

LGTM. We can also remove all code related to HasFPReturn, it is no longer needed.

This revision is now accepted and ready to land.Mon, Nov 29, 12:27 AM
nickdesaulniers accepted this revision.Mon, Nov 29, 10:53 AM

Thanks for the patch!

LGTM. We can also remove all code related to HasFPReturn, it is no longer needed.

I think we still need this flag, maybe the error message should be changed to "SSE register return with SSE disabled"? https://godbolt.org/z/KcGf751GE
I can do it as a follow up.