Skip to content

8278994: riscv: RVC support #34

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 21 commits into from
Closed

8278994: riscv: RVC support #34

wants to merge 21 commits into from

Conversation

zhengxiaolinX
Copy link
Collaborator

@zhengxiaolinX zhengxiaolinX commented Dec 20, 2021 •
edited
Loading

Hi team,

This patch includes the basic definition of the RVC instruction set and some cleanups. Tested a simple test/hotspot/jtreg/compiler/ folder on qemu.

Using <JAVA_HOME>/bin/java -XX:+UnlockExperimentalVMOptions -XX:+UseRVC -XX:+UnlockDiagnosticVMOptions -XX:+PrintAssembly -XX:PrintAssemblyOptions=no-aliases,numeric -XX:+PrintStubCode -XX:-TieredCompilation could show RVC instructions.

Thanks,
Xiaolin

Update: a compression rate calculation after this patch, evaluated by a general springboot petclinic program for all instructions generated. The toy source code of this histogram is at here - just used for the calculation.
fe82bcc

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Contributors

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/riscv-port pull/34/head:pull/34
$ git checkout pull/34

Update a local copy of the PR:
$ git checkout pull/34
$ git pull https://git.openjdk.java.net/riscv-port pull/34/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 34

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/riscv-port/pull/34.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 20, 2021

👋 Welcome back xlinzheng! 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.

@openjdk openjdk bot added the rfr Pull request is ready for review label Dec 20, 2021
@mlbridge
Copy link

mlbridge bot commented Dec 20, 2021 •
edited
Loading

Webrevs

@RealFYang
Copy link
Member

Looks like the previous discussion about compressible region is not reflected here in this PR. Could you please explain your approach?

@zhengxiaolinX
Copy link
Collaborator Author

zhengxiaolinX commented Dec 21, 2021 •
edited
Loading

Looks like the previous discussion about compressible region is not reflected here in this PR. Could you please explain your approach?

Thanks for the reviews, Felix. The Region things are at here in this diff and here, and currently, there is no usage of them. Besides, I sent messages to Yadong privately to discuss the previous design RVC_Assembler things. Maybe this patch could hold for a little moment after the discussion.

-- I have updated the links in this comment, for Github is prone to fold the file, causing the links to lose effect.

@zhengxiaolinX
Copy link
Collaborator Author

I have discussed with Yadong and we may hold the optional RVC_Assembler implementation temporarily. This will not affect the CompressibleRegion implementation so please feel safe. Now this patch might be ready to review because it seems less possible to change in design, and thank you for your patience.

RealFYang
RealFYang requested changes Dec 24, 2021
View reviewed changes
@zhengxiaolinX zhengxiaolinX changed the title 8278994: riscv: RVC: basic instruction set 8278994: riscv: RVC support Dec 24, 2021
@zhengxiaolinX
Copy link
Collaborator Author

Thanks for your reviews, Felix. In the morning Yadong discussed with me your review comments, so I have pushed additional patches to ensure nearly all functions I defined have been properly 'use'd. I think that might increase the burden to review this patch because of the new changes, and I feel quite sorry for that.

Now excluding c.beqz, c.bnez and c.j, which need to be supported in MachBranchNodes afterward by Wei Kuai, all RVC instructions have been covered in usage by adding CompressibleRegions in the ad file. Also, I revised the comments in the code based on your suggestions and hope they look good.

This may need another period to review so no rush. Also, due to Wei Kuai's code is added into this patch now, I assume I should add him as a co-author of this patch.

/contributor add Wei Kuai [email protected]

This changes are tested under test/hotspot/jtreg/compiler and test/hotspot/jtreg/gc/shenandoah on qemu, and a springboot application with UseRVV and UseZGC together on qemu. I think they could cover these new changes.

@openjdk
Copy link

openjdk bot commented Dec 24, 2021

@zhengxiaolinX Could not parse Wei Kuai [email protected] as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <[email protected]>

@zhengxiaolinX
Copy link
Collaborator Author

Hmm. Hope this time it could succeed.

/contributor add Wei Kuai [email protected]

@openjdk
Copy link

openjdk bot commented Dec 24, 2021

@zhengxiaolinX Could not parse Wei Kuai [email protected] as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <[email protected]>

@zhengxiaolinX
Copy link
Collaborator Author

-- Quite feel sorry for the disturbance :-(

/contributor add Wei Kuai [email protected]

@openjdk
Copy link

openjdk bot commented Dec 24, 2021

@zhengxiaolinX
Contributor Wei Kuai <[email protected]> successfully added.

@zhengxiaolinX
Copy link
Collaborator Author

Rebased and fixed conflicts with RVB(#37)'s initial load. Tested build and a simple spring-boot application with/without RVC.

yadongw
yadongw reviewed Jan 5, 2022
View reviewed changes
RealFYang
RealFYang requested changes Jan 17, 2022
View reviewed changes
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.

Still some suggestions about the comments, otherwise looks good.

RealFYang
RealFYang approved these changes Jan 18, 2022
View reviewed changes
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.

Approved. Thanks.

@openjdk
Copy link

openjdk bot commented Jan 18, 2022 •
edited
Loading

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

8278994: riscv: RVC support

Co-authored-by: Wei Kuai <[email protected]>
Reviewed-by: fyang, 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 no new commits pushed to the riscv-port branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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, @yadongw) 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 Jan 18, 2022
@zhengxiaolinX
Copy link
Collaborator Author

Thank you for the nice reviews, Felix and Yadong!

/integrate

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

openjdk bot commented Jan 18, 2022

@zhengxiaolinX
Your change (at version 502ff3d) is now ready to be sponsored by a Committer.

@yadongw
Copy link
Collaborator

yadongw commented Jan 18, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Jan 18, 2022

Going to push as commit fe82bcc.

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

openjdk bot commented Jan 18, 2022

@yadongw @zhengxiaolinX Pushed as commit fe82bcc.

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

@zhengxiaolinX zhengxiaolinX deleted the cext-patch1 branch January 18, 2022 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yadongw yadongw yadongw approved these changes

@RealFYang RealFYang RealFYang approved these changes

Assignees
No one assigned
Labels
integrated Pull request has been integrated
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zhengxiaolinX @RealFYang @yadongw