[InstSimplify] Missed optimization in math expression: log10(pow(10.0,x)) == x, log2(pow(2.0,x)) == x

[Quolyk]

Summary

This patch enables folding following instructions under -ffast-math flag: log10(pow(10.0,x)) -> x, log2(pow(2.0,x)) -> x

[Quolyk]

Ping

[Quolyk]

Ping.

[lebedev.ri]

Seems ok.

lib/Analysis/InstructionSimplify.cpp

4944

Hmm, do we want to canonicalize Intrinsic::pow(2.0, x) to exp2(x) instead ?

[Quolyk]

lib/Analysis/InstructionSimplify.cpp

4944

If Intrinsic::pow(2.0, x) to exp2(x) transform is useful somewhere else or you have strong opinion about it we do. Otherwise I don't see the reason for this.

[spatel]

lib/Analysis/InstructionSimplify.cpp

4944

It's a specialized form of the more general math function, so we can assume it has a faster implementation. I think we should add that fold and reduce this patch.

[lebedev.ri]

lib/Analysis/InstructionSimplify.cpp

4944

There is a counter-argument to that: (that i have thought of after asking, but did not post)
do we *know* that instcombine would have always been run after such an Intrinsic call was added,
and before this simplifyUnaryIntrinsic() was called?

I.e. is there some situation where we might end up with simplifyUnaryIntrinsic() being called
*before* instcombine had a chance to do that canonicalization?
If yes, do we care?

[spatel]

lib/Analysis/InstructionSimplify.cpp

4944

Instsimplify may be used independently of instcombine by other passes, so yes, it is possible that the non-canonical form will be seen here. It's not generally a concern with the typical optimization pipeline since we run instcombine...a lot.

So the full solution is: leave this patch as-is (handle both forms) and add the instcombine too.

[Quolyk]

lib/Analysis/InstructionSimplify.cpp

4944

I suppose this transform needs to be done in a separate patch. Do I need to commit tests before this patch as in D41342?

[lebedev.ri]

lib/Analysis/InstructionSimplify.cpp

4944

I suppose this transform needs to be done in a separate patch.

Yep, it's a good idea to keep patches small.

Do I need to commit tests before this patch as in D41342?

It really helps with review.

[Quolyk]

Update tests.

[lebedev.ri]

LG unless others have more comments (@spatel ?)
You probably know this already, but there are some other low-hanging fruit here, e.g.
https://godbolt.org/z/Trdf_e
https://www.wolframalpha.com/input/?i=log2(x)+%2F+log2(10)+%3D+log10(x)

[Quolyk]

In D41940#1378402, @lebedev.ri wrote:

LG unless others have more comments (@spatel ?)
You probably know this already, but there are some other low-hanging fruit here, e.g.
https://godbolt.org/z/Trdf_e
https://www.wolframalpha.com/input/?i=log2(x)+%2F+log2(10)+%3D+log10(x)

Thanks, I'll dive into it. Btw, pow(2.0, x) --> exp2(x) is already implemented, see https://godbolt.org/z/UADcWe.

[spatel]

In D41940#1378426, @Quolyk wrote:

In D41940#1378402, @lebedev.ri wrote:

LG unless others have more comments (@spatel ?)
You probably know this already, but there are some other low-hanging fruit here, e.g.
https://godbolt.org/z/Trdf_e
https://www.wolframalpha.com/input/?i=log2(x)+%2F+log2(10)+%3D+log10(x)

Thanks, I'll dive into it. Btw, pow(2.0, x) --> exp2(x) is already implemented, see https://godbolt.org/z/UADcWe.

If you're interested in math call transforms, you might also want to look at:
https://bugs.llvm.org/show_bug.cgi?id=40541

One more comment: 'reassoc' is generally enough to go crazy with FP, but we might also want to require 'nsz' on this and related transforms to be safer because:
x = -0.0
exp2(x) = 1.0
log2(exp2(x)) = 0.0 <-- output flipped sign from input

lib/Analysis/InstructionSimplify.cpp

static Value *simplifyUnaryIntrinsic(Function *F, Value *Op0,
   case Intrinsic::log:
     // log(exp(x)) -> x
     if (Q.CxtI->hasAllowReassoc() &&
         match(Op0, m_Intrinsic<Intrinsic::exp>(m_Value(X)))) return X;
     break;
   case Intrinsic::log2:
     // log2(exp2(x)) -> x
     if (Q.CxtI->hasAllowReassoc() &&
-        match(Op0, m_Intrinsic<Intrinsic::exp2>(m_Value(X)))) return X;
+        (match(Op0, m_Intrinsic<Intrinsic::exp2>(m_Value(X))) ||
+         match(Op0, m_Intrinsic<Intrinsic::pow>(m_SpecificFP(2.0),
+                                                m_Value(X))))) return X;
+    break;
+  case Intrinsic::log10:
+    // log10(pow(10.0, x)) -> x
+    if (Q.CxtI->hasAllowReassoc() &&
+        match(Op0, m_Intrinsic<Intrinsic::pow>(m_SpecificFP(10.0),
+                                               m_Value(X)))) return X;
     break;
   default:
     break;
   }
 
   return nullptr;
 }
 

test/Transforms/InstSimplify/log10-pow10-intrinsic.ll

 ;
   %tmp = call reassoc double @llvm.pow.f64(double 1.000000e+01, double %x)
   %tmp1 = call double @llvm.log10.f64(double %tmp)
   ret double %tmp1
 }
 
 define double @log10_reassoc_pow10_strict(double %x) {
 ; CHECK-LABEL: @log10_reassoc_pow10_strict(
-; CHECK-NEXT:    [[TMP1:%.*]] = call double @llvm.pow.f64(double 1.000000e+01, double [[X:%.*]])
-; CHECK-NEXT:    [[TMP2:%.*]] = call reassoc double @llvm.log10.f64(double [[TMP1]])
-; CHECK-NEXT:    ret double [[TMP2]]
+; CHECK-NEXT:    ret double [[X:%.*]]
 ;
   %tmp = call double @llvm.pow.f64(double 1.000000e+01, double %x)
   %tmp1 = call reassoc double @llvm.log10.f64(double %tmp)
   ret double %tmp1
 }
 
 define double @log10_pow10_reassoc(double %x) {
 ; CHECK-LABEL: @log10_pow10_reassoc(
-; CHECK-NEXT:    [[TMP1:%.*]] = call reassoc double @llvm.pow.f64(double 1.000000e+01, double [[X:%.*]])
-; CHECK-NEXT:    [[TMP2:%.*]] = call reassoc double @llvm.log10.f64(double [[TMP1]])
-; CHECK-NEXT:    ret double [[TMP2]]
+; CHECK-NEXT:    ret double [[X:%.*]]
 ;
   %tmp = call reassoc double @llvm.pow.f64(double 1.000000e+01, double %x)
   %tmp1 = call reassoc double @llvm.log10.f64(double %tmp)
   ret double %tmp1
 }

test/Transforms/InstSimplify/log2-pow2-intrinsic.ll

 ;
   %tmp = call reassoc double @llvm.pow.f64(double 2.000000e+00, double %x)
   %tmp1 = call double @llvm.log2.f64(double %tmp)
   ret double %tmp1
 }
 
 define double @log2_reassoc_pow2_strict(double %x) {
 ; CHECK-LABEL: @log2_reassoc_pow2_strict(
-; CHECK-NEXT:    [[TMP:%.*]] = call double @llvm.pow.f64(double 2.000000e+00, double [[X:%.*]])
-; CHECK-NEXT:    [[TMP1:%.*]] = call reassoc double @llvm.log2.f64(double [[TMP]])
-; CHECK-NEXT:    ret double [[TMP1]]
+; CHECK-NEXT:    ret double [[X:%.*]]
 ;
   %tmp = call double @llvm.pow.f64(double 2.000000e+00, double %x)
   %tmp1 = call reassoc double @llvm.log2.f64(double %tmp)
   ret double %tmp1
 }
 
 define double @log2_pow2_reassoc(double %x) {
 ; CHECK-LABEL: @log2_pow2_reassoc(
-; CHECK-NEXT:    [[TMP:%.*]] = call reassoc double @llvm.pow.f64(double 2.000000e+00, double [[X:%.*]])
-; CHECK-NEXT:    [[TMP1:%.*]] = call reassoc double @llvm.log2.f64(double [[TMP]])
-; CHECK-NEXT:    ret double [[TMP1]]
+; CHECK-NEXT:    ret double [[X:%.*]]
 ;
   %tmp = call reassoc double @llvm.pow.f64(double 2.000000e+00, double %x)
   %tmp1 = call reassoc double @llvm.log2.f64(double %tmp)
   ret double %tmp1
 }