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

8276799: Implementation of JEP 422: Linux/RISC-V Port #1

Merged
merged 13 commits into from Feb 20, 2023

Conversation

yadongw
Copy link
Collaborator

@yadongw yadongw commented Feb 7, 2023

This PR backports the implementation of RISC-V port into jdk17u.

It based on the initial implementation of JEP-422 with some modifications for jdk17u:

  1. The initial patch
    8276799: Implementation of JEP 422: Linux/RISC-V Port
  2. remove features that do not belong to jdk17u
    revert 8277417: C1 LIR instruction for load-klass
    revert 8278387: Implement UseHeavyMonitors consistently
    revert 8227369: pd_disjoint_words_atomic() needs to be atomic
    revert 8258192: Obsolete the CriticalNatives flag
    revert 8281632: riscv: Improve interpreter stack banging
    revert 8283364: Intrinsify countPositives
  3. add features that belongs to jdk17u
    enable the biased locking
  4. Wrap modifications to shared code under RISCV and revert changes to other platforms
    isolate modification to other platforms
  5. Patches for making compilation pass
    8282306: os::is_first_C_frame(frame*) crashes on invalid link access
    8282477: vmassert(_last_Java_pc == NULL, "already walkable"); fails with async profiler

Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issues

  • JDK-8276799: Implementation of JEP 422: Linux/RISC-V Port
  • JDK-8282306: os::is_first_C_frame(frame*) crashes on invalid link access
  • JDK-8282477: [x86, aarch64] vmassert(_last_Java_pc == NULL, "already walkable"); fails with async profiler

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/riscv-port-jdk17u pull/1/head:pull/1
$ git checkout pull/1

Update a local copy of the PR:
$ git checkout pull/1
$ git pull https://git.openjdk.org/riscv-port-jdk17u pull/1/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/riscv-port-jdk17u/pull/1.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 7, 2023

👋 Welcome back yadongwang! A progress list of the required criteria for merging this PR into riscv-port will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@RealFYang RealFYang changed the title Backport riscv port 8276799: Implementation of JEP 422: Linux/RISC-V Port Feb 7, 2023
@luhenry
Copy link
Member

luhenry commented Feb 7, 2023

Love to see this!

How has it been tested? I'm happy to submit some changes to this riscv-port branch to add testing for tier1 on GHA if that's any help (similar to https://github.com/rivosinc/jdk/actions/runs/4028920698).

@RealFYang
Copy link
Member

RealFYang commented Feb 7, 2023

@luhenry : That sounds great. It's nice if we could have some basic tier1 testing enabled for this branch on GHA.
PS: I believe Yadong is still testing patch and I am also trying this on my Unmatched board.

@yadongw
Copy link
Collaborator Author

yadongw commented Feb 7, 2023

Love to see this!

How has it been tested? I'm happy to submit some changes to this riscv-port branch to add testing for tier1 on GHA if that's any help (similar to https://github.com/rivosinc/jdk/actions/runs/4028920698).

Tier1 tests are running on unmatched boards, and a full jtreg tests are running with Qemu. Happy to see more tests, and some failures may be related to existed issues.

@luhenry
Copy link
Member

luhenry commented Feb 7, 2023

Ok, I'll work on submitting a PR tomorrow. It'll be limited to .github/ so should be fairly easy to isolate.

@yadongw yadongw marked this pull request as ready for review February 12, 2023 12:18
@openjdk openjdk bot added the rfr Pull request is ready for review label Feb 12, 2023
@RealFYang RealFYang changed the title 8276799: Implementation of JEP 422: Linux/RISC-V Port Backport 5905b02c0e2643ae8d097562f181953f6c88fc89 Feb 12, 2023
@openjdk openjdk bot changed the title Backport 5905b02c0e2643ae8d097562f181953f6c88fc89 8276799: Implementation of JEP 422: Linux/RISC-V Port Feb 12, 2023
@openjdk
Copy link

openjdk bot commented Feb 12, 2023

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added the backport label Feb 12, 2023
@mlbridge
Copy link

mlbridge bot commented Feb 12, 2023

Webrevs

@VladimirKempik
Copy link

Hello
could you include https://bugs.openjdk.org/browse/JDK-8293100 into initial backport ?

it was really critical for risc-v and some jni stress tests from vmtestBase/hotspot:tier4 was failing

JDK17 ready backport can be used from syntacore/syntaj17@2053a4b

@RealFYang
Copy link
Member

RealFYang commented Feb 12, 2023

Let me share some of the testing results carried out on my unmatched boards:

  • boot-cycle (fastdebug)
  • Tier1-3 tests (release)
  • Non-trivial benchmark tests like: renaissance, specjbb2015 (release)
  • Tier4 hotspot (release)

@RealFYang
Copy link
Member

RealFYang commented Feb 12, 2023

Hello could you include https://bugs.openjdk.org/browse/JDK-8293100 into initial backport ?

it was really critical for risc-v and some jni stress tests from vmtestBase/hotspot:tier4 was failing

JDK17 ready backport can be used from syntacore/syntaj17@2053a4b

I think that deserves another seperate backporting PR after this one? I see the robot is trying to link each backporting PR with the original issue. So it might not be a good idea to put serveral seperate fixes together into one backporting PR.

But the following two changes included in this PR are kind of special as they are needed for a successfull build:
8282306: os::is_first_C_frame(frame*) crashes on invalid link access
8282477: vmassert(_last_Java_pc == NULL, "already walkable"); fails with async profiler

@VladimirKempik
Copy link

I think that deserves another seperate backporting PR after this one? I see the robot is trying to link each backporting PR with the original issue. So it might not be a good idea to put serveral seperate fixes together into one backporting PR.

Last time I was backporting JEP ( openjdk/jdk11u-dev#715 ) it was ok. Just order scara-bot few commands /issue 8282306 , /issue 8282477.

@RealFYang
Copy link
Member

RealFYang commented Feb 12, 2023

I think that deserves another seperate backporting PR after this one? I see the robot is trying to link each backporting PR with the original issue. So it might not be a good idea to put serveral seperate fixes together into one backporting PR.

Last time I was backporting JEP ( openjdk/jdk11u-dev#715 ) it was ok. Just order scara-bot few commands /issue 8282306 , /issue 8282477.

Hmm, I think that might be a good way when we are doing the final upstreaming. For now, I still prefer to keep a simple/clean relationship between a backporting PR and the corresponding issue unless there is a special reason for this staging repo.
@yadongw : Please link these two issues (8282306 & 8282477) with this backporting PR.

@openjdk openjdk deleted a comment from openjdk bot Feb 13, 2023
@openjdk openjdk deleted a comment from openjdk bot Feb 13, 2023
@yadongw
Copy link
Collaborator Author

yadongw commented Feb 13, 2023

/issue 8282306

@yadongw
Copy link
Collaborator Author

yadongw commented Feb 13, 2023

/issue 8282477

@openjdk
Copy link

openjdk bot commented Feb 13, 2023

@yadongw
Adding additional issue to issue list: 8282306: os::is_first_C_frame(frame*) crashes on invalid link access.

@openjdk
Copy link

openjdk bot commented Feb 13, 2023

@yadongw
Adding additional issue to issue list: 8282477: [x86, aarch64] vmassert(_last_Java_pc == NULL, "already walkable"); fails with async profiler.

@RealFYang
Copy link
Member

RealFYang commented Feb 15, 2023

So I have finished the aforementioned tests on my unmatched boards.
The test results are expected. I think we can proceed reviewing this now.

PS: The tests also expose several build/test issues that should be resolved by backporting other fixes.

0. https://bugs.openjdk.org/browse/JDK-8290496
- To resolve build warnings-as-errors

1. https://bugs.openjdk.org/browse/JDK-8290164
- TEST: compiler/runtime/TestConstantsInError.java

2. https://bugs.openjdk.org/browse/JDK-8292407
- TEST: compiler/unsafe/JdkInternalMiscUnsafeAccessTestChar.java
- TEST: java/lang/invoke/VarHandles/VarHandleTestAccessChar.java
- And others

3. https://bugs.openjdk.org/browse/JDK-8293100
- TEST: vmTestbase/nsk/stress/jni/jnistress002.java

4. https://bugs.openjdk.org/browse/JDK-8285437
- To resolve JVM crash with -XX:+VerifyOops

Copy link
Member

@feilongjiang feilongjiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, thanks.

src/hotspot/cpu/riscv/riscv.ad Show resolved Hide resolved
src/hotspot/cpu/riscv/riscv.ad Outdated Show resolved Hide resolved
Copy link
Member

@RealFYang RealFYang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine from the cursory review.

@openjdk
Copy link

openjdk bot commented Feb 20, 2023

@yadongw This pull request has not yet been marked as ready for integration.

@RealFYang RealFYang merged commit d4c1e23 into openjdk:riscv-port Feb 20, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport rfr Pull request is ready for review
5 participants