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
8254827: JVMCI: Enable it for Windows+AArch64 #307
Conversation
👋 Welcome back rnkovacs! A progress list of the required criteria for merging this PR into |
This backport pull request has now been updated with issue from the original commit. |
/contributor add Bernhard Urban-Forster burban@openjdk.org |
@rnkovacs |
f7f50ab
to
142c38d
Compare
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 patch seems also to include a great many non-JVMCI changes.
142c38d
to
bd1adc8
Compare
fadd5b9
to
fd1e620
Compare
Forgot to rebase after the recent changes - sorry about that. Could you please check again? |
7ae8001
to
609ad26
Compare
609ad26
to
6700588
Compare
@rnkovacs This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
512177c
to
697c339
Compare
The dependent pull request has now been integrated, and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork: git checkout 8248238-win-aarch64
git fetch https://git.openjdk.java.net/jdk11u-dev master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push |
@rnkovacs this pull request can not be integrated into git checkout 8254827-jvmci
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 |
fd1e620
to
3c30abb
Compare
Rebased. |
@theRealAph since the dependencies are resolved for this, could you have another look? Thank you |
Sure, but you have to address review comments. |
I think @rnkovacs addressed it in this comment: #307 (comment) |
RegisterArray allRegisters = arch.getAvailableValueRegisters(); | ||
Register[] registers = new Register[allRegisters.size() - reservedRegisters.size() - (reserveForHeapBase ? 1 : 0)]; | ||
Register[] registers = new Register[allRegisters.size() - reservedRegisters.size() - (reserveForHeapBase ? 1 : 0) - (!canUsePlatformRegister ? 1 : 0)]; |
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 is confusing and complicated. If you set platformRegister
to be either r18
or Register.None
depending on the OS, you don't need a separate boolean canUsePlatformRegister
.
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 agree, it doesn't read nice. However the code around the definition of platformRegister
doesn't know which platform we are dealing with. We could potentially pass down the config
object via the constructor... or use the boolean flag as proposed here. Do you think it's worth changing it? I'm mostly worried about diverging from tip.
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.
OK, I see. Let it stand.
Argh! My review comment was stuck in the "pending" queue. |
@rnkovacs This change now passes all automated pre-integration checks. 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 44 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. 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 (@theRealAph) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
Thank you both. I tagged the JBS issue. |
Thank you @GoeLin. /integrate |
/sponsor |
Going to push as commit 994276c.
Your commit was automatically rebased without conflicts. |
@VladimirKempik @rnkovacs Pushed as commit 994276c. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Thank you @VladimirKempik! |
Changes are almost identical to the original commit, only that JVMCI and Graal are enabled in
hotspot.m4
instead ofjvm-features.m4
.Depends on #301.
Part of the Windows/AArch64 port.
Progress
Issue
Reviewers
Contributors
<burban@openjdk.org>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk11u-dev pull/307/head:pull/307
$ git checkout pull/307
Update a local copy of the PR:
$ git checkout pull/307
$ git pull https://git.openjdk.java.net/jdk11u-dev pull/307/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 307
View PR using the GUI difftool:
$ git pr show -t 307
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk11u-dev/pull/307.diff