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

[Jun Zhang via Phabricator via llvm-commits]

junaire marked an inline comment as done.
junaire added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:124
     case ISD::FSIN:        R = SoftenFloatRes_FSIN(N); break;
+    case ISD::FTAN:        R = SoftenFloatRes_FTAN(N); break;
     case ISD::STRICT_FSQRT:
----------------
RKSimon wrote:
> We probably want to add STRICT_FTAN at the same time.
I have to admit I know nothing about it and I just simply grep `STRICT_FSIN` and add corresponding cases. I'm confused about where it comes from since there's no occurrence in `SelectionDAGBuilder.cpp`.

It'll be helpful if you can elaborate a bit about how these things work under the hood! :)


================
Comment at: llvm/test/CodeGen/X86/llvm.tan.ll:39
+  ret ppc_fp128 %x
+}
+
----------------
RKSimon wrote:
> It'd be better to put these in the existing test files with other libm calls - fp128-libcalls.ll etc. - I'm not sure how thorough our existing tests are for these though - grepping for sin.f32 doesn't bring up much for instance.
I'm uncertain about what's the right way to go. It seems the tests for these trig intrinsics are pretty messy and we're lacking test coverage for the X86 target. I created this new file since I saw a test in `llvm/test/CodeGen/AMDGPU/llvm.sin.ll`.

I'd propose just leaving it as is, but if you insist I can move them (Then what about llvm.tan.f32 f64 ?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146905