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

ASSERT_OFF #2233

Merged
merged 3 commits into from
Oct 12, 2023
Merged

ASSERT_OFF #2233

merged 3 commits into from
Oct 12, 2023

Conversation

silabs-robin
Copy link
Contributor

@silabs-robin silabs-robin commented Oct 10, 2023

This PR makes compilation of assertions optional.

For blazing fast re-compilation and next level productivity iteration times, simply use +define+COREV_ASSERT_OFF (and optionally comment away the ifndef for a specific assertion set you want to work on).

Compilation times (mm:ss):

  • 00:20 - No assertion sets
  • 00:23 - One assertion set (umode_assert)
  • 01:30 - All assertion sets
  • 02:10 - Internal formal

Test results:

  • ci_check - All pass (except "clic").
  • Cvverif formal - Compiles and runs decently.
  • Internal formal - No reds after a few minutes.

I got tired of formal taking so long to recompile just for simple changes, so that is why I implemented this.

An additional bonus, is that the warnings log is much smaller, so you specifically see all warnings related to your module.

The reason for using ifndef is because 1) this is consistent with the way it is already done in the RTL repo, and 2) we don't want to change the hierarchy with several additional paths with generates.

Signed-off-by: Robin Pedersen <[email protected]>
@silabs-robin silabs-robin marked this pull request as ready for review October 11, 2023 07:35
@silabs-robin silabs-robin changed the title Assert off TODO ASSERT_OFF Oct 11, 2023
Copy link
Contributor

@silabs-mateilga silabs-mateilga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very tricky diff to read, but I don't see any issues. I approve of the motivation for this change, so as long as nothing breaks this is a good change.

Approving

@silabs-hfegran
Copy link
Contributor

This is a very tricky diff to read, but I don't see any issues. I approve of the motivation for this change, so as long as nothing breaks this is a good change.

Approving

Much easier to read this diff with the "hide whitespace" option set.

@silabs-hfegran
Copy link
Contributor

Looks good to me, thanks @silabs-robin

@silabs-hfegran silabs-hfegran merged commit 57ba371 into openhwgroup:cv32e40s/dev Oct 12, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants