[PATCH] D41286: [InstCombine] Missed optimization in math expression: sin(x) / cos(x) => tan(x)

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1472
 
+  // sin(a) / cos(a) -> tan(a)
+  if (AllowReassociate) {
----------------
spatel wrote:
> davide wrote:
> > spatel wrote:
> > > Should we also handle:
> > >   cos(a) / sin(a) -> 1 / tan(a)
> > > ?
> > Please wait for @scanon opinion before implementing every possible 10th grade trigonometrical identity.
> Do you have some specific numerical concern here? As you've noted, this is a well-known math transform.
> 
> We can make cos(a) / sin(a) a 'TODO' if you think we should use a different transform.
> https://stackoverflow.com/questions/3738384/stable-cotangent
> 
> 
The concern is that those transformation can overflow quite dramatically, even for `-ffast-math`.
The other concern is that I'm not a numerical expert, so I'd love to have this signed off from somebody who knows better than me. 
The last concern is, again, we shouldn't pattern match every possible thing just because, it slows down the compiler without real benefit, so, do you know how this pattern is frequent?


https://reviews.llvm.org/D41286