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

[Phoebe Wang via Phabricator via cfe-commits]

pengfei added inline comments.


================
Comment at: clang/lib/Basic/Targets/X86.cpp:385
 
-  if (!HasX87) {
-    if (LongDoubleFormat == &llvm::APFloat::x87DoubleExtended())
-      HasLongDouble = false;
-    if (getTriple().getArch() == llvm::Triple::x86)
-      HasFPReturn = false;
-  }
+  if (!HasX87 && getTriple().getArch() == llvm::Triple::x86_64 &&
+      LongDoubleFormat == &llvm::APFloat::x87DoubleExtended())
----------------
asavonic wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114162/new/

https://reviews.llvm.org/D114162