RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v3]

[Magnus Ihse Bursie]

On Wed, 15 Nov 2023 01:32:00 GMT, Xiaohong Gong <xgong at openjdk.org> wrote:

>> Currently the vector floating-point math APIs like `VectorOperators.SIN/COS/TAN...` are not intrinsified on AArch64 platform, which causes large performance gap on AArch64. Note that those APIs are optimized by C2 compiler on X86 platforms by calling Intel's SVML code [1]. To close the gap, we would like to optimize these APIs for AArch64 by calling a third-party vector library called libsleef [2], which are available in mainstream Linux distros (e.g. [3] [4]).
>> 
>> SLEEF supports multiple accuracies. To match Vector API's requirement and implement the math ops on AArch64, we 1) call 1.0 ULP accuracy with FMA instructions used stubs in libsleef for most of the operations by default, and 2) add the vector calling convention to apply with the runtime calls to stub code in libsleef. Note that for those APIs that libsleef does not support 1.0 ULP, we choose 0.5 ULP instead.
>> 
>> To help loading the expected libsleef library, this patch also adds an experimental JVM option (i.e. `-XX:UseSleefLib`) for AArch64 platforms. People can use it to denote the libsleef path/name explicitly. By default, it points to the system installed library. If the library does not exist or the dynamic loading of it in runtime fails, the math vector ops will fall-back to use the default scalar version without error. But a warning is printed out if people specifies a nonexistent library explicitly.
>> 
>> Note that this is a part of the original proposed patch in panama-dev [5], just with some initial review comments addressed. And now we'd like to get some wider feedbacks from more hotspot experts.
>> 
>> [1] https://github.com/openjdk/jdk/pull/3638
>> [2] https://sleef.org/
>> [3] https://packages.fedoraproject.org/pkgs/sleef/sleef/
>> [4] https://packages.debian.org/bookworm/libsleef3
>> [5] https://mail.openjdk.org/pipermail/panama-dev/2022-December/018172.html
>
> Xiaohong Gong has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
> 
>  - Add a bundled native lib in jdk as a bridge to libsleef
>  - Merge 'jdk:master' into JDK-8312425
>  - Disable sleef by default
>  - Merge 'jdk:master' into JDK-8312425
>  - 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF

There's a lot to work with regarding the build system changes here...

make/autoconf/lib-vmath.m4 line 39:

> 37:   LIBVMATH_CFLAGS=
> 38:   LIBVMATH_LIBS=
> 39: 

There are multiple issues with this function. Please have a look at how other libraries are handled. Some remarks:
1) You always need to pair AC_MSG_CHECKING and AC_MSG_RESULT. Do not make any operations in between that can cause output.
2) If the user runs just --with-libsleef, the value will be "yes". You need to treat this, not as a path, but as a request to enable the library using default methods (like pkg-check or well known locations).

make/autoconf/lib-vmath.m4 line 49:

> 47:          test -e ${with_libsleef}/include/sleef.h; then
> 48:         LIBSLEEF_FOUND=yes
> 49:         LIBVMATH_LIBS="-L${with_libsleef}/lib"

This should be LIBSLEEF_LIBS and ...CFLAGS.

make/autoconf/lib-vmath.m4 line 92:

> 90:             []
> 91:         )
> 92:         AC_MSG_RESULT([${SVE_FEATURE_SUPPORT}])

What is this test even for? I can't see any usage of SVE_FEATURE_SUPPORT outside this function.

make/autoconf/libraries.m4 line 129:

> 127:   LIB_SETUP_LIBFFI
> 128:   LIB_SETUP_MISC_LIBS
> 129:   LIB_SETUP_VMATH

The function (and file) should be named after "sleef", not "vmath".

-------------

Changes requested by ihse (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16234#pullrequestreview-1734359314
PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1395684054
PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1395687129
PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1395684964
PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1395686104