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

Use of SV compile macros in RTL #301

Open
MikeOpenHWGroup opened this issue Apr 8, 2020 · 2 comments
Open

Use of SV compile macros in RTL #301

MikeOpenHWGroup opened this issue Apr 8, 2020 · 2 comments
Assignees
Labels
Component:RTL For issues in the RTL (e.g. for files in the rtl directory) Type:Enhancement For feature requests and enhancements WAIVED:CV32E40P Issue does not impact a major release of CV32E40P and is waived

Comments

@MikeOpenHWGroup
Copy link
Member

The RTL code for cv32e40p contains a large number of compile-time macro checks. For example, in riscv_pmp.sv, lines 233..242 we see:

`ifdef EN_NAPOT_RULE_8B
       `RULE_0:
       begin: BYTE_ALIGN_8B
          mask_addr[i]  = 32'hFFFF_FFFE;
          start_addr[i] = pmp_addr_i[i] & mask_addr[i];
          stop_addr[i]  = pmp_addr_i[i];

          `ifdef DEBUG_RULE $display(" NAPOT[%d]  --> BYTE_ALIGN_8B: %8h<-- addr --> %8h", i, start_addr[i]<<2, stop_addr[i]<<2 ); `endif
       end
`endif

There are three issues highlighted here:

  1. Macros like EN_NAPOT_RULE_8B are defined in the file that uses them. These should be moved to a central location for all such macros.
  2. Macros likes RULE_0 are used as constants. However, since macros have global scope, this could affect the entire design, so it is recommended to define these as localparams.
  3. Macros are global, so things like DEBUG_RULE could create very unexpected behavior if it is used in another way elsewhere in the RTL.

I would like to see the following:

  1. Prefix all macros with CV32E40 (e.g. CV32E40P_EN_NAPOT) to maximize the likelihood of having unique macros.
  2. Move all macro definitions (`define) to a central file.
  3. Update the manifest to ensure that the macro definitions are read in first to ensure that all simulation, formal verification and synthesis are using the same model.
  4. Replace macros used as constants with localparams.

Attached are two files: rtl_ifdefs.uniq.txt lists all the unique uses of macros in the RTL and rtl_ifdefs.uniq.txt shows where these are defined.
rtl_ifdefs.uniq.txt
rtl-defines.txt

@MikeOpenHWGroup MikeOpenHWGroup added the Type:Enhancement For feature requests and enhancements label Apr 8, 2020
@bluewww
Copy link
Contributor

bluewww commented Apr 9, 2020

I agree with all the points you have brought up, especially the namespacing is useful. This is a common pitfall in the C language too.

One more point I'd like to add: The lowrisc style guide recommends macros that are only (file)locally used to be undefd after usage. A local macro could e.g. be some code generation thing that severly reduces repetitive code, say exploding dozens of AXI interfaces to signals.

@Silabs-ArjanB
Copy link
Contributor

The issue has been solved except for the PMP file. PMP is however not used in CV32E40P, but file itself is kept. Once we start on a core using the PMP the issue will be addressed there as well (e.g. as was done in the Ibex PMP). Therefore I am waiving the 'remainder of this issue' for CV32E40P.

@Silabs-ArjanB Silabs-ArjanB added Component:RTL For issues in the RTL (e.g. for files in the rtl directory) WAIVED:CV32E40P Issue does not impact a major release of CV32E40P and is waived labels Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:RTL For issues in the RTL (e.g. for files in the rtl directory) Type:Enhancement For feature requests and enhancements WAIVED:CV32E40P Issue does not impact a major release of CV32E40P and is waived
Projects
None yet
Development

No branches or pull requests

4 participants