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

2023-11-29 Thread Xiaohong Gong
On Thu, 23 Nov 2023 14:05:51 GMT, Magnus Ihse Bursie  wrote:

>> Xiaohong Gong has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Address review comments in build system
>
> make/autoconf/lib-vmath.m4 line 70:
> 
>> 68: if test "x$SYSROOT" = "x" &&
>> 69:test "x${LIBSLEEF_FOUND}" = "xno"; then
>> 70:   PKG_CHECK_MODULES([LIBSLEEF], [sleef], [LIBSLEEF_FOUND=yes], 
>> [LIBSLEEF_FOUND=no])
> 
> Suggestion:
> 
>   PKG_CHECK_MODULES([SLEEF], [sleef], [LIBSLEEF_FOUND=yes], 
> [LIBSLEEF_FOUND=no])
> 
> 
> Otherwise `PKG_CHECK_MODULES` will set the variables  LIBSLEEF_CFLAGS and 
> LIBSLEEF_LIBS.

Keep using `LIBSLEEF`, as the cflags and libs are named with `LIBSLEEF_` 
prefix. Thanks!

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1410275161


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

2023-11-28 Thread Andrew Haley
On Tue, 28 Nov 2023 01:37:01 GMT, Xiaohong Gong  wrote:

>>> In fact, I am not even sure why it seems to the PR author to be a good idea 
>>> to let the default be dependent on the build machine at all. My personal 
>>> opinion is that it would be better to select either "SVE enabled" or "SVE 
>>> disabled" as the default, and then let the user override this on the 
>>> configure command line, if they target a platform with different SVE 
>>> availability.
>> 
>> SVE support should be enabled regardless of the build machine. The same 
>> binary must run on both SVE and non-SVE machines, using SVE if it is 
>> advantageous. I suppose some ancient C++ compilers without SVE support still 
>> exist, but I see no very good reason to support them building JDK 22+.
>> 
>> Making a configure option to disable SVE support for vector math is a 
>> mistake, but IMO mostly harmless because no-one will ever turn it off.
>
>> That's fine, but we must make sure that SVE is not used by the compiler in 
>> any other places. If you've already built on non-SVE and tested the result 
>> on both SVE and non-SVE, I'm happy.
> 
> Agree. 
> 
> Since it contains both NEON and SVE functions in libvmath.so, we have to 
> disable SVE feature when building those NEON functions. We want to separate 
> NEON/SVE functions in two files, build them with different cflags (i.e. only 
> build SVE sources with `-march=armv8-a+sve`), and then link to the single 
> `libvmath.so`. Do we have such similar examples or functions in current jdk 
> make system? I'm still struggling on finding out an effective way for it.
> 
>> SVE support should be enabled regardless of the build machine. The same 
>> binary must run on both SVE and non-SVE machines, using SVE if it is 
>> advantageous. I suppose some ancient C++ compilers without SVE support still 
>> exist, but I see no very good reason to support them building JDK 22+.
>> 
>> Making a configure option to disable SVE support for vector math is a 
>> mistake, but IMO mostly harmless because no-one will ever turn it off.
> 
> Yes, SVE feature is also always enabled in jdk hotspot on SVE machines. If we 
> add the option to give people disable SVE, it's weird that we disabling the 
> SVE just in libvmath.so, and enabling it in hotspot. Besides, we choose the 
> NEON stubs for smaller than 128-bit vector operations no matter whether the 
> runtime machine supports SVE or not. So performance may not be an issue. 
> Hence, I don't think people have reason disabling SVE features.

It makes no sense to configure any of this at build time. Postpone all of the 
decisions to runtime.

Don't touch the make system.Instead, try to open the library at runtime with 
`os::dll_open()`, after (or inside) `VM_Version::initialize()`.

If you're not running on an SVE system, none of the SVE routines will be 
called, so it doesn't matter, right?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1407511713


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

2023-11-27 Thread Xiaohong Gong
On Mon, 27 Nov 2023 16:43:09 GMT, Andrew Haley  wrote:

>> Apparently the situation is this: If your build machine happens to have SVE, 
>> then you will get SVE support in the vmath library. The SVE support will be 
>> used during runtime if the machine you run on has SVE support.
>> 
>> If your build host happens to to not have SVE, then the vmath library will 
>> be built without SVE support, and no matter if your runtime machine has SVE 
>> or not, it will not provide SVE support in the vmath library.
>> 
>> Now, if your CI farm has an arbitrarily selection of aarch64 machines with 
>> and without SVE, then you have no idea what you are going to get in your 
>> build.
>> 
>> We have been very careful in staying clear of this kind of "random" build 
>> system behavior. The system you build on should not affect the output -- at 
>> least, not without a chance to override the default value.
>> 
>> In fact, I am not even sure why it seems to the PR author to be a good idea 
>> to let the default be dependent on the build machine at all. My personal 
>> opinion is that it would be better to select either "SVE enabled" or "SVE 
>> disabled" as the default, and then let the user override this on the 
>> configure command line, if they target a platform with different SVE 
>> availability.
>
>> In fact, I am not even sure why it seems to the PR author to be a good idea 
>> to let the default be dependent on the build machine at all. My personal 
>> opinion is that it would be better to select either "SVE enabled" or "SVE 
>> disabled" as the default, and then let the user override this on the 
>> configure command line, if they target a platform with different SVE 
>> availability.
> 
> SVE support should be enabled regardless of the build machine. The same 
> binary must run on both SVE and non-SVE machines, using SVE if it is 
> advantageous. I suppose some ancient C++ compilers without SVE support still 
> exist, but I see no very good reason to support them building JDK 22+.
> 
> Making a configure option to disable SVE support for vector math is a 
> mistake, but IMO mostly harmless because no-one will ever turn it off.

> That's fine, but we must make sure that SVE is not used by the compiler in 
> any other places. If you've already built on non-SVE and tested the result on 
> both SVE and non-SVE, I'm happy.

Agree. 

Since it contains both NEON and SVE functions in libvmath.so, we have to 
disable SVE feature when building those NEON functions. We want to separate 
NEON/SVE functions in two files, build them with different cflags (i.e. only 
build SVE sources with `-march=armv8-a+sve`), and then link to the single 
`libvmath.so`. Do we have such similar examples or functions in current jdk 
make system? I'm still struggling on finding out an effective way for it.

> SVE support should be enabled regardless of the build machine. The same 
> binary must run on both SVE and non-SVE machines, using SVE if it is 
> advantageous. I suppose some ancient C++ compilers without SVE support still 
> exist, but I see no very good reason to support them building JDK 22+.
> 
> Making a configure option to disable SVE support for vector math is a 
> mistake, but IMO mostly harmless because no-one will ever turn it off.

Yes, SVE feature is also always enabled in jdk hotspot on SVE machines. If we 
add the option to give people disable SVE, it's weird that we disabling the SVE 
just in libvmath.so, and enabling it in hotspot. Besides, we choose the NEON 
stubs for smaller than 128-bit vector operations no matter whether the runtime 
machine supports SVE or not. So performance may not be an issue. Hence, I don't 
think people have reason disabling SVE features.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1406971807


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

2023-11-27 Thread Andrew Haley
On Mon, 27 Nov 2023 15:22:32 GMT, Magnus Ihse Bursie  wrote:

> In fact, I am not even sure why it seems to the PR author to be a good idea 
> to let the default be dependent on the build machine at all. My personal 
> opinion is that it would be better to select either "SVE enabled" or "SVE 
> disabled" as the default, and then let the user override this on the 
> configure command line, if they target a platform with different SVE 
> availability.

SVE support should be enabled regardless of the build machine. The same binary 
must run on both SVE and non-SVE machines, using SVE if it is advantageous. I 
suppose some ancient C++ compilers without SVE support still exist, but I see 
no very good reason to support them building JDK 22+.

Making a configure option to disable SVE support for vector math is a mistake, 
but IMO mostly harmless because no-one will ever turn it off.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1406450888


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

2023-11-27 Thread Magnus Ihse Bursie
On Mon, 27 Nov 2023 15:11:32 GMT, Andrew Haley  wrote:

>> You still need to separate out the SVE detection from the libsleef code, and 
>> provide a way to enable/disable it from the configure command line. It is 
>> not okay to auto-detect if features should be turned on or off by default, 
>> but it should always be possible to override.
>
>> You still need to separate out the SVE detection from the libsleef code, and 
>> provide a way to enable/disable it from the configure command line.
> 
> Why? I don't think this should be a build-time option at all, because it puts 
> the people who build binaries in an impossible position. Can't this all be 
> built unconditionally, with run-time feature detection?

Apparently the situation is this: If your build machine happens to have SVE, 
then you will get SVE support in the vmath library. The SVE support will be 
used during runtime if the machine you run on has SVE support.

If your build host happens to to not have SVE, then the vmath library will be 
built without SVE support, and no matter if your runtime machine has SVE or 
not, it will not provide SVE support in the vmath library.

Now, if your CI farm has an arbitrarily selection of aarch64 machines with and 
without SVE, then you have no idea what you are going to get in your build.

We have been very careful in staying clear of this kind of "random" build 
system behavior. The system you build on should not affect the output -- at 
least, not without a chance to override the default value.

In fact, I am not even sure why it seems to the PR author to be a good idea to 
let the default be dependent on the build machine at all. My personal opinion 
is that it would be better to select either "SVE enabled" or "SVE disabled" as 
the default, and then let the user override this on the configure command line, 
if they target a platform with different SVE availability.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1406326711


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

2023-11-27 Thread Andrew Haley
On Mon, 27 Nov 2023 14:59:23 GMT, Magnus Ihse Bursie  wrote:

> You still need to separate out the SVE detection from the libsleef code, and 
> provide a way to enable/disable it from the configure command line.

Why? I don't think this should be a build-time option at all, because it puts 
the people who build binaries in an impossible position. Can't this all be 
built unconditionally, with run-time feature detection?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1406310998


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

2023-11-27 Thread Magnus Ihse Bursie
On Mon, 27 Nov 2023 10:28:45 GMT, Andrew Haley  wrote:

>> We have to use this c-compiler option to build out the SVE ABIs (e.g. 
>> `svfloat32_t sinfx_u10sve(svfloat32_t input)`) in `libvmath.so`. Without 
>> this option, at build time, all the sve related featues like `arm_sve.h / 
>> __ARM_FEATURE_SVE` are missing, together with the sve symbols in 
>> `libvmath.so` we needed at runtime.  That means at runtime, hotspot cannot 
>> find out the sve symbols and the vector operations will fall back to the 
>> default java implementation.
>
> That's fine, but we must make sure that SVE is not used by the compiler in 
> any other places. If you've already built on non-SVE and tested the result on 
> both SVE and non-SVE, I'm happy.

You still need to separate out the SVE detection from the libsleef code, and 
provide a way to enable/disable it from the configure command line. It is not 
okay to auto-detect if features should be turned on or off by default, but it 
should always be possible to override.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1406291471


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

2023-11-27 Thread Andrew Haley
On Mon, 27 Nov 2023 01:06:21 GMT, Xiaohong Gong  wrote:

>> make/autoconf/lib-vmath.m4 line 94:
>> 
>>> 92:   # Check the ARM SVE feature
>>> 93:   SVE_CFLAGS="-march=armv8-a+sve"
>>> 94: 
>> 
>> What's this about? We're building a standard binary, and all SVE detection 
>> is to be deferred to runtime.
>
> We have to use this c-compiler option to build out the SVE ABIs (e.g. 
> `svfloat32_t sinfx_u10sve(svfloat32_t input)`) in `libvmath.so`. Without this 
> option, at build time, all the sve related featues like `arm_sve.h / 
> __ARM_FEATURE_SVE` are missing, together with the sve symbols in 
> `libvmath.so` we needed at runtime.  That means at runtime, hotspot cannot 
> find out the sve symbols and the vector operations will fall back to the 
> default java implementation.

That's fine, but we must make sure that SVE is not used by the compiler in any 
other places. If you've already built on non-SVE and tested the result on both 
SVE and non-SVE, I'm happy.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1405950723


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

2023-11-26 Thread Xiaohong Gong
On Thu, 23 Nov 2023 15:43:34 GMT, Andrew Haley  wrote:

>> Xiaohong Gong has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Address review comments in build system
>
> make/autoconf/lib-vmath.m4 line 94:
> 
>> 92:   # Check the ARM SVE feature
>> 93:   SVE_CFLAGS="-march=armv8-a+sve"
>> 94: 
> 
> What's this about? We're building a standard binary, and all SVE detection is 
> to be deferred to runtime.

We have to use this c-compiler option to build out the SVE ABIs (e.g. 
`svfloat32_t sinfx_u10sve(svfloat32_t input)`) in `libvmath.so`. Without this 
option, at build time, all the sve related featues like `arm_sve.h / 
__ARM_FEATURE_SVE` are missing, together with the sve symbols in `libvmath.so` 
we needed at runtime.  That means at runtime, hotspot cannot find out the sve 
symbols and the vector operations will fall back to the default java 
implementation.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1405529521


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

2023-11-26 Thread Xiaohong Gong
On Thu, 23 Nov 2023 14:10:02 GMT, Magnus Ihse Bursie  wrote:

>> Xiaohong Gong has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Address review comments in build system
>
> make/autoconf/lib-vmath.m4 line 89:
> 
>> 87:   if test "x${LIBSLEEF_FOUND}" = "xyes"; then
>> 88: ENABLE_LIBSLEEF=true
>> 89: LIBVMATH_LIBS="${LIBVMATH_LIBS} -lsleef"
> 
> Remove this line. It would just add `-lsleef` twice if you go via 
> `PKG_CHECK_MODULES`. You need to set -lsleef at the correct places.

Correct. Thanks! I will adjust all the relative names/cflags once the sve 
cflags is removed in the m4 file.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1405526805


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

2023-11-23 Thread Andrew Haley
On Thu, 23 Nov 2023 08:57:23 GMT, Xiaohong Gong  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 incrementally with one additional 
> commit since the last revision:
> 
>   Address review comments in build system

src/jdk.incubator.vector/linux/native/libvmath/vect_math.c line 63:

> 61: DEFINE_VECTOR_MATH_UNARY(expm1d2_u10, float64x2_t, advsimd)
> 62: 
> 63: #ifdef __ARM_FEATURE_SVE

No, we're building a standard binary that will use SVE if it's present on the 
machine it's running on. Such compile-time feature tests are inappropriate. You 
need to provide SVE entry points for the target machine, no matter how HotSpot 
is configured and built.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1403545717


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

2023-11-23 Thread Andrew Haley
On Thu, 23 Nov 2023 08:57:23 GMT, Xiaohong Gong  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 incrementally with one additional 
> commit since the last revision:
> 
>   Address review comments in build system

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

> 92:   # Check the ARM SVE feature
> 93:   SVE_CFLAGS="-march=armv8-a+sve"
> 94: 

What's this about? We're building a standard binary, and all SVE detection is 
to be deferred to runtime.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1403541835


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

2023-11-23 Thread Magnus Ihse Bursie
On Thu, 23 Nov 2023 08:57:23 GMT, Xiaohong Gong  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 incrementally with one additional 
> commit since the last revision:
> 
>   Address review comments in build system

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

> 56:test -e ${with_libsleef}/include/sleef.h; then
> 57:   LIBSLEEF_FOUND=yes
> 58:   LIBVMATH_LIBS="-L${with_libsleef}/lib"

Suggestion:

  LIBVMATH_LIBS="-L${with_libsleef}/lib -lsleef"

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

> 68: if test "x$SYSROOT" = "x" &&
> 69:test "x${LIBSLEEF_FOUND}" = "xno"; then
> 70:   PKG_CHECK_MODULES([LIBSLEEF], [sleef], [LIBSLEEF_FOUND=yes], 
> [LIBSLEEF_FOUND=no])

Suggestion:

  PKG_CHECK_MODULES([SLEEF], [sleef], [LIBSLEEF_FOUND=yes], 
[LIBSLEEF_FOUND=no])


Otherwise `PKG_CHECK_MODULES` will set the variables  LIBSLEEF_CFLAGS and 
LIBSLEEF_LIBS.

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

> 72: if test "x${LIBSLEEF_FOUND}" = "xno"; then
> 73:   AC_CHECK_HEADERS([sleef.h],
> 74:   [LIBSLEEF_FOUND=yes],

Suggestion:

  [
LIBSLEEF_FOUND=yes
SLEEF_LIBS=-lsleef
  ],

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

> 87:   if test "x${LIBSLEEF_FOUND}" = "xyes"; then
> 88: ENABLE_LIBSLEEF=true
> 89: LIBVMATH_LIBS="${LIBVMATH_LIBS} -lsleef"

Remove this line. It would just add `-lsleef` twice if you go via 
`PKG_CHECK_MODULES`. You need to set -lsleef at the correct places.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1403436252
PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1403433087
PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1403438730
PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1403437474


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

2023-11-23 Thread Magnus Ihse Bursie
On Thu, 23 Nov 2023 08:57:23 GMT, Xiaohong Gong  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 incrementally with one additional 
> commit since the last revision:
> 
>   Address review comments in build system

make/autoconf/spec.gmk.in line 894:

> 892: ENABLE_LIBSLEEF:=@ENABLE_LIBSLEEF@
> 893: LIBVMATH_CFLAGS:=@LIBVMATH_CFLAGS@
> 894: LIBVMATH_LIBS:=@LIBVMATH_LIBS@

It's getting better, but you still need to handle the naming here. It should be 
SLEEF_LIBS and SLEEF_CFLAGS, named after the library you import -- not the 
library you build. What if some other lib wants to use libsleef?

And you need to untangle the SVE flags (whatever we end doing with that) from 
LIBVMATH_CFLAGS.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1403423892


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

2023-11-23 Thread Xiaohong Gong
> 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 incrementally with one additional 
commit since the last revision:

  Address review comments in build system

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16234/files
  - new: https://git.openjdk.org/jdk/pull/16234/files/b29df846..2c3c4a64

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16234&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16234&range=02-03

  Stats: 126 lines in 7 files changed: 72 ins; 22 del; 32 mod
  Patch: https://git.openjdk.org/jdk/pull/16234.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16234/head:pull/16234

PR: https://git.openjdk.org/jdk/pull/16234