[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

4599

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

[Quolyk]

lib/Analysis/InstructionSimplify.cpp

4599

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

4599

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

4599

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

4599

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

4599

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

4599

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

for (unsigned I = 0, E = ConstMask->getType()->getVectorNumElements(); I != E;
     if (auto *MaskElt = ConstMask->getAggregateElement(I))
       if (MaskElt->isNullValue() || isa<UndefValue>(MaskElt))
         continue;
     return false;
   }
   return true;
 }
 
 template <typename IterTy>
 static Value *SimplifyIntrinsic(Function *F, IterTy ArgBegin, IterTy ArgEnd,
                                 const SimplifyQuery &Q, unsigned MaxRecurse) {
   Intrinsic::ID IID = F->getIntrinsicID();
   unsigned NumOperands = std::distance(ArgBegin, ArgEnd);
 
   // Unary Ops
   if (NumOperands == 1) {
     // Perform idempotent optimizations
case Intrinsic::log: {
       if (Q.CxtI->hasAllowReassoc() &&
           match(IIOperand, m_Intrinsic<Intrinsic::exp>(m_Value(X))))
         return X;
       return nullptr;
     }
     case Intrinsic::log2: {
       // log2(exp2(x)) -> x
       if (Q.CxtI->hasAllowReassoc() &&
-          match(IIOperand, m_Intrinsic<Intrinsic::exp2>(m_Value(X)))) {
+          (match(IIOperand, m_Intrinsic<Intrinsic::exp2>(m_Value(X))) ||
+           match(IIOperand, m_Intrinsic<Intrinsic::pow>(m_SpecificFP(2.0),
+                                                        m_Value(X)))))
         return X;
-      }
+      return nullptr;
+    case Intrinsic::log10: {
+      // log10(pow(10.0, x)) -> x
+      if (Q.CxtI->hasAllowReassoc() &&
+          match(IIOperand, m_Intrinsic<Intrinsic::pow>(m_SpecificFP(10.0),
+                                                       m_Value(X))))
+        return X;
       return nullptr;
     }
     default:
       return nullptr;
     }
   }
 
   // Binary Ops

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

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -instsimplify < %s | FileCheck %s
+
+declare double @llvm.log10.f64(double)
+declare double @llvm.pow.f64(double, double)
+
+define double @log10_pow10(double %x) {
+; CHECK-LABEL: @log10_pow10(
+; CHECK-NEXT:    [[TMP1:%.*]] = call double @llvm.pow.f64(double 1.000000e+01, double [[X:%.*]])
+; CHECK-NEXT:    [[TMP2:%.*]] = call double @llvm.log10.f64(double [[TMP1]])
+; CHECK-NEXT:    ret double [[TMP2]]
+;
+  %1 = call double @llvm.pow.f64(double 10.000000e+00, double %x)
+  %2 = call double @llvm.log10.f64(double %1)
+  ret double %2
+}
+
+define double @log10_strict_pow10_reassoc(double %x) {
+; CHECK-LABEL: @log10_strict_pow10_reassoc(
+; CHECK-NEXT:    [[TMP1:%.*]] = call reassoc double @llvm.pow.f64(double 1.000000e+01, double [[X:%.*]])
+; CHECK-NEXT:    [[TMP2:%.*]] = call double @llvm.log10.f64(double [[TMP1]])
+; CHECK-NEXT:    ret double [[TMP2]]
+;
+  %1 = call reassoc double @llvm.pow.f64(double 10.000000e+00, double %x)
+  %2 = call double @llvm.log10.f64(double %1)
+  ret double %2
+}
+
+define double @log10_reassoc_pow10_strict(double %x) {
+; CHECK-LABEL: @log10_reassoc_pow10_strict(
+; CHECK-NEXT:    ret double [[X:%.*]]
+;
+  %1 = call double @llvm.pow.f64(double 10.000000e+00, double %x)
+  %2 = call reassoc double @llvm.log10.f64(double %1)
+  ret double %2
+}
+
+define double @log10_pow10_reassoc(double %x) {
+; CHECK-LABEL: @log10_pow10_reassoc(
+; CHECK-NEXT:    ret double [[X:%.*]]
+;
+  %1 = call reassoc double @llvm.pow.f64(double 10.000000e+00, double %x)
+  %2 = call reassoc double @llvm.log10.f64(double %1)
+  ret double %2
+}

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

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -instsimplify < %s | FileCheck %s
+
+declare double @llvm.log2.f64(double)
+declare double @llvm.pow.f64(double, double)
+
+define double @log2_pow2(double %x) {
+; CHECK-LABEL: @log2_pow2(
+; CHECK-NEXT:    [[TMP1:%.*]] = call double @llvm.pow.f64(double 2.000000e+00, double [[X:%.*]])
+; CHECK-NEXT:    [[TMP2:%.*]] = call double @llvm.log2.f64(double [[TMP1]])
+; CHECK-NEXT:    ret double [[TMP2]]
+;
+  %1 = call double @llvm.pow.f64(double 2.000000e+00, double %x)
+  %2 = call double @llvm.log2.f64(double %1)
+  ret double %2
+}
+
+define double @log2_strict_pow2_reassoc(double %x) {
+; CHECK-LABEL: @log2_strict_pow2_reassoc(
+; CHECK-NEXT:    [[TMP1:%.*]] = call reassoc double @llvm.pow.f64(double 2.000000e+00, double [[X:%.*]])
+; CHECK-NEXT:    [[TMP2:%.*]] = call double @llvm.log2.f64(double [[TMP1]])
+; CHECK-NEXT:    ret double [[TMP2]]
+;
+  %1 = call reassoc double @llvm.pow.f64(double 2.000000e+00, double %x)
+  %2 = call double @llvm.log2.f64(double %1)
+  ret double %2
+}
+
+define double @log2_reassoc_pow2_strict(double %x) {
+; CHECK-LABEL: @log2_reassoc_pow2_strict(
+; CHECK-NEXT:    ret double [[X:%.*]]
+;
+  %1 = call double @llvm.pow.f64(double 2.000000e+00, double %x)
+  %2 = call reassoc double @llvm.log2.f64(double %1)
+  ret double %2
+}
+
+define double @log2_pow2_reassoc(double %x) {
+; CHECK-LABEL: @log2_pow2_reassoc(
+; CHECK-NEXT:    ret double [[X:%.*]]
+;
+  %1 = call reassoc double @llvm.pow.f64(double 2.000000e+00, double %x)
+  %2 = call reassoc double @llvm.log2.f64(double %1)
+  ret double %2
+}