valhalla
valhalla copied to clipboard
8336406: Add support for FP16 binary operations
This patch adds middle end support in C2 for a few FP16 binary operations, namely - subtract, multiply, divide, min and max. It also adds aarch64 backend support for these operations.
Tested JTREG tests - hotspot/jtreg/compiler/vectorization/TestFloat16VectorOps.java, hotspot/jtreg/compiler/vectorization/TestFloat16VectorReinterpretConv.java, hotspot/jtreg/compiler/intrinsics/float16 and test/jdk/java/lang/Float16 and they successfully pass on aarch64 and x86 machines.
Progress
- [x] Change must not contain extraneous whitespace
Issue
- JDK-8336406: Add support for FP16 binary operations (Enhancement - P4)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1175/head:pull/1175
$ git checkout pull/1175
Update a local copy of the PR:
$ git checkout pull/1175
$ git pull https://git.openjdk.org/valhalla.git pull/1175/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1175
View PR using the GUI difftool:
$ git pr show -t 1175
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1175.diff
Webrev
:wave: Welcome back bkilambi! A progress list of the required criteria for merging this PR into lworld+fp16 will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
@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:
8336406: Add support for FP16 binary operations
Reviewed-by: jbhateja
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 no new commits pushed to the lworld+fp16 branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 (@jatin-bhateja) 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).
Hi @jatin-bhateja , can I please get some review for this patch? Thank you !
Hi @Bhavana-Kilambi ,
Your patch looks good to me, just one comment. Once it gets integrated I will add x86 backend support for newly supported intrinsic. Best Regards, Jatin
Please address the closing comments and integrate.
Patch looks good to me. Will add x86 backend support for newly added intrinsic.
Thank you for the review. /integrate
@Bhavana-Kilambi Your change (at version b58c73761e8106bf2fbd7b0370821289479cae97) is now ready to be sponsored by a Committer.
/sponsor
Going to push as commit ce32e8c9bb2e99b20d088b88819feac9da6125f5.
@jatin-bhateja @Bhavana-Kilambi Pushed as commit ce32e8c9bb2e99b20d088b88819feac9da6125f5.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.
Hi @jatin-bhateja, I just noticed that the IR tests for x *2 -> x + x already exist for float and double here - test/hotspot/jtreg/compiler/c2/irTests/TestMulNodeIdealization.java. Shall I update this test with the Half float Ideal tests and remove this file introduced in this patch - test/hotspot/jtreg/compiler/c2/irTests/MulHFNodeIdealizationTests.java?
I am thinking, we will have to pass --enable-preview flag for the whole file which is applicable even for float and double tests which may not need it and we might not be able to test these testcases in older JDKs. What do you suggest? Is it better to keep this test separate for half-float or integrate it with float and double tests?