Re: RFR: 8329816: Add SLEEF version 3.6.1 [v2]
On Thu, 23 May 2024 12:49:42 GMT, Hamlin Li wrote: >> Okay, suffix works fine too. But the files currently in the patch is named >> e.g. `sleefinline_advsimd.h`, which does not indicate any platform at all. >> Is it a generic file, and the platform specific ones are still missing from >> this PR? > > I think both `sleefinline_advsimd.h` and `sleefinline_sve.h` are specific for > arm. > In the future, on riscv the corresponding file name will be > `sleefinline_rvvm1.h`. > > Only `misc.h` is a generic file shared among platforms. Oh, you mean that the `sve` suffix signals that it is for ARM. I thought you were talking about having a name like `sleefinline_aarch64.h`. Sure, if the only files generated by sleef are ever .h files, the logic for chosing which to include can be done entirely by `#ifdef`s in the source code. But if there ever needs to be different .c or .cpp files to include in the build, the build system needs to be able to determine automatically if they should be included or included, and that can only be made if the path or the file name includes the CPU moniker. Personally, I think it would show good alignment with the prevailing norms in the JDK to also use this way of naming files for .h files. But I confess that for .h files it is more a matter of style, rather than a necessity from the build system. - PR Review Comment: https://git.openjdk.org/jdk/pull/19185#discussion_r1611964018
Re: RFR: 8329816: Add SLEEF version 3.6.1 [v2]
On Thu, 23 May 2024 16:06:42 GMT, Magnus Ihse Bursie wrote: >> I think both `sleefinline_advsimd.h` and `sleefinline_sve.h` are specific >> for arm. >> In the future, on riscv the corresponding file name will be >> `sleefinline_rvvm1.h`. >> >> Only `misc.h` is a generic file shared among platforms. > > Oh, you mean that the `sve` suffix signals that it is for ARM. I thought you > were talking about having a name like `sleefinline_aarch64.h`. > > Sure, if the only files generated by sleef are ever .h files, the logic for > chosing which to include can be done entirely by `#ifdef`s in the source > code. But if there ever needs to be different .c or .cpp files to include in > the build, the build system needs to be able to determine automatically if > they should be included or included, and that can only be made if the path or > the file name includes the CPU moniker. > > Personally, I think it would show good alignment with the prevailing norms in > the JDK to also use this way of naming files for .h files. But I confess that > for .h files it is more a matter of style, rather than a necessity from the > build system. Thanks for the information. I think the files from sleef will be .h headers only. I'm also fine to align with prevailing norms in the JDK, it's always good to do so. Just a reminder for us in the future discussion of https://github.com/openjdk/jdk/pull/18605, maybe we should consider this dir or file naming norms (e.g. currently there are vector_math_sve.c and vector_math_neon.c under src/jdk.incubator.vector/linux/native/libvectormath), as there will be more files related to different platforms added in the future, e.g. riscv64. - PR Review Comment: https://git.openjdk.org/jdk/pull/19185#discussion_r1612933942
Re: RFR: 8329816: Add SLEEF version 3.6.1 [v2]
On Thu, 23 May 2024 10:40:26 GMT, Magnus Ihse Bursie wrote: >>> So you'd need a different copy of sleef for each platform? >> >> I think it's one or more. >> >>> The files you have put in linux/native/libvectormath, what platform are >>> they for? Should we not put them in a platform-specific subdirectory? >> >> we could, but not necessary, as long as they have different suffixes, and >> normally that suffixes indicate what platform it's for. > > Okay, suffix works fine too. But the files currently in the patch is named > e.g. `sleefinline_advsimd.h`, which does not indicate any platform at all. Is > it a generic file, and the platform specific ones are still missing from this > PR? I think both `sleefinline_advsimd.h` and `sleefinline_sve.h` are specific for arm. In the future, on riscv the corresponding file name will be `sleefinline_rvvm1.h`. Only `misc.h` is a generic file shared among platforms. - PR Review Comment: https://git.openjdk.org/jdk/pull/19185#discussion_r1611636701
Re: RFR: 8329816: Add SLEEF version 3.6.1 [v2]
On Wed, 22 May 2024 09:39:43 GMT, Hamlin Li wrote: >> make/devkit/createSleef.sh line 32: >> >>> 30: # >>> 31: # 1. cd >>> 32: # 2. bash /make/devkit/createSleef.sh aarch64-gcc.cmake >>> /devkit >> >> So you'd need a different copy of sleef for each platform? The files you >> have put in `linux/native/libvectormath`, what platform are they for? Should >> we not put them in a platform-specific subdirectory? > >> So you'd need a different copy of sleef for each platform? > > I think it's one or more. > >> The files you have put in linux/native/libvectormath, what platform are they >> for? Should we not put them in a platform-specific subdirectory? > > we could, but not necessary, as long as they have different suffixes, and > normally that suffixes indicate what platform it's for. Okay, suffix works fine too. But the files currently in the patch is named e.g. `sleefinline_advsimd.h`, which does not indicate any platform at all. Is it a generic file, and the platform specific ones are still missing from this PR? - PR Review Comment: https://git.openjdk.org/jdk/pull/19185#discussion_r1611448592
Re: RFR: 8329816: Add SLEEF version 3.6.1 [v2]
On Wed, 22 May 2024 09:31:27 GMT, Magnus Ihse Bursie wrote: > So you'd need a different copy of sleef for each platform? I think it's one or more. > The files you have put in linux/native/libvectormath, what platform are they > for? Should we not put them in a platform-specific subdirectory? we could, but not necessary, as long as they have different suffixes, and normally that suffixes indicate what platform it's for. - PR Review Comment: https://git.openjdk.org/jdk/pull/19185#discussion_r1609635623
Re: RFR: 8329816: Add SLEEF version 3.6.1 [v2]
On Thu, 27 Jun 2024 21:56:19 GMT, Mikael Vidstedt wrote: >> [JDK-8312425](https://bugs.openjdk.org/browse/JDK-8312425) is looking to >> optimize vector math operations by leveraging the SLEEF library. For legal >> reasons the actual contribution of the SLEEF files needs to be handled >> separately. This enhancement adds the relevant files, enabling the rest of >> [JDK-8312425](https://bugs.openjdk.org/browse/JDK-8312425) to move forward. > > Mikael Vidstedt 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 three additional > commits since the last revision: > > - Add SLEEF version 3.6.1 > - Merge branch 'master' into 8329816-sleef > - 8329816: Add SLEEF version 3.6 make/devkit/createSleef.sh line 32: > 30: # > 31: # 1. cd > 32: # 2. bash /make/devkit/createSleef.sh aarch64-gcc.cmake > /devkit So you'd need a different copy of sleef for each platform? The files you have put in `linux/native/libvectormath`, what platform are they for? Should we not put them in a platform-specific subdirectory? - PR Review Comment: https://git.openjdk.org/jdk/pull/19185#discussion_r1609623756
Re: RFR: 8329816: Add SLEEF version 3.6.1 [v2]
> [JDK-8312425](https://bugs.openjdk.org/browse/JDK-8312425) is looking to > optimize vector math operations by leveraging the SLEEF library. For legal > reasons the actual contribution of the SLEEF files needs to be handled > separately. This enhancement adds the relevant files, enabling the rest of > [JDK-8312425](https://bugs.openjdk.org/browse/JDK-8312425) to move forward. Mikael Vidstedt 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 three additional commits since the last revision: - Add SLEEF version 3.6.1 - Merge branch 'master' into 8329816-sleef - 8329816: Add SLEEF version 3.6 - Changes: - all: https://git.openjdk.org/jdk/pull/19185/files - new: https://git.openjdk.org/jdk/pull/19185/files/b02efa6b..b29de559 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19185=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19185=00-01 Stats: 160121 lines in 2836 files changed: 111602 ins; 34872 del; 13647 mod Patch: https://git.openjdk.org/jdk/pull/19185.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19185/head:pull/19185 PR: https://git.openjdk.org/jdk/pull/19185
