[LibCallSimplifier] fix pow(x, 0.5) -> sqrt() transforms

[spatel]

Summary

This would be a lot simpler if we could disregard errno when we have relaxed FP math, but I'm assuming it's possible to do something like '-ffast-math -fmath-errno' and still expect errno to be set for domain errors?

As the first test shows, we could transform an llvm intrinsic into a libcall which could then set errno, so that's just wrong...unless I'm not seeing some loophole. The second test diff is a phantom change (same output as before), but I've updated the CHECK lines to highlight that we get that case right on purpose now.

[efriedma]

This would be a lot simpler if we could disregard errno when we have relaxed FP math, but I'm assuming it's possible to do something like '-ffast-math -fmath-errno' and still expect errno to be set for domain errors?

nnan implies we don't care about the result in cases where it would be nan, so it's probably okay if we lower an nnan "pow(x, 0.5)" to something which never sets errno. But there's still the underlying problem that the backend can't lower the intrinsics correctly for targets where libm sets errno, so we shouldn't generate libm intrinsics for code which isn't already using them.

lib/Transforms/Utils/SimplifyLibCalls.cpp

1080 ↗

(On Diff #123248)

Probably something like ninf nsz? But let's do that in a followup.

[hfinkel]

In D40150#928114, @efriedma wrote:

This would be a lot simpler if we could disregard errno when we have relaxed FP math, but I'm assuming it's possible to do something like '-ffast-math -fmath-errno' and still expect errno to be set for domain errors?

nnan implies we don't care about the result in cases where it would be nan, so it's probably okay if we lower an nnan "pow(x, 0.5)" to something which never sets errno. But there's still the underlying problem that the backend can't lower the intrinsics correctly for targets where libm sets errno, so we shouldn't generate libm intrinsics for code which isn't already using them.

We should, at some point soon, come up with a real plan to handle this problem. On systems that don't have appropriate functions, we might generate wrappers that save/restore errno and call those wrappers instead of the libm functions directly. For -gnu systems, we can/should generate calls to pow_finite instead of pow and similar for other functions (these are in /usr/include/bits/math-finite.h and are what you generally get from math.h when FINITE_MATH_ONLY__ is positive).

[spatel]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1080 ↗

(On Diff #123248)

That might be it. As you can tell from the other tests that I added, I initially set out to make this transform a potential test of the meaning of the new 'afn' FMF...but then I got stuck in another batch of errno quicksand. :)

[spatel]

In D40150#928146, @hfinkel wrote:

In D40150#928114, @efriedma wrote:

This would be a lot simpler if we could disregard errno when we have relaxed FP math, but I'm assuming it's possible to do something like '-ffast-math -fmath-errno' and still expect errno to be set for domain errors?

nnan implies we don't care about the result in cases where it would be nan, so it's probably okay if we lower an nnan "pow(x, 0.5)" to something which never sets errno. But there's still the underlying problem that the backend can't lower the intrinsics correctly for targets where libm sets errno, so we shouldn't generate libm intrinsics for code which isn't already using them.

We should, at some point soon, come up with a real plan to handle this problem. On systems that don't have appropriate functions, we might generate wrappers that save/restore errno and call those wrappers instead of the libm functions directly. For -gnu systems, we can/should generate calls to pow_finite instead of pow and similar for other functions (these are in /usr/include/bits/math-finite.h and are what you generally get from math.h when FINITE_MATH_ONLY__ is positive).

Let me check if I have this right for this example: We can't safely make progress here currently - we must at least add a TLI hook like "bool canLowerCallWithoutSettingErrno(CallInst *)". Without that, we have this potential:

1. The incoming call is llvm.pow() - can't set errno.
2. We transform it to llvm.sqrt() here in IR.
3. The backend lowers it to a libcall sqrt() which can set errno.

Given that, should we:

1. Go ahead with this patch as-is? We're at least avoiding the case where we knowingly turn intrinsic llvm.pow() into libcall sqrt() in IR.
2. Simplify this patch to transform both pow() and llvm.pow() into llvm.sqrt() given the protective cover of isFast()?
3. Postpone this patch until we have the TLI hook and backend support to do the right thing?

[efriedma]

I think this is okay for now; we can clean it up when the backend is fixed.

llvm/trunk/include/llvm/Transforms/Utils/SimplifyLibCalls.h

private:
   Value *optimizeMemSet(CallInst *CI, IRBuilder<> &B);
   Value *optimizeWcslen(CallInst *CI, IRBuilder<> &B);
   // Wrapper for all String/Memory Library Call Optimizations
   Value *optimizeStringMemoryLibCall(CallInst *CI, IRBuilder<> &B);
 
   // Math Library Optimizations
   Value *optimizeCos(CallInst *CI, IRBuilder<> &B);
   Value *optimizePow(CallInst *CI, IRBuilder<> &B);
+  Value *replacePowWithSqrt(CallInst *Pow, IRBuilder<> &B);
   Value *optimizeExp2(CallInst *CI, IRBuilder<> &B);
   Value *optimizeFMinFMax(CallInst *CI, IRBuilder<> &B);
   Value *optimizeLog(CallInst *CI, IRBuilder<> &B);
   Value *optimizeSqrt(CallInst *CI, IRBuilder<> &B);
   Value *optimizeSinCosPi(CallInst *CI, IRBuilder<> &B);
   Value *optimizeTan(CallInst *CI, IRBuilder<> &B);
   // Wrapper for all floating point library call optimizations
   Value *optimizeFloatingPointLibCall(CallInst *CI, LibFunc Func,

llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp

static const unsigned AddChain[33][2] = {
       {3, 24}, {14, 14}, {4, 25}, {15, 15}, {3, 28}, {16, 16},
   };
 
   InnerChain[Exp] = B.CreateFMul(getPow(InnerChain, AddChain[Exp][0], B),
                                  getPow(InnerChain, AddChain[Exp][1], B));
   return InnerChain[Exp];
 }
 
+/// Use square root in place of pow(x, +/-0.5).
+Value *LibCallSimplifier::replacePowWithSqrt(CallInst *Pow, IRBuilder<> &B) {
+  // TODO: There is some subset of 'fast' under which these transforms should
+  // be allowed.
+  if (!Pow->isFast())
+    return nullptr;
+
+  // TODO: This should use m_APFloat to allow vector splats.
+  ConstantFP *Op2C = dyn_cast<ConstantFP>(Pow->getArgOperand(1));
+  if (!Op2C)
+    return nullptr;
+  if (!Op2C->isExactlyValue(0.5) && !Op2C->isExactlyValue(-0.5))
+    return nullptr;
+
+  // Fast-math flags from the pow() are propagated to all replacement ops.
+  IRBuilder<>::FastMathFlagGuard Guard(B);
+  B.setFastMathFlags(Pow->getFastMathFlags());
+  Type *Ty = Pow->getType();
+  Value *Sqrt;
+  if (Pow->hasFnAttr(Attribute::ReadNone)) {
+    // We know that errno is never set, so replace with an intrinsic:
+    // pow(x, 0.5) --> llvm.sqrt(x)
+    // llvm.pow(x, 0.5) --> llvm.sqrt(x)
+    auto *F = Intrinsic::getDeclaration(Pow->getModule(), Intrinsic::sqrt, Ty);
+    Sqrt = B.CreateCall(F, Pow->getArgOperand(0));
+  } else if (hasUnaryFloatFn(TLI, Ty, LibFunc_sqrt, LibFunc_sqrtf,
+                             LibFunc_sqrtl)) {
+    // Errno could be set, so we must use a sqrt libcall.
+    // TODO: We also should check that the target can in fact lower the sqrt
+    // libcall. We currently have no way to ask this question, so we ask
+    // whether the target has a sqrt libcall which is not exactly the same.
+    Sqrt = emitUnaryFloatFnCall(Pow->getArgOperand(0),
+                                TLI->getName(LibFunc_sqrt), B,
+                                Pow->getCalledFunction()->getAttributes());
+  } else {
+    // We can't replace with an intrinsic or a libcall.
+    return nullptr;
+  }
+
+  // If this is pow(x, -0.5), get the reciprocal.
+  if (Op2C->isExactlyValue(-0.5))
+    Sqrt = B.CreateFDiv(ConstantFP::get(Ty, 1.0), Sqrt);
+
+  return Sqrt;
+}
+
 Value *LibCallSimplifier::optimizePow(CallInst *CI, IRBuilder<> &B) {
   Function *Callee = CI->getCalledFunction();
   Value *Ret = nullptr;
   StringRef Name = Callee->getName();
   if (UnsafeFPShrink && Name == "pow" && hasFloatVersion(Name))
     Ret = optimizeUnaryDoubleFP(CI, B, true);
 
   Value *Op1 = CI->getArgOperand(0), *Op2 = CI->getArgOperand(1);
Value *LibCallSimplifier::optimizePow(CallInst *CI, IRBuilder<> &B) {
 
   ConstantFP *Op2C = dyn_cast<ConstantFP>(Op2);
   if (!Op2C)
     return Ret;
 
   if (Op2C->getValueAPF().isZero()) // pow(x, 0.0) -> 1.0
     return ConstantFP::get(CI->getType(), 1.0);
 
-  if (Op2C->isExactlyValue(-0.5) &&
-      hasUnaryFloatFn(TLI, Op2->getType(), LibFunc_sqrt, LibFunc_sqrtf,
-                      LibFunc_sqrtl)) {
-    // If -ffast-math:
-    // pow(x, -0.5) -> 1.0 / sqrt(x)
-    if (CI->isFast()) {
-      IRBuilder<>::FastMathFlagGuard Guard(B);
-      B.setFastMathFlags(CI->getFastMathFlags());
-
-      // TODO: If the pow call is an intrinsic, we should lower to the sqrt
-      // intrinsic, so we match errno semantics.  We also should check that the
-      // target can in fact lower the sqrt intrinsic -- we currently have no way
-      // to ask this question other than asking whether the target has a sqrt
-      // libcall, which is a sufficient but not necessary condition.
-      Value *Sqrt = emitUnaryFloatFnCall(Op1, TLI->getName(LibFunc_sqrt), B,
-                                         Callee->getAttributes());
-
-      return B.CreateFDiv(ConstantFP::get(CI->getType(), 1.0), Sqrt, "sqrtrecip");
-    }
-  }
+  if (Value *Sqrt = replacePowWithSqrt(CI, B))
+    return Sqrt;
 
+  // FIXME: Correct the transforms and pull this into replacePowWithSqrt().
   if (Op2C->isExactlyValue(0.5) &&
       hasUnaryFloatFn(TLI, Op2->getType(), LibFunc_sqrt, LibFunc_sqrtf,
                       LibFunc_sqrtl)) {
-
-    // In -ffast-math, pow(x, 0.5) -> sqrt(x).
-    if (CI->isFast()) {
-      IRBuilder<>::FastMathFlagGuard Guard(B);
-      B.setFastMathFlags(CI->getFastMathFlags());
-
-      // TODO: As above, we should lower to the sqrt intrinsic if the pow is an
-      // intrinsic, to match errno semantics.
-      return emitUnaryFloatFnCall(Op1, TLI->getName(LibFunc_sqrt), B,
-                                  Callee->getAttributes());
-    }
-
     // Expand pow(x, 0.5) to (x == -infinity ? +infinity : fabs(sqrt(x))).
     // This is faster than calling pow, and still handles negative zero
     // and negative infinity correctly.
     // TODO: In finite-only mode, this could be just fabs(sqrt(x)).
     Value *Inf = ConstantFP::getInfinity(CI->getType());
     Value *NegInf = ConstantFP::getInfinity(CI->getType(), true);
 
     // TODO: As above, we should lower to the sqrt intrinsic if the pow is an

llvm/trunk/test/Transforms/InstCombine/pow-sqrt.ll

 ; RUN: opt < %s -instcombine -S | FileCheck %s
 
 define double @pow_intrinsic_half_fast(double %x) {
 ; CHECK-LABEL: @pow_intrinsic_half_fast(
-; CHECK-NEXT:    [[SQRT:%.*]] = call fast double @sqrt(double %x) #1
-; CHECK-NEXT:    ret double [[SQRT]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call fast double @llvm.sqrt.f64(double %x)
+; CHECK-NEXT:    ret double [[TMP1]]
 ;
   %pow = call fast double @llvm.pow.f64(double %x, double 5.000000e-01)
   ret double %pow
 }
 
 define <2 x double> @pow_intrinsic_half_approx(<2 x double> %x) {
 ; CHECK-LABEL: @pow_intrinsic_half_approx(
 ; CHECK-NEXT:    [[POW:%.*]] = call afn <2 x double> @llvm.pow.v2f64(<2 x double> %x, <2 x double> <double 5.000000e-01, double 5.000000e-01>)

 ;
   %pow = call afn double @llvm.pow.f64(double %x, double -5.0e-01)
   ret double %pow
 }
 
 define float @pow_libcall_neghalf_fast(float %x) {
 ; CHECK-LABEL: @pow_libcall_neghalf_fast(
 ; CHECK-NEXT:    [[SQRTF:%.*]] = call fast float @sqrtf(float %x)
-; CHECK-NEXT:    [[SQRTRECIP:%.*]] = fdiv fast float 1.000000e+00, [[SQRTF]]
-; CHECK-NEXT:    ret float [[SQRTRECIP]]
+; CHECK-NEXT:    [[TMP1:%.*]] = fdiv fast float 1.000000e+00, [[SQRTF]]
+; CHECK-NEXT:    ret float [[TMP1]]
 ;
   %pow = call fast float @powf(float %x, float -5.0e-01)
   ret float %pow
 }
 
 declare double @llvm.pow.f64(double, double) #0
 declare <2 x double> @llvm.pow.v2f64(<2 x double>, <2 x double>) #0
 declare double @pow(double, double)
 declare float @powf(float, float)
 
 attributes #0 = { nounwind readnone speculatable }
 attributes #1 = { nounwind readnone }