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

[Stefan Teleman via Phabricator via llvm-commits]

steleman marked 11 inline comments as done.
steleman added a comment.

Added inline comments/answers to Renato's questions.



================
Comment at: lib/Analysis/TargetLibraryInfo.cpp:368
   default:
-    TLI.setUnavailable(LibFunc_exp10);
-    TLI.setUnavailable(LibFunc_exp10f);
-    TLI.setUnavailable(LibFunc_exp10l);
+    if (T.getArch() != Triple::aarch64) {
+      TLI.setUnavailable(LibFunc_exp10);
----------------
rengolin wrote:
> This would probably fail in many non-Arm64 arches, too. 
> 
> Best to restrict to what this was meant for, which I'm guessing is x86.
I am a bit worried about this one. The original set exp10|exp10f|exp10l as unavailable on all arches. I originally enabled these functions for AArch64 only.

If we restrict this to x86|x86_64 only, then these functions will become available
on MIPS, PPC, PPC64, etc. I have no way of testing these ISA's to determine if they're broken or not. 

So I disabled these functions for x86|x86_64 only. Let me know if you'd like to revert this to the original, which was to enable them on AArch64 only.



================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4952
+// expandExperimentalIntrinsics - return the canonical non-vector libm
+// function name for the experimental Intrinsics.
+static const char*
----------------
rengolin wrote:
> I think we can have these in the same place as the others, but with an "experimental" comment before.
> 
> The main idea behind the experimental tag is so that people don't rely on it until it's complete, but once it is, they can.
> 
> So if we create them in different places, we'd have to move them in once they're no longer experimental and that would be a pain.
> 
> I'm not expecting this to be in experimental for too long and am hoping to get this official by the next release (july-ish 2019), so, let's keep the experimental status on comments only and then remove the comments when everything is working as expected.
Renamed this function **to expandNonVectorIntrinsics**.



================
Comment at: test/Transforms/LoopVectorize/AArch64/sleef-calls-aarch64-fast.ll:3
+; RUN: opt -vector-library=SLEEF -loop-unroll -loop-vectorize -S < %s | FileCheck %s
+; RUN: opt -vector-library=SLEEF -loop-unroll -loop-vectorize -fp-contract=fast -S < %s | FileCheck %s
+
----------------
rengolin wrote:
> does fp-contract=fast really make a difference for these transformations?
It does not. Which is good. :-) That's the point of the test - the transformations should stay identical, and pass, with or without -fp-contract=fast.



================
Comment at: test/Transforms/LoopVectorize/AArch64/sleef-calls-aarch64-fast.ll:6
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64-unknown-linux-gnu"
+
----------------
rengolin wrote:
> is this really specific to Arm64? At least the generation could be done for any target, if the library supports it or not is not relevant for this test, no?
SLEEF is only currently enabled for AArch64 in TargetLibraryInfo. So, for any other ISA, all the tests will fail, because the transformations won't happen.

Plus, for any ISA other than AArch64, the SLEEF function name mangling will be different than on AArch64. Which means this particular test - as written for AArch64 - can't be used on any other ISA; all the CHECKs will fail.


================
Comment at: test/Transforms/LoopVectorize/AArch64/sleef-calls-aarch64.ll:3
+; RUN: opt -vector-library=SLEEF -loop-unroll -loop-vectorize -S < %s | FileCheck %s
+; RUN: opt -vector-library=SLEEF -loop-unroll -loop-vectorize -fp-contract=fast -S < %s | FileCheck %s
+
----------------
rengolin wrote:
> What's the difference between these two test files?
sleef-calls-aarch64-fast.ll calls

```
%call = call fast <type> <function-name>(<argument>)
```

sleef-calls-aarch64.ll calls

```
%call = tail call <type> <function-name>(<argument>)
```







Repository:
  rL LLVM

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

https://reviews.llvm.org/D53927