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

8266621: Add masking support for unary/ternary vector intrinsics #80

Closed

Conversation

@XiaohongGong
Copy link
Collaborator

@XiaohongGong XiaohongGong commented May 10, 2021

Similar with [1], this patch adds the masking support for unary/ternary vector intrinsics. It adds the mask information to the original intrinsic methods which are called both by masked and non-masked unary/ternary operations. For non-masked operations, the mask class and value are set to null.

Also change to use the "binaryMaskedOp" for vector mask logical operations instead of "binaryOp". So that all the original nary intrinsics and hotspot implementation can be removed.

Note that this patch only contains the Vector API java implementation and the hotspot intrinsics changes. No compiler mid-end and backend implementations are included.

[1] https://bugs.openjdk.java.net/browse/JDK-8264563


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Issue

  • JDK-8266621: Add masking support for unary/ternary vector intrinsics

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/panama-vector pull/80/head:pull/80
$ git checkout pull/80

Update a local copy of the PR:
$ git checkout pull/80
$ git pull https://git.openjdk.java.net/panama-vector pull/80/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 80

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/panama-vector/pull/80.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 10, 2021

👋 Welcome back xgong! A progress list of the required criteria for merging this PR into vectorIntrinsics+mask will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label May 10, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 10, 2021

Webrevs

Copy link
Member

@PaulSandoz PaulSandoz left a comment

Java changes look good.

@openjdk
Copy link

@openjdk openjdk bot commented May 11, 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:

8266621: Add masking support for unary/ternary vector intrinsics

Reviewed-by: psandoz, vlivanov

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 vectorIntrinsics+mask 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.

➡️ To integrate this PR with the above commit message to the vectorIntrinsics+mask branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label May 11, 2021
@XiaohongGong
Copy link
Collaborator Author

@XiaohongGong XiaohongGong commented May 12, 2021

Thanks for your review @PaulSandoz !

@XiaohongGong
Copy link
Collaborator Author

@XiaohongGong XiaohongGong commented May 13, 2021

@iwanowww , could you please take a look at this PR? Thanks so much!

@XiaohongGong
Copy link
Collaborator Author

@XiaohongGong XiaohongGong commented May 13, 2021

Since all the naryOp are totally replaced with naryMaskedOp (e.g. binaryOp -> binaryMaskedOp), does it make sense that we still use the name naryOp for both masked and non-masked operations? @PaulSandoz which do you think is better binaryOp or binaryMaskedOp ?

@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented May 13, 2021

I think what you have is fine, lets get the code in the branch and iterate. It's easy to revisit naming decisions like this later.

@XiaohongGong
Copy link
Collaborator Author

@XiaohongGong XiaohongGong commented May 14, 2021

I think what you have is fine, lets get the code in the branch and iterate. It's easy to revisit naming decisions like this later.

OK, agree! Thanks so much for your advice!

Copy link
Collaborator

@iwanowww iwanowww left a comment

Overall, looks good.

2 comments:

  • On naming: while browsing the patch and seeing Masked I constantly think about non-Masked counterpart. What about reusing unaryOp/binaryOp/ternaryOp names for the merged intrinsic?

  • Can you remind me what happens for SVML stubs when mask != NULL? Does the JVM reject the intrinsification? I believe the stubs don't support masked operation mode.

@XiaohongGong
Copy link
Collaborator Author

@XiaohongGong XiaohongGong commented May 17, 2021

Overall, looks good.

Thanks for your review!

2 comments:

  • On naming: while browsing the patch and seeing Masked I constantly think about non-Masked counterpart. What about reusing unaryOp/binaryOp/ternaryOp names for the merged intrinsic?

Yeah, totally agree. I also pointed out the naming issue above (see: #80 (comment)). And I agree with what @PaulSandoz's comment about it: "lets get the code in the branch and iterate. It's easy to revisit naming decisions like this later.". Does this make sense to you?

Can you remind me what happens for SVML stubs when mask != NULL? Does the JVM reject the intrinsification? I believe the stubs don't support masked operation mode.

It will generate the vector blend mode like before. Currently the compiler only checks whether the predicated mode is supported for normal operations (sopc != 0). For SVML ops, the use_predicate is false which means it generates "VectorBlend" pattern like before.

@XiaohongGong
Copy link
Collaborator Author

@XiaohongGong XiaohongGong commented May 19, 2021

Hi @iwanowww , do you have any other comments for this PR? Can we merge this PR first and revisit the naming issue after then?

@XiaohongGong
Copy link
Collaborator Author

@XiaohongGong XiaohongGong commented May 20, 2021

@PaulSandoz @iwanowww , I'v rename all the masked op back to the original names. Could you please help to look at again? Waiting for your feedback, Thanks so much!

Copy link
Collaborator

@iwanowww iwanowww left a comment

Looks good.

@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented May 20, 2021

@PaulSandoz @iwanowww , I'v rename all the masked op back to the original names. Could you please help to look at again? Waiting for your feedback, Thanks so much!

Looks good. Integrate it!

@XiaohongGong
Copy link
Collaborator Author

@XiaohongGong XiaohongGong commented May 24, 2021

Thanks for the review @PaulSandoz @iwanowww !

@XiaohongGong
Copy link
Collaborator Author

@XiaohongGong XiaohongGong commented May 24, 2021

/integrate

@openjdk openjdk bot closed this May 24, 2021
@openjdk openjdk bot added integrated and removed ready labels May 24, 2021
@XiaohongGong XiaohongGong deleted the JDK-8266621 branch May 24, 2021
@openjdk openjdk bot removed the rfr label May 24, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 24, 2021

@XiaohongGong Pushed as commit 24b5262.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants