8292587: AArch64: Support SVE fabd instruction #10011

shqking · 2022-08-25T01:52:41Z

Scalar and NEON fabd instructions were initially supported in
JDK-8256318. In this patch, we support SVE fabd instruction [1] and add
one Jtreg test case as well.

With this patch, two instructions fsub + fabs would be combined into
one single fabd instruction.

  fsub    z16.s, z16.s, z17.s
  fabs    z16.s, p7/m, z16.s

  -->

  fabd    z16.s, p7/m, z16.s, z17.s

In the initial evaluation of JMH case, i.e.
FloatingScalarVectorAbsDiff.java, we found the performance uplift done
by this optimization was easily hidden by the heavy memory load/store
instructions. To avoid that, we updated the JMH case a bit, adding one
more group of subtraction and Math.abs operations in the loop body.

Here shows the data with the new JMH case on one 256-bit SVE machine. We
can observe about 39% and 35% improvements for the two functions
respectively.

Benchmark                                             Before    After  Units
FloatingScalarVectorAbsDiff.testVectorAbsDiffDouble  260.468  160.965  ns/op
FloatingScalarVectorAbsDiff.testVectorAbsDiffFloat   133.963   87.292  ns/op

Jtreg testing: tier1~3 passed on one NEON-only machine and one 256-bit SVE machine.

[1] https://developer.arm.com/documentation/ddi0596/2021-12/SVE-Instructions/FABD--Floating-point-absolute-difference--predicated--

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-8292587: AArch64: Support SVE fabd instruction

Reviewers

Ningsheng Jian (@nsjian - Committer) ⚠️ Review applies to c6157252
Fei Gao (@fg1417 - Author)
Nick Gasson (@nick-arm - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10011/head:pull/10011
$ git checkout pull/10011

Update a local copy of the PR:
$ git checkout pull/10011
$ git pull https://git.openjdk.org/jdk pull/10011/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10011

View PR using the GUI difftool:
$ git pr show -t 10011

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10011.diff

Scalar and NEON fabd instructions were initially supported in JDK-8256318. In this patch, we support SVE fabd instruction [1] and add one Jtreg test case as well. With this patch, two instructions `fsub + fabs` would be combined into one single `fabd` instruction. ``` fsub z16.s, z16.s, z17.s fabs z16.s, p7/m, z16.s --> fabd z16.s, p7/m, z16.s, z17.s ``` In the initial evaluation of JMH case, i.e. FloatingScalarVectorAbsDiff.java, we found the performance uplift done by this optimization was easily hidden by the heavy memory load/store instructions. To avoid that, we updated the JMH case a bit, adding one more group of subtraction and Math.abs operations in the loop body. Here shows the data with the new JMH case on one 256-bit SVE machine. We can observe about 39% and 35% improvements for the two functions respectively. ``` Benchmark Before After Units FloatingScalarVectorAbsDiff.testVectorAbsDiffDouble 260.468 160.965 ns/op FloatingScalarVectorAbsDiff.testVectorAbsDiffFloat 133.963 87.292 ns/op ``` [1] https://developer.arm.com/documentation/ddi0596/2021-12/SVE-Instructions/FABD--Floating-point-absolute-difference--predicated--

bridgekeeper · 2022-08-25T01:53:52Z

👋 Welcome back haosun! 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 · 2022-08-25T01:56:08Z

@shqking 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.

mlbridge · 2022-08-25T01:59:55Z

Webrevs

01: Full - Incremental (38501195)
00: Full (c6157252)

shqking · 2022-09-05T00:47:13Z

Ping? Can anyone help to review this patch? Thanks.

nsjian

Looks good.

shqking · 2022-09-06T23:18:46Z

Hi @nick-arm, could you help to review this patch when you have spare time? Thanks in advance.

test/hotspot/jtreg/compiler/vectorapi/VectorAbsDiffTest.java

As pointed out by Faye Gao, the test results are not fully verified due to incorrect loop limits. Updated it. Reran the test and no regression.

TobiHartmann · 2022-09-09T09:54:03Z

I tested this in our CI. All tests passed.

openjdk · 2022-09-09T13:00:59Z

@shqking 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:

8292587: AArch64: Support SVE fabd instruction

Reviewed-by: njian, fgao, ngasson

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 210 new commits pushed to the master branch:

526eb54: 8293669: SA: Remove unnecssary "InstanceStackChunkKlass: InstanceStackChunkKlass" output when scanning heap
41ce658: 8292225: Rename ArchiveBuilder APIs related to source and buffered addresses
155b10a: 8293329: x86: Improve handling of constants in AES/GHASH stubs
d3f7e3b: 8293339: vm/jvmti/StopThread/stop001/stop00103 crashes with SIGSEGV in Continuation::is_continuation_mounted
524af94: 8283627: Outdated comment in MachineDescriptionTwosComplement.isLP64
cea409c: 8292738: JInternalFrame backgroundShadowBorder & foregroundShadowBorder line is longer in Mac Look and Feel
9ef6c09: 8287908: Use non-cloning reflection methods where acceptable
0c61bf1: 8293282: LoadLibraryUnloadTest.java fails with "Too few cleared WeakReferences"
91c9091: 8293343: sun/management/jmxremote/bootstrap/RmiSslNoKeyStoreTest.java failed with "Agent communication error: java.io.EOFException"
4c77bd3: 6529151: NullPointerException in swing.plaf.synth.SynthLookAndFeel$Handler
... and 200 more: https://git.openjdk.org/jdk/compare/76ee5495cd00f5546a5748051cc36965a8e936db...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 (@nsjian, @nick-arm) 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).

shqking · 2022-09-13T00:54:24Z

Thanks for your code reviews, @nick-arm @nsjian and @fg1417 .
Thanks for your testing, @TobiHartmann !

I don't think the GHA test failure is related to this patch.
Hence, I suppose we can integrate this PR now.

/integrate

openjdk · 2022-09-13T00:55:56Z

@shqking
Your change (at version 3850119) is now ready to be sponsored by a Committer.

nsjian · 2022-09-13T02:17:27Z

/sponsor

openjdk · 2022-09-13T02:19:12Z

Going to push as commit cbee0bc.
Since your change was applied there have been 211 commits pushed to the master branch:

68645eb: 8293566: RISC-V: Clean up push and pop registers
526eb54: 8293669: SA: Remove unnecssary "InstanceStackChunkKlass: InstanceStackChunkKlass" output when scanning heap
41ce658: 8292225: Rename ArchiveBuilder APIs related to source and buffered addresses
155b10a: 8293329: x86: Improve handling of constants in AES/GHASH stubs
d3f7e3b: 8293339: vm/jvmti/StopThread/stop001/stop00103 crashes with SIGSEGV in Continuation::is_continuation_mounted
524af94: 8283627: Outdated comment in MachineDescriptionTwosComplement.isLP64
cea409c: 8292738: JInternalFrame backgroundShadowBorder & foregroundShadowBorder line is longer in Mac Look and Feel
9ef6c09: 8287908: Use non-cloning reflection methods where acceptable
0c61bf1: 8293282: LoadLibraryUnloadTest.java fails with "Too few cleared WeakReferences"
91c9091: 8293343: sun/management/jmxremote/bootstrap/RmiSslNoKeyStoreTest.java failed with "Agent communication error: java.io.EOFException"
... and 201 more: https://git.openjdk.org/jdk/compare/76ee5495cd00f5546a5748051cc36965a8e936db...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-13T02:19:32Z

@nsjian @shqking Pushed as commit cbee0bc.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk bot added the rfr Pull request is ready for review label Aug 25, 2022

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Aug 25, 2022

nsjian approved these changes Sep 5, 2022

View reviewed changes

fg1417 reviewed Sep 7, 2022

View reviewed changes

test/hotspot/jtreg/compiler/vectorapi/VectorAbsDiffTest.java Outdated Show resolved Hide resolved

Update the loop limit in VectorAbsDiffTest.java

3850119

As pointed out by Faye Gao, the test results are not fully verified due to incorrect loop limits. Updated it. Reran the test and no regression.

fg1417 approved these changes Sep 8, 2022

View reviewed changes

nick-arm approved these changes Sep 9, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 9, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 13, 2022

openjdk bot added the integrated Pull request has been integrated label Sep 13, 2022

openjdk bot closed this Sep 13, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Sep 13, 2022

shqking deleted the 8292587-fabd branch September 13, 2022 05:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8292587: AArch64: Support SVE fabd instruction #10011

8292587: AArch64: Support SVE fabd instruction #10011

shqking commented Aug 25, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 25, 2022

openjdk bot commented Aug 25, 2022

mlbridge bot commented Aug 25, 2022 •

edited

Loading

shqking commented Sep 5, 2022

nsjian left a comment

shqking commented Sep 6, 2022

TobiHartmann commented Sep 9, 2022

openjdk bot commented Sep 9, 2022 •

edited

Loading

shqking commented Sep 13, 2022

openjdk bot commented Sep 13, 2022

nsjian commented Sep 13, 2022

openjdk bot commented Sep 13, 2022

openjdk bot commented Sep 13, 2022

8292587: AArch64: Support SVE fabd instruction #10011

8292587: AArch64: Support SVE fabd instruction #10011

Conversation

shqking commented Aug 25, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Aug 25, 2022

openjdk bot commented Aug 25, 2022

mlbridge bot commented Aug 25, 2022 • edited Loading

Webrevs

shqking commented Sep 5, 2022

nsjian left a comment

Choose a reason for hiding this comment

shqking commented Sep 6, 2022

TobiHartmann commented Sep 9, 2022

openjdk bot commented Sep 9, 2022 • edited Loading

shqking commented Sep 13, 2022

openjdk bot commented Sep 13, 2022

nsjian commented Sep 13, 2022

openjdk bot commented Sep 13, 2022

openjdk bot commented Sep 13, 2022

shqking commented Aug 25, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 25, 2022 •

edited

Loading

openjdk bot commented Sep 9, 2022 •

edited

Loading