cv32e40p icon indicating copy to clipboard operation
cv32e40p copied to clipboard

Use of SV compile macros in RTL

Open MikeOpenHWGroup opened this issue 4 years ago • 2 comments

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 avatar Apr 08 '20 18:04 MikeOpenHWGroup

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.

bluewww avatar Apr 09 '20 08:04 bluewww

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 avatar Jul 14 '20 05:07 Silabs-ArjanB