-
Couldn't load subscription status.
- Fork 6.1k
8351159: Remaining cleanups in cpu/x86 after 32-bit x86 removal #27987
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
base: master
Are you sure you want to change the base?
Conversation
|
👋 Welcome back aseoane! A progress list of the required criteria for merging this PR into |
|
@anton-seoane 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 34 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 (@stefank) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@anton-seoane The following label 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 list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
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've looked through all files except the .ad files.
| #ifdef AMD64 | ||
| define_pd_global(intx, InteriorEntryAlignment, 16); | ||
| define_pd_global(size_t, NewSizeThreadIncrease, ScaleForWordSize(4*K)); | ||
| define_pd_global(size_t, NewSizeThreadIncrease, ScaleForWordSize(4*K)); |
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.
FWIW, I think the comment previously only applied to MaxRAM but now it sounds like it is meant for both:
// Ergonomics related flags
define_pd_global(uint64_t, MaxRAM, 128ULL*G);
define_pd_global(intx, RegisterCostAreaRatio, 16000);
Is it meant for both?
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 believe so, it is the same in aarch64 and riscv c2 globals files
| interpreter_frame_monitor_block_bottom_offset = interpreter_frame_initial_sp_offset, | ||
|
|
||
| // Entry frames | ||
| #ifdef AMD64 |
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.
Did you consider if _WIN64 could be replaced with _WINDOWS?
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 had missed it. In this context, it can
| } | ||
|
|
||
| #if defined(_M_AMD64) | ||
| #if defined(_WINDOWS) |
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.
Do we really need to check for _WINDOWS in a windows file?
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.
We don't. Fixed it
This PR carries out cleanup on the
cpuandos_cpudirectories after the removal of the 32-bit x86 architecture from HotSpot.Mainly, code guarded by
IA32orX32, or!AMD64or!LP64in a x86 context, has been removed. These guards are now redundant (e.g. checking for 64bit on a x86 context) or the code blocks never executed (e.g. code under#ifdef X32). Additionally, x86 AD files have been merged.Please note that this changeset limits itself to changes under
cpu/x86andos_cpu/x86. Other cleanups are filed under a different RFE, and consequently addressed in a different PR: JDK-8351149.Testing: passes tiers 1-5
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27987/head:pull/27987$ git checkout pull/27987Update a local copy of the PR:
$ git checkout pull/27987$ git pull https://git.openjdk.org/jdk.git pull/27987/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27987View PR using the GUI difftool:
$ git pr show -t 27987Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27987.diff
Using Webrev
Link to Webrev Comment