8329816: Add SLEEF version 3.6.1

[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

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

Issue

• JDK-8329816: Add SLEEF version 3.6.1 (Enhancement - P4) ⚠️ Issue is already resolved. Consider making this a "backport pull request" by setting the PR title to Backport <hash> with the hash of the original commit. See Backports.

Reviewers

• Erik Joelsson (@erikj79 - Reviewer) Review applies to 0c91d4b1
• 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]

👋 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]

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

[openjdk]

@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]

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 shibatch/sleef#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 cd70f5a), or wait the next version after 3.6 (which should include changes in shibatch/sleef#537), I think we prefer the latter (i.e. depends on next version after 3.6)
That means #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 shibatch/sleef#536, shibatch/sleef#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 c279a3c

[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 #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]

Just some minor comments in README, otherwise looks good.

[Hamlin-Li]

-DSLEEF_ENFORCE_SVE=ON can be removed.

[Hamlin-Li]

These lines can be removed now.

[Hamlin-Li]

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

[bridgekeeper]

@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 #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. :-(

[Hamlin-Li]

By understanding the previous discussion, there should be 2 parts of source code related to sleef integrated into jdk:
1]. original sleef source
2]. pre-processed sleef header files

Besides of above sources, there should be another part connecting above 2 parts, i.e.
3]. scripts to transfering from 1] to 2], and necessary documents, e.g. record which tag of sleef to use, full git hash.

So, for the source layout/organization, the previous conclusion is to have both 1] and 2], and 1] is just for verification and reproducing, general build of jdk only needs 2].

As to where to put various new files, I think current location for 2] in this pr is good, 1] could be put somewhere with minimum impact on current build system, src/utils/sleef suggested by @magicus is a good candidate, 3] should be put together with 1].
In summary, one solution could be as below:

${JDK_ROOT}/src
  |-- utils/
      |-- sleef-src/   # i.e. part 1]
           |-- ... # original sleef source, we could trim or not trim the src tree. 
      |-- sleef-misc/
          |-- createSleef.sh    # accept a sleef source root(i.e. part 1], or an external sleef repo location) as input, and generate sleef header files(i.e. part 2] )
          |-- sleef.md
          |-- ..  # other misc files, e.g. recording tag of sleef to use, full git hash.
  |-- jdk.incubator.vector/linux/native/libvectormath      # i.e. part 2]
      |-- README  # also including a reference pointing to src/utils/sleef*
      |-- misc.h
      |-- sleefinline_advsimd.h
      |-- sleefinline_rvvm1.h
      |-- sleefinline_sve.h

As for which part of original sleef source to integreate into jdk, we could trim the original source in some way, but I think it's more simple and maintainable to just put all sleef source into jdk without any trim.

[erikj79]

3]. scripts to transfering from 1] to 2], and necessary documents, e.g. record which tag of sleef to use, .

In addition to the tag, we should include the full git hash as well. A tag can be moved, but it's very hard to fake a new git hash for a commit if any changes are made.

[Hamlin-Li]

3]. scripts to transfering from 1] to 2], and necessary documents, e.g. record which tag of sleef to use, .

In addition to the tag, we should include the full git hash as well. A tag can be moved, but it's very hard to fake a new git hash for a commit if any changes are made.

I agree, thanks for reminding! I also updated my comment above.

[magicus]

After thinking a bit more on this, I would recommend a somewhat different layout:

${JDK_ROOT}/src
  |-- jdk.incubator.vector/linux/native/libsleef
      |-- README.md
      |-- upstream/ 
           |-- ... # original sleef source (trimmed or not)
      |-- generated/
           |-- misc.h
           |-- sleefinline_advsimd.h
           |-- sleefinline_rvvm1.h
           |-- sleefinline_sve.h
  |-- jdk.incubator.vector/linux/legal/libsleef.md

jdk.incubator.vector/linux/legal/libsleef.md needs to contain the libsleef
license; see other legal/*.md files for reference.

The README.md file should briefly but clearly explain how we handle libsleef
code in the JDK, explain how to update the generated files, and describe
from where the source was obtained and at what git tag+hash.

[magicus]

In the build system, we will then add jdk.incubator.vector/linux/native/libsleef/generated as an additional source root when building libvectormath.

[magicus]

Furthermore, despite what Erik said above, I would really, really like to not have a stand-alone shell script mixed in with the spleef sources. Instead, we should treat updating the generated spleef sources as any other build-related activities, that is, we should have a build target for that.

I imagine we would have a make update-spleef-source that calls into UpdateSpleefSource.gmk. This in turn will more or less call cmake with the proper arguments needed to build spleef, and copy the resulting files into the generated directory.

The prerequisites required for doing this, e.g. having cmake available (and possibly any other requirements?) should be checked by configure.

There are two slightly different nuances on how we handle these dependencies: We could either add a --enable-spleef-updating argument to configure, which will fail configure if some dependencies are missing, and if will be responsible for making the update-spleef-source target available. (This would have a typical true/false/auto setup, so the target will be available if you happen to have all requirements present.)

The other alternative, if the one above would be too much, is to just set the dependencies in configure if they are found, and then verify in UpdateSpleefSource.gmk that they are non-empty (and fail with an error otherwise). This is contrary to the general design of the build system, but might be acceptable in a special case like this, to gain a bit of simplicity in the code.

[magicus]

I can help you with the build system changes, if what I wrote above made no sense to you.

[erikj79]

Furthermore, despite what Erik said above, I would really, really like to not have a stand-alone shell script mixed in with the spleef sources. Instead, we should treat updating the generated spleef sources as any other build-related activities, that is, we should have a build target for that.

I'm ok with this as you are offering to help implement it. My suggestion for using a separate script was out of simplicity and an assumed lack of resourcing for a more involved solution. Your suggested solution would generally be a better end result.

[Hamlin-Li]

${JDK_ROOT}/src
  |-- jdk.incubator.vector/linux/native/libsleef
      |-- README.md
      |-- upstream/ 
           |-- ... # original sleef source (trimmed or not)
      |-- generated/
           |-- misc.h
           |-- sleefinline_advsimd.h
           |-- sleefinline_rvvm1.h
           |-- sleefinline_sve.h
  |-- jdk.incubator.vector/linux/legal/libsleef.md

Thanks, this is much better.

Furthermore, despite what Erik said above, I would really, really like to not have a stand-alone shell script mixed in with the spleef sources. Instead, we should treat updating the generated spleef sources as any other build-related activities, that is, we should have a build target for that.
...
There are two slightly different nuances on how we handle these dependencies.

I'm ok too with either of solutions.
Only reminding is that we should treat sleef build/source-generation as a blackbox.

[magicus]

@vidmik @Hamlin-Li I kind of hijacked this...

I created the necessary build system changes to update the generated sleef code. At that point, also importing the actual source code was just a trivial addition.

The new PR is here: #20781

I think it implements everything we've talked about here, in the way I understood consensus to be.

[openjdk]

@vidmik this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8329816-sleef
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push