On Thu, 16 Nov 2023 13:00:03 GMT, Magnus Ihse Bursie <i...@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
