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
Support short-circuiting #487
Comments
Original Redmine Comment Isn't short-circuiting optional in Verilog-2001? Either way I do not believe Verilator should do anything different WRT this in Verilog vs SystemVerilog unless the spec forces it, as that will just confuse people. Anyhow the big problem is test case currently fails on both VCS and NC. (And fails identically with either SystemVerilog or Verilog 2005 mode.) Also minor issues with the specific patch; first the test.pl should use verilator_flags2 not v_flags2 as the flags are verilator specific, and second testing the --language switch is incorrect; --language is just the default language if not specified; it can be overridden on a file-by-file basis, so the best thing to do would be to put the language setting into the FileLine flags, then test it at the appropriate point. (This hasn't been needed yet as there isn't anything that simulates differently.) |
Original Redmine Comment I have to confess that I don't have a copy of the 2005 standard, but the 2001 standard definitely leaves it up to the user. It may be that 2005 tightened things up. I have an example which passes with VCS, but fails with Verilator. Since this is commercially confidential, I extracted the test case, which passes with Icarus Verilog, but fails with Verilator. I don't routinely have access to VCS - does it pass with VCS if you specify 2001 as the Verilog version? Maybe I need to tighten down the language variant test. My objective was that synthesizable code that is acceptable to VCS (or for that matter any major simulator) should also be OK with Verilator. Maybe I should make this issue a warning about short-circuiting that can be turned off? I've changed the code on github to use verilator_flags2. I'll work on making the language test per module as you suggest - I see the comment in the definition of class FileLine about doing this. |
Original Redmine Comment VCS fails with $stop at line 88. NC complains about bit-select out of bounds on 67 as the AND isn't short circuited. Both are identical in v2001/2005/SV modes, emphasizing my point that changing behavior on the language isn't appropriate. |
Original Redmine Comment Hmmm. Things get more and more messy. VCS appears to do bitwise constant folding, but evidently gets the wrong answer for bitwise OR (the $stop should never be executed). It also seems that for SystemVerilog it is not standards compliant (short-circuiting bitwise AND is explicitly not allowed). NC doesn't do constant folding. Icarus Verilog does constant folding and seems to get it right in both cases. The original code I have only uses bitwise AND short-circuiting and the designers use VCS, so that explains why it works for them. The Verilog 2001 standard is not helpful by using bitwise AND as the example of where short-circuiting could be used - presumably why VCS does it. I have a ton of legacy code which uses this, and with a very strong prohibition on not modifying the RTL. Should I add a new option to offer bitwise short-circuiting? |
Original Redmine Comment Can you post a test that shows what you just said; namely it passes in VCS and fails in NC? (Warnings not being considered failures, for this purpose.) |
Original Redmine Comment As noted, I don't have VCS to check this, but I have reduced the test to just using bitwise-AND. This passes in Icarus Verilog. From your previous comment, I believe this should pass with VCS and fail with a range check with NC. With --verbose, the output from both Verilator is:
and from IV is:
Identical, apart from the order in which parallel always blocks are evaluated. New test t_gen_cond_v2k1_bitrange_and.{pl,v} pushed to GitHub, and attached here for convenience. |
Original Redmine Comment Sorry - I should have added, that is Verilator with my patch to enable bitwise AND short-circuiting with Verilog. It fails with standard Verilator as follows:
|
Original Redmine Comment NC does print a warning, but then passes and VCS passes identically, so this test doesn't prove the difference you indicated. I think what you're seeing is VCS just doesn't warn about this case, it presumes Xs. If that's true then what Verilator should do is make it a warning instead of an error, and the user should turn off the warning. |
Original Redmine Comment Thanks for running the tests. I'll update my patch accordingly. |
Original Redmine Comment Continuing the conversation from Bug 532 (sorry for duplicating)... Although Verilog 2001 does not mandate short-circuiting of bitwise-AND and bitwise-OR, it does use bitwise-AND as an example. As you note, NC and VCS accept this with a warning. I have legacy code that breaks with Verilator (generate loops that go out of range) because of this. I'll follow your suggestion and modify the patch, so it generates a warning, and the warning can be turned off. |
Original Redmine Comment Not currently being worked on. |
Some notes from #2861 isTPure() always returns true at this moment because m_doShort is true. (too optimistic) Lines 911 to 916 in 273fcce
There are some issues if isTPure() becomes too pessimistic.
#2861 (comment) has experimental implementation. Test to reproduce the issue will be added via #2869 I guess the challenge would be
|
The problem with conversion of logical expressions to bit expressions is fixed by #4437. |
Still need a new warning when side effects may cause problems. Hope to commit shortly. |
The new SIDEEFFECT warning should tell us what additional fixes are needed in this area. Closing as general solution handled. |
Author Name: Jeremy Bennett (@jeremybennett)
Original Redmine Issue: 487 from https://www.veripool.org
Verilog 2001 (and I believe 2005) allows considerable flexibility in short-circuiting. IEEE 1364-2001, section 4.1.4 states:
The standard does not make the precise circumstances explicit, but the example used is based on bitwise-AND where the first operand evaluates to zero. I understand that VCS short-circuits evaluation of bitwise AND and bitwise OR.
I have written a patch with regression test to implement this when the Verilator language specified is Verilog, but not SystemVerilog. This is an extension of the work in Issue 413, which implements short-circuiting compliant with the SystemVerilog IEEE 1800-2009 standard. Please pull the changes from:
https://github.com/jeremybennett/verilator/tree/v2k1-short-circuit
The text was updated successfully, but these errors were encountered: