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

Add Bitmanipulation Support #900

Closed
wants to merge 1 commit into from

Conversation

gullahmed1
Copy link

Introduction

This PR adds support for the zba, zbb, zbc and zbs extensions in CV32E40P core. This support will be enabled by changing parameter ZBITMANIP = 1 in cv32e40p_pkg.sv file and disabled by changing parameter ZBITMANIP = 0

Implementation

Added the support for all the ratified bitmanip extensions as defined under this bitmanip-spec

  • Zba: Address generation Instructions
  • Zbb: Basic Bit-Manipulation
  • Zbc: Carry-less Multiplication
  • Zbs: Single-bit Instructions

Verification

All the implemented instructions are passing riscv-arch tests for B type instructions. I have implemented the CV32E40P core as a DUT and SAIL as a reference model within the RISCOF framework, the successful execution of riscv-arch tests on both the CV32E40P core and the SAIL reference model and the comparison of their signatures provided the desired test results.

web
ter

@gullahmed1 gullahmed1 changed the base branch from master to dev November 7, 2023 13:09
@gullahmed1 gullahmed1 changed the base branch from dev to master November 7, 2023 13:25
@MikeOpenHWGroup
Copy link
Member

Hi @gullahmed1, thanks for your interest in CV32E40P. This is a significant PR and before we can merge it there are several things to consider:

  1. Please target your pull-request to the dev, not master branch.
  2. You must be covered by the Eclipse Contributor Agreement. If you are an employee of 10x-Engineers, you should be able to be covered by their Member Committer and Contributor Agreement. A colleague at 10x-Engineers can help you with that.
  3. There are some failing checks, probably due to Lint failures, please see CONTRIBUTING for help with that.
  4. The ZBITMANIP should be a top-level parameter (see my review comment about that).
  5. The contribution should also include an update to the documentation explaining the use of the parameter ZBITMANIP. At a minimum the Integration, Parameters and Standards Compliance sections of the User Manual.

Lastly, this is a significant change to the core and it is possible that acceptance will need to be reviewed by the OpenHW Group Cores Task Group or even the Technical Working Group. @davideschiavone and @DBees what are your thoughts on that?

@gullahmed1 gullahmed1 changed the base branch from master to dev November 7, 2023 13:50
@gullahmed1 gullahmed1 changed the base branch from dev to master November 7, 2023 13:52
@pascalgouedo
Copy link

pascalgouedo commented Nov 7, 2023

Hi @MikeOpenHWGroup @DBees @davideschiavone

Lastly, this is a significant change to the core and it is possible that acceptance will need to be reviewed by the OpenHW Group Cores Task Group or even the Technical Working Group. @davideschiavone and @DBees what are your thoughts on that?

For sure we have to discuss about accepting to merge this in CV32E40P or not.
For RISC-V P instructions addition for Tristan project, it has been mentioned that it would be done in a CV32E40P fork lately renamed CV32E42/CV32E42P or whatever.
Zb* instructions could be added to this new core rather than in the E40P.

Second reason is that we are in the final steps of finalizing CV32E40Pv2 to its Project Freeze state.
RISC-V Zb* was not part of Project Concept and we don't want to add any risk.
It would require to change parameter list, documentation and LEC checks must be done with respect to v1 and v2 (only LEC versus v1 is automatically done right now when merging on github).

@Silabs-ArjanB
Copy link
Contributor

@MikeOpenHWGroup @pascalgouedo @DBees @davideschiavone , fully agreed with @pascalgouedo ' post above. Such PRs should be done on a new core, e.g. CV32E42p as this goes against our agreed rules with respect to allowed changes on a frozen core.

rtl/cv32e40p_top.sv Outdated Show resolved Hide resolved
@gullahmed1 gullahmed1 force-pushed the bitmanip_support branch 2 times, most recently from 85bd177 to e3e7127 Compare November 8, 2023 09:12
@gullahmed1 gullahmed1 changed the base branch from master to dev November 8, 2023 09:15
.readthedocs.yaml Outdated Show resolved Hide resolved
@gullahmed1 gullahmed1 marked this pull request as ready for review November 8, 2023 10:53
@MikeOpenHWGroup MikeOpenHWGroup added the Status:Do-not-merge Pull request that should not be merged (yet) label Nov 8, 2023
@davideschiavone
Copy link
Contributor

Dear @gullahmed1, thanks for making the PR with your valuable contribution.

The ISA of the CV32E40P was fixed when the project started in OpenHW Group (back in 2019) and therefore we cannot have the RVB support in this CPU as it would change the ISA, against the maintenance rule of OpenHW Group.

However, as part of OpenHW Group, you can propose to the TWG a new CPU (that can be the fork of the cv32e40p) with a new architecture and new ISA - for your information, there will be a proposal in the TWG for a new CPU (tentative name cv32e40px) that will be a fork of cv32e40p, plus the core-v-x interface, plus the RVP and RVB ISA extensions - therefore we may want to retarget your contribution to this (probably) upcoming new CPU - it would be amazing to have your valuable contribution merged in!

@fatimasaleem
Copy link

Thank you all for the valuable feedback. For now, I think we can close this PR and will discuss the possibility of the new CPU with TWG.

@gullahmed1 gullahmed1 closed this Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:Do-not-merge Pull request that should not be merged (yet)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants