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

8290154: [JVMCI] partially implement JVMCI for RISC-V #9587

Closed
wants to merge 11 commits into from

Conversation

Zeavee
Copy link
Contributor

@Zeavee Zeavee commented Jul 21, 2022

This patch adds a partial JVMCI implementation for RISC-V, to allow using the GraalVM Native Image RISC-V LLVM backend, which does not use JVMCI for code emission.
It creates the jdk.vm.ci.riscv64 and jdk.vm.ci.hotspot.riscv64 packages, as well as implements a part of jvmciCodeInstaller_riscv64.cpp. To check for correctness, it enables JVMCI code installation tests on RISC-V. More testing is performed in Native Image.


Progress

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

Issue

  • JDK-8290154: [JVMCI] partially implement JVMCI for RISC-V

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9587/head:pull/9587
$ git checkout pull/9587

Update a local copy of the PR:
$ git checkout pull/9587
$ git pull https://git.openjdk.org/jdk pull/9587/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9587

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9587.diff

@bridgekeeper bridgekeeper bot added the oca Needs verification of OCA signatory status label Jul 21, 2022
@bridgekeeper
Copy link

bridgekeeper bot commented Jul 21, 2022

Hi @Zeavee, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user Zeavee" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@Zeavee
Copy link
Contributor Author

Zeavee commented Jul 21, 2022

/covered

@openjdk
Copy link

openjdk bot commented Jul 21, 2022

@Zeavee The following labels will be automatically applied to this pull request:

  • build
  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added build build-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org labels Jul 21, 2022
@bridgekeeper bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Jul 21, 2022
@bridgekeeper
Copy link

bridgekeeper bot commented Jul 21, 2022

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@Zeavee Zeavee changed the title 8290154: Add support for JVMCI in RISC-V 8290154: [JVMCI] Implement JVMCI for RISC-V Jul 21, 2022
@bridgekeeper bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Jul 21, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 21, 2022
@mlbridge
Copy link

mlbridge bot commented Jul 21, 2022

@RealFYang
Copy link
Member

RealFYang commented Jul 25, 2022

Hi, I see some JVM crash when I try the following test with fastdebug build with your patch:
test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.code.test/src/jdk/vm/ci/code/test/SimpleDebugInfoTest.java

////////////////////////////////////////////////////
Internal Error (/home/fyang/openjdk-jdk/src/hotspot/cpu/riscv/nativeInst_riscv.cpp:118), pid=1154 063, tid=1154084
assert(NativeCall::is_call_at((address)this)) failed: unexpected code at call site
////////////////////////////////////////////////////

@openjdk-notifier
Copy link

@Zeavee Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

@Zeavee
Copy link
Contributor Author

Zeavee commented Jul 25, 2022

Hi, I see some JVM crash when I try the following test with fastdebug build with your patch: test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.code.test/src/jdk/vm/ci/code/test/SimpleDebugInfoTest.java

Thank you for pointing this out, I did not run the patch with fastdebug. I saw other issues after solving this one, so I will take some time to solve them as well.

}

public boolean isFP() {
switch(this) {
Copy link
Member

Choose a reason for hiding this comment

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

let's add space after switch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

Copy link
Member

@magicus magicus left a comment

Choose a reason for hiding this comment

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

Build changes look ok. You still need reviews for the actual code changes.

@magicus
Copy link
Member

magicus commented Aug 8, 2022

/reviewers 3

@openjdk
Copy link

openjdk bot commented Aug 8, 2022

@Zeavee 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:

8290154: [JVMCI] partially implement JVMCI for RISC-V

Reviewed-by: ihse, dnsimon, yadongwang

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 320 new commits pushed to the master branch:

  • b38bed6: 8294308: Allow dynamically choosing the MEMFLAGS of a type without ResourceObj
  • 118d93b: 8294907: Remove unused NativeLookup::dll_load
  • 1fda842: 8291429: java/lang/Thread/virtual/ThreadAPI.java timed out on single core system
  • 2d25c0a: 8292280: Unused field 'keyListener' in BasicRadioButtonUI
  • 0ad6803: 8293810: Remove granting of RuntimePermission("stopThread") from tests
  • cf84c8e: 8292975: javac produces code that crashes with LambdaConversionException
  • f3a44a4: 8075916: The regression-swing case failed as colored text is not shown on disabled checkbox and radio button with Nimbus LAF
  • 37bd4fb: 6852577: Only for Nimbus LAF UIManager.get("PasswordField.echoChar") is null
  • d4c9a88: 6560981: (cal) unused local variables in GregorianCalendar, etc.
  • 5dd851d: 8281453: New optimization: convert ~x into -1-x when ~x is used in an arithmetic expression
  • ... and 310 more: https://git.openjdk.org/jdk/compare/9ef6c0925ae5a0ca774b23f6318551417a53e6c6...master

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 (@magicus, @dougxc, @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 Aug 8, 2022
@openjdk
Copy link

openjdk bot commented Aug 8, 2022

@magicus
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 1 Reviewer, 2 Authors).

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Aug 8, 2022
@dougxc
Copy link
Member

dougxc commented Aug 8, 2022

Please change the title of this PR and the JBS issue to "[JVMCI] partially implement JVMCI for RISC-V" to more accurately reflect the code changes.

@Zeavee Zeavee changed the title 8290154: [JVMCI] Implement JVMCI for RISC-V 8290154: [JVMCI] partially implement JVMCI for RISC-V Aug 9, 2022
@Zeavee
Copy link
Contributor Author

Zeavee commented Aug 22, 2022

Sorry for the delay, but all the JVMCI tests now pass.

@dougxc
Copy link
Member

dougxc commented Oct 5, 2022

@RealFYang are you ok with deferring further changes to a future RFE?

@RealFYang
Copy link
Member

Hello, this PR has been stuck for some time now. What should I do to proceed?

The current version does not build. I will take another look after this is rebased on the latest jdk master.

@openjdk-notifier
Copy link

@Zeavee Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

@Zeavee
Copy link
Contributor Author

Zeavee commented Oct 6, 2022

The current version does not build. I will take another look after this is rebased on the latest jdk master.

Sorry for the delay, I rebased the PR and fixed the building issue.

Copy link
Contributor

@yadongw yadongw left a comment

Choose a reason for hiding this comment

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

lgtm(not a reviewer)

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 7, 2022
@Zeavee
Copy link
Contributor Author

Zeavee commented Oct 7, 2022

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 7, 2022
@openjdk
Copy link

openjdk bot commented Oct 7, 2022

@Zeavee
Your change (at version e7913fa) is now ready to be sponsored by a Committer.

@dougxc
Copy link
Member

dougxc commented Oct 7, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Oct 7, 2022

Going to push as commit 7a194d3.
Since your change was applied there have been 320 commits pushed to the master branch:

  • b38bed6: 8294308: Allow dynamically choosing the MEMFLAGS of a type without ResourceObj
  • 118d93b: 8294907: Remove unused NativeLookup::dll_load
  • 1fda842: 8291429: java/lang/Thread/virtual/ThreadAPI.java timed out on single core system
  • 2d25c0a: 8292280: Unused field 'keyListener' in BasicRadioButtonUI
  • 0ad6803: 8293810: Remove granting of RuntimePermission("stopThread") from tests
  • cf84c8e: 8292975: javac produces code that crashes with LambdaConversionException
  • f3a44a4: 8075916: The regression-swing case failed as colored text is not shown on disabled checkbox and radio button with Nimbus LAF
  • 37bd4fb: 6852577: Only for Nimbus LAF UIManager.get("PasswordField.echoChar") is null
  • d4c9a88: 6560981: (cal) unused local variables in GregorianCalendar, etc.
  • 5dd851d: 8281453: New optimization: convert ~x into -1-x when ~x is used in an arithmetic expression
  • ... and 310 more: https://git.openjdk.org/jdk/compare/9ef6c0925ae5a0ca774b23f6318551417a53e6c6...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 7, 2022
@openjdk openjdk bot closed this Oct 7, 2022
@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 Oct 7, 2022
@openjdk
Copy link

openjdk bot commented Oct 7, 2022

@dougxc @Zeavee Pushed as commit 7a194d3.

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

@Zeavee Zeavee deleted the JDK-8290154 branch October 10, 2022 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
6 participants