[PATCH] D53927: [AArch64] Enable libm vectorized functions via SLEEF

[Stefan Teleman via Phabricator via llvm-commits]

steleman marked 3 inline comments as done.
steleman added inline comments.


================
Comment at: include/llvm/IR/Intrinsics.td:516-533
+  def int_acos : Intrinsic<[llvm_anyfloat_ty], [LLVMMatchType<0>]>;
+  def int_asin : Intrinsic<[llvm_anyfloat_ty], [LLVMMatchType<0>]>;
+  def int_atan : Intrinsic<[llvm_anyfloat_ty], [LLVMMatchType<0>]>;
+  def int_atan2 : Intrinsic<[llvm_anyfloat_ty],
+                            [LLVMMatchType<0>, LLVMMatchType<0>]>;
+  def int_tan  : Intrinsic<[llvm_anyfloat_ty], [LLVMMatchType<0>]>;
   def int_pow  : Intrinsic<[llvm_anyfloat_ty],
----------------
echristo wrote:
> steleman wrote:
> > arsenm wrote:
> > > I would split adding the new intrinsics into a separate patch
> > That won't work.
> > 
> > 
> "Why not" is always a good follow up to that sort of response :)
Because:

- It adds unnecessary complexity to this changeset **and** to D53928.
- There is no obvious benefit to this added complexity.
- There are many drawbacks to this added complexity.
- Splitting it based on new vs. existing intrinsics seems arbitrary. One might as well ask for a separate changeset for each and every new intrinsic. 

And, now it's my turn to ask:
- Why should it be split?
- What is the benefit of splitting it?
- What will be accomplished by splitting it?
- Is increased complexity - with all its drawbacks and pitfalls - a goal?
- Does integrating it //as is// prevent, or hinder something else?
- Why does this come up now, after this changeset - and its friend D53928 - have been reviewed for 4+ months, and already approved, and re-approved, and there are 21 subscribers to this changeset?




Repository:
  rL LLVM

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

https://reviews.llvm.org/D53927