-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
8271515: Integration of JEP 417: Vector API (Third Incubator) #5873
Conversation
👋 Welcome back psandoz! A progress list of the required criteria for merging this PR into |
@PaulSandoz The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
/contributor add sviswanathan |
@PaulSandoz |
@PaulSandoz |
@PaulSandoz |
@PaulSandoz |
@PaulSandoz |
@PaulSandoz |
@PaulSandoz |
@PaulSandoz |
@PaulSandoz |
/contributor add Rado Smogura mail@smogura.eu |
@PaulSandoz |
Webrevs
|
/csr |
@PaulSandoz this pull request will not be integrated until the CSR request JDK-8274028 for issue JDK-8271515 has been approved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C2 and x86 changes seems fine.
The Java changes look good to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AArch64 changes look ok apart from some minor comments.
} | ||
|
||
const int num_of_regs = PRegisterImpl::number_of_saved_registers; | ||
unsigned char regs[num_of_regs]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems clearer to use PRegisterImpl::number_of_saved_registers
directly rather than introducing num_of_regs
.
return total_push_bytes / 8; | ||
} | ||
|
||
// Return the number of dwords poped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
popped
@@ -243,6 +243,10 @@ class PRegisterImpl: public AbstractRegisterImpl { | |||
enum { | |||
number_of_registers = 16, | |||
number_of_governing_registers = 8, | |||
// AArch64 has 8 governing predicate registers, but p7 is used as an | |||
// all-1s register so the predicates to save are from p0 to p6 if we | |||
// don't have non-governing predicate registers support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is a bit difficult to read. How about:
// AArch64 has 8 governing predicate registers, but p7 is used as an
// all-1s register so the predicates to save are from p0 to p6. We
// don't support non-governing predicate registers.
@@ -39,20 +39,18 @@ inline bool is_PRegister() { | |||
} | |||
|
|||
inline Register as_Register() { | |||
assert( is_Register(), "must be"); | |||
// Yuk | |||
assert(is_Register(), "must be"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to leave this file as it was if you're only making whitespace changes here.
Address AArch64 review comments from Nick.
|
Thank you @nick-arm for the review! All your comments have been addressed. |
__ sve_$5(as_FloatRegister($dst$$reg), __ $6, as_FloatRegister($dst$$reg)); | ||
BasicType to_bt = Matcher::vector_element_basic_type(this); | ||
Assembler::SIMD_RegVariant to_size = __ elemType_to_regVariant(to_bt); | ||
__ sve_fcvtzs(as_FloatRegister($dst$$reg), __ D, ptrue, as_FloatRegister($src$$reg), __ D); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converting from double to long and then narrow to target types did not follow JLS. I will fix it. Thanks to @fg1417 for helping to find out this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converting from double to long and then narrow to target types did not follow JLS. I will fix it. Thanks to @fg1417 for helping to find out this issue.
Fixed in the new commit. Thanks to @PaulSandoz for integrating the fix!
Hi Nick @nick-arm ,
Could you please help to review the new commit, which fixes the same issue as JDK-8276151 for SVE? Thanks!
Like JDK-8276151, SVE vector double to int and float to long conversions have similar issue. According to Java language specification [1], we should convert double/float to integer/long directly, instead of converting to long/int and then narrowing/extending to target types. Test cases will be updated in JDK-8276151. [1] https://docs.oracle.com/javase/specs/jls/se17/html/jls-5.html#jls-5.1.3
AArch64: Incorrect SVE double to int and float to long vector conversion
@PaulSandoz 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:
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 3 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit a59c9b2.
Your commit was automatically rebased without conflicts. |
@PaulSandoz Pushed as commit a59c9b2. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This PR improves the performance of vector operations that accept masks on architectures that support masking in hardware, specifically Intel AVX512 and ARM SVE.
On architectures that do not support masking in hardware the same technique as before is applied to most operations, specifically composition using blend.
Masked loads/stores are a special form of masked operation that require additional care to ensure out-of-bounds access throw exceptions. The range checking has not been fully optimized and will require further work.
No API enhancements were required and only a few additional tests were needed.
Progress
Issue
Reviewers
Contributors
<sviswanathan@openjdk.org>
<jbhateja@openjdk.org>
<njian@openjdk.org>
<xgong@openjdk.org>
<eliu@openjdk.org>
<jiefu@openjdk.org>
<vlivanov@openjdk.org>
<jrose@openjdk.org>
<psandoz@openjdk.org>
<mail@smogura.eu>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5873/head:pull/5873
$ git checkout pull/5873
Update a local copy of the PR:
$ git checkout pull/5873
$ git pull https://git.openjdk.java.net/jdk pull/5873/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 5873
View PR using the GUI difftool:
$ git pr show -t 5873
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5873.diff