-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Remove Verilog delay from DPI outputs and use falling edge instead #2635
Conversation
Verilator ignores these delays (after warning the user), so they can result in race conditions.
In general, using negedge clocking on half of a ready/valid interface is functionally incorrect. It might be the case that this interface is constrained in such a way that it's OK in this case, but I don't know. |
Co-authored-by: Ernie Edgar <43148441+ernie-sifive@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All checks passed on latest commit. Changed output back to posedge to confine any negedge to within the SimDTM module.
Is there a fundamental reason why the logic needs to be different for Verilator? Why does the |
This is a red flag for me, too. |
I haven't thought too hard about the details, but superficially, this looks good to me. |
Let me verify that this works on the 3.4 branch then hopefully we're good to go! |
It's working on the 3.4 branch! |
I believe this is right so I'm going to merge |
Verilator ignores these delays (after warning the user), so they can
result in race conditions.
This fixes an issue visible on #2617 where some assertions fire erroneously due to some optimizations Verilator makes that are incorrect due to the race conditions.
Thank you to @ernie-sifive for doing this work
Related issue:
Type of change: bug fix
Impact: no functional change
Development Phase: implementation
Release Notes
Remove Verilog delay statements from SimJTAG.v and SimDTM.v