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

Wed Jan 30 05:52:41 PST 2019

Quolyk marked an inline comment as done.
Quolyk added inline comments.

================
Comment at: lib/Analysis/InstructionSimplify.cpp:4944
+        (match(Op0, m_Intrinsic<Intrinsic::exp2>(m_Value(X))) ||
+         match(Op0, m_Intrinsic<Intrinsic::pow>(m_SpecificFP(2.0),
+                                                m_Value(X))))) return X;
----------------
spatel wrote:
> lebedev.ri wrote:
> > spatel wrote:
> > > Quolyk wrote:
> > > > lebedev.ri wrote:
> > > > > Hmm, do we want to canonicalize `Intrinsic::pow(2.0, x)` to `exp2(x)` instead ?
> > > > 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.
> > > 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.
> > 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?
> > 
> 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.
I suppose this transform needs to be done in a separate patch. Do I need to commit tests before this patch as in D41342?

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D41940/new/

https://reviews.llvm.org/D41940