[PATCH] D146905: [IR] Add llvm.tan.* intrinsic

[Jun Zhang via Phabricator via llvm-commits]

junaire added a comment.

In D146905#4223155 <https://reviews.llvm.org/D146905#4223155>, @craig.topper wrote:

> In D146905#4222910 <https://reviews.llvm.org/D146905#4222910>, @junaire wrote:
>
>> In D146905#4222696 <https://reviews.llvm.org/D146905#4222696>, @jdoerfert wrote:
>>
>>> I voiced this before <https://discourse.llvm.org/t/math-intrinsics/67192/6?u=jdoerfert> and while I will repeat it here I won't block this patch all by myself:
>>>
>>> This is a bad idea.
>>>
>>> We are playing whack-a-mole and assuming we don't want to commit to all of them (math intrinsics), this is always going to be a source of "weird behavior", to say the least.
>>> Even if we had all of them there are still unsolved problems as soon as you leave the space of systems with a libm that is linked in late since not all intrinsics lower to target instructions but rather libm calls.
>>>
>>> What we should do is to remove all math intrinsics and instead work on the math functions themselves, as we do with other library functions (malloc, free, ...).
>>> A math intrinsic is literally a regular call to the corresponding math function with a `no_errno` flag attached to it (and the type encoded in the name):
>>> We probably even have all the attributes today to express that:
>>>
>>>   declare double @tan(double) memory(readonly)
>>>
>>> [Not to mention that this makes tan, which is represented as target independent math intrinsic, special only for X86...]
>>>
>>> Now there have been arguments for intrinsics <https://discourse.llvm.org/t/math-intrinsics/67192/8?u=jdoerfert>, e.g., lib call matching is bad. I don't disagree, but we can fix that.
>>> It was also raised that we treat intrinsics as cheap and calls as expensive in heuristics, though, that is just not right to begin with.
>>> A math call is the same if we call the math function as part of the lowering anyway, no matter if it was a call or intrinsic call in the IR.
>>> That said, we can make smarter choices for heuristics fairly easily, e.g., a `readnone` call is probably not going to be too expensive.
>>>
>>> All that said, I think we should recognize math functions and annotate them, but just not as intrinsics. Maybe something like this:
>>>
>>>   define double @tan(double) libm(tan,double))
>>>   define float @tanf(float) libm(tan,float))
>>>   ...
>>
>> Hi @jdoerfert, thanks for the heads up. I've read the whole thread you linked and believe what you proposed is reasonable. However, it looks like there's no obvious consensus so far in the community, and everything is just in discussion? The one thing I agree with is that the whole thing is a mess. I'd suggest you start an RFC about how we handle math intrinsic and libcall matchings. But before that, the best choice for now I think is to keep those trig calls consistent so it's easier to do folds in the middle end, and that's the primary reason for me to create the patch.
>
> We already do math folds in the middle end. See `LibCallSimplifier::optimizeFloatingPointLibCall`. We even recognize `tan` there. Adding an intrinsic would disable the existing optimizations for `tan`.

Yes and no. I mean those 10th-grade trigonometrical identities like https://godbolt.org/z/nxvPe5en5. These should be handled in InstCombine. Yes, you can argue that it's unnecessary to add an intrinsic to do these folds but obviously, it will make things a lot easier and more clear if we have consistent intrinsics. In fact, there's a patch long ago trying to fix the problem: https://reviews.llvm.org/D41283


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146905/new/

https://reviews.llvm.org/D146905