jdk
jdk copied to clipboard
openjdk
8348868: AArch64: Add backend support for SelectFromTwoVector
This patch adds aarch64 backend support for SelectFromTwoVector operation which was recently introduced in VectorAPI.
It implements this operation using a two table vector lookup instruction - "tbl" which is available only in Neon and SVE2.
For 128-bit vector length : Neon tbl instruction is generated if UseSVE < 2 and SVE2 "tbl" instruction is generated if UseSVE == 2.
For > 128-bit vector length : Currently there are no machines which have vector length > 128-bit and support SVE2. For all those machines with vector length > 128-bit and UseSVE < 2, this operation is not supported. The inline expander for this operation would fail and lowered IR will be generated which is a mix of two rearrange and one blend operation.
This patch also adds a boolean "need_load_shuffle" in the inline expander for this operation to test if the platform requires VectorLoadShuffle operation to be generated. Without this, the lowering IR was not being generated on aarch64 and the performance was quite poor.
Performance numbers with this patch on a 128-bit, SVE2 supporting machine is shown below -
Benchmark (size) Mode Cnt Gain
SelectFromBenchmark.selectFromByteVector 1024 thrpt 9 1.43
SelectFromBenchmark.selectFromByteVector 2048 thrpt 9 1.48
SelectFromBenchmark.selectFromDoubleVector 1024 thrpt 9 68.55
SelectFromBenchmark.selectFromDoubleVector 2048 thrpt 9 72.07
SelectFromBenchmark.selectFromFloatVector 1024 thrpt 9 1.69
SelectFromBenchmark.selectFromFloatVector 2048 thrpt 9 1.52
SelectFromBenchmark.selectFromIntVector 1024 thrpt 9 1.50
SelectFromBenchmark.selectFromIntVector 2048 thrpt 9 1.52
SelectFromBenchmark.selectFromLongVector 1024 thrpt 9 85.38
SelectFromBenchmark.selectFromLongVector 2048 thrpt 9 80.93
SelectFromBenchmark.selectFromShortVector 1024 thrpt 9 1.48
SelectFromBenchmark.selectFromShortVector 2048 thrpt 9 1.49
Gain column refers to the ratio of thrpt between this patch and the master branch after applying changes in the inline expander.
Progress
- [ ] 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-8348868: AArch64: Add backend support for SelectFromTwoVector (Enhancement - P4)
Reviewers
- Xiaohong Gong (@XiaohongGong - Committer)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23570/head:pull/23570
$ git checkout pull/23570
Update a local copy of the PR:
$ git checkout pull/23570
$ git pull https://git.openjdk.org/jdk.git pull/23570/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23570
View PR using the GUI difftool:
$ git pr show -t 23570
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23570.diff
Using Webrev
:wave: Welcome back bkilambi! 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@Bhavana-Kilambi 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:
8348868: AArch64: Add backend support for SelectFromTwoVector
Co-authored-by: Jatin Bhateja <[email protected]>
Reviewed-by: haosun, aph, sviswanathan, xgong
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 87 new commits pushed to the master branch:
- 8ac4a88f3c5ad57824dd192cb3f0af5e71cbceeb: 8362237: IllegalArgumentException in the launcher when exception without stack trace is thrown
- 6c5804722b5b2064e0d6ade2180c3126d8f2dabc: 8364296: Set IntelJccErratumMitigation flag ergonomically
- 812bd8e94d22f9751651e28a2ef8affdf6a33220: 8364199: Enhance list of environment variables printed in hserr/hsinfo file
- ... and 84 more: https://git.openjdk.org/jdk/compare/518d5f4bbb78ae35db793d7fd15b3cd35c881664...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.
As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@XiaohongGong, @shqking, @theRealAph, @sviswa7) but any other Committer may sponsor as well.
➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).
@Bhavana-Kilambi The following label will be automatically applied to this pull request:
hotspot-compiler
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
- 17: Full - Incremental (3675bf34)
- 16: Full (f79d2f00)
- 15: Full - Incremental (1d553a94)
- 14: Full - Incremental (6c7266d7)
- 13: Full - Incremental (8fcab4f1)
- 12: Full - Incremental (34566e7d)
- 11: Full - Incremental (8025db0c)
- 10: Full - Incremental (cec6f148)
- 09: Full - Incremental (e86d55df)
- 08: Full (80a1f67f)
- 07: Full - Incremental (22e42de3)
- 06: Full - Incremental (f8978870)
- 05: Full - Incremental (234d40c7)
- 04: Full - Incremental (956518ec)
- 03: Full - Incremental (31973045)
- 02: Full - Incremental (aa9e53e1)
- 01: Full (70e88489)
- 00: Full (65bd521d)
Hi @XiaohongGong Thanks for your review comments :) I will get back soon with a new patchset addressing your comments.
Thanks for your review comments @shqking This commit added mid-end support for SelectFromTwoVector operation - https://github.com/openjdk/jdk/commit/709914fc92dd180c8f081ff70ef476554a04f4ce. It adds intrinsics for SelectFromTwoVector operation and on machines that do not support this operation, a lowering vector operation (VectorRearrange + VectorBlend combination) is generated.
On aarch64 after the above commit, we expect the lowering operations to be generated as we have support for both of these operations but in the inline expander for SelectFromTwoVector, it did not consider targets that do not need to generate VectorLoadShuffle node (like aarch64) for the Lowering operation - https://github.com/openjdk/jdk/blob/e1d0a9c832ef3e92faaed7f290ff56c0ed8a9d94/src/hotspot/share/opto/vectorIntrinsics.cpp#L2736. As a result, the compiler was not generating the VectorRearrange + VectorBlend operation on aarch64 as it is supposed to when SelectFromTwoVector is not supported. The default java impl was being executed which is too slow. So after my small change in vectorIntrinsics.cpp file, the Lowered vector operations are being correctly generated.
I felt it would be right to compare the numbers after the change I made in vectorIntrinsics.cpp file with this patch that adds support for SelectFromTwoVector so that we are comparing performance with (VectorRearrange + VectorBlend) vs SelectFromTwoVector rather than compare it with default java implementation. If we compare the performance of this patch with the master branch then the numbers you have shown are correct. Hope this explanation helps :)
Hello, I would not be able to respond to comments until the next couple months or so due to some urgent tasks at work. Until then, I'd move this PR to draft status so that it would not be closed due to lack of activity. Thank you for the review!
@Bhavana-Kilambi 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 JDK-8348868
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
@Bhavana-Kilambi This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@Bhavana-Kilambi The pull request is being re-evaluated and the inactivity timeout has been reset.
Good job @Bhavana-Kilambi ! Generally looks good to me. Just some minor issues that I have left the comments. Besides, could you please add some IR tests for this optimization? Thanks!
Hi @XiaohongGong , there are tests already for this operation under jdk/jdk/incubator/vector for all the types and sizes to verify the results. Did you mean IR tests for verifying if the correct backend match rule is being generated ?
Good job @Bhavana-Kilambi ! Generally looks good to me. Just some minor issues that I have left the comments. Besides, could you please add some IR tests for this optimization? Thanks!
Hi @XiaohongGong , there are tests already for this operation under
jdk/jdk/incubator/vectorfor all the types and sizes to verify the results. Did you mean IR tests for verifying if the correct backend match rule is being generated ?
Yes, I think adding an IR check tests for this operation will be better. I think checking the mid-end IR is enough.
Hi @theRealAph @XiaohongGong @shqking I have addressed your review comments. Please do review the new patch. Thanks a lot!
Hi @XiaohongGong @theRealAph @shqking Can I please ask for another round of review for the latest patch? Thanks in advance!
Hi @theRealAph I have refined my comments. Could I please get another review? Thanks!
@theRealAph The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).
Hi @jatin-bhateja could I ask for your review for the x86 part please? I also fixed a minor merge conflict in the latest merge which is related to x86.
Hi @sviswa7 there's some x86 code in this patch which I would like an x86 expert to review. Would you be able to take a look please? It's not a big change. Thanks!
Hi @theRealAph Would it be ok for me to integrate this patch now? Two reviewers have approved, however if you feel there needs to be another aarch64 specific reviewer please let me know. Thanks!
@Bhavana-Kilambi Your change (at version 3675bf34b29121b5265bf53f2257738cb4ee591e) is now ready to be sponsored by a Committer.
Going to push as commit 2ba8a06f0c0a598a6ca7f74e75bab4208e6fa689.
Since your change was applied there have been 87 commits pushed to the master branch:
- 8ac4a88f3c5ad57824dd192cb3f0af5e71cbceeb: 8362237: IllegalArgumentException in the launcher when exception without stack trace is thrown
- 6c5804722b5b2064e0d6ade2180c3126d8f2dabc: 8364296: Set IntelJccErratumMitigation flag ergonomically
- 812bd8e94d22f9751651e28a2ef8affdf6a33220: 8364199: Enhance list of environment variables printed in hserr/hsinfo file
- ... and 84 more: https://git.openjdk.org/jdk/compare/518d5f4bbb78ae35db793d7fd15b3cd35c881664...master
Your commit was automatically rebased without conflicts.
@jatin-bhateja @Bhavana-Kilambi Pushed as commit 2ba8a06f0c0a598a6ca7f74e75bab4208e6fa689.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.