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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1472
 
+  // sin(a) / cos(a) -> tan(a)
+  if (AllowReassociate) {
----------------
davide wrote:
> 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?
The compile-time concern is misguided. 

This pattern, like every other "10th grade trigonometrical identity", should be optimized by an optimizing compiler because that's the job of an optimizing compiler.

These patterns can occur by way of templated code, inlining, or because the programmer may not be a computer performance expert. Think: scientists who are trying to model/simulate some math problem, but don't know much about perf...because again: that's the optimizing compiler's job.

If this patch or pass is causing a compile-time problem for you, please point to or file a bug. Obstructing patches like this is doubly bad when you're undermining the efforts of new contributors.



https://reviews.llvm.org/D41286