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

[Daniel Kiss via Phabricator via llvm-commits]

danielkiss 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:
----------------
paulwalker-arm wrote:
> 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.
ack


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:296
+      case llvm::Triple::aarch64_be:
+        TLII->addVectorizableFunctionsFromVecLib(TargetLibraryInfoImpl::SLEEFGNUABI);
+        break;
----------------
paulwalker-arm wrote:
> 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.
> 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.
> 
IIRC there is no other platform right now for libmvec beside x86 so `LIBMVEC_X86` is what it is, once others are added I'd rename it too just `LIBMVEC`.

Once SVE is enabled for in Sleef the SVE version of functions are additions to the current set.
Depends on how the sleef library is packed into the toolchain\platform one could add conditional logic for certain platform, version etc based on the triplet. Right now it will be useful to add x86 base versions.
`-mveclib_options=<arch_specifics>` or similar option could be there to override, drive such a decision.



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

https://reviews.llvm.org/D134719