Skip to content
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

8264352: AArch64: Optimize vector "not/andNot" for NEON and SVE #3370

Closed
wants to merge 1 commit into from

Conversation

@XiaohongGong
Copy link
Contributor

@XiaohongGong XiaohongGong commented Apr 7, 2021

Since the vector bitwise "andNot" is implemented with "v1.and(v2.xor(-1))", the generated codes with SVE look like:

  mov z16.b, #-1
  eor z17.d, z20.d, z16.d
  and z18.d, z18.d, z17.d

This could be improved with a single instruction:

  bic z16.d, z16.d, z18.d

Similarly, the following optimization for NEON is also needed:

  not v21.16b, v21.16b
  and v21.16b, v21.16b, v18.16b  ==>  bic v21.16b, v18.16b, v21.16b

This patch also adds the following optimization to vector "not" for SVE which has already been added for NEON:

  mov z16.b, #-1
  eor z17.d, z20.d, z16.d     ==>   not z17.d, p7/m, z20.d

The performance can improve about 16% ~ 36% with NEON for the "AND_NOT" benchmark [1].

[1] https://github.com/openjdk/panama-vector/blob/vectorIntrinsics/test/jdk/jdk/incubator/vector/benchmark/src/main/java/benchmark/jdk/incubator/vector/ByteMaxVector.java#L343

Tested tier1 and jdk:tier3.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8264352: AArch64: Optimize vector "not/andNot" for NEON and SVE

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3370/head:pull/3370
$ git checkout pull/3370

Update a local copy of the PR:
$ git checkout pull/3370
$ git pull https://git.openjdk.java.net/jdk pull/3370/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3370

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3370.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 7, 2021

👋 Welcome back xgong! 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.

Loading

@openjdk openjdk bot added the rfr label Apr 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

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

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 7, 2021

Webrevs

Loading

@theRealAph
Copy link
Contributor

@theRealAph theRealAph commented Apr 7, 2021

Looks OK. Is there any test code for this is mainline?

Loading

@XiaohongGong
Copy link
Contributor Author

@XiaohongGong XiaohongGong commented Apr 7, 2021

Looks OK. Is there any test code for this is mainline?

Hi @theRealAph , thanks for looking at this PR. Yes, there is the Vector API jtreg tests that have covered the opcode NOT/AND_NOT.
Please see the tests for byte vector: https://github.com/openjdk/jdk/blob/master/test/jdk/jdk/incubator/vector/ByteMaxVectorTests.java#L1708
and https://github.com/openjdk/jdk/blob/master/test/jdk/jdk/incubator/vector/ByteMaxVectorTests.java#L4602

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

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

8264352: AArch64: Optimize vector "not/andNot" for NEON and SVE

Reviewed-by: aph, njian

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

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 (@theRealAph, @nsjian) 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).

Loading

@openjdk openjdk bot added the ready label Apr 7, 2021
nsjian
nsjian approved these changes Apr 8, 2021
Copy link

@nsjian nsjian left a comment

Looks good.

Loading

@XiaohongGong
Copy link
Contributor Author

@XiaohongGong XiaohongGong commented Apr 8, 2021

Thanks for the review @theRealAph @nsjian !

Loading

@XiaohongGong
Copy link
Contributor Author

@XiaohongGong XiaohongGong commented Apr 8, 2021

/integrate

Loading

1 similar comment
@XiaohongGong
Copy link
Contributor Author

@XiaohongGong XiaohongGong commented Apr 8, 2021

/integrate

Loading

@openjdk openjdk bot added the sponsor label Apr 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 8, 2021

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

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 8, 2021

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

Loading

@nsjian
Copy link

@nsjian nsjian commented Apr 8, 2021

/sponsor

Loading

@openjdk openjdk bot closed this Apr 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 8, 2021

@nsjian @XiaohongGong Since your change was applied there have been 15 commits pushed to the master branch:

  • 016db40: 8263907: Specification of CellRendererPane::paintComponent(..Rectangle) should clearly mention which method it delegates the call to
  • 78d1164: 8214455: Relocate CDS archived regions to the top of the G1 heap
  • 88eb291: 8264809: test-lib fails to build due to some warnings in ASN1Formatter and jfr
  • a863ab6: 8264551: Unexpected warning when jpackage creates an exe
  • 6e2b82a: 8264731: Introduce InstanceKlass::method_at_itable_or_null()
  • 22b20f8: 8264424: Support OopStorage bulk allocation
  • ab3be72: 8264863: Update JCov version to support JDK 17
  • 774e5ae: 8264742: member variable _monitor of MonitorLocker is redundant
  • 7a99a98: 8262316: Reducing locks in RSA Blinding
  • d3fdd73: 8264173: [s390] Improve Hardware Feature Detection And Reporting
  • ... and 5 more: https://git.openjdk.java.net/jdk/compare/c3abdc9aadc734053dbcc43d5294d5f16a0b0ce3...master

Your commit was automatically rebased without conflicts.

Pushed as commit e89542f.

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

Loading

@XiaohongGong XiaohongGong deleted the JDK-8264352 branch Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants