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

RV_ASSERT_ON is always enabled #58

Open
nstewart-amd opened this issue Feb 17, 2023 · 5 comments
Open

RV_ASSERT_ON is always enabled #58

nstewart-amd opened this issue Feb 17, 2023 · 5 comments

Comments

@nstewart-amd
Copy link

RISCV core has assertions guarded by `ifdef RV_ASSERT_ON​

RV_ASSERT_ON is always enabled in src/rtl/riscv_core/rtl/common_defines.sv​

ifndef VERILATOR​ define RV_ASSERT_ON ​
`endif​

Recommendation:​
Add `ifndef VEEREL2_SYNTHESIS or similar in addition to VERILATOR blocker.

Additionally -
Recommend all macro defines have VEEREL2 or similar prefix.

Example: VERILATOR -> VEEREL2_VERILATOR

Note that macro defines are global namespace. We must avoid simple names with reasonable opportunity for collision at integration level.

@algrobman
Copy link

VERILATOR is a simulator automatic define, predefined by Verilator

@nstewart-amd
Copy link
Author

nstewart-amd commented Feb 17, 2023

The following are generic and subject to contention:

common_defines.vh
`define REGWIDTH 32
`define CLOCK_PERIOD 100
`define TOP tb_top

el2_param.vh
Most everything.
From a generic perspective, VeeR users should be able to integrate mutliple Veer configurations in parallel without collision.
Hence, anything "defined" in {veer.config -default} must not collide with defines in {veer.config -high_perf}.
Defines must be uniquified allowing multiple concurrent instantiation of differently configured VeeR cores.

@nstewart-amd
Copy link
Author

I've opened a unique case to discuss defines uniquification:
#59

This thread should continue to focus specifically on the the issue of RV_ASSERT_ON always on.

@rahuljainNV
Copy link

this is dupe of #49 ?

@mkurc-ant
Copy link
Collaborator

@nstewart-amd Please take a look at this PR #55. Does it solve the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants