-
Notifications
You must be signed in to change notification settings - Fork 679
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
Adding Support for Zba, Zbb, Zbc and Zbs extensions to CVA6 #878
Adding Support for Zba, Zbb, Zbc and Zbs extensions to CVA6 #878
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is really great, thanks for the pull request! Logic looks good to me in general.
The only comments I have are around we handle the inclusion, we usually use parameters for that and try to avoid ifdefs
as much as possible. So I've added some comments on how I think we can make this work using only parameters :-) Please do let me know whether there is something unclear from the comments?
Thank you @zarubaf for the review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Terrific, thanks soo much. Only minor code formatting things and then we are good to go!
That's a very great contribution !! Thank for that As I am not in office in the coming days, I will available to check the PR from June1st to check max frequency, gate count, and functional regression in 32 and 64 bits. I let you know. |
Sure. No Problem! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything on my end is green! I will trigger the CI and @JeanRochCoulon you can merge if the PPA is okay for you! Thanks again @M-Ijaz-10x, that is a great contribution!
Happy to contribute. :) Looking forward to contribute more in the future developments. |
While making the suggested changes on the PR, a bug was overlooked in the design related to exceptions (illegal instruction in the decoder) which was probably causing the tests to fail (CI checks timed-out). This is fixed in the latest commit on this PR. These changes are tested by running RISC-V Compliance tests and Bitmanip tests which came out to be clean now. |
Hello, thanks again for this contribution. I would request some modifications to make VCS and synthesis work. By running the Continuous Integration, I get some errors:
In another hand, as we would be pleased to enable/disable the bitmanip support by CVA6, could you add a config variable, as done in core/include/cv32a6_XXX_config_pkg.sv to configure it? The generation of the RTL dedicated to bitmanip should be conditioned by it (decode, alu,...). Regards |
Hello @JeanRochCoulon, the above error was due to the changes made in PR-847. Above commit proposed a workable solution for this error, but @spersvold will need to review the fix. The remaining two errors have also been resolved in the latest commits.
From the start, we are confining all the bitmanip RTL changes under the bitmanip parameter defined in ariane_pkg.sv as suggested by @zarubaf |
core/alu.sv
Outdated
ROL: result_o = (fu_data_i.operand_a << fu_data_i.operand_b[5:0]) | (fu_data_i.operand_a >> (riscv::XLEN-fu_data_i.operand_b[5:0])); | ||
ROLW: result_o = {{riscv::XLEN-32{rolw[31]}}, rolw}; | ||
ROR, RORI: result_o = (fu_data_i.operand_a >> fu_data_i.operand_b[5:0]) | (fu_data_i.operand_a << (riscv::XLEN-fu_data_i.operand_b[5:0])); | ||
RORW, RORIW: result_o = {{riscv::XLEN-32{rorw[31]}}, rorw}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, it seems that you did not resolve it. It turned out that I knew another technique but forget to add it in last review
- It is observed that
rol(x) = ror(reverse(w))
, you can useror(shift_op_a)
to getrol(a)
, just as the current design usessrl
to getsll
- It is observed that
rorw(x[31:0]) = ror({x[31:0],x[31:0]})[31:0]
. If you still want to leaveRORW
andROR
as two separate datapaths, at leastRORW
andROLW
could be merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @ZenithalHourlyRate for your review, really appreciate it. I will make this last change in a separate follow-up PR. For now, I am keeping it in my notes. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good to me. However, I noticed that "instret" will not work as intended when in 32-bit mode if you wrap it (because the incrementing logic in lines 333-349 is only dealing with the temporary variable "instret" which is xlen_t only, and then casting to instret_d after).
This should be fixed in a different commit though, so I approve this one.
Hello, |
I synthetized in 32 bits version, please find the error reported by dc_shell Synsopsys tool: Error: ../..//core/alu.sv:287: Array index out of bounds fu_data_i[63:56], valid bounds are [31:0]. (ELAB-298) Regards |
9e1c6b7
to
20a181a
Compare
Hi @fatimasaleem, in order to make the validation of CVA6 synthesis smoother, we are implementing synthesis error/warning message extraction in the Thales public CI dashboard (https://riscv-ci.pages.thales-invia.fr/dashboard/dashboard_cva6.html). Since this is still work in progress, here are the summaries of synthesis error/warning logs produced by our prototype filter on your latest push (20a181a). Please let @JeanRochCoulon, @yanicasa and myself (@zchamski) know if they are sufficient for you to work on your code. Synthesis 32b (cv32a60x): Synthesis 64b (cv64a6_imafdc_sv39): As soon as the log extractor is stable (and incorporates the required usability adjustments if any) we will announce it in due way. |
Co-authored-by: Florian Zaruba <florianzaruba@gmail.com>
Co-authored-by: Florian Zaruba <florianzaruba@gmail.com>
20a181a
to
e25192b
Compare
Hi @zchamski FYI: I have also run the latest arch-tests regression using the core-v-verif infrastructure with the PR-1254, it would be great if we could add these new arch-tests in the CI checks also. |
Hi @fatimasaleem, the synthesis in the CI looks good! BTW, the Thales public CI is triggered automatically on every Github push to either cva6 or core-v-verif, and you can check the CI results as follows:
Please let the usual suspects (@zchamski, @yanicasa, @JeanRochCoulon) know about your experience of using the Thales CI dashboard. And to answer an almost certain question: no, there are no direct links to the inside of the dashboard, you have to click your way through... |
Thank you @zchamski for the details. It really is very helpful. @JeanRochCoulon Since the synthesis and regression run look green, can we merge this? or is there anything else that needs to be done/fixed? Thanks & Regards, |
Hello @fatimasaleem @M-Ijaz-10x @spersvold @zchamski |
In parallel, @ASintzoff will have a look to the arc-test suite PR. It would be nice to get it |
Introduction
This PR is the continuation of issue-451 and adds support for zba, zbb, zbc and zbs extensions under the wrapper of a compiler directive BITMANIP. These changes have been tested with self-written single instruction tests and using the compliance tests with the core-v-verif infrastructure.
Implementation
Added the support for all the ratified bitmanip extensions as defined under this bitmanip-spec
Integration of bitmanip hardware with cva6
The major changes have been done in the decoder which includes adding the RISC-V Bitmanip opcodes, selecting the Functional Unit from Multiplier and ALU of the Ariane core and implementing design logic for Bitmanip instructions in ALU and Multiplier FUs.
Verification
Assembly Tests
The functionality of each implemented Bitmanip instruction has been verified by running individual assembly tests related to that particular instruction on the updated design.
Core-v-verif environment
The OpenHW group’s core-v-verif verification environment has been modified to support the latest riscv-arch-test infrastructure (PR: Added the support to run latest riscv-arch-tests in core-v-verif CVA6 infrastructure) and used to run the compliance tests (base tests + bitmanip tests) on the updated design. All the required bitmanip tests have successfully passed.