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 new extensions Zcb, Zfh, Zbkb, partial Zfa #921

Open
wants to merge 77 commits into
base: master
Choose a base branch
from

Conversation

JJ-Gaisler
Copy link

I have added a few new extensions to RISC-DV, I would appreciate if someone could take a look at the major changes and see if they disagree with any of the decisions.

The major changes include:

+Zcb (code reduction):
since no gcc support is in the mainline assembler yet these instructions are injected as raw data. Coverage class has been extended with these instructions and the illegal instruction class should not be able to generate legal Zcb instruction if the extension is enabled.

+Zfh (half-precision floats):
Similarly to single and double precission, RISC-V now support half-precission. Most things are identical to the F & D extension which is why the pre-existing floating_point class was extended rather than a separate class. Inital coverage support has also been added. FMV_X_H is temporarily excluded due to a bug in OVPsim.

+Zbkb (crypto):
The complementary instruction not defined in Zbb has been added to the instruction generator. This required a few instructions which were previously defined in the non-ratified bitmanip instruction class to be moved to Zbkb. I believe this makes more sense since Zbkb is ratified. Inital coverage has also been added for these instructions.

+Zfa:
Instructions has been defined for Zfa, more work will follow once OVPsim supports this extension.

The illegal instruction class has seen the following bug fixes: 1) Not all legal op-codes were considered for compressed floating instructions leading to legal instructions being injected in undesired locations (sometimes overwriting the stack-pointer since no reserved gpr check was performed).
2) ldsp and lqsp are only reserved for some XLENs.

ovpsim_log_to_trace:
Fixed a bug where some CSRs were not considered CSRs but GPRs

run.py:
Fixed bug in the regex replace, not all C's in the march string should be replaced since that breaks all zc... extension substrings.

load_store_instr_lib:
Added the Zcb load and store instructions to this class.

riscv_instr_pkg:
Added new extensions and instructions.

@scottj97
Copy link
Contributor

I'm only a user of this project and not a contributor or even a regular reviewer, so take this with a grain of salt.

When the change is presented as one giant 1400+ line commit, it is difficult to understand, difficult to review, difficult to merge with other changes, etc.

Consider instead using Git best practices and building the PR as a sequence of commits. I can easily see this being a dozen commits or more.

Comment on lines 114 to 116
// TODO: There is a confirmd bug in the Jan 2023 version OVPsim where sign extension is
// not handled correctly, remove the FMV_X_H exclusion once that is fixed.
if (instr_name == FMV_X_H) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems inappropriate for the master branch of this project. OVPsim bugs are not our problem. If a user of this tool needs to disable it because they need OVPsim to work then they can add this in their private fork.

At least, it should be a separate commit on a separate branch ovpsim-bug-workaround.

Copy link
Author

Choose a reason for hiding this comment

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

Fair point, I have removed the lines from the PR.

@scottj97
Copy link
Contributor

Suggested new PR title: "Add new extensions Zcb, Zfh, Zbkb, partial Zfa"

@JJ-Gaisler JJ-Gaisler changed the title RISCV-DV has been extended with new RISC-V extensions Add new extensions Zcb, Zfh, Zbkb, partial Zfa Jan 31, 2023
@JJ-Gaisler
Copy link
Author

I'm only a user of this project and not a contributor or even a regular reviewer, so take this with a grain of salt.

When the change is presented as one giant 1400+ line commit, it is difficult to understand, difficult to review, difficult to merge with other changes, etc.

Consider instead using Git best practices and building the PR as a sequence of commits. I can easily see this being a dozen commits or more.

* [Writing Reviewable Code](https://secure.phabricator.com/book/phabflavor/article/writing_reviewable_code/)

* [Understanding the Git Workflow](https://sandofsky.com/workflow/git-workflow/)

Hi! I am a complete novice when it comes to PRs and I realize this may not have been the ideal PR in retrospect. I do unfortunately not have any intermediate commits from when I was developing this, which is why it ended up being one large commit. I could however make an effort to try to separate the PR into more than one commit, most extensions and bug-fixes should be feasible to put into different commits. I could do that if that makes things on the merging end much easier? What do other reviewers think? Is this PR acceptable as is or would you prefer if I with some delay re-submit the PR?

@scottj97
Copy link
Contributor

Also it's preferable to squash bugfixes (such as 030c3e8 appears to be) back into the original commits (using git rebase --interactive followed by git push --force-with-lease) so that the bug never appears in the history.

fanghuaqi and others added 2 commits February 1, 2023 09:12
This vi swap file maybe added accidently.
Zcb (code reduction):
since no gcc support is in the mainline assembler yet these instructions are
injected as raw data. Coverage class has been extended with these
instructions and the illegal instruction class should not be able to
generate legal Zcb instruction if the extension is enabled.

Zfh (half-precision floats):
Similarly to single and double precission, RISC-V now support
half-precission is now supported. Most things are identical to the F & D
extension which is why the pre-existing floating_point class was
extended rather than a separate class. Inital coverage support has also been
added. If testing with OVPsim the Jan 2023 version has a bug for FMV_X_H.

Zbkb (crypto):
The complementary instruction not defined in Zbb has been added to the
instruction generator. This required a few instructions which were
previously defined in the non-ratified bitmanip instruction class to be
moved to Zbkb. I believe this makes more sense since Zbkb is ratified.
Inital coverage has also been added for these instructions.

Zfa:
Instructions has been defined for Zfa, more work will follow once OVPsim
supports this extension.

The illegal instruction class has seen the following bug fixes:
1) Not all legal op-codes were considered for compressed floating
instructions leading to leagal instructions being injected in undesired
locations (sometimes overwriting stack-pointer since no reserved gpr check was
performed).
2) ldsp and lqsp are only reserved at some XLENs.

ovpsim_log_to_trace:
Fixed a bug where some CSRs were not considered CSRs but GPRs

run.py:
Fixed bug in the regex replace, not all C's in the march string should
be replaced since that breaks all zc... extension substrings.

load_store_instr_lib:
Added the Zcb load and store instructions to this class.

riscv_instr_pkg:
Added new extensions and instructions.
Fixed bug introduced in aee6c4d where
"Push USP from gpr.SP onto the kernel stack" should be XLEN dependent and not
hardcoded to +-4.
JJ-Gaisler and others added 20 commits October 2, 2023 15:57
* Prior commit grouped zfh convert instructions incorrectly.
* the get_fpr function needed to handle ZERO as a special case
compiled but code generated seem to be incorrect
rv64zbkc removed, since there are not additionall instructios from rv32zbkc
typos when copying code from bkb to bkc or zbc to zbkc
…era sim)

contraint for order random generation wasnt compatible with rand_mode(0) in those variables
Alfonso Carballo Boullosa added 30 commits February 21, 2024 13:50
…en converted to gorci which produced coverage errors

conversion removed and now orc.b can be covered
…rrect and was producing a c.and instruction
round mode is now processed from spike
unreachable hazards are now removed from cov
the csr values are not being used yet, but the new expression is required for the reg wr
since LUI doesnt have any rs, cant have a RAW hazard
lqsp coverage group is now created if RV32Q ext is enabled
exlusions in zero register for some instr that are others when rs1==0 or rs2==0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants