This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Quolyk on Jan 11 2018, 2:07 AM.

Diff Detail

Event Timeline

Quolyk created this revision.Jan 11 2018, 2:07 AM
Quolyk updated this revision to Diff 135632.Feb 23 2018, 6:08 AM
lebedev.ri retitled this revision from [InstCombine] Missed optimization in math expression: log10(pow(10.0,x)) == x, log2(pow(2.0,x)) == x to [InstSimplify] Missed optimization in math expression: log10(pow(10.0,x)) == x, log2(pow(2.0,x)) == x.Mar 17 2018, 2:34 AM
lebedev.ri added reviewers: craig.topper, zvi, majnemer.
lebedev.ri set the repository for this revision to rL LLVM.
Quolyk updated this revision to Diff 155850.Jul 17 2018, 4:32 AM

Seems ok.

lib/Analysis/InstructionSimplify.cpp
4533

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

Quolyk marked an inline comment as done.Jan 29 2019, 5:42 AM
Quolyk added inline comments.
lib/Analysis/InstructionSimplify.cpp
4533

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 added inline comments.Jan 29 2019, 12:10 PM
lib/Analysis/InstructionSimplify.cpp
4533

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 added inline comments.Jan 29 2019, 12:24 PM
lib/Analysis/InstructionSimplify.cpp
4533

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 added inline comments.Jan 29 2019, 12:41 PM
lib/Analysis/InstructionSimplify.cpp
4533

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 marked an inline comment as done.Jan 30 2019, 5:51 AM
Quolyk added inline comments.
lib/Analysis/InstructionSimplify.cpp
4533

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 added inline comments.Jan 30 2019, 5:56 AM
lib/Analysis/InstructionSimplify.cpp
4533

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 updated this revision to Diff 184462.Jan 30 2019, 11:51 PM

Update tests.

lebedev.ri accepted this revision.Jan 31 2019, 12:17 AM

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)

This revision is now accepted and ready to land.Jan 31 2019, 12:17 AM

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 accepted this revision.Jan 31 2019, 6:09 AM

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

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2019, 7:48 PM