Limit sincos to Linux

[andrew]

Summary

FreeBSD doesn't have sincos, and as far as I can tell nether do the other BSDs. As such restrict the calls to this to just Linux.

[emaste]

lib/CodeGen/TargetLoweringBase.cpp

490

Which systems have isGNUEnvironment true?

[andrew]

lib/CodeGen/TargetLoweringBase.cpp

490

It seems to be when the triple ends with gnu*

[emaste]

Not part of this change, but it seems unfortunate that we use isGNUEnvironment or isOSLinux here to determine if sincos is available, but then in lib/Target/ARM/ARMSubtarget.cpp:

bool ARMSubtarget::hasSinCos() const {
  return isTargetWatchOS() ||
    (isTargetIOS() && !getTargetTriple().isOSVersionLT(7, 0));
}

and lib/Target/X86/X86ISelLowering.cpp:

bool X86Subtarget::hasSinCos() const {
  return getTargetTriple().isMacOSX() &&
    !getTargetTriple().isMacOSXVersionLT(10, 9) &&
    is64Bit();
}

[dim]

It looks good to me.

It is strange that both TargetLoweringBase.cpp and the ARM/X86 SubTargets have their own "hasSinCos" logic, though.

[rengolin]

I don't understand this change. I don't expect to see sincos in freebsd even in the old code. If it did, than there was something wrong that wasn't fixed...

I'm also surprised that linux-gnu is not a GNU environment...

Can you dig a bit more the reason for those changes?

Thanks!
--renato

[andrew]

The ARM/X86 hasSinCos seems to be for macOS/iOS/watchOS.

We are getting sincos on FreeBSD on arm because we use armv6-freebsd-gnueabihf so TT.isGNUEnvironment() returns true enabling these functions.

linux-gnu is a gnu environment, however it's missing --enable-unsafe-fp-math which is needed to enable the call to sincos.

[rengolin]

In D28570#643850, @andrew wrote:

The ARM/X86 hasSinCos seems to be for macOS/iOS/watchOS.

Hum, that sounds wrong. GLIBC has sincos since version 2.1, and if we're using it (whether ARM or x86), we can include it.

We are getting sincos on FreeBSD on arm because we use armv6-freebsd-gnueabihf so TT.isGNUEnvironment() returns true enabling these functions.

Why do you use "gnueabihf" if you don't use GLIBC? Why not just EABIHF, which would *also* work with GLIBC and GCC, but not have the idiosyncrasies of GLIBC.

linux-gnu is a gnu environment, however it's missing --enable-unsafe-fp-math which is needed to enable the call to sincos.

Right. "linux-gnu" is not used with ARM. I know it's not in this patch, but we should change that to "gnueabi" instead.

cheers,
--renato

[dim]

lib/CodeGen/TargetLoweringBase.cpp

490

See the code in include/llvm/ADT/Triple.h:

bool isGNUEnvironment() const {
  EnvironmentType Env = getEnvironment();
  return Env == Triple::GNU || Env == Triple::GNUABI64 ||
         Env == Triple::GNUEABI || Env == Triple::GNUEABIHF ||
         Env == Triple::GNUX32;
}

Though maybe a better name for the function would be isGLibCEnvironment() ... :)

[rengolin]

lib/CodeGen/TargetLoweringBase.cpp

490

No, other libraries can emulate the GNU environment just as well, and they still need to provide whatever GNU does, or they shouldn't be called "gnuabi".

This just looks like a nomenclature problem. FreeBSD uses "gnuabi" names and don't want to provide "gnuabi" features.

I can see only two solutions:

1. Short-term: include a " && ! Triple::BSD" or something on isGNUEnvironment with a BIG FIXME line.

2. Long-term: emulate the full GNU ABI, or stop calling itself "gnueabi".

lib/CodeGen/TargetLoweringBase.cpp

static void InitLibcallNames(const char **Names, const Triple &TT) {
   Names[RTLIB::ATOMIC_FETCH_XOR_8] = "__atomic_fetch_xor_8";
   Names[RTLIB::ATOMIC_FETCH_XOR_16] = "__atomic_fetch_xor_16";
   Names[RTLIB::ATOMIC_FETCH_NAND_1] = "__atomic_fetch_nand_1";
   Names[RTLIB::ATOMIC_FETCH_NAND_2] = "__atomic_fetch_nand_2";
   Names[RTLIB::ATOMIC_FETCH_NAND_4] = "__atomic_fetch_nand_4";
   Names[RTLIB::ATOMIC_FETCH_NAND_8] = "__atomic_fetch_nand_8";
   Names[RTLIB::ATOMIC_FETCH_NAND_16] = "__atomic_fetch_nand_16";
 
-  if (TT.isGNUEnvironment()) {
+  if (TT.isOSLinux()) {
     Names[RTLIB::SINCOS_F32] = "sincosf";
     Names[RTLIB::SINCOS_F64] = "sincos";
     Names[RTLIB::SINCOS_F80] = "sincosl";
     Names[RTLIB::SINCOS_F128] = "sincosl";
     Names[RTLIB::SINCOS_PPCF128] = "sincosl";
   }
 
   if (!TT.isOSOpenBSD()) {

test/CodeGen/ARM/sincos.ll

 ; RUN: llc < %s -mtriple=armv7-apple-ios6 -mcpu=cortex-a8 | FileCheck %s --check-prefix=NOOPT
 ; RUN: llc < %s -mtriple=armv7-apple-ios7 -mcpu=cortex-a8 | FileCheck %s --check-prefix=SINCOS
 ; RUN: llc < %s -mtriple=armv7-linux-gnu -mcpu=cortex-a8 | FileCheck %s --check-prefix=NOOPT-GNU
 ; RUN: llc < %s -mtriple=armv7-linux-gnueabi -mcpu=cortex-a8 \
 ; RUN:   --enable-unsafe-fp-math | FileCheck %s --check-prefix=SINCOS-GNU
+; RUN: llc < %s -mtriple=armv7-freebsd-gnueabi -mcpu=cortex-a8 \
+; RUN:   --enable-unsafe-fp-math | FileCheck %s --check-prefix=NOOPT-FREEBSD
 
 ; Combine sin / cos into a single call.
 ; rdar://12856873
 
 define float @test1(float %x) nounwind {
 entry:
 ; SINCOS-LABEL: test1:
 ; SINCOS: bl ___sincosf_stret
 
 ; SINCOS-GNU-LABEL: test1:
 ; SINCOS-GNU: bl sincosf
 
 ; NOOPT-LABEL: test1:
 ; NOOPT: bl _sinf
 ; NOOPT: bl _cosf
 
 ; NOOPT-GNU-LABEL: test1:
 ; NOOPT-GNU: bl sinf
 ; NOOPT-GNU: bl cosf
 
+; NOOPT-FREEBSD-LABEL: test1:
+; NOOPT-FREEBSD: bl sinf
+; NOOPT-FREEBSD: bl cosf
+
   %call = tail call float @sinf(float %x) nounwind readnone
   %call1 = tail call float @cosf(float %x) nounwind readnone
   %add = fadd float %call, %call1
   ret float %add
 }
 
 define double @test2(double %x) nounwind {
 entry:
 ; SINCOS-LABEL: test2:
 ; SINCOS: bl ___sincos_stret
 
 ; SINCOS-GNU-LABEL: test2:
 ; SINCOS-GNU: bl sincos
 
 ; NOOPT-LABEL: test2:
 ; NOOPT: bl _sin
 ; NOOPT: bl _cos
 
 ; NOOPT-GNU-LABEL: test2:
 ; NOOPT-GNU: bl sin
 ; NOOPT-GNU: bl cos
+
+; NOOPT-FREEBSD-LABEL: test2:
+; NOOPT-FREEBSD: bl sin
+; NOOPT-FREEBSD: bl cos
   %call = tail call double @sin(double %x) nounwind readnone
   %call1 = tail call double @cos(double %x) nounwind readnone
   %add = fadd double %call, %call1
   ret double %add
 }
 
 declare float  @sinf(float) readonly
 declare double @sin(double) readonly
 declare float @cosf(float) readonly
 declare double @cos(double) readonly