Page MenuHomePhabricator

[InstCombine] Missed optimization in math expression: tan(a) * cos(a) == sin(a)
Needs ReviewPublic

Authored by Quolyk on Dec 15 2017, 4:55 AM.

Details

Reviewers
hfinkel
spatel
Summary

This patch enables folding tan(x) * cos(x) -> sin(x) under -ffast-math flag

Diff Detail

Event Timeline

Quolyk created this revision.Dec 15 2017, 4:55 AM
Quolyk retitled this revision from [InstCombine] Missed optimization in math expression: tan(a) * cos(a) == sin(a) to [WIP][InstCombine] Missed optimization in math expression: tan(a) * cos(a) == sin(a).
Quolyk added a reviewer: davide.
hfinkel added inline comments.Dec 19 2017, 8:06 PM
lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
728

Please see my comments in D41389 about names.

731

Needs

BuilderTy::FastMathFlagGuard Guard(Builder);

above this.

Quolyk updated this revision to Diff 127830.Dec 20 2017, 11:02 PM
Quolyk marked 2 inline comments as done.
Quolyk updated this revision to Diff 128602.Jan 4 2018, 1:13 AM
Quolyk edited the summary of this revision. (Show Details)

As in D41389 I have tan calls that are never used and never erased.

Quolyk retitled this revision from [WIP][InstCombine] Missed optimization in math expression: tan(a) * cos(a) == sin(a) to [InstCombine] Missed optimization in math expression: tan(a) * cos(a) == sin(a).Jan 4 2018, 1:14 AM
Quolyk updated this revision to Diff 129401.Jan 11 2018, 12:36 AM

This needs to be merged first, because it introduces m_LibFunc match, that would be useful for other optimizations.

Why doesn't tan have an LLVM intrinsic like sin/cos?

Why doesn't tan have an LLVM intrinsic like sin/cos?

I don't know

davide removed a reviewer: davide.Jan 12 2018, 9:57 AM

Why doesn't tan have an LLVM intrinsic like sin/cos?

I don't know

Then we should find out. :)
I should've asked before D41286, but oops.
cc @efriedma in case he knows the history. If not, you should ask on llvm-dev. Maybe it's just not as common to see tan in code, but if we're going to do more trig transforms, I think it would make sense to have uniformity of trig intrinsics, so it's easier to do these folds. Also, if the vectorizers recognize trig functions, it would make it easier for them if we have more intrinsics for these functions?

Why doesn't tan have an LLVM intrinsic like sin/cos?

I don't know

Then we should find out. :)
I should've asked before D41286, but oops.
cc @efriedma in case he knows the history. If not, you should ask on llvm-dev. Maybe it's just not as common to see tan in code, but if we're going to do more trig transforms, I think it would make sense to have uniformity of trig intrinsics, so it's easier to do these folds. Also, if the vectorizers recognize trig functions, it would make it easier for them if we have more intrinsics for these functions?

I agree. In the past, AFAIK, we've added the intrinsics on an as-needed basis.

Quolyk added a comment.EditedJan 12 2018, 10:54 AM

I'll try introduce llvm.tan intrinsic then in different patch