[X86] Don't use fsin/fcos/fsincos instructions ever

[craig.topper]

Summary

Previously we would use these instructions if sse was disabled and fastmath was enabled.

As mentioned in D28335, this is a bad idea.

[DavidKreitzer]

Just a minor question on the test changes. Otherwise, this LGTM.

test/CodeGen/X86/sincos-opt.ll

124 ↗

(On Diff #109839)

Was moving the check for 'ret' here intentional?

[craig.topper]

test/CodeGen/X86/sincos-opt.ll

124 ↗

(On Diff #109839)

I think all I did was copy the GNU_SINCOS check lines from above and changed them to say GNU_SINCOS_FASTMATH. I hadn't noticed the difference in the ret.

[craig.topper]

test/CodeGen/X86/sincos-opt.ll

124 ↗

(On Diff #109839)

Do you want me to add it back or is it ok like this?

[DavidKreitzer]

Adding Zvi as an FYI, but I think this is fine to land.

test/CodeGen/X86/sincos-opt.ll

124 ↗

(On Diff #109839)

As far as I'm concerned it's okay either way.

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

if (!Subtarget.useSoftFloat() && X86ScalarSSEf64) {
 
     // Special cases we handle for FP constants.
     addLegalFPImmediate(APFloat(+0.0f)); // xorps
     addLegalFPImmediate(APFloat(+0.0)); // FLD0
     addLegalFPImmediate(APFloat(+1.0)); // FLD1
     addLegalFPImmediate(APFloat(-0.0)); // FLD0/FCHS
     addLegalFPImmediate(APFloat(-1.0)); // FLD1/FCHS
 
-    if (!TM.Options.UnsafeFPMath) {
-      setOperationAction(ISD::FSIN   , MVT::f64, Expand);
-      setOperationAction(ISD::FCOS   , MVT::f64, Expand);
-      setOperationAction(ISD::FSINCOS, MVT::f64, Expand);
-    }
+    // Always expand sin/cos functions even though x87 has an instruction.
   } else if (UseX87) {
     // f32 and f64 in x87.
     // Set up the FP register classes.
     addRegisterClass(MVT::f64, &X86::RFP64RegClass);
     addRegisterClass(MVT::f32, &X86::RFP32RegClass);
 
     for (auto VT : { MVT::f32, MVT::f64 }) {
       setOperationAction(ISD::UNDEF,     VT, Expand);
       setOperationAction(ISD::FCOPYSIGN, VT, Expand);
 
-      if (!TM.Options.UnsafeFPMath) {
-        setOperationAction(ISD::FSIN   , VT, Expand);
-        setOperationAction(ISD::FCOS   , VT, Expand);
-        setOperationAction(ISD::FSINCOS, VT, Expand);
-      }
-    }
+      // Always expand sin/cos functions even though x87 has an instruction.
     addLegalFPImmediate(APFloat(+0.0)); // FLD0
     addLegalFPImmediate(APFloat(+1.0)); // FLD1
     addLegalFPImmediate(APFloat(-0.0)); // FLD0/FCHS
     addLegalFPImmediate(APFloat(-1.0)); // FLD1/FCHS
     addLegalFPImmediate(APFloat(+0.0f)); // FLD0
     addLegalFPImmediate(APFloat(+1.0f)); // FLD1
     addLegalFPImmediate(APFloat(-0.0f)); // FLD0/FCHS
     addLegalFPImmediate(APFloat(-1.0f)); // FLD1/FCHS
setOperationAction(ISD::FCOPYSIGN, MVT::f80, Expand);
       APFloat TmpFlt2(+1.0);
       TmpFlt2.convert(APFloat::x87DoubleExtended(), APFloat::rmNearestTiesToEven,
                       &ignored);
       addLegalFPImmediate(TmpFlt2);  // FLD1
       TmpFlt2.changeSign();
       addLegalFPImmediate(TmpFlt2);  // FLD1/FCHS
     }
 
-    if (!TM.Options.UnsafeFPMath) {
-      setOperationAction(ISD::FSIN   , MVT::f80, Expand);
-      setOperationAction(ISD::FCOS   , MVT::f80, Expand);
-      setOperationAction(ISD::FSINCOS, MVT::f80, Expand);
-    }
+    // Always expand sin/cos functions even though x87 has an instruction.
 
     setOperationAction(ISD::FFLOOR, MVT::f80, Expand);
     setOperationAction(ISD::FCEIL,  MVT::f80, Expand);
     setOperationAction(ISD::FTRUNC, MVT::f80, Expand);
     setOperationAction(ISD::FRINT,  MVT::f80, Expand);
     setOperationAction(ISD::FNEARBYINT, MVT::f80, Expand);
     setOperationAction(ISD::FMA, MVT::f80, Expand);
   }

llvm/trunk/test/CodeGen/X86/sincos-opt.ll

 entry:
 ; GNU_SINCOS-LABEL: test3:
 ; GNU_SINCOS: callq sincosl
 ; GNU_SINCOS: fldt 16(%rsp)
 ; GNU_SINCOS: fldt 32(%rsp)
 ; GNU_SINCOS: faddp %st(1)
 
 ; GNU_SINCOS_FASTMATH-LABEL: test3:
-; GNU_SINCOS_FASTMATH: fsin
-; GNU_SINCOS_FASTMATH: fcos
+; GNU_SINCOS_FASTMATH: callq sincosl
+; GNU_SINCOS_FASTMATH: fldt 16(%{{[re]}}sp)
+; GNU_SINCOS_FASTMATH: fldt 32(%{{[re]}}sp)
 ; GNU_SINCOS_FASTMATH: faddp %st(1)
-; GNU_SINCOS_FASTMATH: ret
   %call = tail call x86_fp80 @sinl(x86_fp80 %x) readnone
   %call1 = tail call x86_fp80 @cosl(x86_fp80 %x) readnone
   %add = fadd x86_fp80 %call, %call1
   ret x86_fp80 %add
 }
 
 define x86_fp80 @test3_errno(x86_fp80 %x) nounwind {
 entry:

llvm/trunk/test/CodeGen/X86/sincos.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; Make sure this testcase codegens to the sin and cos instructions, not calls
 ; RUN: llc < %s -mtriple=i686-apple-macosx -mattr=-sse,-sse2,-sse3 -enable-unsafe-fp-math  | FileCheck %s --check-prefix=CHECK --check-prefix=UNSAFE
 ; RUN: llc < %s -mtriple=i686-apple-macosx -mattr=-sse,-sse2,-sse3 | FileCheck %s --check-prefix=CHECK --check-prefix=SAFE
 
 declare float  @sinf(float) readonly
 
 declare double @sin(double) readonly
 
 declare x86_fp80 @sinl(x86_fp80) readonly
 
 define float @test1(float %X) {
-; UNSAFE-LABEL: test1:
-; UNSAFE:       ## BB#0:
-; UNSAFE-NEXT:    flds {{[0-9]+}}(%esp)
-; UNSAFE-NEXT:    fsin
-; UNSAFE-NEXT:    retl
-;
-; SAFE-LABEL: test1:
-; SAFE:       ## BB#0:
-; SAFE-NEXT:    subl $12, %esp
-; SAFE-NEXT:  Lcfi0:
-; SAFE-NEXT:    .cfi_def_cfa_offset 16
-; SAFE-NEXT:    flds {{[0-9]+}}(%esp)
-; SAFE-NEXT:    fstps (%esp)
-; SAFE-NEXT:    calll _sinf
-; SAFE-NEXT:    addl $12, %esp
-; SAFE-NEXT:    retl
+; CHECK-LABEL: test1:
+; CHECK:       ## BB#0:
+; CHECK-NEXT:    subl $12, %esp
+; CHECK-NEXT:  Lcfi0:
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
+; CHECK-NEXT:    flds {{[0-9]+}}(%esp)
+; CHECK-NEXT:    fstps (%esp)
+; CHECK-NEXT:    calll _sinf
+; CHECK-NEXT:    addl $12, %esp
+; CHECK-NEXT:    retl
         %Y = call float @sinf(float %X) readonly
         ret float %Y
 }
 
 define double @test2(double %X) {
-; UNSAFE-LABEL: test2:
-; UNSAFE:       ## BB#0:
-; UNSAFE-NEXT:    fldl {{[0-9]+}}(%esp)
-; UNSAFE-NEXT:    fsin
-; UNSAFE-NEXT:    retl
-;
-; SAFE-LABEL: test2:
-; SAFE:       ## BB#0:
-; SAFE-NEXT:    subl $12, %esp
-; SAFE-NEXT:  Lcfi1:
-; SAFE-NEXT:    .cfi_def_cfa_offset 16
-; SAFE-NEXT:    fldl {{[0-9]+}}(%esp)
-; SAFE-NEXT:    fstpl (%esp)
-; SAFE-NEXT:    calll _sin
-; SAFE-NEXT:    addl $12, %esp
-; SAFE-NEXT:    retl
+; CHECK-LABEL: test2:
+; CHECK:       ## BB#0:
+; CHECK-NEXT:    subl $12, %esp
+; CHECK-NEXT:  Lcfi1:
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
+; CHECK-NEXT:    fldl {{[0-9]+}}(%esp)
+; CHECK-NEXT:    fstpl (%esp)
+; CHECK-NEXT:    calll _sin
+; CHECK-NEXT:    addl $12, %esp
+; CHECK-NEXT:    retl
         %Y = call double @sin(double %X) readonly
         ret double %Y
 }
 
 define x86_fp80 @test3(x86_fp80 %X) {
-; UNSAFE-LABEL: test3:
-; UNSAFE:       ## BB#0:
-; UNSAFE-NEXT:    fldt {{[0-9]+}}(%esp)
-; UNSAFE-NEXT:    fsin
-; UNSAFE-NEXT:    retl
-;
-; SAFE-LABEL: test3:
-; SAFE:       ## BB#0:
-; SAFE-NEXT:    subl $28, %esp
-; SAFE-NEXT:  Lcfi2:
-; SAFE-NEXT:    .cfi_def_cfa_offset 32
-; SAFE-NEXT:    fldt {{[0-9]+}}(%esp)
-; SAFE-NEXT:    fstpt (%esp)
-; SAFE-NEXT:    calll _sinl
-; SAFE-NEXT:    addl $28, %esp
-; SAFE-NEXT:    retl
+; CHECK-LABEL: test3:
+; CHECK:       ## BB#0:
+; CHECK-NEXT:    subl $28, %esp
+; CHECK-NEXT:  Lcfi2:
+; CHECK-NEXT:    .cfi_def_cfa_offset 32
+; CHECK-NEXT:    fldt {{[0-9]+}}(%esp)
+; CHECK-NEXT:    fstpt (%esp)
+; CHECK-NEXT:    calll _sinl
+; CHECK-NEXT:    addl $28, %esp
+; CHECK-NEXT:    retl
         %Y = call x86_fp80 @sinl(x86_fp80 %X) readonly
         ret x86_fp80 %Y
 }
 
 declare float @cosf(float) readonly
 
 declare double @cos(double) readonly
 
 declare x86_fp80 @cosl(x86_fp80) readonly
 
 
 define float @test4(float %X) {
-; UNSAFE-LABEL: test4:
-; UNSAFE:       ## BB#0:
-; UNSAFE-NEXT:    flds {{[0-9]+}}(%esp)
-; UNSAFE-NEXT:    fcos
-; UNSAFE-NEXT:    retl
-;
-; SAFE-LABEL: test4:
-; SAFE:       ## BB#0:
-; SAFE-NEXT:    subl $12, %esp
-; SAFE-NEXT:  Lcfi3:
-; SAFE-NEXT:    .cfi_def_cfa_offset 16
-; SAFE-NEXT:    flds {{[0-9]+}}(%esp)
-; SAFE-NEXT:    fstps (%esp)
-; SAFE-NEXT:    calll _cosf
-; SAFE-NEXT:    addl $12, %esp
-; SAFE-NEXT:    retl
+; CHECK-LABEL: test4:
+; CHECK:       ## BB#0:
+; CHECK-NEXT:    subl $12, %esp
+; CHECK-NEXT:  Lcfi3:
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
+; CHECK-NEXT:    flds {{[0-9]+}}(%esp)
+; CHECK-NEXT:    fstps (%esp)
+; CHECK-NEXT:    calll _cosf
+; CHECK-NEXT:    addl $12, %esp
+; CHECK-NEXT:    retl
         %Y = call float @cosf(float %X) readonly
         ret float %Y
 }
 
 define double @test5(double %X) {
-; UNSAFE-LABEL: test5:
-; UNSAFE:       ## BB#0:
-; UNSAFE-NEXT:    fldl {{[0-9]+}}(%esp)
-; UNSAFE-NEXT:    fcos
-; UNSAFE-NEXT:    retl
-;
-; SAFE-LABEL: test5:
-; SAFE:       ## BB#0:
-; SAFE-NEXT:    subl $12, %esp
-; SAFE-NEXT:  Lcfi4:
-; SAFE-NEXT:    .cfi_def_cfa_offset 16
-; SAFE-NEXT:    fldl {{[0-9]+}}(%esp)
-; SAFE-NEXT:    fstpl (%esp)
-; SAFE-NEXT:    calll _cos
-; SAFE-NEXT:    addl $12, %esp
-; SAFE-NEXT:    retl
+; CHECK-LABEL: test5:
+; CHECK:       ## BB#0:
+; CHECK-NEXT:    subl $12, %esp
+; CHECK-NEXT:  Lcfi4:
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
+; CHECK-NEXT:    fldl {{[0-9]+}}(%esp)
+; CHECK-NEXT:    fstpl (%esp)
+; CHECK-NEXT:    calll _cos
+; CHECK-NEXT:    addl $12, %esp
+; CHECK-NEXT:    retl
         %Y = call double @cos(double %X) readonly
         ret double %Y
 }
 
 define x86_fp80 @test6(x86_fp80 %X) {
-; UNSAFE-LABEL: test6:
-; UNSAFE:       ## BB#0:
-; UNSAFE-NEXT:    fldt {{[0-9]+}}(%esp)
-; UNSAFE-NEXT:    fcos
-; UNSAFE-NEXT:    retl
-;
-; SAFE-LABEL: test6:
-; SAFE:       ## BB#0:
-; SAFE-NEXT:    subl $28, %esp
-; SAFE-NEXT:  Lcfi5:
-; SAFE-NEXT:    .cfi_def_cfa_offset 32
-; SAFE-NEXT:    fldt {{[0-9]+}}(%esp)
-; SAFE-NEXT:    fstpt (%esp)
-; SAFE-NEXT:    calll _cosl
-; SAFE-NEXT:    addl $28, %esp
-; SAFE-NEXT:    retl
+; CHECK-LABEL: test6:
+; CHECK:       ## BB#0:
+; CHECK-NEXT:    subl $28, %esp
+; CHECK-NEXT:  Lcfi5:
+; CHECK-NEXT:    .cfi_def_cfa_offset 32
+; CHECK-NEXT:    fldt {{[0-9]+}}(%esp)
+; CHECK-NEXT:    fstpt (%esp)
+; CHECK-NEXT:    calll _cosl
+; CHECK-NEXT:    addl $28, %esp
+; CHECK-NEXT:    retl
         %Y = call x86_fp80 @cosl(x86_fp80 %X) readonly
         ret x86_fp80 %Y
 }