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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:294-298
+    switch (TargetTriple.getArch()) {
+    default:
+      break;
+    case llvm::Triple::aarch64:
+    case llvm::Triple::aarch64_be:
----------------
NOTE: A following comment might make this one redundant.

Given you emit a diagnostic for `-fveclib` for currently unsupported targets and `addVectorizableFunctionsFromVecLib` takes a target triple so that it'll be a NOP for unsupported targets, can this restriction be removed? It just seems like redundant duplication.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:296
+      case llvm::Triple::aarch64_be:
+        TLII->addVectorizableFunctionsFromVecLib(TargetLibraryInfoImpl::SLEEFGNUABI);
+        break;
----------------
danielkiss wrote:
> paulwalker-arm wrote:
> > What does `GNUABI` signify? My understanding is the SLEEF functions use the Vector ABI (which is somewhat target neutral?) to construct their name.
> > 
> > Just a thought but to be a bit more target neutral what about splitting `TargetLibraryInfoImpl::SLEEF` based on vector length.  So here we'll have
> > ```
> > TLII->addVectorizableFunctionsFromVecLib(TargetLibraryInfoImpl::SLEEF_VF2_...)
> > TLII->addVectorizableFunctionsFromVecLib(TargetLibraryInfoImpl::SLEEF_VF4_...)
> > ```
> > This way there will be one version of TargetLibraryInfo for SLEEF and different targets can pick and choose based on vectorisation factors.
> > What does `GNUABI` signify? My understanding is the SLEEF functions use the Vector ABI (which is somewhat target neutral?) to construct their name.
> `GNUABI` comes from the [[ https://sleef.org/additional.xhtml#gnuabi | libsleefgnuabi.so ]]. it is compatible with `libmvec` but has more functions. Normal sleef ABI is different.
> 
> I added triplet to `addVectorizableFunctionsFromVecLib` so vectorisation factors per target then can be handled there without bloating the enum.
The target triple doesn't convey things like SVE being available does it?  The same goes for other targets I guess in that for x86 does the triple specify the target's vector length?

There's also the question of how the SLEEF library has been built. Can we guarantee the SLEEF library will always match the target the user has chosen.  (i.e. just because the user specifies `+sve` doesn't mean the SLEEF library has functions for the scalable vector types).

Perhaps this is really a toolchain packaging problem but either way it feels like something that must be solved within clang via either a tighter `TLI` interface and/or a more specific set of `-fveclib` options?

That's really the crux of the problem.  The other `-fveclib` options has a very specific set of functions they provide whereas `SLEEF` does not. So perhaps the options should be `SLEEF_NEON` or `SLEEF64`, `SLEEF128` (not sure if there's a generic way to describe scalable vectors so perhaps we cannot escape SLEEF_SVE?), which might also suggests the parsing for `-fveclib` requires expanding if for example we want to allow `-fveclib= SLEEF64,SLEEF128`. NOTE: I can see for MVEC `LIBMVEC_X86` exists.

For what it's worth this is why there's the idea to add pragmas to decorate the math function declarations so vector function mappings can be added automatically based on whatever math.h header is included.


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

https://reviews.llvm.org/D134719