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

JDK-8253947: Implementation: JEP 388: Windows AArch64 Support #222

Closed
wants to merge 18 commits into from

Conversation

@rnkovacs
Copy link
Contributor

@rnkovacs rnkovacs commented Aug 10, 2021

This is a more recent version of openjdk/jdk11u#2.

Changes since then:

Similarly to how it was done on tip, we have incorporated parts of JDK-8253015: Aarch64: Move linux code out from generic CPU feature detection by @AntonKozlov into the JDK-8253947: Implementation: JEP 388: Windows AArch64 Support commit.

Please let me know how I can make the review process easier / faster.


Progress

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

Integration blocker

 ⚠️ Issue of type Backport is not allowed for integrations

Issue

  • JDK-8253947: Implementation: JEP 388: Windows AArch64 Support

Contributors

  • Monica Beckwith <mbeckwit@openjdk.org>
  • Ludovic Henry <luhenry@openjdk.org>
  • Bernhard Urban-Forster <burban@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk11u-dev pull/222/head:pull/222
$ git checkout pull/222

Update a local copy of the PR:
$ git checkout pull/222
$ git pull https://git.openjdk.java.net/jdk11u-dev pull/222/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 222

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk11u-dev/pull/222.diff

@bridgekeeper bridgekeeper bot added the oca label Aug 10, 2021
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 10, 2021

Hi @rnkovacs, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user rnkovacs" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@rnkovacs
Copy link
Contributor Author

@rnkovacs rnkovacs commented Aug 10, 2021

/covered

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 10, 2021

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@rnkovacs
Copy link
Contributor Author

@rnkovacs rnkovacs commented Aug 10, 2021

/contributor add Monica Beckwith mbeckwit@openjdk.org
/contributor add Ludovic Henry luhenry@openjdk.org
/contributor add Bernhard Urban-Forster burban@openjdk.org

@openjdk
Copy link

@openjdk openjdk bot commented Aug 10, 2021

@rnkovacs
Contributor Monica Beckwith <mbeckwit@openjdk.org> successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 10, 2021

@rnkovacs
Contributor Ludovic Henry <luhenry@openjdk.org> successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 10, 2021

@rnkovacs
Contributor Bernhard Urban-Forster <burban@openjdk.org> successfully added.

@rnkovacs rnkovacs force-pushed the win-arm64-port branch 2 times, most recently from 424bcc9 to 72bfbd3 Aug 11, 2021
@rnkovacs
Copy link
Contributor Author

@rnkovacs rnkovacs commented Aug 11, 2021

Removed tabs and trailing spaces from 8253947 and 8254640.

Andrew Haley and others added 18 commits Aug 12, 2021
Reviewed-by: kbarrett, tschatzl, dholmes
Push changes from JDK-8248817 that were accidentally excluded from the commit.

Reviewed-by: kbarrett, dholmes
Remove unused variables in the AArch64 backend. Detected by compiling with MSVC, which warns about them.

Reviewed-by: aph, dholmes
__thread is gcc-ism, instead rely on compiler independent macro.

Reviewed-by: dholmes
MSVC employs min/max as macros

Co-authored-by: Ludovic Henry <luhenry@microsoft.com>
Reviewed-by: tschatzl, kbarrett
…ultiply

Co-authored-by: Monica Beckwith <monica.beckwith@microsoft.com>
Co-authored-by: Ludovic Henry <luhenry@microsoft.com>
Reviewed-by: aph
The original change missed to update an assert.

Co-authored-by: Ludovic Henry <luhenry@microsoft.com>
Co-authored-by: Bernhard Urban-Forster <beurba@microsoft.com>
Reviewed-by: dholmes
Co-authored-by: Monica Beckwith <mbeckwit@openjdk.org>
Co-authored-by: Ludovic Henry <luhenry@openjdk.org>
Co-authored-by: Bernhard Urban-Forster <burban@openjdk.org>
... Windows AArch64 stack page growth requirement in template interpreter

Reviewed-by: adinn, aph
@lewurm
Copy link
Member

@lewurm lewurm commented Aug 13, 2021

Thanks @rnkovacs for taking this over 🙂 Overall it looks good to me (but I'm not a formal reviewer). Do you have some numbers handy on the tests passed before/after this change on other platforms like Linux/AArch64 and Windows/x86_64 that you could share here?

@VladimirKempik
Copy link

@VladimirKempik VladimirKempik commented Aug 13, 2021

Hello
for 8248414 which is part of this PR, I have made a similar PR few days ago, I will withdraw it.
You backport seems identical except one thing

  1. ul was replaced with ull few times https://github.com/openjdk/jdk11u-dev/pull/215/files#diff-9c5120cff0dd87fbd1122067c44170768702b61cba7b1faf0c04dcba4c8fdfa3L135
    this patch ( the place to patch ) is not present in you tree at all, that's strange.

@VladimirKempik
Copy link

@VladimirKempik VladimirKempik commented Aug 13, 2021

Also, could you please be more specific, which parts of openjdk/jdk@ec9bee6 were taken and which wasn't ?
it's not trivial to find them in this PR.
Thanks

@phohensee
Copy link
Member

@phohensee phohensee commented Aug 13, 2021

Copy-paste errors are one of the problems that can occur with a giant squashed backport such as this one. It's difficult to review because it's a lot of code and there's no way to tell which code belongs to which JBS issue, and there's no way to triage the almost inevitable problems that will occur with such a large patch. I'd prefer separate per-JBS-issue backports, such as Vladimir's #215.

@phohensee
Copy link
Member

@phohensee phohensee commented Aug 13, 2021

I've posted a backport PR for JDK-8248403 at #246.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 13, 2021

Webrevs

@rnkovacs
Copy link
Contributor Author

@rnkovacs rnkovacs commented Aug 13, 2021

@VladimirKempik, changes to

  • src/hotspot/cpu/aarch64/vm_version_aarch64.cpp,
  • src/hotspot/cpu/aarch64/vm_version_aarch64.hpp, and
  • src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp

were taken over from openjdk/jdk@ec9bee6. However, now that we're breaking up the PR into its elements, it might make sense to backport this whole commit separately too. WDYT?

@rnkovacs
Copy link
Contributor Author

@rnkovacs rnkovacs commented Aug 13, 2021

Thank you @phohensee. I understand that reviewing such a huge patch is a pain, and the chance of overlooking something goes up with size. I'll proceed to open PRs for the rest of the issues.

@VladimirKempik
Copy link

@VladimirKempik VladimirKempik commented Aug 15, 2021

@VladimirKempik, changes to

  • src/hotspot/cpu/aarch64/vm_version_aarch64.cpp,
  • src/hotspot/cpu/aarch64/vm_version_aarch64.hpp, and
  • src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp

were taken over from openjdk/jdk@ec9bee6. However, now that we're breaking up the PR into its elements, it might make sense to backport this whole commit separately too. WDYT?

Ok, I will do the JDK-8248414 ( it's already reviewed) and its follow-ups: JDK-8250824 and JDK-JDK-8251930.
And Anton's fix for vm_version

@rnkovacs
Copy link
Contributor Author

@rnkovacs rnkovacs commented Aug 20, 2021

@VladimirKempik, changes to

  • src/hotspot/cpu/aarch64/vm_version_aarch64.cpp,
  • src/hotspot/cpu/aarch64/vm_version_aarch64.hpp, and
  • src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp

were taken over from openjdk/jdk@ec9bee6. However, now that we're breaking up the PR into its elements, it might make sense to backport this whole commit separately too. WDYT?

Ok, I will do the JDK-8248414 ( it's already reviewed) and its follow-ups: JDK-8250824 and JDK-JDK-8251930.

Thank you. I assume these are prerequisites to JDK-8253015.

And Anton's fix for vm_version

Which one is that?

@rnkovacs
Copy link
Contributor Author

@rnkovacs rnkovacs commented Aug 20, 2021

Following @phohensee's list of JBS issues, links to the individual PRs:

@VladimirKempik
Copy link

@VladimirKempik VladimirKempik commented Aug 20, 2021

@VladimirKempik, changes to

  • src/hotspot/cpu/aarch64/vm_version_aarch64.cpp,
  • src/hotspot/cpu/aarch64/vm_version_aarch64.hpp, and
  • src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp

were taken over from openjdk/jdk@ec9bee6. However, now that we're breaking up the PR into its elements, it might make sense to backport this whole commit separately too. WDYT?

Ok, I will do the JDK-8248414 ( it's already reviewed) and its follow-ups: JDK-8250824 and JDK-JDK-8251930.

Thank you. I assume these are prerequisites to JDK-8253015.

And Anton's fix for vm_version

Which one is that?

You can take https://bugs.openjdk.java.net/browse/JDK-8253015

@RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Aug 23, 2021

Hi,
I have a suggestion: For this series of backports you can use dependent PRs. That is, you create a standard PR for the first change. Then, create the next backport to the branch "pr/#" which will appear in the repo after the first PR is opened. This way, the whole chain is based upon each other and can be reviewed in parallel.

You have to integrate the PRs in order then. Once a PR is merged, its successor will automatically be updated to target the master branch.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 24, 2021

@rnkovacs this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout win-arm64-port
git fetch https://git.openjdk.java.net/jdk11u-dev master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@rnkovacs
Copy link
Contributor Author

@rnkovacs rnkovacs commented Aug 30, 2021

@RealCLanger thanks for the suggestion. I'm opening a chain of PRs for the last few commits in the following order: #274 - #299 - #301 - 8254827 - 8252114. I liked the simplicity of most of the PRs going in independently, but now in the end chaining makes a lot of sense.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 30, 2021

Mailing list message from Andrew Haley on jdk-updates-dev:

On 8/14/21 12:07 AM, Reka Kovacs wrote:

Thank you @phohensee. I understand that reviewing such a huge patch is a pain, and the chance of overlooking something goes up with size. I'll proceed to open PRs for the rest of the issues.

Thanks. In particular, the integer types patch is pretty much standalone,
and it touches many files. It's good for that one to be reviewed and committed
entirely independently, because while it is a requirement for Windows
support it is in no way Windows specific.

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

@RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Sep 8, 2021

@RealCLanger thanks for the suggestion. I'm opening a chain of PRs for the last few commits in the following order: #274 - #299 - #301 - 8254827 - 8252114. I liked the simplicity of most of the PRs going in independently, but now in the end chaining makes a lot of sense.

Hi @rnkovacs, maybe you want to close this PR in favor of #301 and the others? Looks like you've opened all required PRs by now and this work is done when all of them are reviewed and integrated...

@rnkovacs
Copy link
Contributor Author

@rnkovacs rnkovacs commented Oct 4, 2021

@RealCLanger Sure, I'll close this one. I've been keeping it around to track all the smaller PRs, but now that there's only a few left, we can keep those in mind.

@rnkovacs rnkovacs closed this Oct 4, 2021
@RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Oct 4, 2021

@RealCLanger Sure, I'll close this one. I've been keeping it around to track all the smaller PRs, but now that there's only a few left, we can keep those in mind.

Thanks. Do we have a list of what's still left? Maybe add it as a comment to #301?

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