-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
8320622: [TEST] Improve coverage of compiler/loopopts/superword/TestMulAddS2I.java on different platforms #19305
Conversation
…ulAddS2I.java on different platforms It would be worthwhile to improve the test coverage on all platforms by applying another common VM flag.
👋 Welcome back fgao! A progress list of the required criteria for merging this PR into |
@fg1417 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 48 new commits pushed to the
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. ➡️ To integrate this PR with the above commit message to the |
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.
Welcome back, long time no see
Looks good to me!
Thanks @eme64 ! Happy to see you again |
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.
Are all platforms support both (true
and false
) values of AlignVector
?
I see it is only adjusted for x86 and aarch64 in vm_version_<cpu>.cpp
files.
Thanks for your review @vnkozlov . Yeah, and jdk/src/hotspot/share/opto/superword.hpp Line 575 in d6b7f9b
Matcher::misaligned_vectors_ok() is architecture dependent. AlignVector is true by default. When we sets false by cmd line, if the platform doesn't support misaligned load/store, Matcher::misaligned_vectors_ok() should return false and the actual value of vectors_should_be_aligned() is still true . It should be okay to set true or false on all platforms. WDYT?
|
I have tested it on s390x, look good for us. |
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.
Good.
Thanks for your review @vnkozlov @eme64 @offamitkumar . I'm going to integrate it. |
/integrate |
Going to push as commit 8a9d77d.
Your commit was automatically rebased without conflicts. |
It would be worthwhile to improve the test coverage on all platforms by applying another common VM flag.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19305/head:pull/19305
$ git checkout pull/19305
Update a local copy of the PR:
$ git checkout pull/19305
$ git pull https://git.openjdk.org/jdk.git pull/19305/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19305
View PR using the GUI difftool:
$ git pr show -t 19305
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19305.diff
Webrev
Link to Webrev Comment