-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF #16234
Conversation
Here is the performance improvement for JMH benchmarks [1] [2] after enabling libsleef for AArch64 NEON and SVE: NEON:
SVE 512-bit vector size:
[1] https://github.com/openjdk/panama-vector/blob/vectorIntrinsics/test/micro/org/openjdk/bench/jdk/incubator/vector/operation/FloatMaxVector.java#L1068 |
👋 Welcome back xgong! A progress list of the required criteria for merging this PR into |
@XiaohongGong The following label will be automatically applied to this pull request:
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. |
Webrevs
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. As far as I can tell the choice you've made of accuracy matches what we need to meet the spec.
I'm very nervous about binding ourselves to a specific version of the SLEEF ABI, because Java releases are maintained for decades, and we don't want to be dependent on other projects.
We'll have to make a plan for version evolution.
Thanks! I agree that this is a somewhat short term close-the-gap solution, and bundling a good library makes more sense. Also, as John mentioned, implementing those math APIs using Vector API itself, sounds very desirable in the future. |
Hi @theRealAph , The latest commit created a native library as a bridge to the third-party sleef library. Could you please help check whether it's a better solution to fix the hard-coding sleef ABI version issue and the further evolution? It re-defines all the vector math functions and implements them by calling the relative functions in libsleef. The library is bundled into the jdk image. With this way, we doesn't need to hard-code the libsleef ABI version into jdk. And the potential issue caused by the future ABI updating may be catched earlier. Meanwhile, the original added VM option (i.e. Thanks, |
Hi,
That looks rather nice.
That sounds good. We're still rather vulnerable if sleef changes its ABI, but I don't suppose that will happen, and we can deal with it if it does. |
/label add build |
@XiaohongGong |
/label add panama-dev |
@dholmes-ora
|
/label add core-libs Really this should go to panama-dev but core-libs will have to do. |
@dholmes-ora |
So to be clear, now you have to opt-in to using |
Thanks for adding the labels @dholmes-ora !
Yes, libsleef is used to build the binary if found. And at runtime, if the libsleef with right version is not found, |
So you need to check both the flag and the header file? Oh well, then this is probably as good as it gets. |
Yes, we have to check both the flag and the header file. |
All the makefile changes we've discussed previously now look good. However, I just noticed the additional -f flag. Why are you not exporting the functions from source code instead? That is the way we normally do it in JDK libraries. In your case, it seems like you only need to add the export to the macro. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build changes finally look good. Great, actually! Thanks for persisting, despite the many rounds of review.
You will still need the 2 hotspot reviews for the hotspot part of the patch.
/reviewers 3
@XiaohongGong This change is no longer ready for integration - check the PR body for details. |
Thanks for the review and all the comments! |
I can add that we are interested to use that for Linux + RISC-V support given the RISC-V support was recently merged into sleef upstream. shibatch/sleef#477 |
@XiaohongGong 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! |
@XiaohongGong This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
@XiaohongGong What happened? Did you not intend to continue working to get this integrated? |
Yeah, I,m so sorry for that due to some uncontrollable changes to my work. I have to stop it. Maybe others from Arm will revisit this feature in future. Thanks for your time on this PR and all your comments!
…---- Replied Message ----
| From | Magnus Ihse ***@***.***> |
| Date | 02/06/2024 16:02 |
| To | openjdk/jdk ***@***.***> |
| Cc | XiaohongGong ***@***.***>,
Mention ***@***.***> |
| Subject | Re: [openjdk/jdk] 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF (PR #16234) |
@XiaohongGong What happened? Did you not intend to continue working to get this integrated?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Ooookay. I hope someone picks it up. I've spent quite a lot of time trying to get this PR into shape. |
@luhenry Maybe you are interested in helping with bringing this forward? I can assist with risc-v related fixes in the makefiles. I'd just hate to see all this work go to waste. |
We are planning to pick this up again later in the year, thanks for your help so far! |
Same here! |
I could also take a look at it by the end of this month if no one are going to do it. |
Just a quick update, I've rebased the code, and will continue the work soon. I've looked through the previous discussion, seems there is no blocking issues, if I misunderstood or miss some information, please feel free to let me know, also if you have any further comment. Thanks! |
Iirc, your assessment is right; the code should be ready for integration; I just don't know about the state of testing. |
Hi @magicus @theRealAph @dholmes-ora, |
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] #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
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16234/head:pull/16234
$ git checkout pull/16234
Update a local copy of the PR:
$ git checkout pull/16234
$ git pull https://git.openjdk.org/jdk.git pull/16234/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 16234
View PR using the GUI difftool:
$ git pr show -t 16234
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16234.diff
Webrev
Link to Webrev Comment