On Mon, 1 Jul 2024 16:54:55 GMT, Hamlin Li <m...@openjdk.org> wrote: >> Hi, >> Can you help to review the patch? >> This pr is based on previous work and discussion in [pr >> 16234](https://github.com/openjdk/jdk/pull/16234), [pr >> 18294](https://github.com/openjdk/jdk/pull/18294). >> * NOTE: This pr depends on https://github.com/openjdk/jdk/pull/19185, which >> includes a README, a script to generate sleef inline headers and generated >> sleef inline headers. >> >> Compared with previous prs, the major change in this pr is to integrate the >> source of sleef (for the steps, please check >> `src/jdk.incubator.vector/linux/native/libvectormath/README`), rather than >> depends on external sleef things (header or lib) at build or run time. >> Besides of this change, also modify the previous changes accordingly, e.g. >> remove some uncessary files or changes especially in make dir of jdk. >> >> Besides of the code changes, one important task is to handle the legal >> process. >> >> Thanks! >> >> ## Test >> tests: >> * test/jdk/jdk/incubator/vector/ >> * test/hotspot/jtreg/compiler/vectorapi/ >> >> options: >> * -XX:UseSVE=1 -XX:+EnableVectorSupport -XX:+UseVectorStubs >> * -XX:UseSVE=0 -XX:+EnableVectorSupport -XX:+UseVectorStubs >> * -XX:+EnableVectorSupport -XX:-UseVectorStubs >> >> ## Performance >> >> ### Options >> * +intrinsic: >> 'FORK=1;ITER=10;WARMUP_ITER=10;JAVA_OPTIONS=-XX:+UnlockExperimentalVMOptions >> -XX:+EnableVectorSupport -XX:+UseVectorStubs' >> * -intrinsic: >> 'FORK=1;ITER=10;WARMUP_ITER=10;JAVA_OPTIONS=-XX:+UnlockExperimentalVMOptions >> -XX:+EnableVectorSupport -XX:-UseVectorStubs' >> >> ### Float >> data >> <google-sheets-html-origin style="caret-color: rgb(0, 0, 0); color: rgb(0, >> 0, 0); font-style: normal; font-variant-caps: normal; font-weight: 400; >> letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; >> text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; >> -webkit-text-stroke-width: 0px; text-decoration: none;"> >> Benchmark | (size) | Mode | Cnt | Error | Units | Score +intrinsic >> (UseSVE=1) | Score -intrinsic | Improvement(UseSVE=1) | Score +intrinsic >> (UseSVE=0) | Score -intrinsic | Improvement (UseSVE=0) >> -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- >> Float128Vector.ACOS | 1024 | thrpt | 10 | 0.015 | ops/ms | 245.439 | 101.483 >> | 2.419 | 245.733 | 102.033 | 2.408 >> Float128Vector.ASIN | 1024 | thrpt | 10 | 0.013 | ops/ms | 296.702 | 103.559 >> | 2.865 | 296.741 | 103.18 | 2.876 >> Float128Vector.ATAN | 1024 | thrpt | 10 | 0.004 | ops/ms | 196.862 | 49.627 >> | 3.967 | 195.891 | 49.771 | 3.936 >> Float128Vector.ATAN... > > Hamlin Li has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 33 commits: > > - Merge branch 'master' into sleef-aarch64-integrate-source > - merge master > - sleef 3.6.1 for riscv > - sleef 3.6.1 > - update header files for arm > - add inline header file for riscv64 > - remove notes about sleef changes > - fix performance issue > - disable unused-function warnings; add log msg > - minor > - ... and 23 more: https://git.openjdk.org/jdk/compare/2f4f6cc3...b54fc863
> While I agree with you in principle, we chose to import Sleef this way for > practical reasons. (The actual importing of Sleef is happening in #19185 / > [JDK-8329816](https://bugs.openjdk.org/browse/JDK-8329816).) The > "preprocessing/code-generation" part of the Sleef build was considered too > complex to reasonably replicate in the OpenJDK build system. Sleef is built > using Cmake and we do not want to add a build dependency on Cmake and call > out to a foreign build system at build time, for efficiency and complexity > reasons. Of course, there is no reason to rebuild the preprocessed headers every time we build the JDK. I'd never ask for that; the last thing I want is to make building the JDK slower. However, it should be possible to do so on a checked-out JDK source tree, at the builder's option. If there is a script, it doesn't have to be included in the OpenJDK build system itself, but it does have to be in the OpenJDK source tree. (It could be part of make/devkit, for example.) With a script to produce preprocessed files, it should be possible for anyone building the JDK to run that script, and produce the preprocessed source. SLEEF won't take up a prohibitive amount of space. We shouldn't be depending on some other web site somewhere being able to come up with the exact SLEEF sources we used, either. That fails the test of reproducibility. > JDK-8329816 comes with a script to automatically generate the imported source > files, to make it easy to update Sleef in the future. It should also be easy > enough to verify the imported contents using the same script for anyone who > wants to check the validity of the import step. I get it, but not including everything we use in the OpenJDK tree is a dangerous precedent. It should be no big deal to do this right, given that we have the SLEEF sources and the build scripts already. I'm not asking for anything that doesn't exist already, I'm just saying that it must be checked in. Avoiding inconvenience, however great, is not sufficient to justify such a step. This is perhaps something to discuss at the next Committers' Workshop. ------------- PR Comment: https://git.openjdk.org/jdk/pull/18605#issuecomment-2214663777
