Page MenuHomePhabricator

Enable builtins necessary for SLEEF [AArch64] vectorized trigonometry libm functions
AcceptedPublic

Authored by steleman on Oct 31 2018, 4:04 AM.

Details

Summary

This changeset adds the required Builtins for enabling AArch64 vectorization of libm trigonometry functions via SLEEF: http://sleef.org/.

  • A new argument is added to -fveclib=<X>: SLEEF.
  • A number of Builtins that were previously unimplemented are now enabled.

This changeset depends on https://reviews.llvm.org/D53927.

List of SLEEF vectorized trigonometry functions:

acos
asin
atan
atanh
cos
cosh
exp
exp2
exp10
lgamma
log10
log2
log
sin
sinh
sqrt
tan
tanh
tgamma

Diff Detail

Repository
rC Clang

Event Timeline

steleman created this revision.Oct 31 2018, 4:04 AM
steleman edited the summary of this revision. (Show Details)Oct 31 2018, 4:48 AM
steleman updated this revision to Diff 172638.Nov 5 2018, 12:49 PM
  • changed the -fveclib=<X> argument value to 'sleefgnuabi'.
  • added atan2 and pow.
  • spreadsheet with comparison between libm and sleef is here:

https://docs.google.com/spreadsheets/d/1lcpESCnuzEoTl_XHBqE9FLL0tXJB_tZGR8yciCx1yjg/edit?usp=sharing

  • comprehensive test case in C with timings is here:

https://drive.google.com/open?id=1PGKRUdL29_ANoYebOo3Q59syhKp_mNSj

steleman updated this revision to Diff 178477.Dec 17 2018, 9:07 AM

Updated version of this changeset/patch, as per Renato's latest comments from D53927 (https://reviews.llvm.org/D53927).

New benchmark comparing SLEEF performance when built with GCC 8.2 vs. clang ToT from 12/15/2018:

https://docs.google.com/spreadsheets/d/1QgdJjgCTFA9I2_5MvJU0uR4JT4knoW5xu-ujxszRbd0/edit?usp=sharing

rengolin added a subscriber: rengolin.

Adding clang/omp developers for proper review. Please feel free to add more.

FYI, there are four main ongoing discussions on the LLVM thread (D53927):

  1. There is collusion between clang pragmas (clang vectorise), OpenMP pragmas (omp simd) and newly proposed veclib pragmas, which IMHO is too much. We need more OpenMP people looking at them. My proposal is to not add new ones and use OpenMP SIMD without creating library dependencies with OMP, but the interpretation of other pragmas in the same source needs sorting.
  1. How we're going to handle the naming scheme, currently proposed to have a local math.h and mask the SLEEF includes with macros. My proposal is to have a special header that the compiler only includes upon -mveclibabi instead. Library includes end up as -lsleef in the linker command line.
  1. Who controls the header file. The proposal is Clang control the headers for all vector libraries (all in math.h with guards), which means the evolution of Clang and each library is now tied. My proposal is for the library to have their own headers in a standard place (/usr/include/arch/something') and get Clang to agree on the right place, so that it can include the header from $default_path/$library_name.h`.
  1. The option was called fveclib but GCC calls it mveclibabi. There doesn't seem to be any behavioural differences, so I think we should follow the same name. We may be forced to create an alias, because this is not new in LLVM (already existed for SVML). This can go on a separate patch. But it's good to be aware of that. Making it an alias, we are fixing the implementation to be at least compatible with GCC, which is a good thing.

cheers,
--renato

lib/Index/CommentToXML.cpp
230 ↗(On Diff #178477)

Spurious change and unrelated to the patch. Please revert this part.

steleman updated this revision to Diff 178884.Dec 19 2018, 7:26 AM

Removed spurious patch for an unrelated change.

Ping!

Yes, I know, everyone was away for the holidays. :-)

Could someone please take a look at this. We'd like to make sure everything is OK on this side before D53927 can proceed.

Yes, it's a bit of a circular dependency in that sense.

This changeset doesn't cover any of the problems with OpenMP and conflicting pragmas and semantics described above by Renato. This is just the clang side of straightforward mapping of SLEEF vectorized function calls.

hfinkel accepted this revision.Jan 17 2019, 9:42 AM
hfinkel added a subscriber: hfinkel.

LGTM

Please, in the future, make sure you post full-context patches.

This revision is now accepted and ready to land.Jan 17 2019, 9:42 AM
steleman updated this revision to Diff 185578.Feb 6 2019, 9:25 AM

This is a small - but signifcant - update to the original changeset.

What's new in this update:

${top_srcdir}/lib/Frontend/CompilerInvocation.cpp:

When -fveclib=SLEEF was passed on compile-line, neither clang nor LLVM emit any warnings if SLEEF is not supported on the Target architecture. SLEEF is currently supported only on AArch64. So, for other ISA's, -fveclib=SLEEF would be quietly ignored, without any hints.

This new version of the changeset adds a diagnostic if -fveclib=SLEEF is called on an ISA that doesn't (yet) support SLEEF.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 9:25 AM
rengolin requested changes to this revision.Feb 9 2019, 5:07 AM

Since the previous version was approved already, I'm "requesting changes" so that we can look at it again, together with D53927 to make sure it's still the right way to go.

This revision now requires changes to proceed.Feb 9 2019, 5:07 AM

@hfinkel There are changes in this patch, as well as its LLVM counterpart D53927. Please, re-review.

Funny thing is, SVML is also only supported, AFAIK, for Intel. I agree that we should emit errors, but we should also emit a similar error on SVML.

I know it's not entirely relevant to this patch, but we should keep the behaviour consistent on all veclibs.

Also, can you add a test for the new error messages, please?

The new changes look good to me and don't substantially deviate from the previous, approved, patch, so LGTM after addressing the comments.

Thanks!

lib/Frontend/CompilerInvocation.cpp
678

This is not really invalid value for the flag, it's invalid architecture for the value.

I think there's already a string for that somewhere in Clang.

Hi Renato,

Thank you very much for the comments.

I will create a new changeset incorporating your comments, and then re-submit.

I will also add a Diagnostic for SVML and X86/X86_64.

steleman updated this revision to Diff 186963.Feb 14 2019, 10:38 PM

Addressed comments from Renato:

  • Diagnostic for unsupported argument to -fveclib=<N> is now:
error: unsupported option '<X>' for target '<Y>'
  • Added check and diagnostic for SVML on non-Intel ISA's.
rengolin accepted this revision.Feb 15 2019, 2:27 AM

Thanks, LGTM!

This revision is now accepted and ready to land.Feb 15 2019, 2:27 AM