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

8277426: Optimize mask reduction operations on x86 #6447

Closed
wants to merge 4 commits into from

Conversation

merykitty
Copy link
Contributor

@merykitty merykitty commented Nov 18, 2021

Hi,

This patch improves the performance of mask reduction operations on AVX by matching the pattern VectorMaskReduction (VectorStoreMask mask) to eliminate the extra VectorStoreMaskNode. I have also done some refactoring to unify the logic of toLong with the other reduction operations.

The patch has been discussed partially in panama-vector repository.

Thank you very much.


Progress

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

Issue

  • JDK-8277426: Optimize mask reduction operations on x86

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6447

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 18, 2021

👋 Welcome back merykitty! 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
Copy link

@openjdk openjdk bot commented Nov 18, 2021

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

@openjdk openjdk bot added the hotspot-compiler label Nov 18, 2021
@merykitty merykitty changed the title improve mask reduction logic on AVX 8277426: Optimize mask reduction operations on x86 Nov 19, 2021
@openjdk openjdk bot added the rfr label Nov 19, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 19, 2021

Webrevs

@sviswa7
Copy link

@sviswa7 sviswa7 commented Nov 22, 2021

@merykitty Thanks for contributing this optimization. The patch looks good to me.

@sviswa7
Copy link

@sviswa7 sviswa7 commented Nov 22, 2021

@PaulSandoz Could you please run this through Oracle testing.

@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented Nov 22, 2021

@merykitty does this PR still disable the operations on Neon?

Answering myself, i think not from looking at the changes in the panama repo version: openjdk/panama-vector@9b578a3

@sviswa7
Copy link

@sviswa7 sviswa7 commented Nov 23, 2021

@PaulSandoz The changes are specific to x86 code gen. Only x86 backend files are changed.

@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented Nov 23, 2021

@PaulSandoz Could you please run this through Oracle testing.

Tests passed.

@sviswa7
Copy link

@sviswa7 sviswa7 commented Nov 23, 2021

Thanks a lot @PaulSandoz for the testing.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 23, 2021

⚠️ @merykitty the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout vectorMaskReduction
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

@openjdk
Copy link

@openjdk openjdk bot commented Nov 23, 2021

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

8277426: Optimize mask reduction operations on x86

Reviewed-by: sviswanathan, jiefu

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 105 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 (@sviswa7, @DamonFool) 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).

@openjdk openjdk bot added the ready label Nov 23, 2021
@merykitty
Copy link
Contributor Author

@merykitty merykitty commented Nov 23, 2021

/integrate

@openjdk openjdk bot added the sponsor label Nov 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 23, 2021

@merykitty
Your change (at version 1dae02d) is now ready to be sponsored by a Committer.

@merykitty
Copy link
Contributor Author

@merykitty merykitty commented Nov 23, 2021

@PaulSandoz @sviswa7 Thank you very much for the reviews and testing. Could I get this PR sponsored, pleased?

@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented Nov 23, 2021

This needs another hotspot reviewer to review before integration.

@sviswa7
Copy link

@sviswa7 sviswa7 commented Nov 23, 2021

@DamonFool Could you please review this patch?

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Nov 23, 2021

@DamonFool Could you please review this patch?

I will do some testing and feedback.
Thanks.

Register tmp, int masklen, int masksize,
int vec_enc) {
int masklen, int masksize, int vec_enc) {
assert(VM_Version::supports_popcnt() &&
Copy link
Member

@DamonFool DamonFool Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New instructions like lzcntq and tzcntq are used for the optimized code gen without detecting the availability.
I'm a bit worried about that.

So do all AVX512 platforms support them?
Thanks.

Copy link
Contributor Author

@merykitty merykitty Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right. I can't find concrete evidence that AVX512 implies BMI1. In addition, VectorMaskGen does a check for AVX3 simultaneously with a check for BMI2. So it seems safer to do the same as we did with AVX1 - 2. As a result, I refactored the reduction step into a separate function.
Thank you very much.

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Nov 24, 2021

@DamonFool Could you please review this patch?

I will do some testing and feedback. Thanks.

All the tests passed.
But I'm not sure whether all avx512 machines support bmi1 and lzcnt features.

@openjdk openjdk bot removed the sponsor label Nov 24, 2021
Copy link
Member

@DamonFool DamonFool left a comment

Thanks for your update.

All my tests passed with the latest version.
So LGTM.

@merykitty
Copy link
Contributor Author

@merykitty merykitty commented Nov 25, 2021

/integrate

@openjdk openjdk bot added the sponsor label Nov 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 25, 2021

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

@merykitty
Copy link
Contributor Author

@merykitty merykitty commented Nov 25, 2021

@DamonFool Thanks a lot for your review. Do I need a re-review from @sviswa7 now?

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Nov 25, 2021

@DamonFool Thanks a lot for your review. Do I need a re-review from @sviswa7 now?

Yes, I think so.
Thanks.

Copy link

@sviswa7 sviswa7 left a comment

Looks good to me.

@sviswa7
Copy link

@sviswa7 sviswa7 commented Nov 29, 2021

@PaulSandoz Do we need another test run after the changes? If so, could you please help in running through your testing.

@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented Nov 29, 2021

@sviswa7 re-running tests since there were some subtle changes in the code

@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented Nov 29, 2021

Latest tests are good.

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Nov 29, 2021

Latest tests are good.

So let's integrate it.
Thanks.

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Nov 29, 2021

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

  • 3a4a94e: 8277854: The upper bound of GCCardSizeInBytes should be limited to 512 for 32-bit platforms
  • 825e633: 8277944: JDK 18 - update GA Release Date
  • 3d39f09: 8277654: Shenandoah: Don't produce new memory state in C2 LRB runtime call
  • 05ab176: 8277797: Remove undefined/unused SharedRuntime::trampoline_size()
  • ad51d06: 8277789: G1: G1CardSetConfiguration prefixes num_ and max_ used interchangeably
  • 614c6e6: 8277878: Fix compiler tests after JDK-8275908
  • 960bdde: 8277904: G1: Remove G1CardSetArray::max_entries
  • 45e8973: 8277896: Remove unused BOTConstants member methods
  • e5676f8: 8277450: Record number of references into collection set during gc
  • 2622ab3: 8277928: Fix compilation on macosx-aarch64 after 8276108
  • ... and 96 more: https://git.openjdk.java.net/jdk/compare/b15e6f076afe5ac68e9af68756860d0b25adea4b...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Nov 29, 2021
@openjdk openjdk bot added integrated and removed ready rfr sponsor labels Nov 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 29, 2021

@DamonFool @merykitty Pushed as commit 560f9c9.

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

@merykitty merykitty deleted the vectorMaskReduction branch Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler integrated
4 participants