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

Avoid wide signals in sensitivity lists of immediate assertions #229

Conversation

michael-platzer
Copy link
Contributor

The addr_decode_dync and multiaddr_decode modules feature some complex assertions (actually assumes to be precise) that are quite complex — too complex for Verilator it seems, which fails in unpredictable ways when these modules are present. Therefore, this PR disables those assertions in Verilator (based on whether the VERILATOR pre-processor macro is defined).

@colluca
Copy link
Contributor

colluca commented Sep 6, 2024

@michael-platzer Thanks for reporting this. With what Verilator version did you observe the failure?

@michael-platzer
Copy link
Contributor Author

@colluca With version 5.018:

Verilator 5.018 2023-10-30 rev v5.018

I have not tested it with the latest version though. Do you happen to use a newer version for which these are handled correctly?

@michael-platzer
Copy link
Contributor Author

Update: I tested the current code from master with the latest release of Verilator (5.028), which already fails at elaboration of addr_decode_dync.sv:

%Error: /workspaces/hw.riscv/.bender/git/checkouts/common_cells-2220a26021f1ec02/src/addr_decode_dync.sv:149:12: Value too wide for 64-bits expected in this context 224'h8000000000c00000000000000100100000000010010000
  149 |   always @(addr_map_i or config_ongoing_i) #0 begin : proc_check_addr_map
      |            ^~~~~~~~~~
%Error: /workspaces/hw.riscv/.bender/git/checkouts/common_cells-2220a26021f1ec02/src/addr_decode_dync.sv:149:12: Value too wide for 64-bits expected in this context 132'h24180000000121000000000008
  149 |   always @(addr_map_i or config_ongoing_i) #0 begin : proc_check_addr_map
      |            ^~~~~~~~~~
%Error: Exiting due to 2 error(s)

Maybe this is just Verilator not properly supporting these complex assertions, but it could also be an indication there is something wrong and in need of fixing. @WRoenninger Could you please have a look?

@michael-platzer michael-platzer force-pushed the michael-platzer/fix/no-complex-assertions-in-verilator branch from a904252 to a0525f7 Compare September 30, 2024 13:37
@michael-platzer
Copy link
Contributor Author

I did some investigation with @WRoenninger and we found that Verilator has issues with wide signals in sensitivity lists. With the latest version (5.028) all issues can be resolved by using always @* instead of a hand-crafted sensitivity list. Most tools are pretty smart when it comes to figuring out the dependencies of always @* blocks, so this should not have performance implications. Also, with a wildcard sensitivity list it is not possible to forget adding signals to the sensitivity list as new signals are added to the assertions later on. I adapted the files accordingly.

@michael-platzer michael-platzer changed the title Disable complex addr decoder assertions in Verilator Avoid wide signals in sensitivity lists of immediate assertions Sep 30, 2024
Copy link
Contributor

@colluca colluca left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @michael-platzer :)

Verilator has decent support for various types of assertions by now but
dislikes wide signals (e.g., wider than 64 bits) in sensitivity lists.
@michael-platzer michael-platzer force-pushed the michael-platzer/fix/no-complex-assertions-in-verilator branch from a0525f7 to 38a94fe Compare October 1, 2024 05:05
@michael-platzer
Copy link
Contributor Author

Changed always @* to always_comb to make the reviewdog happy 🐶

Copy link
Collaborator

@niwis niwis left a comment

Choose a reason for hiding this comment

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

Thank you @michael-platzer @WRoenninger for getting to the bottom of this!

@niwis niwis merged commit 8ac55cf into pulp-platform:master Oct 3, 2024
5 checks passed
@michael-platzer michael-platzer deleted the michael-platzer/fix/no-complex-assertions-in-verilator branch October 3, 2024 05:20
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

Successfully merging this pull request may close these issues.

3 participants