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

[BUG] csr_regfile is not compilant with SystemVerilog Standard #2824

Open
1 task done
cathales opened this issue Mar 12, 2025 · 0 comments
Open
1 task done

[BUG] csr_regfile is not compilant with SystemVerilog Standard #2824

cathales opened this issue Mar 12, 2025 · 0 comments
Labels
Type:Bug For bugs in the RTL, Documentation, Verification environment or Tool and Build system

Comments

@cathales
Copy link
Contributor

Is there an existing CVA6 bug for this?

  • I have searched the existing bug issues

Bug Description

According to IEEE Std 1800-2017 SystemVerilog 3.1a Language Reference Manual, Accellera’s Extensions to Verilog®, Section 3.14 Casting, the syntax before the ' can be:

  • a decimal-based literal number >0 (the size as a literal)
  • the signing (signed or unsigned)
  • a type identifier, optionally package-scoped (with ::)

Unless I missed something (a newer version of the standard?), the casts used to build IsaCode in core/csr_regfile.sv are not covered by the official specification of the language.
If we do not comply with the standard, we have no guarantee that the tools perform as we expect.
So we should probably find a way to comply with the standard.

Here is an example from csr_regfile:

cva6/core/csr_regfile.sv

Lines 282 to 295 in c3fe25a

localparam logic [CVA6Cfg.XLEN-1:0] IsaCode = (CVA6Cfg.XLEN'(CVA6Cfg.RVA) << 0) // A - Atomic Instructions extension
| (CVA6Cfg.XLEN'(CVA6Cfg.RVB) << 1) // B - Bitmanip extension
| (CVA6Cfg.XLEN'(CVA6Cfg.RVC) << 2) // C - Compressed extension
| (CVA6Cfg.XLEN'(CVA6Cfg.RVD) << 3) // D - Double precision floating-point extension
| (CVA6Cfg.XLEN'(CVA6Cfg.RVF) << 5) // F - Single precision floating-point extension
| (CVA6Cfg.XLEN'(CVA6Cfg.RVH) << 7) // H - Hypervisor extension
| (CVA6Cfg.XLEN'(1) << 8) // I - RV32I/64I/128I base ISA
| (CVA6Cfg.XLEN'(1) << 12) // M - Integer Multiply/Divide extension
| (CVA6Cfg.XLEN'(0) << 13) // N - User level interrupts supported
| (CVA6Cfg.XLEN'(CVA6Cfg.RVS) << 18) // S - Supervisor mode implemented
| (CVA6Cfg.XLEN'(CVA6Cfg.RVU) << 20) // U - User mode implemented
| (CVA6Cfg.XLEN'(CVA6Cfg.RVV) << 21) // V - Vector extension
| (CVA6Cfg.XLEN'(CVA6Cfg.NSX) << 23) // X - Non-standard extensions present
| ((CVA6Cfg.XLEN == 64 ? 2 : 1) << CVA6Cfg.XLEN - 2); // MXL

Do we really need these casts?
All the values around the casts can fit in a signed 32 bit number, which is IIRC the default size for integers.
The one which is the closest to overflow is MXL, which does not have the cast.
So we could probably remove all these casts?

Later in the same file, we use MIP values, which are little integer constants (at most 4096).
So these casts should probably be removed there too.
To avoid implicit cast warnings, we could probably make the constants unsized (e.g. int unsigned).
After all, if we always re-size them, there is no real reason to have them sized.

If we really need these casts, the standard suggests to use a local typedef <the type> the_type_alias; (types with generic parameters do not exist in SystemVerilog so we cannot put them in a package) and then cast with the_type_alias'(expression).

@cathales cathales added the Type:Bug For bugs in the RTL, Documentation, Verification environment or Tool and Build system label Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type:Bug For bugs in the RTL, Documentation, Verification environment or Tool and Build system
Projects
None yet
Development

No branches or pull requests

1 participant