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 #3

Closed
wants to merge 145 commits into from

Conversation

kuaiwei
Copy link
Contributor

@kuaiwei kuaiwei commented Aug 4, 2023

It's the initial patch based on the JDK19 initial load openjdk/jdk@5905b02, and revert patches not required in JDK11u. The testing is running on a linux riscv board. There are some failed cases and we are working on fix them , the progress will updated.

Thanks the contribution of OpenJDK community and hard working of @zhengxiaolinX .

  • slowdebug/fastdebug/release build
  • Benchmark: SPECJbb2015, Renaissance
  • Tier1 tests
  • Tier2 tests
  • Tier3 tests
  • Tier4 tests

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 (Sub-task - P2)
  • JDK-8283737: riscv: MacroAssembler::stop() should emit fixed-length instruction sequence (Bug - P4)
  • JDK-8285437: riscv: Fix MachNode size mismatch for MacroAssembler::verify_oops* (Bug - P4)
  • JDK-8287418: riscv: Fix correctness issue of MacroAssembler::movptr (Bug - P3)
  • JDK-8293100: RISC-V: Need to save and restore callee-saved FloatRegisters in StubGenerator::generate_call_stub (Bug - P3)
  • JDK-8295926: RISC-V: C1: Fix LIRGenerator::do_LibmIntrinsic (Bug - P2)
  • JDK-8291952: riscv: Remove PRAGMA_NONNULL_IGNORED (Enhancement - P4)
  • JDK-8308277: RISC-V: Improve vectorization of Match.sqrt() on floats (Enhancement - P4)
  • JDK-8282306: os::is_first_C_frame(frame*) crashes on invalid link access (Enhancement - P4)

Reviewers

Contributors

  • Xiaolin Zheng <xlinzheng@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 4, 2023

👋 Welcome back kuaiwei! 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
Copy link
Member

RealFYang commented Aug 9, 2023

Hi, we need to make sure it does not break other ports. So please fix the jcheck errors and then go to https://github.com/kuaiwei/riscv-port-jdk11u/actions and enable GHA workflows.

@kuaiwei kuaiwei changed the title Initial patch of JDK11u riscv port 8314420: Initial patch of JDK11u riscv port Aug 16, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 16, 2023
@mlbridge
Copy link

mlbridge bot commented Aug 16, 2023

@kuaiwei
Copy link
Contributor Author

kuaiwei commented Aug 16, 2023

Hi, we need to make sure it does not break other ports. So please fix the jcheck errors and then go to https://github.com/kuaiwei/riscv-port-jdk11u/actions and enable GHA workflows.

Hi, Thanks for the comment. I found you changed cross build config in #1 .May I cherry pick it or wait for its integration?

@RealFYang
Copy link
Member

RealFYang commented Aug 16, 2023

Hi, we need to make sure it does not break other ports. So please fix the jcheck errors and then go to https://github.com/kuaiwei/riscv-port-jdk11u/actions and enable GHA workflows.

Hi, Thanks for the comment. I found you changed cross build config in #1 .May I cherry pick it or wait for its integration?

Looks like GHA for jdk11u-dev upstream are currently failing. I plan to do some merging for #1 when the issue is fixed. I think #1 should come after your current PR. So you should not depend on it.

BTW: I think your current PR should be made a backport of JDK-8276799. So you should change the PR title into something like: "Backport 5905b02c0e2643ae8d097562f181953f6c88fc89". And let the bot do the rest.

Reference: https://wiki.openjdk.org/display/JDKUpdates/How+to+contribute+or+backport+a+fix

@kuaiwei kuaiwei changed the title 8314420: Initial patch of JDK11u riscv port 8314420: Implementation of JEP 422: Linux/RISC-V Port Aug 17, 2023
@RealFYang
Copy link
Member

RealFYang commented Aug 18, 2023

Seems that I can not do a native build on hifive unmatched with --disable-precompiled-headers or without --disable-warnings-as-errors. The GCC version is gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04). Please have a look.

PS: And we should add RISC-V code for https://bugs.openjdk.org/browse/JDK-8313796 which has just been backported to jdk11u-dev

@kuaiwei
Copy link
Contributor Author

kuaiwei commented Aug 21, 2023

Seems that I can not do a native build on hifive unmatched with --disable-precompiled-headers or without --disable-warnings-as-errors. The GCC version is gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04). Please have a look.

PS: And we should add RISC-V code for https://bugs.openjdk.org/browse/JDK-8313796 which has just been backported to jdk11u-dev

I built the jdk on qemu with gcc10. I will check if it can be built with other gcc version.
For JDK-8313796, I will add it to backport patch list.
Thanks for your suggestion.

@kuaiwei
Copy link
Contributor Author

kuaiwei commented Aug 22, 2023

Seems that I can not do a native build on hifive unmatched with --disable-precompiled-headers or without --disable-warnings-as-errors. The GCC version is gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04). Please have a look.
PS: And we should add RISC-V code for https://bugs.openjdk.org/browse/JDK-8313796 which has just been backported to jdk11u-dev

I built the jdk on qemu with gcc10. I will check if it can be built with other gcc version. For JDK-8313796, I will add it to backport patch list. Thanks for your suggestion.

The new commit 1b8778b fixed gcc compilation error. I can built fastdebug image with gcc13 and "--disable-warnings-as-errors" option.

RealFYang and others added 19 commits August 22, 2023 17:03
JDK-8253180: ZGC: Implementation of JEP 376: ZGC: Concurrent Thread-Stack Processing
JDK-8220051: Remove global safepoint code
JDK-8234562: Move OrderAccess::release_store*/load_acquire to Atomic
JDK-8234736: Harmonize parameter order in Atomic - store
JDK-8234737: Harmonize parameter order in Atomic - add
JDK-8234740: Harmonize parameter order in Atomic - cmpxchg
JDK-8234739: Harmonize parameter order in Atomic - xchg
JDK-8236778: Add Atomic::fetch_and_add
JDK-8239895: assert(_stack_base != 0LL) failed: Sanity check
JDK-8238988: Rename thread "in stack" methods and add in_stack_range
JDK-8234372: Investigate use of Thread::stack_base() and queries for "in stack"
JDK-8203481: Incorrect constraint for unextended_sp in frame:safe_for_sender
JDK-8248240: Remove extendedPC.hpp and fetch_frame_from_ucontext
JDK-8253742: POSIX signal code cleanup
@zifeihan
Copy link
Member

zifeihan commented Feb 7, 2024

Hi, Maybe you forgot to add issue: 8282306, 8301313, 8308277, 8310873

@zhengxiaolinX
Copy link
Collaborator

zhengxiaolinX commented Feb 7, 2024

I checked and it seems that this contains fixes for the following two issues: ... https://bugs.openjdk.org/browse/JDK-8291952: riscv: Remove PRAGMA_NONNULL_IGNORED

  1. For JDK-8291952: riscv: Remove PRAGMA_NONNULL_IGNORED: Yes you are right. This one is contained but wrongly documented in the commit of this PR. It should've been a port instead of a revert.

Also I don't understand why we have the following two issues in the list?

  1. For JDK-8243155: AArch64: Add support for SqrtVF, it was committed to the upstream and only the first line of it is needed for us, and it then got backported to JDK11u. The initial load of the RISC-V backend lacks this, so needs a port for that line.
  2. For JDK-8202976: Add C1 lea patching support for x86, when backporting the code to 11u, noticing that the RISC-V initial patch on JDK19 didn't contain that as well, I added the trivial assertion from it by the way. It's not important at all. But for the record's sake, I still added a commit entry for that. Again, it is not important at all so actually whether dropping the issue or remaining it is okay...

Hi, I think you should add this issue https://bugs.openjdk.org/browse/JDK-8308277, to replace JDK-8243155

  1. This is fine, one is the AArch64 original code and the other is the RISC-V ported code. Redirecting to the latter one is fine. Thank you.

Hi, Maybe you forgot to add issue: 8282306, 8301313, 8308277, 8310873

  1. For JDK-8282306: os::is_first_C_frame(frame*) crashes on invalid link access: the original RISC-V initial patch on JDK19 contains this patch. Then, the issue got backported to JDK11u. So, after backporting the RISC-V backend, they naturally merged together without changes needed. Subtle, but adding this issue to the list of this PR is fine I think, by aligning with the JDK17u PR.

  2. For JDK-8301313: RISC-V: C2: assert(false) failed: bad AD file due to missing match rule: Sorry I don't get this one... The original code here doesn't seem to exist in this PR?

  3. For 8308277: Discussed in the 3rd item and it is fine.

  4. For JDK-8310873: Re-enable locked_create_entry symbol check in runtime/NMT/CheckForProperDetailStackTrace.java for RISC-V: The upstream code doesn't seem to exist in this initial patch, which is the same as the 5th item.

So finally and in summary, is adding issues 8291952, 8308277, 8282306 and dropping issues 8243155, 8202976 okay?

P.S. Please feel free to reply at any time, also fine after next week, i.e. after the holiday. Happy Lunar New Year.

@zifeihan
Copy link
Member

zifeihan commented Feb 8, 2024

Hi, Maybe you forgot to add issue: 8282306, 8301313, 8308277, 8310873

  1. For JDK-8282306: os::is_first_C_frame(frame*) crashes on invalid link access: the original RISC-V initial patch on JDK19 contains this patch. Then, the issue got backported to JDK11u. So, after backporting the RISC-V backend, they naturally merged together without changes needed. Subtle, but adding this issue to the list of this PR is fine I think, by aligning with the JDK17u PR.
  2. For JDK-8301313: RISC-V: C2: assert(false) failed: bad AD file due to missing match rule: Sorry I don't get this one... The original code here doesn't seem to exist in this PR?
  3. For 8308277: Discussed in the 3rd item and it is fine.
  4. For JDK-8310873: Re-enable locked_create_entry symbol check in runtime/NMT/CheckForProperDetailStackTrace.java for RISC-V: The upstream code doesn't seem to exist in this initial patch, which is the same as the 5th item.

Thank you for your reply, I checked again and you are right. Happy Lunar New Year!

@kuaiwei
Copy link
Contributor Author

kuaiwei commented Feb 8, 2024

/issue remove 8243155,8202976

@kuaiwei
Copy link
Contributor Author

kuaiwei commented Feb 8, 2024

/issue add 8291952,8308277,8282306

@openjdk
Copy link

openjdk bot commented Feb 8, 2024

@kuaiwei
Removing additional issue from issue list: 8243155.

Removing additional issue from issue list: 8202976.

@openjdk
Copy link

openjdk bot commented Feb 8, 2024

@kuaiwei
Adding additional issue to issue list: 8291952: riscv: Remove PRAGMA_NONNULL_IGNORED.

Adding additional issue to issue list: 8308277: RISC-V: Improve vectorization of Match.sqrt() on floats.

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

@RealFYang
Copy link
Member

It looks https://bugs.openjdk.org/browse/JDK-8247533 can fix the Clhsdb test error

Better to backport this fix in jdk11u-dev upstream if it really resolves this error. Then we can merge master and bring it to our riscv-port-jdk11u repo.

I may revert the change. Because I find the root cause of Clhsdb test failure is ptrace function is not fully supported in linux riscv. For example, PTRACE_GETREGS. We cat set kernel parameter "sysctl -w kernel.yama.ptrace_scope=1" to pass these tests.

@kuaiwei : I think the correct way to fix those Clhsdb test failure is to backport fix for https://bugs.openjdk.org/browse/JDK-8307955. I see they are passing with the following backport patch:
8307955-11u-backport.diff.txt

@kuaiwei
Copy link
Contributor Author

kuaiwei commented Feb 19, 2024

It looks https://bugs.openjdk.org/browse/JDK-8247533 can fix the Clhsdb test error

Better to backport this fix in jdk11u-dev upstream if it really resolves this error. Then we can merge master and bring it to our riscv-port-jdk11u repo.

I may revert the change. Because I find the root cause of Clhsdb test failure is ptrace function is not fully supported in linux riscv. For example, PTRACE_GETREGS. We cat set kernel parameter "sysctl -w kernel.yama.ptrace_scope=1" to pass these tests.

@kuaiwei : I think the correct way to fix those Clhsdb test failure is to backport fix for https://bugs.openjdk.org/browse/JDK-8307955. I see they are passing with the following backport patch: 8307955-11u-backport.diff.txt

It's a platform independent change. Do we need backport it to jdk11u upstream?

I can pass CLHSDB tests by changing sys parameter.

sysctl -w kernel.yama.ptrace_scope=1

@RealFYang
Copy link
Member

It looks https://bugs.openjdk.org/browse/JDK-8247533 can fix the Clhsdb test error

Better to backport this fix in jdk11u-dev upstream if it really resolves this error. Then we can merge master and bring it to our riscv-port-jdk11u repo.

I may revert the change. Because I find the root cause of Clhsdb test failure is ptrace function is not fully supported in linux riscv. For example, PTRACE_GETREGS. We cat set kernel parameter "sysctl -w kernel.yama.ptrace_scope=1" to pass these tests.

@kuaiwei : I think the correct way to fix those Clhsdb test failure is to backport fix for https://bugs.openjdk.org/browse/JDK-8307955. I see they are passing with the following backport patch: 8307955-11u-backport.diff.txt

It's a platform independent change. Do we need backport it to jdk11u upstream?

I can pass CLHSDB tests by changing sys parameter.

sysctl -w kernel.yama.ptrace_scope=1

Yeah, I think you are right. It's better to fix that in 11u upstream and merge to this repo.
I also performed tier1-3 and hotspot:tier4 tests on unmatched & licheepi-4a boards. Still good.
So I intend to think this is OK to go if there are no other opinions.

@zifeihan
Copy link
Member

Hi, I tested minecraft server, netbeans with this patch and everything works fine.

@RealFYang
Copy link
Member

@kuaiwei : We should list @zhengxiaolinX as co-author of this PR. You can use /contributor pull request command to do that [1].

[1] https://wiki.openjdk.org/display/SKARA/Pull+Request+Commands#PullRequestCommands-/contributor

@kuaiwei
Copy link
Contributor Author

kuaiwei commented Feb 20, 2024

/contributor @zhengxiaolinX

@openjdk
Copy link

openjdk bot commented Feb 20, 2024

@kuaiwei Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

@kuaiwei
Copy link
Contributor Author

kuaiwei commented Feb 20, 2024

/contributor add @zhengxiaolinX

@openjdk
Copy link

openjdk bot commented Feb 20, 2024

@kuaiwei
Contributor Xiaolin Zheng <xlinzheng@openjdk.org> successfully added.

@openjdk
Copy link

openjdk bot commented Feb 20, 2024

@kuaiwei This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8276799: Implementation of JEP 422: Linux/RISC-V Port
8283737: riscv: MacroAssembler::stop() should emit fixed-length instruction sequence
8285437: riscv: Fix MachNode size mismatch for MacroAssembler::verify_oops*
8287418: riscv: Fix correctness issue of MacroAssembler::movptr
8293100: RISC-V: Need to save and restore callee-saved FloatRegisters in StubGenerator::generate_call_stub
8295926: RISC-V: C1: Fix LIRGenerator::do_LibmIntrinsic
8291952: riscv: Remove PRAGMA_NONNULL_IGNORED
8308277: RISC-V: Improve vectorization of Match.sqrt() on floats
8282306: os::is_first_C_frame(frame*) crashes on invalid link access

Co-authored-by: Xiaolin Zheng <xlinzheng@openjdk.org>
Reviewed-by: fyang

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 59 new commits pushed to the riscv-port branch:

  • 89c9bff: Merge remote-tracking branch 'origin/master' into riscv-port
  • de3ae6f: 8318468: compiler/tiered/LevelTransitionTest.java fails with -XX:CompileThreshold=100 -XX:TieredStopAtLevel=1
  • 38a60d7: 8318983: Fix comment typo in PKCS12Passwd.java
  • cc88c3c: 8317307: test/jdk/com/sun/jndi/ldap/LdapPoolTimeoutTest.java fails with ConnectException: Connection timed out: no further information
  • ccd4c98: 8319668: Fixup of jar filename typo in BadFactoryTest.sh
  • a5e6577: 8315680: java/lang/ref/ReachabilityFenceTest.java should run with -Xbatch
  • 92cb5eb: 8319456: jdk/jfr/event/gc/collection/TestGCCauseWith[Serial|Parallel].java : GC cause 'GCLocker Initiated GC' not in the valid causes
  • ddc3e41: 8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use"
  • 6ecf198: 8316001: GC: Make TestArrayAllocatorMallocLimit use createTestJvm
  • f7cd14c: 8307123: Fix deprecation warnings in DPrinter
  • ... and 49 more: https://git.openjdk.org/riscv-port-jdk11u/compare/a0a61dda38bb7e7c11385d2acc9cd7deeecbd614...riscv-port

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 (@RealFYang) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Feb 20, 2024
@kuaiwei
Copy link
Contributor Author

kuaiwei commented Feb 20, 2024

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 20, 2024
@openjdk
Copy link

openjdk bot commented Feb 20, 2024

@kuaiwei
Your change (at version 4b01e13) is now ready to be sponsored by a Committer.

@RealFYang
Copy link
Member

Still test good on latest riscv-port branch of the repo. Let's get this merged.
/sponsor

@openjdk
Copy link

openjdk bot commented Feb 21, 2024

Going to push as commit 309291f.
Since your change was applied there have been 59 commits pushed to the riscv-port branch:

  • 89c9bff: Merge remote-tracking branch 'origin/master' into riscv-port
  • de3ae6f: 8318468: compiler/tiered/LevelTransitionTest.java fails with -XX:CompileThreshold=100 -XX:TieredStopAtLevel=1
  • 38a60d7: 8318983: Fix comment typo in PKCS12Passwd.java
  • cc88c3c: 8317307: test/jdk/com/sun/jndi/ldap/LdapPoolTimeoutTest.java fails with ConnectException: Connection timed out: no further information
  • ccd4c98: 8319668: Fixup of jar filename typo in BadFactoryTest.sh
  • a5e6577: 8315680: java/lang/ref/ReachabilityFenceTest.java should run with -Xbatch
  • 92cb5eb: 8319456: jdk/jfr/event/gc/collection/TestGCCauseWith[Serial|Parallel].java : GC cause 'GCLocker Initiated GC' not in the valid causes
  • ddc3e41: 8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use"
  • 6ecf198: 8316001: GC: Make TestArrayAllocatorMallocLimit use createTestJvm
  • f7cd14c: 8307123: Fix deprecation warnings in DPrinter
  • ... and 49 more: https://git.openjdk.org/riscv-port-jdk11u/compare/a0a61dda38bb7e7c11385d2acc9cd7deeecbd614...riscv-port

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Feb 21, 2024
@openjdk openjdk bot closed this Feb 21, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Feb 21, 2024
@openjdk
Copy link

openjdk bot commented Feb 21, 2024

@RealFYang @kuaiwei Pushed as commit 309291f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport integrated Pull request has been integrated
9 participants