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

Hiding assertions code from design tools #49

Open
rahuljainNV opened this issue Jan 18, 2023 · 12 comments
Open

Hiding assertions code from design tools #49

rahuljainNV opened this issue Jan 18, 2023 · 12 comments

Comments

@rahuljainNV
Copy link

Usually, we hide all the assertion code in RTL from frontend tools like synthesis/lint etc.
The core has used below define to add guard against asserts. So we could "not define" this and asserts should get excluded.
However, since this define is already getting set in common_defines.sv directly which has other design defines as well, there is no way to unset this without editing the code.

swerv_el2/rtl/common_defines.sv: `define RV_ASSERT_ON

Should we move this define to outside of design code, so that it is set in verif environment only and not seen by design tools?

@mkurc-ant
Copy link
Collaborator

Hello @rahuljainNV. I've created a PR #55 which provides a solution for the problem you described. Please tell if the solution is acceptable.

@rahuljainNV
Copy link
Author

I could not fully follow the changes, how they are helping in hiding asserts from frontend tools while still enabling them for testbench/simulations. Could you pls explain them a bit?

@mkurc-ant
Copy link
Collaborator

My understanding of this issue was that you want to have a way to control the RV_ASSERT_ON define without touching the source code. I assume that the define guard is actually needed there because when unset it effectively "hides" assertions and this is desired.

Do you consider common_defines.sv as part of the code you don't want to change? It is generated by the config script anyways hence it is not a fixed part of the design.

@rahuljainNV
Copy link
Author

you are right, need a way to control the RV_ASSERT_ON without touching the source code.
you are right, the define guard is needed to hide asserts.
however, by merely passing "undef" on commandline of any tool won't work as common_defines.sv will be parsed later as part of design and it would set this define.

A possible solution that I had in mind was to add below guard around RV_ASSERT_ON in common_defines.sv
ifndef SYNTHESIS define RV_ASSERT_ON
`endif

This would be the usage scenario.

  1. Core is configured and submitted to version control under //project/design/. RV_ASSERT_ON is under the guard as above, and SYNTHESIS is not defined.
  2. My synthesis/formality/lint setup is under //project/synthesis, it defines SYNTHESIS define and then reads the design files from //project/design . So effectively all asserts are hidden.
  3. My TB/Formal setup is under //project/verif, and it directly reads the design as is. So SYNTHESIS is not defined and tools see all the asserts by default.

This way by default everyone sees the asserts, but if any tool has a problem, it can set SYNTHESIS and then all asserts are hidden from it.

@mkurc-ant
Copy link
Collaborator

Alright, so you need an external define to control whether assertions are enabled or not. I'll work on the approach with the "SYNTHESIS" define.

@mkurc-ant
Copy link
Collaborator

I've updated the PR #55 so now it generates common_defines.vh with RV_ASSERT_ON inside the ifndef SYNTHESIS guard.

@rahuljainNV
Copy link
Author

I don't understand all the config system of VeeR, so can't comment on the changes and if they align with general guidelines within VeeR codebase (though I am baffled on how a simple of change of adding ifndef SYTHESIS guard is done using so many files/scripts).
If this generates the intended output, I am good.

@mkurc-ant
Copy link
Collaborator

The output is as intended, here is a fragment:

...
`define RV_BUILD_AXI4 1
`define CPU_TOP `RV_TOP.veer
`ifndef SYNTHESIS
`define RV_ASSERT_ON 1
`endif
`define RV_LDERR_ROLLBACK 1
`define TOP tb_top
...

If that's fine I can merge PR #55

@algrobman
Copy link

algrobman commented Feb 22, 2023 via email

@algrobman
Copy link

BTW, you guys could use pd_defines.vh file for synthesis flow, instead of common_defines.vh, which is also generated and contains just this :

`include "common_defines.vh"
`undef RV_ASSERT_ON
`define RV_PHYSICAL 1

@rahuljainNV
Copy link
Author

@mkurc-ant looks good to me.
@algrobman if verilator has issue with assertion code, it should not set SYNTHESIS, that way its hidden from it.

@mkurc-ant
Copy link
Collaborator

mkurc-ant commented Feb 23, 2023

@algrobman Yes indeed. But that happens even without PR #55. That's why undef RV_ASSERT was injected in the Makefile for the Verilator target. Now it is just not set in the config n the first place.

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

3 participants