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

[Xiaohong Gong]

On Thu, 16 Nov 2023 13:00:03 GMT, Magnus Ihse Bursie <ihse at openjdk.org> wrote:

>> 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
>
> doc/building.md line 552:
> 
>> 550: 
>> 551: libsleef, the [SIMD Library for Evaluating Elementary Functions](
>> 552: https://sleef.org/) is required when building libvmath.so on Linux+AArch64
> 
> The conventional way we have refered to os/cpu combinations in the build documentation is like this: `Linux/aarch64`.
> 
> I also think you need to expand a bit that this is optional, and if you do not provide libsleef, the build will succeed but without the vector performance enhancements provided by libvmath.

Thanks for the review! This sounds good to me. I will add it.

> 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.

Seems as above. The target library is `libvmath.so`, and the cflags/libs are used for building it instead of `libsleef.so`.

> 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.

This is just used to print the result of `AC_MSG_CEHCKING[if ARM SVE feature is supported]` in configure.

> make/autoconf/lib-vmath.m4 line 102:
> 
>> 100:   fi
>> 101: 
>> 102:   AC_SUBST(LIBSLEEF_FOUND)
> 
> Do not export LIBSLEEF_FOUND. It is okay to use internally here, but you should instead export ENABLE_LIBSLEEF, using true/false (instead of yes/no). This is the way we handle all other optional components.

Make sense to me. Thanks for the comment!

> 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".

Yes, it seems weird. But the library we want to built out is `libvmath.so` instead of `libsleef.so`. And we not only check the sleef library, but also the ARM SVE feature inside it. So using `VMATH` suffix is more reasonable to me. WDYT?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1398574834
PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1398573871
PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1398571212
PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1398575161
PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1398572906