-
Notifications
You must be signed in to change notification settings - Fork 76
feat: add SystemVerilog backend generator #1090
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
base: main
Are you sure you want to change the base?
Conversation
612dfc3
to
e9cc253
Compare
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.
This is looking great and has addressed most of my feedback from #1054. I've left some new comments inline (and carried over any from #1054 that haven't been addressed yet.
The other things mentioned in #1054 that haven't been handled here yet and don't fit into an inline comment are the following:
- Similar to the C header, it might be worth including the exception causes in the generated SystemVerilog header as localparams.
- Add a task to the regression GitHub Action to generate the SystemVerilog header (similar to what is done for the C header).
- Add a golden output comparison to ensure the output is not unintentionally changed (similar to what is done for the instruction appendix).
def parse_args(): | ||
parser = argparse.ArgumentParser( | ||
description="Generate SystemVerilog package from RISC-V instruction definitions" | ||
) | ||
parser.add_argument( | ||
"--inst-dir", | ||
default="../../../gen/resolved_spec/_/inst/", | ||
help="Directory containing instruction YAML files", | ||
) | ||
parser.add_argument( | ||
"--csr-dir", | ||
default="../../../gen/resolved_spec/_/csr/", | ||
help="Directory containing CSR YAML files", | ||
) | ||
parser.add_argument( | ||
"--output", | ||
default="riscv_decode_package.svh", | ||
help="Output SystemVerilog file name", | ||
) | ||
parser.add_argument( | ||
"--extensions", | ||
default="A,D,F,I,M,Q,Zba,Zbb,Zbs,S,System,V,Zicsr,Smpmp,Sm,H,U,Zicntr,Zihpm,Smhpm", | ||
help="Comma-separated list of enabled extensions. Default includes standard extensions.", | ||
) | ||
parser.add_argument( | ||
"--arch", | ||
default="BOTH", | ||
choices=["RV32", "RV64", "BOTH"], | ||
help="Target architecture (RV32, RV64, or BOTH). Default is RV64.", | ||
) | ||
parser.add_argument( | ||
"--verbose", "-v", action="store_true", help="Enable verbose logging" | ||
) | ||
parser.add_argument( | ||
"--include-all", | ||
action="store_true", | ||
help="Include all instructions and CSRs regardless of extensions", | ||
) |
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 C and SystemVerilog headers should have the same CLI and same defaults. They produce similar artifacts, so having different interfaces introduces unnecessary confusion and complexity.
For example, the C generator has a --debug
flag while this uses a --verbose
flag. Both flags seem to do the same thing. Another example is the C header includes an empty list as the default argument for --extensions
but this includes an arbitrary list of extensions by default.
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.
Looks like --extensions
was changed to match, but there is still --debug
in one and --verbose
in the other. Shall we unify them?
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.
What about moving a version of the parse_args
function into the shared generators.py
file? I imagine we will want similar arguments for most (if not all) of the generators. Then individual generators can add any extra arguments that they need on top of that.
e9cc253
to
6880f49
Compare
6880f49
to
3ece684
Compare
Thanks for the review Mr. Jordan, Should it be an int or a 6 bit logic signal? What should be the number format? |
I'd probably be in favor of using
The new format should be fine. |
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.
Looking very close. A few more comments below. We also still need to add a golden version of this to compare against in CI, but maybe that should be done in a separate PR for both this and the C backends (and maybe the Go backend as well).
3ece684
to
5098a8e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1090 +/- ##
=======================================
Coverage 46.05% 46.05%
=======================================
Files 11 11
Lines 4942 4942
Branches 1345 1345
=======================================
Hits 2276 2276
Misses 2666 2666
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
One final, minor nit, but otherwise this looks ready. I discovered a fair amount of missing data while running it (issues opened for some of it, more to come soon), but with those entries added manually, I was about to successfully use the generated output in place of the riscv-opcodes version.
"This branch has conflicts that must be resolved." Can you rebase this PR, @AliAlaa88 ? |
Implements a new backend generator for SystemVerilog output, matching the exact format used by riscv-opcodes/inst.sverilog. This provides direct compatibility with hardware designs using the riscv-opcodes SystemVerilog package format. Features: - Generates SystemVerilog package with instruction and CSR definitions - Outputs 32-bit instruction patterns with proper bit encoding - Handles compressed (16-bit) instructions correctly - Supports all standard RISC-V extensions - Integrated with the ./do build system as gen:sverilog task The generator produces output identical to riscv-opcodes format: - Instructions as 'localparam [31:0] NAME = 32'bpattern' - CSRs as 'localparam logic [11:0] CSR_NAME = 12'haddr' - Proper alignment and formatting for readability Tested against riscv-opcodes/inst.sverilog to ensure format compatibility. Signed-off-by: Afonso Oliveira <[email protected]>
…and output file naming
… the other items in the package
Co-authored-by: Jordan Carlin <[email protected]> Signed-off-by: Alieldin Alaa <[email protected]>
79ceb0c
to
0ba1fb9
Compare
I did. |
This pull request adds support for generating a SystemVerilog package containing RISC-V instruction and CSR definitions, and enhances the instruction encoding logic to support a new schema format. The changes include introducing a new generator script, updating the encoding extraction logic to handle new formats, and integrating the SystemVerilog generation into the build process.
SystemVerilog Generation:
sverilog_generator.py
that generates a SystemVerilog package (riscv_decode_package.svh
) with instruction and CSR encodings, including formatting and argument parsing for flexible generation.gen:sverilog
) to invoke the SystemVerilog generator, with options for configuration and output directory.gen/sverilog
output directory as part of the build process.Instruction Encoding Enhancements:
build_match_from_format
function to extract encoding match strings from the newformat
field in instruction definitions, supporting opcodes with specific bit fields.build_match_from_format
when the legacyencoding
field is missing, improving compatibility with new schema definitions.CSR Handling:
.RV32
suffix, regardless of architecture.