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

Yosys SV Support Misrepresented Due to $display #5082

Open
sifferman opened this issue Oct 1, 2023 · 7 comments · Fixed by YosysHQ/yosys#4129
Open

Yosys SV Support Misrepresented Due to $display #5082

sifferman opened this issue Oct 1, 2023 · 7 comments · Fixed by YosysHQ/yosys#4129

Comments

@sifferman
Copy link
Contributor

Hello, and thank you for assembling this very useful record of SV support in tools! It has been super interesting to follow.

Problem

There are several tests that Yosys is incorrectly failing due to using $display. Yosys does not support the $display statement for non-constant values:

ERROR: Failed to evaluate system task `$display' with non-constant argument.

Many more tests would pass if their values were verified by other means than $display. To list a few:

Many more tests correctly fail because the feature is unsupported, but will eventually incorrectly fail because they use $display. To list a few:

Possible Solutions

To fix this, a custom assert macro/function should be added that would operate differently depending on the tool choice. Here are a couple possibilities for what the custom assert macro/function should do:

output ERROR Port

Possibly all the tests mentioned could have an output ERROR port added to the module definition, and the assert function/macro could just run ERROR |= !(<assertion>); for Yosys runs. Then, eval could be run to check the value of ERROR.

External Script

Another possibility is that the custom assert macro/function does nothing when run by Yosys. Instead, an external script will parse all the asserts and run eval on each one.

(Note that this will not work for the tests onebit.sv and read-write.sv because there are modifications to values after they have an assertion)

@tgorochowik
Copy link
Member

Hi, thank you for the report.

Let me start by saying that originally all the tests that use display with asserts inside were meant for simulator tests. Then we added "elaboration-only" class which made it possible to run those in other tools (like yosys). So we could also just disable those tests for yosys, the things they cover (besides the asserts meant for simulation) should already be covered by other test cases.

That being said, I'm not opposed to adjusting those tests to make then usable for tools that only run elaboration. I'm not sure if having an error output port makes sense for yosys? would we be able to read the calculated value there?

One additional idea - maybe we could wrap those display/assert calls with some ifdefs (e.g. ifdef SIMULATION) and define this flag for all the simulators we test.

Feel free to propose a PR with a fix or a workaround for the problem.

@sifferman
Copy link
Contributor Author

sifferman commented Oct 10, 2023

I'm not sure if having an error output port makes sense for yosys? would we be able to read the calculated value there?

Yes! Yosys' eval command calculates the values of all output ports of a specified module. If we add an ERROR port, then Yosys can calculate the value

I just made an example repo that demonstrates this: sifferman/eval-example. I adjusted the tests mentioned above, then run synthesis and eval -set-undef top.

we could also just disable those tests for yosys, the things they cover (besides the asserts meant for simulation) should already be covered by other test cases.

I guess I am also concerned about false positives when the synthesis is incorrect but Yosys doesn't throw an error when parsing. Using eval allows for extra confidence.

@sifferman
Copy link
Contributor Author

I would be happy to make these changes if you think it sounds good

@sifferman
Copy link
Contributor Author

sifferman commented Oct 10, 2023

I spent some time sorting through the tests to determine which should be enabled/disabled for Yosys.

These should be modified for Yosys:

These should be disabled for Yosys:

Note: it's a bit difficult what tests should be enabled/disabled for synthesis tools, because ideally they should be able to run all constant functions/constructs that don't depend on input ports. I will go back through this list and check how other synthesis tools handle ambiguous cases. Or is there a place to see how the Synopsys/Cadence/Siemens tools respond to all tests in this repo? That could be a useful metric.

@tgorochowik
Copy link
Member

Thank you for the work. You suggestions sound good, just make sure the tests are not getting much more complicated than they are now (at least in terms of readability and number of different SV constructs).

Or is there a place to see how the Synopsys/Cadence/Siemens tools respond to all tests in this repo? That could be a useful metric.

The licenses of the proprietary tool disallow using them to run in any benchmarks or comparisons so we cannot have this here.

If you do wish to compare (other) tools that allow it, take a look at how the runners in the tools directory are implemented. Adding your own tool should be fairly straightforward - so you should be able to run some comparisons locally

@daglem
Copy link
Contributor

daglem commented Jan 9, 2024

I just added a simulation pass to the Yosys runner in #5498, in anticipation of merge of YosysHQ/yosys#3963 in Yosys.
I have tested the Yosys PR, and it solves the current issues with $display in sv-tests, and gives a nice boost in passed tests for Yosys.
I believe this negates the need to change any tests.

@daglem
Copy link
Contributor

daglem commented Jan 17, 2024

The recent boost in passed tests for Yosys is due to YosysHQ/yosys#4129.

By default, output from $display et al. in initial blocks is produced both from elaboration and simulation. The tests still work, however duplication of the output is confusing, and could possibly cause problems later on. For simulation tests, #5525 suppresses the output from elaboration, leaving only output from simulation.

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 a pull request may close this issue.

3 participants