-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8345169: Implement JEP XXX: Remove the 32-bit x86 Port #22567
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
Conversation
|
👋 Welcome back shade! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@shipilev this pull request can not be integrated into git checkout JDK-8345168-remove-32bit-x86
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
1cb9d4e to
b251ff3
Compare
b251ff3 to
b55fc75
Compare
I think we can keep them separate (big .ad files is difficult to navigate). |
|
I don't see make files changes. |
|
It would be nice to split this into separate PRs for easy review. Removing "rounding of x87 FPU" could be definitely done separately. |
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 reviewed the template interpreter changes. They look great.
| } | ||
| } | ||
|
|
||
| void TemplateTable::dconst(int value) { | ||
| transition(vtos, dtos); | ||
| if (UseSSE >= 2) { |
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 admit that I don't know what UseSSE is but now this is unconditional? Is there a further cleanup necessary for this option?
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.
Yes, now it is unconditional. x86_64 requires UseSSE >= 2. Only x86_32 cared about UseSSE < 2, so now we can eliminate these checks. I think I got the majority, if not all of the cases where these checks are now redundant: there are more in various assemblers and compiler code.
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.
Maybe this should change from range (2,4) then.
product(int, UseSSE, 4,
"Highest supported SSE instructions set on x86/x64")
range(0, 4) \
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.
Right. Now that I am thinking more deeply about it, maybe that would be a first step here: lift UseSSE >= 2 for x86_32 ahead of this JEP, eliminate all UseSSE < 2 parts. I can see how intrusive this gets.
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.
[not reviewing, just a drive-by comment] Does UseSSE < 2 provide a way to avoid using relevant parts of
SSE on x86_64, perhaps for debugging? Or does x86_64 effectively hard-wire UseSSE >= 2?
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.
By default all 64-bits x86 CPU (starting from AMD64) supports all instructions up to SSE2. 32-bit x86 CPU may not support SSE2.
We can generated sse1 or use FPU instructions in 64-bit VM but we decided not to do that - SSE2 instructions version were much easier to use. We purged all uses of FPU in JDK 15: JDK-7175279 by using SSE set of instructions because we did not want to mess (save/restore state) with FPU anymore in 64-bit VM.
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 there are several places in 64-bit VM where we assume SSE2 instructions are always available.
So if you set UseSSE=1 or = 0 in debugger VM may crash.
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.
Having some kind of pre-JEP patch for this this might be helpful so that we don't drill down on this rather than the whole patch. Maybe the JEP patch could simply be what @iwanowww suggests. Then have a post-JEP patch to remove everything else. Sort of like what we did with Security Manager.
|
Don't forget the 32-bit x86 classes under |
|
Personally, I'd prefer to see initial x86-32 removal changeset as straighforward as possible: x86-32-specific files, plus (optionally) x86-32-specific code in x86-specific files. IMO it's better to cover the rest (getting rid of unused features after x86-32 removal) as follow-up cleanups. |
|
@shipilev 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! |
|
Great, thanks for the feedback. I think we are going to go with the JEP implementation that removes the easy parts of x86_32 code, and then do the deeper cleanups under JDK-8351148 umbrella. I added some subtasks there, based on the commits from this bulk PR. I am closing this PR in favor of cleaner PR for JEP 503: #23906 |
NOTE: This is work-in-progress draft for interested parties. The JEP is not even submitted, let alone targeted.
My plan is to to get this done in a quiet time in mainline to limit the ongoing conflicts with mainline. Feel free to comment in this PR, if you see something ahead of time. These comments might adjust the trajectory we take to implement this removal and/or allows us submit and work out more RFEs ahead of this removal. I plan to re-open a clean PR after this preliminary PR is done, maybe after the round of preliminary reviews.
This removes the 32-bit x86 port and does a deeper cleaning in Hotspot. The following paragraphs describe what and why was being done.
Easy stuff first: all files named
*_x86_32are gone. Those are only built when build system knows we are compiling for x86_32. There is therefore no impact on x86_64.The code under
!LP64,!AMD64andIA32is removed inx86-specific files. There is quite a bit of the code, especially aroundAssemblerandMacroAssembler. I think these removals make the whole thing cleaner. The downside is that some of theMacroAssembler::*ptrfunctions that were used to select the "machine pointer" instructions either from x86_64 or x86_32 are now exclusively for x86_64. I don't think we want to rewrite*ptr->*qat this point. I think we gradually morph the code base to use*q-flavored methods in new code.x86_32 is the only platform that has special cases for x87 FPU.
C1 even implements the whole separate thing to deal with x87 FPU: the parts of regalloc treat it specially, there is
FpuStackSim, there isVerifyFPUfamily of flags, etc. There are also peculiarities with FP conversions that use FPU, that's why x86_32 used to have template interpreter stubs for FP conversion methods. None of that is needed anymore without x86_32. This cleans up some arch-specific code as well.Both C1 and C2 implement the workarounds for non-IEEE compliant rounding of x87 FPU. After x86_32 is gone, these are not needed anymore. This removes some C2 nodes, removes the rounding instructions in C1.
x86_64 is baselined on SSE2+, the VM would not even start if SSE2 is not supported. Most of the checks that we have for
UseSSE < 2are for the benefit of x86_32. Because of this I folded redundantUseSSEchecks around Hotspot.The one thing I deliberately avoided doing is merging
x86.adandx86_64.ad. It would likely introduce uncomfortable amount of conflicts with pending work in mainline, so I would like to do it separately as the follow-up.Progress
Integration blocker
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22567/head:pull/22567$ git checkout pull/22567Update a local copy of the PR:
$ git checkout pull/22567$ git pull https://git.openjdk.org/jdk.git pull/22567/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 22567View PR using the GUI difftool:
$ git pr show -t 22567Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22567.diff
Using Webrev
Link to Webrev Comment