[PATCH] D28236: InstCombine: Fold cos(-x) -> cos(x)

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In https://reviews.llvm.org/D28236#634597, @davide wrote:

> In https://reviews.llvm.org/D28236#634466, @arsenm wrote:
>
> > In https://reviews.llvm.org/D28236#634154, @davide wrote:
> >
> > > In https://reviews.llvm.org/D28236#634134, @arsenm wrote:
> > >
> > > > In https://reviews.llvm.org/D28236#634125, @davide wrote:
> > > >
> > > > > `SimplifyLibCalls.cpp` does a similar transformation (`cos(-x) -> cos(x)`).
> > > > >  I think we should have the logic for this simplifications in a single place, if possible, so maybe there we should unconditionally lower libc `cos()` -> `llvm.cos` and simplify here?
> > > >
> > > >
> > > > I think that needs to be part of a larger change to fix the mess of math intrinsic and libcall handling. In the case of cos, I view the cos function as somewhat different from llvm.cos. I expect the intrinsic to be a possibly less precise machine instruction version of cos rather than a fully correct library call so I'm not sure it should always be replaced with the intrinsic.
> > >
> > >
> > > Not quite, as that would violate LangRef cos semantic. `http://llvm.org/docs/LangRef.html#llvm-cos-intrinsic`
> >
> >
> > The C standard is uselessly underspecified. It doesn't have any constraints on the precision of math functions. IMO having the intrinsic be the same as the libcall is not useful. e.g. errno is not a meaningful concept on all platforms, and the intent of intrinsics is usually to map to a machine instruction.
>
>
>
>
> In https://reviews.llvm.org/D28236#634466, @arsenm wrote:
>
> > In https://reviews.llvm.org/D28236#634154, @davide wrote:
> >
> > > In https://reviews.llvm.org/D28236#634134, @arsenm wrote:
> > >
> > > > In https://reviews.llvm.org/D28236#634125, @davide wrote:
> > > >
> > > > > `SimplifyLibCalls.cpp` does a similar transformation (`cos(-x) -> cos(x)`).
> > > > >  I think we should have the logic for this simplifications in a single place, if possible, so maybe there we should unconditionally lower libc `cos()` -> `llvm.cos` and simplify here?
> > > >
> > > >
> > > > I think that needs to be part of a larger change to fix the mess of math intrinsic and libcall handling. In the case of cos, I view the cos function as somewhat different from llvm.cos. I expect the intrinsic to be a possibly less precise machine instruction version of cos rather than a fully correct library call so I'm not sure it should always be replaced with the intrinsic.
> > >
> > >
> > > Not quite, as that would violate LangRef cos semantic. `http://llvm.org/docs/LangRef.html#llvm-cos-intrinsic`
> >
> >
> > The C standard is uselessly underspecified. It doesn't have any constraints on the precision of math functions. IMO having the intrinsic be the same as the libcall is not useful. e.g. errno is not a meaningful concept on all platforms, and the intent of intrinsics is usually to map to a machine instruction.
>
>
> I don't necessarily disagree, but I think that changing the semantic of `cos` would require a much wider discussion on `llvm-dev`.


Getting back on topic, is there any objection to doing this optimization for the intrinsic (and for llvm.amdgcn.cos) separately here?


https://reviews.llvm.org/D28236