8329816: Add SLEEF version 3.6

[vidmik]

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 to move forward.

---

Progress

• [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue

Issue

• JDK-8329816: Add SLEEF version 3.6 (Enhancement - P4)

Reviewers

• Erik Joelsson (@erikj79 - Reviewer)
• Hamlin Li (@Hamlin-Li - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19185/head:pull/19185
$ git checkout pull/19185

Update a local copy of the PR:
$ git checkout pull/19185
$ git pull https://git.openjdk.org/jdk.git pull/19185/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19185

View PR using the GUI difftool:
$ git pr show -t 19185

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19185.diff

Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back mikael! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk[bot]]

@vidmik This change is no longer ready for integration - check the PR body for details.

[openjdk[bot]]

@vidmik The following label will be automatically applied to this pull request:

• build

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge[bot]]

Webrevs

• 02: Full - Incremental (8a08ffa1)
• 01: Full - Incremental (b29de559)
• 00: Full (b02efa6b)

[Hamlin-Li]

Thanks for working on this. Just one question, currently in sleefinline_{advsimd,sve}.h there is #define SLEEF_ALWAYS_INLINE inline, but what expected is something like #define SLEEF_ALWAYS_INLINE inline __attribute__((always_inline)) (please check https://github.com/shibatch/sleef/pull/537 for details). In the future, when we change it, do we need to go through the legal process again? I guess we don't need it, but just double check it.

[vidmik]

Looks like that change is not yet in a released version of SLEEF, and in particular not in 3.6.

We do need updated approvals when we pick up new versions. Since we've gone through the process once it's typically easier/faster to do so. It will be technically easier/faster as well, now that I know how to do it and have encoded it in the createSleef.sh script.

[Hamlin-Li]

Looks like that change is not yet in a released version of SLEEF, and in particular not in 3.6.

We do need updated approvals when we pick up new versions. Since we've gone through the process once it's typically easier/faster to do so. It will be technically easier/faster as well, now that I know how to do it and have encoded it in the createSleef.sh script.

Thanks, I see. As the version 3.6 will introduce some performance regression compared to non-intrinsic version, so to bring the fix into jdk, we need either do a manual change (like https://github.com/openjdk/jdk/pull/18605/commits/cd70f5a970514938d05f7a2426b15f78245236d3), or wait the next version after 3.6 (which should include changes in https://github.com/shibatch/sleef/pull/537), I think we prefer the latter (i.e. depends on next version after 3.6) That means https://github.com/openjdk/jdk/pull/18605 (JDK-8312425) requires sleef version next to 3.6 (could be 3.6.1 or 3.7).

I'm OK to push this pr first, if it's also convenient for you, as we will need to push the change between 3.6.1/3.7 and 3.6 again when 3.6.1/3.7 is released.

[vidmik]

Let me suggest that we wait for the next release of SLEEF to be released in that case since that release seems to be imminent. That way we'll get both the performance fixes and we can include the RISC-V header files at the same time. Fair?

[Hamlin-Li]

Let me suggest that we wait for the next release of SLEEF to be released in that case since that release seems to be imminent. That way we'll get both the performance fixes and we can include the RISC-V header files at the same time. Fair?

Yes, it makes sense to me. Thanks!

[vidmik]

Thank you Hamlin. I'll try to keep my eyes open for the announcement of the upcoming SLEEF release but do feel free to notify me if you see it first!

[Hamlin-Li]

Thank you Hamlin. I'll try to keep my eyes open for the announcement of the upcoming SLEEF release but do feel free to notify me if you see it first!

Thank you @vidmik , sure, I will do it.

[vidmik]

I, too, envision that we'll be importing header files (only). That said, I'd very much prefer not to rename them as part of the import. If anything I could see us have architecture specific directories in which we place the respective files (and a common directory for misc.h), but it's not immediately clear to me that it's worth it given that the files are used in such a narrow context in the JDK.

[magicus]

I understand that you want to avoid renaming files, if they are imported. That is a good point. Moving them to an arch subdirectory does not seem like much additional hassle (there's apparently still a lot of manual work involved in upgrading the source from the third party upstream), and might help readers that are not deeply familiar with these platforms. But then again, if we only talk about header files, it is not strictly needed, so if you don't want to do it, then skip it.

[Hamlin-Li]

Hi @vidmik, Sleef 3.6.1 was just released, https://github.com/shibatch/sleef/releases/tag/3.6.1, which includes the fixes https://github.com/shibatch/sleef/pull/536, https://github.com/shibatch/sleef/pull/537 which fixed the performance issue.

[Hamlin-Li]

In case you need it and avoid to generate yourself, I've generated sleef inline header of 3.6.1 for riscv, it's at https://github.com/openjdk/jdk/pull/18605/commits/c279a3c290554892939267d9ebe88198988d31a4

[vidmik]

@Hamlin-Li I have generated the header files (two aarch64 and the new one for riscv64) using SLEEF 3.6.1. Please make sure they match your expectations!

[Hamlin-Li]

@vidmik Thanks for updating, I think I'd better to verify it in case we need uncessary further changes. Will update when I finish.

[Hamlin-Li]

@vidmik I have verified the tests and performance for aarch64, it's good as before, I also updated related information in https://github.com/openjdk/jdk/pull/18605. Although have some issue for riscv64, but I think it's some env issues rather than issue in sleefinline_rvvm1.h. So, I think we are good to go with this pr. Thanks a lot for the work!

[Hamlin-Li]

An quick update, I just verified via QEMU that sleefinline_rvvm1.h works well too.

[bridgekeeper[bot]]

@vidmik This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[Hamlin-Li]

Hi @vidmik @theRealAph , Just a kindly reminder, could you please bring up the topic (about integrating 3rd party source into jdk, mentioned in 1, 2) on workshop or summit? Thanks!

[erikj79]

Responding to the discussion in https://github.com/openjdk/jdk/pull/18605 here as this is the PR actually adding SLEEF to the JDK source.

I was initially of the opinion that the solution already provided here was enough. We could potentially have added a Git hash in addition to the version/tag to more precisely and permanently identify the exact Sleef source we are depending on. At least a Git hash wouldn't change externally.

However, Andrew's arguments have swayed me. I now think we should add a more or less complete dump of the Sleef source into the JDK repository. I'm still open to trimming it down somewhat as long as the build steps we need to run to generate the headers we need, using the Sleef build system, are still functional. I'm basically agreeing with his suggestion but will spell it out in detail here for completeness in this PR.

We should then add a script that automatically performs the necessary build steps, using the Sleef official build system, to generate the headers we need, and outputs them into the JDK source tree, in the location where we will also commit those headers. This script should document what dependencies and configuration is necessary to run it, which will include cmake and possibly other things. Performing this step doesn't need to be completely streamlined, just reasonably possible to run. It's meant to be an import/verification step. With this solution I would recommend putting the script next to the Sleef source tree instead of in make/devkit/.

The normal JDK build will just use the committed pre-generated headers.

I looked briefly at the heroic build work provided by @fitzsim and while I admire the effort, I don't think this is the right way and we already dismissed this approach earlier. Not because I didn't think it was feasible to implement, but because of the future maintenance burden. The Sleef build is non trivial so we shouldn't try to replicate it in our build. The risk of our implementation diverging in the future is too great.

[Hamlin-Li]

With this solution I would recommend putting the script next to the Sleef source tree instead of in make/devkit/.

The normal JDK build will just use the committed pre-generated headers.

Not because I didn't think it was feasible to implement, but because of the future maintenance burden.

I agree with @erikj79, in particular the above 3 points.

Shall we be ready to move forward? Or we're still blocked by some other issues, such as legal process?

[theRealAph]

Responding to the discussion in #18605 here as this is the PR actually adding SLEEF to the JDK source.

I was initially of the opinion that the solution already provided here was enough. We could potentially have added a Git hash in addition to the version/tag to more precisely and permanently identify the exact Sleef source we are depending on. At least a Git hash wouldn't change externally.

However, Andrew's arguments have swayed me. I now think we should add a more or less complete dump of the Sleef source into the JDK repository. I'm still open to trimming it down somewhat as long as the build steps we need to run to generate the headers we need, using the Sleef build system, are still functional. I'm basically agreeing with his suggestion but will spell it out in detail here for completeness in this PR.

Sounds good, thanks.

We should then add a script that automatically performs the necessary build steps, using the Sleef official build system, to generate the headers we need, and outputs them into the JDK source tree, in the location where we will also commit those headers. This script should document what dependencies and configuration is necessary to run it, which will include cmake and possibly other things. Performing this step doesn't need to be completely streamlined, just reasonably possible to run. It's meant to be an import/verification step. With this solution I would recommend putting the script next to the Sleef source tree instead of in make/devkit/.

The normal JDK build will just use the committed pre-generated headers.

I looked briefly at the heroic build work provided by @fitzsim and while I admire the effort, I don't think this is the right way and we already dismissed this approach earlier. Not because I didn't think it was feasible to implement, but because of the future maintenance burden. The Sleef build is non trivial so we shouldn't try to replicate it in our build. The risk of our implementation diverging in the future is too great.

OK.

I had a brief meeting with Mark Reinhold to discuss this issue. I asked him if it would ever be acceptable to check in preprocessed code without the corresponding preferred from, and the immediate answer was an emphatic "no". So that's that.

I'm neutral on just how much of SLEEF should be checked into our source tree. As long as we include the full source for whatever subset of SLEEF we use, and it is the real source in its preferred form, I'm happy.

[vidmik]

I'm following up on the necessary logistics given the new plan.

[vidmik]

@Hamlin-Li We now have the necessary approvals in place for the new plan to include all of SLEEF (and the pre-generated header files) in the JDK. I (or somebody else from Oracle) will need to be the one committing/contributing the actual SLEEF code itself in the end. Can you take on the work of implementing the actual plan/logic and make sure it's all effectively ready for integration?

[Hamlin-Li]

@vidmik Thanks for pushing forward this work. Sure, I can take the work.

Just several questions, Sorry, as this is the first time I do this kind of work in jdk. In this pr, we already have the pre-generated sleef files and the scripts to generate it, what we are missing is the sleef original file (in my understanding, we also need to checkin them), besides of sleef original files, do we need any other files or document? In particular, do you mind to clarify further about plan/logic mentioned above?

[vidmik]

@Hamlin-Li Thank you for taking it on!

Part of the work is figuring out exactly what exactly we want - see the comments from @erikj79 and @theRealAph in this PR for inspiration. In particular, we'll want to include all or part of the SLEEF sources in the JDK, but we'll likely also want some additional build scripts and documentation to make it easy for developers to reproduce and update going forward. This could probably be based on the README and createSleef script.

[Hamlin-Li]

@vidmik Thanks for clarifying, I'll figure it out.

[magicus]

I think this will need some discussion to get it right, both from a a) source layout/organization, and b) build system perspective.

We're doing something that is quite new to the JDK source code base and build system, after all.

Some questions that needs to be answered:

• Do we store the full source code "off tree", e.g. like in src/utils/spleef, just for reference? And then store the generated files that are actually compiled in the "proper" place, like src/java.base/native/libspleef. Doing it this way will simplify the build. It will possibly also simplify for developers, since they can look at the actual code that is compiled and used.

• Or do we store the complete code base at e.g. src/java.base/native/libspleef, but filter out that from the build, and instead have the generated files in something like src/java.base/native/libspleef/preprocessed-src? I think that sounds like a bad idea, in general. It would be confusing, harder to implement in the build, and overall not generally helpful for the understanding of the code base.

• Or do we only store the complete code base at e.g. src/java.base/native/libspleef, and generate the preprocessed files at build time, every time we build? That would in effect mean we incorporate the libsleef build in our system, including all their dependencies. The advantage is that it would make the source code placement logical, but it would be a hard hit on how we build the product.

• If we chose to check in pre-processed sources, where should the logic for doing the pre-processing reside? As part of configure, make, or a separate script? My spontaneous reaction is to have it as part of the build system, like make update-spleef-source, but there might be more to it. For checking requirements, configure is the general go-to place. I am not a fan of the idea of having a specially hacked shell script dropped down in the middle of the spleef sources. :-(