jdk
jdk copied to clipboard
8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF
Hi, Can you help to review this patch? Thanks
This is a continuation of work based on [1] by @XiaohongGong, most work was done in that pr. In this new pr, just rebased the code in [1], then added some minor changes (renaming of calling method, add libsleef as extra lib in CI cross-build on aarch64 in github workflow); I aslo tested the combination of following scenarios:
- at build time
- with/without sleef
- with/without sve support
- at runtime
- with/without sleef
- with/without sve support
[1] https://github.com/openjdk/jdk/pull/16234
Regression Test
- test/jdk/jdk/incubator/vector/
- test/hotspot/jtreg/compiler/vectorapi/
Performance Test
Previously, @XiaohongGong has shared the data: https://github.com/openjdk/jdk/pull/16234#issuecomment-1767727028
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-8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF (Enhancement - P4)
Reviewers
- Magnus Ihse Bursie (@magicus - Reviewer)
Contributors
- Hamlin Li
<[email protected]>
- Xiaohong Gong
<[email protected]>
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18294/head:pull/18294
$ git checkout pull/18294
Update a local copy of the PR:
$ git checkout pull/18294
$ git pull https://git.openjdk.org/jdk.git pull/18294/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18294
View PR using the GUI difftool:
$ git pr show -t 18294
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18294.diff
Webrev
:wave: Welcome back mli! 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.
@Hamlin-Li This change now passes all automated pre-integration checks.
ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.
After integration, the commit message for the final commit will be:
8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF
Co-authored-by: Hamlin Li <[email protected]>
Co-authored-by: Xiaohong Gong <[email protected]>
Reviewed-by: ihse
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.
At the time when this comment was updated there had been 151 new commits pushed to the master
branch:
- c342188fd978dd94e7788fb0fb0345fd8c0eaa9a: 8328074: Add jcheck whitespace checking for assembly files
- 3c70f26b2f3fa9bc143e2506af30f9b1daf20022: 8328112: Remove CardTable::_guard_region
- 48717d63cc58f693f0917e61eafd672cd6af02ed: 8326333: jshell <TAB> completion on arrays is incomplete
- ece4124f25f676da9bf2d1b7fd8e4394dd7d31af: 8328247: Remove redundant dir for tests converted from applet to main
- d32ce65781c1d7815a69ceac720cdf3ae39caa9e: 8327651: Rename DictionaryEntry members related to protection domain
- 07194195cefc568048fa639b6f8534ce3718c8d2: 8328236: module_entry in CDS map file has stale value
- 0204aacb0305e94a7d6d5299a5ae835f3f71b030: 8328115: Convert java/awt/font/TextLayout/TestJustification.html applet test to main
- 9bc1b065db238b7c9d0562f9bd55d2f338c6ff3d: 8328242: Add a log area to the PassFailJFrame
- 65a84c2642822862fa186f290f8d6e83537bea06: 8328006: refactor large anonymous inner class in HtmlDocletWriter
- 044f4ed55dfce7f1aed9e10accf459b4af9b975e: 8326979: (jdeps) improve the error message for FindException caused by InvalidModuleDescriptorException
- ... and 141 more: https://git.openjdk.org/jdk/compare/de428daf9adef5afe7347319f7a6f6732e9b6c4b...master
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
➡️ To integrate this PR with the above commit message to the master
branch, type /integrate in a new comment.
@Hamlin-Li The following labels will be automatically applied to this pull request:
-
build
-
hotspot
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.
/author set @XiaohongGong
@Hamlin-Li
Setting overriding author to Xiaohong Gong <[email protected]>
. When this pull request is integrated, the overriding author will be used in the commit.
/contributor add @Hamlin-Li
@Hamlin-Li
Contributor Hamlin Li <[email protected]>
successfully added.
Webrevs
- 03: Full - Incremental (d0ed0323)
- 02: Full - Incremental (7b7ec312)
- 01: Full - Incremental (668d08ce)
- 00: Full (55b118d9)
/contributor add @XiaohongGong
@Hamlin-Li
Contributor Xiaohong Gong <[email protected]>
successfully added.
Hi, thanks for continuing with this.
As this is similar to SVML, comments applies to x86 also:
-
There is no way to stop the VM from trying to load vmath ? There is both a UseNeon and a UseSVE, if I set both to false I would expect no vector and no vmath. The issue with UseNeon not really guarding neon code, but just crc, seems like a pre-existing bug. A flag like 'UseNeon' should turn it on/off :)
-
Doing open dll form stubrountines seems wrong.
@dholmes-ora could I get your opinion here?
* at build time * with/without sleef * with/without sve support
What is the relevance of SVE support at build time? Should it matter what the build machine is?
Its important to realize that almost no one except the JDK devs builds their own JDK, and having SLEEF dependencies at build time will mean that almost no one will use it. All this work you've done will be for nothing.
Hi, thanks for continuing with this.
As this is similar to SVML, comments applies to x86 also:
* There is no way to stop the VM from trying to load vmath ? There is both a UseNeon and a UseSVE, if I set both to false I would expect no vector and no vmath.
The JVM won't work without Neon. It's always there. Better to delete the flag.
I'm running the tests, but I have no way to confirm that SLEEF or SVE is in use. Exactly what did you do, and how did you confirm it?
Hi, thanks for continuing with this.
Thanks for the comments
As this is similar to SVML, comments applies to x86 also:
- There is no way to stop the VM from trying to load vmath ?
No official way, but deleting libvmath.so will have a same effect. I'm not sure if avoiding loading vmath is necessary for typical users, if it turns out to be true, we can add it later.
There is both a UseNeon and a UseSVE, if I set both to false I would expect no vector and no vmath. The issue with UseNeon not really guarding neon code, but just crc, seems like a pre-existing bug. A flag like 'UseNeon' should turn it on/off :)
I think Andrew asked this question, I agree it's better to remove the flag usage in crc32 on aarch64, better to be done in a separate pr.
- Doing open dll form stubrountines seems wrong.
I agree, logically it brings more clear code structure. But as this will be common change (e.g. on both x86 and aarch64) in the stub initialization steps, , and I think we'd better do it in a seprate pr.
* at build time * with/without sleef * with/without sve support
What is the relevance of SVE support at build time? Should it matter what the build machine is?
Its important to realize that almost no one except the JDK devs builds their own JDK, and having SLEEF dependencies at build time will mean that almost no one will use it. All this work you've done will be for nothing.
I agree. On the other hand, I see previously there was discussion about this already, and current solution got some points https://github.com/openjdk/jdk/pull/16234#issuecomment-1836266314, https://github.com/openjdk/jdk/pull/16234#issuecomment-1836449876
So, currently maybe we could go with this solution first, and as an incremental optimization, we could introduce a dynamic loading without dependency on the bridge furnctions in libvmath at runtime (pr). How do you think about it?
Hi, thanks for continuing with this. As this is similar to SVML, comments applies to x86 also:
* There is no way to stop the VM from trying to load vmath ? There is both a UseNeon and a UseSVE, if I set both to false I would expect no vector and no vmath.
The JVM won't work without Neon. It's always there. Better to delete the flag.
Sure, will track this in a seperate pr.
I'm running the tests, but I have no way to confirm that SLEEF or SVE is in use. Exactly what did you do, and how did you confirm it?
Thanks for running test. I just turn on some log, and check the output.
What is the relevance of SVE support at build time? Should it matter what the build machine is? Its important to realize that almost no one except the JDK devs builds their own JDK, and having SLEEF dependencies at build time will mean that almost no one will use it. All this work you've done will be for nothing.
I agree. On the other hand, I see previously there was discussion about this already, and current solution got some points #16234 (comment), #16234 (comment)
So, currently maybe we could go with this solution first, and as an incremental optimization, we could introduce a dynamic loading without dependency on the bridge furnctions in libvmath at runtime (pr). How do you think about it?
If there was some kind of plan, with evidence of the intention to do something to get this valuable tech into people's hands in a form they can use, sure. But as you can tell, I think this may rot because no one will be able use it. If SLEEF were included in the JDK it'd be fine. if SLEEF were a common Linux library it'd be fine.
I'm running the tests, but I have no way to confirm that SLEEF or SVE is in use. Exactly what did you do, and how did you confirm it?
Thanks for running test. I just turn on some log, and check the output.
The problem I see is that J. Random Java User has no way to know if SLEEF is making their program faster without running benchmarks. They'll put SLEEF somewhere and hope that Java uses it.
If there was some kind of plan, with evidence of the intention to do something to get this valuable tech into people's hands in a form they can use, sure. But as you can tell, I think this may rot because no one will be able use it. If SLEEF were included in the JDK it'd be fine. if SLEEF were a common Linux library it'd be fine.
The problem I see is that J. Random Java User has no way to know if SLEEF is making their program faster without running benchmarks. They'll put SLEEF somewhere and hope that Java uses it.
Please kindly correct me if I misunderstood your points. Seems the safest solution to address your above concerns is to integrate the sleef source into jdk? Lack of sleef at either build time or runtime will make the user's code fall back to java implementation.
The problem I see is that J. Random Java User has no way to know if SLEEF is making their program faster without running benchmarks. They'll put SLEEF somewhere and hope that Java uses it.
Please kindly correct me if I misunderstood your points. Seems the safest solution to address your above concerns is to integrate the sleef source into jdk? Lack of sleef at either build time or runtime will make the user's code fall back to java implementation.
Exactly, yes. That's why we've integrated the source code of many other libraries we depend on into the JDK. It's the only way to get the reliability our users expect.
- There is no way to stop the VM from trying to load vmath ?
No official way, but deleting libvmath.so will have a same effect. I'm not sure if avoiding loading vmath is necessary for typical users, if it turns out to be true, we can add it later.
In previous implementation of introducing SVML into jdk on x86, it introduced a option UseVectorStubs
which can be used to avoid using libvmath(either svml or sleef), i.e. user can fall back to java implementation if they want.
The licensing [1] seems to allow us [2] to copy/paste the source code into the JDK. We would only need to include the copyright and license, which are both no-brainers.
@theRealAph Are you saying that bundling the source code of libsleef is a hard requirement from your side to accept this code into the JDK?
I'm not against it, I just want to understand what we're talking about here.
In general, adding new libraries to OpenJDK will require a legal process in Oracle, which may (or may not) take some amount of time T, where T is larger than you'd wish for.
So I guess we can either:
- wait for libsleef source code to become a part of OpenJDK, and then integrate this PR.
- integrate this PR optimistically, and in the background start a process of trying to get libsleef into OpenJDK. (Which, of course, can not be 100% guaranteed to happen.)
- integrate this PR as is, and give up any idea of bundling libsleef.
I also believe there is a fourth option, but that too seems like it has legal implications that needs to be checked:
- if the libsleef dynamic library is found on the system during build time, bundle a copy of the dll with the built JDK. (Similar to how was done with freetype on Windows before.). And, optionally, provide an option for configure to require libsleef to be present, so the build fails if no libsleef can be found and bundled. (Leaving open as to if this should be default or not.)
@theRealAph Are you saying that bundling the source code of libsleef is a hard requirement from your side to accept this code into the JDK?
I'm not against it, I just want to understand what we're talking about here.
In general, adding new libraries to OpenJDK will require a legal process in Oracle, which may (or may not) take some amount of time T, where T is larger than you'd wish for.
So I guess we can either:
1. wait for libsleef source code to become a part of OpenJDK, and then integrate this PR. 2. integrate this PR optimistically, and in the background start a process of trying to get libsleef into OpenJDK. (Which, of course, can not be 100% guaranteed to happen.)
I think that's probably best the best thing to do today. I will approve it.
I'd prefer, long term, to integrate SLEEF into OpenJDK. That would make AArch64 support similar to Intel. We are competing with Intel.
In the short term, I'd build a shim library during the default standard JDK build that does not need SLEEF at build time. Unless weo do that, because SLEEF isn't on anyone's builders, it won't be used.
3. integrate this PR as is, and give up any idea of bundling libsleef.
I also believe there is a fourth option, but that too seems like it has legal implications that needs to be checked:
4. if the libsleef dynamic library is found on the system during build time, bundle a copy of the dll with the built JDK. (Similar to how was done with freetype on Windows before.). And, optionally, provide an option for configure to require libsleef to be present, so the build fails if no libsleef can be found and bundled. (Leaving open as to if this should be default or not.)
Ewww. No. :-)
@magicus @theRealAph Thanks for discussion.
I think that's probably best the best thing to do today. I will approve it.
Great, Thanks.
I'd prefer, long term, to integrate SLEEF into OpenJDK. That would make AArch64 support similar to Intel. We are competing with Intel.
I'm working on it, running some tests. But I think as @magicus pointed out, to achieve this target one of most important tasks is to get the legal process done.
In the short term, I'd build a shim library during the default standard JDK build that does not need SLEEF at build time. Unless weo do that, because SLEEF isn't on anyone's builders, it won't be used.
I agree it's give user more chance to leverage the sleef when running, but I wonder if it's necessary to do that. As we have a long term solution, and the chance that end user lacks sleef library in their environment is much higher than release engineer lacks sleef library in their environment. But it's not harm to do so.
@magicus @theRealAph Thanks for discussion.
In the short term, I'd build a shim library during the default standard JDK build that does not need SLEEF at build time. Unless weo do that, because SLEEF isn't on anyone's builders, it won't be used.
I agree it's give user more chance to leverage the sleef when running, but I wonder if it's necessary to do that. As we have a long term solution, and the chance that end user lacks sleef library in their environment is much higher than release engineer lacks sleef library in their environment.
That situation leads to my nightmare, in which some OpenJDK builds work with vector support, and some don't, and there's no way to find out which unless you try it, in which case there is no warning, your programs just run slowly. Even if you've installed SLEEF. That would be bad for our users, bad for OpenJDK, and bad for Java.
But it's not harm to do so.
What is the relevance of SVE support at build time? Should it matter what the build machine is?
My understanding is that we need a compiler that supports -march=armv8-a+sve
; otherwise we can't build the redirect library properly. But maybe that is a misunderstanding?
What is the relevance of SVE support at build time? Should it matter what the build machine is?
My understanding is that we need a compiler that supports
-march=armv8-a+sve
; otherwise we can't build the redirect library properly. But maybe that is a misunderstanding?
Yes, it should be in gcc 8 and above, and we seem to require gcc 10.