[SLC] Fix shrinking of pow()

[evandro]

Summary

Properly shrink pow() to powf() as a binary function and, when no other simplification applies, do not discard it.

[efriedma]

llvm/test/Transforms/InstCombine/double-float-shrink-1.ll

363

It looks like optimizeBinaryDoubleFP doesn't do all the same checks optimizeUnaryDoubleFP does? optimizeUnaryDoubleFP intentionally chooses not to optimize operations where the result isn't truncated because the extra bits might actually be useful.

[evandro]

llvm/test/Transforms/InstCombine/double-float-shrink-1.ll

363

Arguably, this should not shrink.

[evandro]

llvm/test/Transforms/InstCombine/double-float-shrink-1.ll

363

Besides here, optimizeBinaryDoubleFP() is also used for f{min,max}() and copysign(). Neither of the latter functions need the extra bits in the result.

[spatel]

llvm/test/Transforms/InstCombine/double-float-shrink-1.ll

363

An argument in favor of shrinking: it may reduce the number of instructions as shown in this test.

[evandro]

llvm/test/Transforms/InstCombine/double-float-shrink-1.ll

363

And the series in powf() is probably shorter and faster than in pow().

[spatel]

I think we should commit this based on fixing the miscompile alone; we can debate the other diff in a follow-on patch if needed.
But let's see if @efriedma agrees.

[evandro]

In D50113#1184429, @spatel wrote:

I think we should commit this based on fixing the miscompile alone; we can debate the other diff in a follow-on patch if needed.

The miscompile was never noticed because the shrinking was always discarded. It seems sensible to me that both are fixed in one swoop.

[evandro]

llvm/test/Transforms/InstCombine/double-float-shrink-1.ll

363

On the other hand, this shrink is only performed when -enable-double-float-shrink is specified or with FMF, which implies the attribute afn.

[efriedma]

I'd like to get this right... both the CheckRetType check, and the "infinite loop" check from optimizeUnaryDoubleFP are probably relevant. Can we share code between the two functions?

Maybe the CheckRetType check isn't critical if we're only doing this transform by default when afn is enabled, but it's still losing a lot of bits, particularly if the input is supposed to be an exact number. pow(2.f, 0.5f) is a lot different from (double)powf(2.f, 0.5f).

(If you want to fix the potential miscompile for 7.0, I'd accept a patch to just completely disable shrinking for pow.)

[evandro]

In D50113#1186263, @efriedma wrote:

I'd like to get this right... both the CheckRetType check, and the "infinite loop" check from optimizeUnaryDoubleFP are probably relevant. Can we share code between the two functions?

Maybe the CheckRetType check isn't critical if we're only doing this transform by default when afn is enabled, but it's still losing a lot of bits, particularly if the input is supposed to be an exact number. pow(2.f, 0.5f) is a lot different from (double)powf(2.f, 0.5f).

(If you want to fix the potential miscompile for 7.0, I'd accept a patch to just completely disable shrinking for pow.)

Just to make it clear, CheckRetType only matters for optimizeUnaryDoubleFP(); optimizeBinaryDoubleFP() doesn't have.

I agree with you, but wonder how this could ever work. It didn't, so probably nobody ever used it. So, why was it added? If it has no defensible purpose, then, by all means, it should be dropped.

[efriedma]

So, why was it added?

I would assume someone added it alongside a bunch of other operations where it matters more, and didn't write enough tests.

In general, the point of the transform is to catch code where someone accidentally writes "float z = pow(x, y);" instead of "float z = powf(x, y);" (And sometimes, it isn't practical to fix the code.)

[evandro]

Alternatively, optimizeBinaryDoubleFP() could be fixed to shrink just float f = pow(), while keeping its other users, f{min,max}() and copysign() happy, which should be easy to satisfy, given how simple these functions are.

[evandro]

Refactored the helper functions for shrinking unary and binary functions into a single one, while keeping all their functionality, in rL338905.

[evandro]

Keep the double precision pow() when the result is too.

[efriedma]

LGTM

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

Value *LibCallSimplifier::optimizePow(CallInst *Pow, IRBuilder<> &B) {
   Function *Callee = Pow->getCalledFunction();
   AttributeList Attrs = Callee->getAttributes();
   StringRef Name = Callee->getName();
   Module *Module = Pow->getModule();
   Type *Ty = Pow->getType();
   Value *Shrunk = nullptr;
   bool Ignored;
 
-  if (UnsafeFPShrink &&
-      Name == TLI->getName(LibFunc_pow) && hasFloatVersion(Name))
-    Shrunk = optimizeUnaryDoubleFP(Pow, B, true);
-
   // Propagate the math semantics from the call to any created instructions.
   IRBuilder<>::FastMathFlagGuard Guard(B);
   B.setFastMathFlags(Pow->getFastMathFlags());
 
+  // Shrink pow() to powf() if the arguments are single precision,
+  // unless the result is expected to be double precision.
+  if (UnsafeFPShrink &&
+      Name == TLI->getName(LibFunc_pow) && hasFloatVersion(Name))
+    Shrunk = optimizeBinaryDoubleFP(Pow, B, true);
+
   // Evaluate special cases related to the base.
 
   // pow(1.0, x) -> 1.0
   if (match(Base, m_FPOne()))
     return Base;
 
   // pow(2.0, x) -> exp2(x)
   if (match(Base, m_SpecificFP(2.0))) {
Value *LibCallSimplifier::optimizePow(CallInst *Pow, IRBuilder<> &B) {
   // pow(x, n) -> x * x * x * ....
   if (Pow->isFast()) {
     APFloat ExpoA = abs(ExpoC->getValueAPF());
     // We limit to a max of 7 fmul(s). Thus the maximum exponent is 32.
     // This transformation applies to integer exponents only.
     if (!ExpoA.isInteger() ||
         ExpoA.compare
             (APFloat(ExpoA.getSemantics(), 32.0)) == APFloat::cmpGreaterThan)
-      return nullptr;
+      return Shrunk;
 
     // We will memoize intermediate products of the Addition Chain.
     Value *InnerChain[33] = {nullptr};
     InnerChain[1] = Base;
     InnerChain[2] = B.CreateFMul(Base, Base, "square");
 
     // We cannot readily convert a non-double type (like float) to a double.
     // So we first convert it to something which could be converted to double.
     ExpoA.convert(APFloat::IEEEdouble(), APFloat::rmTowardZero, &Ignored);
     Value *FMul = getPow(InnerChain, ExpoA.convertToDouble(), B);
 
     // If the exponent is negative, then get the reciprocal.
     if (ExpoC->isNegative())
       FMul = B.CreateFDiv(ConstantFP::get(Ty, 1.0), FMul, "reciprocal");
     return FMul;
   }
 
-  return nullptr;
+  return Shrunk;
 }
 
 Value *LibCallSimplifier::optimizeExp2(CallInst *CI, IRBuilder<> &B) {
   Function *Callee = CI->getCalledFunction();
   Value *Ret = nullptr;
   StringRef Name = Callee->getName();
   if (UnsafeFPShrink && Name == "exp2" && hasFloatVersion(Name))
     Ret = optimizeUnaryDoubleFP(CI, B, true);

llvm/test/Transforms/InstCombine/double-float-shrink-1.ll

 ; CHECK-NEXT:    [[CALL:%.*]] = call fast double @logb(double [[CONV]])
 ; CHECK-NEXT:    ret double [[CALL]]
 ;
   %conv = fpext float %f to double
   %call = call fast double @logb(double %conv)
   ret double %call
 }
 
-; FIXME: Miscompile - we dropped the 2nd argument!
-
 define float @pow_test1(float %f, float %g)   {
 ; CHECK-LABEL: @pow_test1(
-; CHECK-NEXT:    [[POWF:%.*]] = call fast float @powf(float [[F:%.*]])
+; CHECK-NEXT:    [[POWF:%.*]] = call fast float @powf(float %f, float %g)
 ; CHECK-NEXT:    ret float [[POWF]]
 ;
   %df = fpext float %f to double
   %dg = fpext float %g to double
   %call = call fast double @pow(double %df, double %dg)
   %fr = fptrunc double %call to float
   ret float %fr
 }
 
-; TODO: This should shrink?
-
 define double @pow_test2(float %f, float %g) {
 ; CHECK-LABEL: @pow_test2(
-; CHECK-NEXT:    [[DF:%.*]] = fpext float [[F:%.*]] to double
-; CHECK-NEXT:    [[DG:%.*]] = fpext float [[G:%.*]] to double
-; CHECK-NEXT:    [[CALL:%.*]] = call fast double @pow(double [[DF]], double [[DG]])
-; CHECK-NEXT:    ret double [[CALL]]
+; CHECK:         [[POW:%.*]] = call fast double @pow(double %df, double %dg)
+; CHECK-NEXT:    ret double [[POW]]
 ;
   %df = fpext float %f to double
   %dg = fpext float %g to double
   %call = call fast double @pow(double %df, double %dg)
   ret double %call
 }
 
 define float @sin_test1(float %f)   {
 ; CHECK-LABEL: @sin_test1(