spatel updated this revision to Diff 120902.
spatel added a comment.
Patch updated:
As suggested, I've morphed this patch to just handle sqrt libcalls based on the
updated LLVM intrinsic definition.
I was going to include the builtins too, but that exposes another bug (marked
here with FIXME) - the builtins are all defined 'const'. Therefore, they can
never set errno (unless I'm still misunderstanding). So I think we are wrongly
currently turning those into libcalls marked 'readnone'.
We could wrongly turn those into intrinsics in this patch if that seems better?
:)
This does make me curious about the use-case of libm-equivalent builtins. If
they are exactly identical to libm (including errno behavior), then why are
they needed in the first place?
https://reviews.llvm.org/D39204
Files:
lib/CodeGen/CGBuiltin.cpp
test/CodeGen/2005-07-20-SqrtNoErrno.c
test/CodeGen/builtin-sqrt.c
test/CodeGen/libcalls.c
Index: test/CodeGen/libcalls.c
===================================================================
--- test/CodeGen/libcalls.c
+++ test/CodeGen/libcalls.c
@@ -6,29 +6,28 @@
// CHECK-NO-LABEL: define void @test_sqrt
// CHECK-FAST-LABEL: define void @test_sqrt
void test_sqrt(float a0, double a1, long double a2) {
- // Following llvm-gcc's lead, we never emit these as intrinsics;
- // no-math-errno isn't good enough. We could probably use intrinsics
- // with appropriate guards if it proves worthwhile.
-
// CHECK-YES: call float @sqrtf
- // CHECK-NO: call float @sqrtf
+ // CHECK-NO: call float @llvm.sqrt.f32(float
+ // CHECK-FAST: call float @llvm.sqrt.f32(float
float l0 = sqrtf(a0);
// CHECK-YES: call double @sqrt
- // CHECK-NO: call double @sqrt
+ // CHECK-NO: call double @llvm.sqrt.f64(double
+ // CHECK-FAST: call double @llvm.sqrt.f64(double
double l1 = sqrt(a1);
// CHECK-YES: call x86_fp80 @sqrtl
- // CHECK-NO: call x86_fp80 @sqrtl
+ // CHECK-NO: call x86_fp80 @llvm.sqrt.f80(x86_fp80
+ // CHECK-FAST: call x86_fp80 @llvm.sqrt.f80(x86_fp80
long double l2 = sqrtl(a2);
}
// CHECK-YES: declare float @sqrtf(float)
// CHECK-YES: declare double @sqrt(double)
// CHECK-YES: declare x86_fp80 @sqrtl(x86_fp80)
-// CHECK-NO: declare float @sqrtf(float) [[NUW_RN:#[0-9]+]]
-// CHECK-NO: declare double @sqrt(double) [[NUW_RN]]
-// CHECK-NO: declare x86_fp80 @sqrtl(x86_fp80) [[NUW_RN]]
+// CHECK-NO: declare float @llvm.sqrt.f32(float)
+// CHECK-NO: declare double @llvm.sqrt.f64(double)
+// CHECK-NO: declare x86_fp80 @llvm.sqrt.f80(x86_fp80)
// CHECK-FAST: declare float @llvm.sqrt.f32(float)
// CHECK-FAST: declare double @llvm.sqrt.f64(double)
// CHECK-FAST: declare x86_fp80 @llvm.sqrt.f80(x86_fp80)
@@ -86,7 +85,7 @@
double atan_ = atan(d);
long double atanl_ = atanl(ld);
float atanf_ = atanf(f);
-// CHECK-NO: declare double @atan(double) [[NUW_RN]]
+// CHECK-NO: declare double @atan(double) [[NUW_RN:#[0-9]+]]
// CHECK-NO: declare x86_fp80 @atanl(x86_fp80) [[NUW_RN]]
// CHECK-NO: declare float @atanf(float) [[NUW_RN]]
// CHECK-YES-NOT: declare double @atan(double) [[NUW_RN]]
@@ -126,5 +125,5 @@
// CHECK-YES: attributes [[NUW_RN]] = { nounwind readnone speculatable }
-// CHECK-NO: attributes [[NUW_RN]] = { nounwind readnone{{.*}} }
-// CHECK-NO: attributes [[NUW_RNI]] = { nounwind readnone speculatable }
+// CHECK-NO-DAG: attributes [[NUW_RN]] = { nounwind readnone{{.*}} }
+// CHECK-NO-DAG: attributes [[NUW_RNI]] = { nounwind readnone speculatable }
Index: test/CodeGen/builtin-sqrt.c
===================================================================
--- test/CodeGen/builtin-sqrt.c
+++ test/CodeGen/builtin-sqrt.c
@@ -1,10 +1,19 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin %s -emit-llvm -o - | FileCheck %s
-// llvm.sqrt has undefined behavior on negative inputs, so it is
-// inappropriate to translate C/C++ sqrt to this.
-float sqrtf(float x);
+// RUN: %clang_cc1 -fmath-errno -triple x86_64-apple-darwin %s -emit-llvm -o - | FileCheck %s --check-prefix=HAS_ERRNO
+// RUN: %clang_cc1 -triple x86_64-apple-darwin %s -emit-llvm -o - | FileCheck %s --check-prefix=NO_ERRNO
+
+// FIXME: If a builtin is supposed to have identical semantics to its libm twin, then it
+// should not be marked "constant" in Builtins.def because that means it can't set errno.
+// Note that both runs have 'readnone' on the libcall here.
+
float foo(float X) {
- // CHECK: foo
- // CHECK: call float @sqrtf(float %
- // Check that this is marked readonly when errno is ignored.
- return sqrtf(X);
+ // HAS_ERRNO: call float @sqrtf(float
+ // NO_ERRNO: call float @sqrtf(float
+ return __builtin_sqrtf(X);
}
+
+// HAS_ERRNO: declare float @sqrtf(float) [[ATTR:#[0-9]+]]
+// HAS_ERRNO: attributes [[ATTR]] = { nounwind readnone {{.*}}}
+
+// NO_ERRNO: declare float @sqrtf(float) [[ATTR:#[0-9]+]]
+// NO_ERRNO: attributes [[ATTR]] = { nounwind readnone {{.*}}}
+
Index: test/CodeGen/2005-07-20-SqrtNoErrno.c
===================================================================
--- test/CodeGen/2005-07-20-SqrtNoErrno.c
+++ test/CodeGen/2005-07-20-SqrtNoErrno.c
@@ -1,10 +0,0 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin %s -emit-llvm -o - | FileCheck %s
-// llvm.sqrt has undefined behavior on negative inputs, so it is
-// inappropriate to translate C/C++ sqrt to this.
-float sqrtf(float x);
-float foo(float X) {
- // CHECK: foo
- // CHECK: call float @sqrtf(float %
- // Check that this is marked readonly when errno is ignored.
- return sqrtf(X);
-}
Index: lib/CodeGen/CGBuiltin.cpp
===================================================================
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -2075,21 +2075,19 @@
// Library functions with special handling.
case Builtin::BIsqrt:
case Builtin::BIsqrtf:
- case Builtin::BIsqrtl: {
- // Transform a call to sqrt* into a @llvm.sqrt.* intrinsic call, but only
- // in finite- or unsafe-math mode (the intrinsic has different semantics
- // for handling negative numbers compared to the library function, so
- // -fmath-errno=0 is not enough).
- if (!FD->hasAttr<ConstAttr>())
- break;
- if (!(CGM.getCodeGenOpts().UnsafeFPMath ||
- CGM.getCodeGenOpts().NoNaNsFPMath))
- break;
- Value *Arg0 = EmitScalarExpr(E->getArg(0));
- llvm::Type *ArgType = Arg0->getType();
- Value *F = CGM.getIntrinsic(Intrinsic::sqrt, ArgType);
- return RValue::get(Builder.CreateCall(F, Arg0));
- }
+ case Builtin::BIsqrtl:
+ // Builtins have the same semantics as library functions. The LLVM intrinsic
+ // has the same semantics as the library function except it does not set
+ // errno. Thus, we can transform either sqrt or __builtin_sqrt to @llvm.sqrt
+ // if the call is 'const' (the call must not set errno).
+ //
+ // FIXME: The builtin cases are not here because they are marked 'const' in
+ // Builtins.def. So that means they are wrongly defined to have different
+ // semantics than the library functions. If we included them here, we would
+ // turn them into LLVM intrinsics regardless of whether -fmath-errno was on.
+ if (FD->hasAttr<ConstAttr>())
+ return RValue::get(emitUnaryBuiltin(*this, E, Intrinsic::sqrt));
+ break;
case Builtin::BI__builtin_pow:
case Builtin::BI__builtin_powf:
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
