On Wed, 15 Nov 2023 01:32:00 GMT, Xiaohong Gong <xg...@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
