-
Notifications
You must be signed in to change notification settings - Fork 208
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
8276799: Implementation of JEP 422: Linux/RISC-V Port #1427
Conversation
👋 Welcome back fyang! 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. |
At some point in future, when this PR is approved, could you link all the bugs mentioned in your email ( to jdk-updates-dev list) to this PR via "/issue" command ( Yeah, I know it's lots of work). So we will know in jbs, if some fix/backport is present in 17u-dev or not |
Sure! That's on my TODO list when this draft PR is ready for review. |
/issue add 8282306,8282477,8283865,8284068,8284937,8285303,8287418,8297644,8291952,8285437,8285699,8285711,8287425,8287552,8287970,8290137,8290164,8290496,8291893,8291947,8292867,8293050,8293100,8293474,8293524,8293566,8294012,8294083,8294086,8294087,8294187,8294366,8294430,8294492,8294679,8295110,8295270,8295396,8295926,8295968,8296435,8296447,8296448,8296602,8296771,8296916,8297359,8297644,8297697,8301067,8297715,8299168,8299847,8300109,8301033,8301036,8301153,8301313,8301628,8301818,8302114,8301852,8302289,8302776,8304293,8305006,8305008,8305112,8305512,8305728,8306667,8307150,8307446,8307651,8308089,8308277,8308997,8309427,8305236 |
@RealFYang Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: |
Webrevs
|
Please go to https://github.com/RealFYang/jdk17u-dev/actions and enable GHA workflows, we need to make sure it does not break. |
Done. And I have successfully triggered a GHA test for this, results are clean. Thanks for reminding. |
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.
The majority of changes to shared part are under proper ifdefs, that looks good and safe for other platforms.
I'm running this change on GHA with testing on RISC-V (using qemu) which should be a good smoke test (nothing more). See https://github.com/rivosinc/jdk17u-dev/actions/workflows/main.yml?query=branch%3Adev%2Fludovic%2Ftest-jdk17-riscv-backport |
It seems like it's missing 8285630, we can't cross-compile without it otherwise. |
/issue add 8285630 |
Thanks for reporting this. Normally, we do cross-build with |
@RealFYang |
It has completed a run of this PR on GHA at https://github.com/rivosinc/jdk17u-dev/actions/runs/5356181476. There are a few failing tests but they are either timeout or related to running in QEMU. The test suites that have been run are:
|
Isn't there anybody in the Risc-V project that can review this? It needs not necessarily be a jdk-updates reviewer. But there should be someone (maybe besides Aleksey) who will maintain this and takes the responsibility to mark this change reviewed. It would be a bad idea to admit a port that has no people that can and will review it. |
I have looked thru linux_riscv os_cpu changes, looks good |
@robehn : I think we have already got a fix for the reported IR Matching failure problem. Turns out that it is the test case that needs to be adapted for RISC-V. So luckily this issue should not block us from proceeding with this PR. For 17u, the test passes with the following trivial change:
|
I think we can indeed ask other RISC-V committers to apply their reviews here. |
Great, thanks. Sorry I don't have time to do a full review (OOO). |
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 have reviewed both the shared and riscv code. It looks good for me. Actually, I did the original backporting patch (openjdk/riscv-port-jdk17u#1) from the first pr of JEP 422 (openjdk/jdk#6294). It took a lot of effort to revert all patches irrelevant to 17, reenable patches related to 17 and make a "minimal and clean" baseline to accept subsequent series of necessary patches to be reviewed on the riscv-port-jdk17u repo (https://github.com/openjdk/riscv-port-jdk17u/pulls).
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.
cpu/riscv
and os_cpu/linux_riscv
looks good! I also participated in the backporting and reviewing of the riscv-port-jdk17u repo. All patches are reviewed by maintainers and well-tested by submitters. So I think it's good to go!
/issue add 8277417 |
@RealFYang |
Got the push approval and my local tier1-3 test results are still clean. So let's integrate this. |
@RealFYang Pushed as commit 966fc82. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Just an FYI, but this does mean a few bugs that weren't RISC-V specific have been marked as being backported to 17u twice:
The original includes the fix for other architectures, and presumably the RISC-V specific change is in this patch. It's not obvious from the commit log what is going on and I had to dig through to this PR to work it out. It might have been an idea to create RISC-V specific bugs for these backports. It looks like we messed up in the same way with AArch64 too e.g. https://bugs.openjdk.org/browse/JDK-8183925 & https://bugs.openjdk.org/browse/JDK-8160748 :( |
According to [1], jdk17 gain risc-v support officially starting at 17.0.9+4, so the riscv specific patch is no longer needed.. also see [2] for more info I've built & tested on Milkv Poineer machine (lp64d systemd profile) [1] https://wiki.riseproject.dev/display/HOME/LR_00_005%3A+Backport+RISC-V+support+to+jdk17u [2] openjdk/jdk17u-dev#1427 Closes: https://bugs.gentoo.org/927145 Signed-off-by: Yixun Lan <dlan@gentoo.org>
According to [1], jdk17 gain risc-v support officially starting at 17.0.9+4, so the riscv specific patch is no longer needed.. also see [2] for more info I've built & tested on Milkv Poineer machine (lp64d systemd profile) [1] https://wiki.riseproject.dev/display/HOME/LR_00_005%3A+Backport+RISC-V+support+to+jdk17u [2] openjdk/jdk17u-dev#1427 Closes: https://bugs.gentoo.org/927145 Signed-off-by: Yixun Lan <dlan@gentoo.org> Signed-off-by: Arthur Zamarin <arthurzam@gentoo.org>
The RISC-V port was originally developed at Huawei Technologies, then integrated into OpenJDK 19.
The 17u version of the port has continued to be maintained in the openjdk/riscv-port-jdk17u repo later
and has been tested for several months. There are few changes to shared HotSpot code (mostly the main
one is C1 conditional move/branch support for RISC-V). As required by 17u maintainer, changes to shared
code has been kept to a minimum. Only enabling shared changes are incorporated and these changes are
properly guarded with macro RISCV. So this 17u port should not breaking existing code and, although it is
a large patch, finally integrating it into 17u upstream should be low risk.
Testing on linux-riscv64 platform:
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/1427/head:pull/1427
$ git checkout pull/1427
Update a local copy of the PR:
$ git checkout pull/1427
$ git pull https://git.openjdk.org/jdk17u-dev.git pull/1427/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1427
View PR using the GUI difftool:
$ git pr show -t 1427
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/1427.diff
Webrev
Link to Webrev Comment