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

Tue Dec 11 04:29:05 PST 2018

rengolin added a comment.

Overall, this change looks ok to me. The tests look good, too.

However, I have a question that I have asked before and don't seem to see a solution here: how do you deal with evolving library support?

Today, we don't have SVE, so we don't generate SVE bindings, all fine. Say, next year you upstream SVE bindings in SLEEF and then change LLVM to emit those too.

The objects compiled with LLVM next year won't link against SLEEF compiled today. You can say "compile SLEEF again", but Linux distros have different schedule for different packages, especially when they have different maintainers.

They cope with the main libraries by validating and releasing a clear set of compatible libraries, and the GNU community participates heavily in that process by testing and patching gcc, glibc, binutils, etc.

I don't think SLEEF maintainers will have the same stamina, so it's possible that an update in either distro or SLEEF breaks things, they reject the new package, but now, LLVM is broken, too.

This is not an LLVM problem, per se, but by supporting SLEEF, you'll be transferring the problem to the LLVM community, as we'll be the ones receiving the complaints first.

This also happens in LLVM proper, when we have just released a new version and people haven't updated SLEEF yet, and we receive a burst of complaints.

The real question is: is the SLEEF community ready to take on the task of fixing compatibility / linkage issues as soon as they occur? The alternative is what we do with other broken code: deletion. I want to avoid that.

The other question, inline, is about the argument name. We really want to use the same as existing one, for the sake of least surprise.

Some additional random comments...

Adding the triple here shows the real dependency that was never added, because SVML assumes Intel hardware, so no problems there.

The discussion on how to simplify the pattern matching is valid, but at this stage, I think we can keep the existing mechanism.

I agree that this is bound to increase with SVE but I'm not sure writing a whole new table-gen back end just for this is worthy.

The code would be slightly longer on the `src` side (instead of `build` side) but it's well organised and clear, so no big deal.

I tend to agree with @steleman that auto-generating as a loop+replace can be tricky if there are support gaps in the library.

But further cleanup changes to this patch could prove me wrong. :)

cheers,
--renato

================
Comment at: llvm/trunk/lib/Analysis/TargetLibraryInfo.cpp:29
+                          "Intel SVML library"),
+               clEnumValN(TargetLibraryInfoImpl::SLEEFGNUABI, "sleefgnuabi",
+                          "SIMD Library for Evaluating Elementary Functions")));
----------------
A quick google search told me GCC uses `-fveclib=SLEEF`. We don't want to be different or that would create big problems in build systems. It seems, at least right now, there's only one ABI, so there's no point in premature classification.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D53927