Invalid ivtests that should be blacklisted?

[MikePopoloski]

I went through failing ivtests for slang. Some of them I believe are failing because the tests themselves are invalid. They fall into the following groups:

Invalid net types

A bunch of tests use 'real' or 2-state net types, which are not allowed in SystemVerilog. Commercial tools fail on this as well. There are many, some examples:

• implicit_cast12_iv
• implicit_cast13_iv
• ca_time_iv
• br_gh99e_iv

Mod of real number

This is not allowed in SystemVerilog. Commercial tools fail on this as well. There are many, some examples:

• cfunc_assign_op_real_iv
• cfunc_assign_op_mixed_iv
• mixed_type_div_mod_iv
• pr1528093_iv

Variables used before they are declared

This is not allowed in SystemVerilog. Some commercial tools correctly fail on this, some do not. Some examples:

• pr1936363_iv
• pr1648365_iv

Use of optional system functions

This is arguable, but the spec clearly says tools may not have these functions. It doesn't make sense to me to have them in the test suite.

Use of unknown system functions

Examples: $deposit, $abstime are not specified anywhere in SystemVerilog.

• sysargs_iv
• pr2590274a_iv

Invalid use of system functions

There are a bunch of tests that pass the wrong number of arguments to system functions, or arguments with incorrect types. Perhaps these are supposed to be marked as should_fail ?

• sf_countbits_fail.iv
• sf_countones_fail.iv
• sf_isunknown_fail.iv
• sf_onehot0_fail.iv
• sf_onehot_fail.iv
• scaninvalid.iv
• readmeminvalid.iv
• freaderror_iv
• pr2800985b_iv

Miscellaneous

• pr1916261_iv - uses a replication constant of 0, which is not allowed
• param_test3_iv - provides parameter assignments to a module that does not have any parameter declarations
• elsif_test_iv - has multiple `else directives for the same `if directive (this is probably just a typo in the test? not sure why Icarus doesn't catch it)
• comment1_iv - has nested block comments, which are not allowed

@caryr can you take a look?

[caryr]

Yes, I will audit this. I thought most of this was moved into the correct Icarus specific list that is no longer loaded and yes, some of those should be marked as fail (we often use _fail to indicate this). I do know there is some oddity in how all this is tested since the number of tests is not constant across the various tools. I have not had time to look at that yet. slang has over 230 extra tests when compare to Icarus.

The option system tasks/ functions should likely be discussed in the union versus intersection testing PR #1214 I opened. I can see it both ways, but this is valid SystemVerilog so why is it being regarded as invalid? It really belongs in a maybe supported category. I may move those into an optional list to make it easier to handle in sv-tests. This also bring up why is vectored and scalared used in the test suite since these are also optional?

And thank you for checking this with such diligence!

[caryr]

I did a quick search and some of this is fairly easy to cleanup. Others are going to be a bit harder (e.g. net data_type). I have started a discussion in the Icarus area: see steveicarus/iverilog#479 for that. Some of the others point to bugs/limitations in Icarus that I will try to resolve and update the tests when I have free time.

[MikePopoloski]

Great, thanks for looking. Agreed that optional functionality could go either way.

[caryr]

The general idea in Icarus development is we try to support the intersection of what the commercial tools support (assuming it makes sense) so our eventual plan for variables used before they are declared is they should be portability warnings (and likely only when enabled), but not parse errors. Of course we could enable a strict mode to turn portability warnings into errors.

This philosophy allows almost any one to use Icarus and if it is implemented it should work as the previous simulator they were using (e.g. $abstime comes from Verilog-A mode and is handy when you want the actual time without scaling). It's certainly not portable, but very handy when needed.

[MikePopoloski]

Icarus can do whatever it wants, I'm not trying to control your project. The thing we're discussing is which of your tests should be in this repository. Just because Icarus has a unit test for some particular feature does not mean that same unit test needs to be included in this project and applied to all tools.

[caryr]

Certainly, just providing information so differences can be understood better. For example we check always loops for the possibility of infinite loops and fail if there is no delay, an infinite loop will definitely happen. This is not even mentioned in the standard. We warn if there is a possibility of an infinite loop, not all paths in the always have delays. This will certainly create a difference for parse/elaboration only tools. As far as I know, no other simulator does this, but our reports for "my simulation is mysteriously just hanging at T0" disappeared completely after adding the infinite loop check. This likely has to do with the fact Icarus appears to be extensively used while learning Verilog.

[caryr]

Here is my complete analysis of all the failing tests in ivtest for slang. I will be working on getting ivtest as clean as possible. Some of this is going to take some time since I would prefer to not just black list these. Some will be fixed and others I would like to make a bit more dynamic. Around half the failures are related to bugs in slang. I believe I indicated all those with "Slang bug:"

slang.txt

[caryr]

I have started a turnin with an updated ivtest clone along with generate script that should address the: invalid net types, mod of real numbers and variables used before they are declared. I did not fix any tests that were failing slang because does not support implicit nets. I'm going to cleanup the unknown system functions, constant out of bound selects, and miscellaneous groups next. I need to think about how to handle the optional and invalid system tasks/functions. Icarus reports the invalid ones correctly, but we check this in the output and not as a fail of the simulator. I agree it should fail the simulation so need to fix Icarus and then update the tests.

[MikePopoloski]

Thanks for making that list, it's very helpful. Some of the big categories like UDPs and specify blocks not yet being supported are known, but I'll double check the rest of them and fix the things that are broken in slang.

[caryr]

Just FYI. If you look at the list in detail there are some borderline cases that may require testing/discussion to resolve so there may be a few more slang bugs coming. If I could not quickly find explicit documentation showing something was supported it was not listed as a slang bug.

[caryr]

The tests using optional items have been added to the exclusion list and the unknown ones have be handled in ivtest with the latest version of the PR. I will look at the miscellaneous and the remaining constant out of bounds tests next.

[MikePopoloski]

By the way I'm considering making the constant out of bounds check a warning instead of an error, to better match what iverilog does, which would clean up a bunch of these.

[caryr]

Thanks, of course I am in the process of making these compile time configurable where if the simulator is Icarus or you have set a special define then the out of bound checks will be tested. We can add the appropriate define to the slang runner if you choose to switch this to a warning to enable full checking. I changed a bunch of them in the latest PR, but still have some to convert and push. Once I get all this working as I want I will add some comments to the ivtest generation script regarding the various definitions and what they enable.

My goal is to do this for most and maybe all of the functionality that is currently excluded, but supported by other simulators.

[caryr]

I found a case where it looks like slang is incorrectly reporting an out of range select. If involves the out of range select being inside the $bits() function and the standard clearly states $bits() does not evaluate the expression. Here is the packed array declaration and the expected result from $bits():

reg [2:1][16:1][8:1] array;
$bits(array[0])=128
$bits(array[0][0])==8

This is br_gh112c and br_gh112d.

[MikePopoloski]

Ah, that's a bit of a weird case. Arguably you don't need to evaluate the expression to know that it's wrong. On the other hand, it's not hard to suppress the error in this case either.

[caryr]

I'm confused regarding what you think is wrong. The code example was just to point out the concept and I did miss the second "=" in the middle line. You should look at the actual test case to see the full example.

Arguably $bits(array[]) is all that is needed to know you are interested in the size of a sub-array. That is not something that can be parsed as an expression so we need to add a select expression, though based on my reading of the standard that select expression should not be evaluated only the fact we have reduced the array dimensions we are looking at.

[MikePopoloski]

Well 0 isn't a valid index, so in that sense it is wrong to try to use it as an index. The programmer could have just as easily used a valid index like 1.

It's also unclear about the evaluation rules when a dynamic array is used, since elements can have differing sizes. For example, what should this output?

module m;
  int foo [1:2][];
  initial begin
    foo[1] = new [3];
    foo[2] = new [5];
    $display($bits(foo[1]), $bits(foo[2]), $bits(foo[0]));
  end
endmodule

VCS fails and errors, some other sims print 96 160 0 or 96 160 x. This is a select of a fixed size array but the result is a dynamic array so the index actually matters here.

[caryr]

Failing certainly seems wrong. Since $bits() returns an integer it does make it complicated regarding does a non-existent element return the same as an empty element? I almost like the 'x' result which returns more information and then for consistency has ramifications regarding our discussion regarding fixed arrays. Where this functionality decision came from is treating a constant out of range select the same as a variable select. So array[0] should return the same value as array[idx] when idx is zero. The actual result is certainly undefined, but it does have a defined width. For consistency I think empty and undefined dynamic array elements need to return 0 since that is consistent with fixed sized arrays.

What are your thoughts?

[MikePopoloski]

The LRM says an empty dynamic array has $bits() == 0, so probably it must return 0 to be correct.

[caryr]

I was asking about the non-existent element return value and should it match the empty value return. My assertion is it should match for consistency and to match what is done for fixed sized arrays.

[MikePopoloski]

Well an out-of-bounds access where the array's element type is itself a dynamic array is specified to return an empty dynamic array. And then $bits() of an empty dynamic array is specified to return 0. So I'm pretty sure the result of $bits(foo[0]) must be 0. The spec also says an error can be issued for out of bounds access, so again we're kind of back at the original point. VCS's behavior is probably fine according to the spec, as is the behavior of returning 0. x is probably not valid, even though I agree it seems nicer.

[caryr]

Okay, I agree it should either report the out of bounds access as an elaboration error or return 96 160 0 possibly with a warning. I had forgotten to check the default value table for dynamic elements.

The evaluation comment in the standard likely is in regards to more complicated expressions that have side effects (e.g. $bits(++idx)) so looking at the constant index values are likely okay. Icarus does not currently issue a warning for this out of bound access.

[caryr]

Here are a couple more things for discussion.

repeat with a real expression
I see nowhere that prohibits this in the standard and at least one commercial tool handles it the same as Icarus (it does the normal rounding when converting a real to an integer). See br967.

delay_or_event_control should not use automatic variables
Since the delay or event control can live past the invocation it cannot use automatic variables to trigger. Commercial tools also report this as an issue. See automatic_error3.

[caryr]

Also add automatic_error11 which uses $monitor with an automatic variable, automatic_error12 which uses $strobe with an automatic variable and automatic_error13 which uses $fstrobe with an automatic variable.

[caryr]

I have created a pull request #1288 that has an updated ivtest that now indicates system tasks/functions that are used incorrectly now have a failing state (RE). In Icarus this is determined in the runtime when the VPI routines are being compiled. This should clean up more of the slang ivtest failures. I think I'm down to just the miscellaneous ones listed above.

[MikePopoloski]

I agree with the mentioned restrictions on repeat expressions and automatic variables used in timing controls (the specific rules here appear to be that automatic variables are restricted to procedural contexts, and timing controls are not procedural contexts even if they themselves are inside of one).

[caryr]

I thought on this and it is more complicated than just timing control. I believe it is non-blocking timing control that cannot use automatics variables since the timing control can outlive the variable. For blocking control it is okay. I tested the following in Icarus and a simplified example with a commercial tool:

module top;
  reg glbl;
  reg in;

  task automatic my_tsk;
    reg [1:0] dly;
    dly = 2'd2;
    glbl = #(dly) in;
  endtask

  initial begin
    $monitor("%0d, %b - %b", $time, glbl,in);
    in = 1'b0;
    fork
      my_tsk;
      #1 in = 1'b1;
    join
    my_tsk;
  end
endmodule

This does compile and delays the assignment as expected. You could likely create some complicated fork/join_none/any that would allow @ to work using a local variable, but I did not go to that level :).

[MikePopoloski]

Yeah, it's probably actually event_expressions that have the restriction.

[caryr]

No, it's blocking versus non-blocking. Icarus correctly handles the following blocking event expression:

module top;
  reg glbl;
  reg in;

  task automatic my_tsk;
    reg evt;
    fork
      begin
        #1 evt = 0;
        #1 evt = 1;
      end
      glbl = @(posedge evt) in;
    join
  endtask

  initial begin
    $monitor("%0d, %b - %b", $time, glbl,in);
    in = 1'b0;
    fork
      my_tsk;
      #1 in = 1'b1;
    join
    my_tsk;
  end
endmodule

[MikePopoloski]

Interesting, that is indeed more complicated than I thought it was.

[caryr]

Yes, now it's fun to note that when you comment out the evt = 1; assignment this does not hang, but that is not related to parsing/elaboration and falls into the realm of scheduling subtlety.

[caryr]

Here's another issue I found while trying to clean things up.

Slang does not fail when a non-blocking assignment is used in a final block.

[caryr]

If you look at the BNF two unary operators are not allowed to be adjacent. They need to be separated by a pair of parenthesis:

expression ::= unary_operator { attribute_instance } primary
primary ::= ( mintypmax_expression )
mintypmax_expression ::= expression

Slang does not report adjacent unary operators as an issue. See ivtest - pr2794144.

[caryr]

In enum_test8 slang errors out because the expression width does not match the enumeration width. This is incorrect since this is an expression not a literal constant. Only literal constants must have the exact same width as the enumeration. This is the pertinent section of the standard.

The integer value expressions are evaluated in the context of a cast to the enum base type. Any enumeration
encoding value that is outside the representable range of the enum base type shall be an error. For an
unsigned base type, this occurs if the cast truncates the value and any of the discarded bits are nonzero. For a
signed base type, this occurs if the cast truncates the value and any of the discarded bits are not equal to the
sign bit of the result.

[caryr]

For check_constant_11 and check_constant_16 I believe slang is not correctly reporting that the variable array select on the left hand side of the continuous assignment is invalid. I do not see where a variable select is allowed on the left side of a continuous assignment and at least one commercial simulator agrees.

[MikePopoloski]

I agree re: unary operators and enum values.

I don't see anything disallowing variable selects in a continuous assignment. I see:

assign [ delay_control ] list_of_variable_assignments
list_of_variable_assignments ::= variable_assignment { , variable_assignment }
variable_assignment ::= variable_lvalue = expression
variable_lvalue ::=
    [ implicit_class_handle . | package_scope ] hierarchical_variable_identifier select
    | { variable_lvalue { , variable_lvalue } }
    | [ assignment_pattern_expression_type ] assignment_pattern_variable_lvalue
    | streaming_concatenation

[caryr]

It's not in the BNF

Table 10-1—Legal left-hand forms in assignment statements
Continuous assignment Net or variable (vector or scalar)
Constant bit-select of a vector net or packed variable
Constant part-select of a vector net or packed variable
Concatenation or nested concatenation of any of the above left-hand sides

[MikePopoloski]

Not sure what you're talking about; I pasted the pieces of the BNF above, starting from "A.6.1 Continuous assignment and net alias statements"

[MikePopoloski]

Nevermind, I understand what you're saying now. You're saying the select index must be a constant expression, not runtime determined. Yeah, I agree.

[caryr]

Correct. The l-val must be constant. I'll try to be more explicit next time.

[caryr]

FYI I'm down to less than a dozen tests to rework/categorize and then I can close this PR.

[caryr]

I was reviewing a couple of the tests I have previously marked as valid SV and they appear to actually be invalid.

array_lval_select3b: this is trying to do an assign/deassign with a non-constant select of a variable array.
array_lval_select4b: does the same with force/release.

What is interesting is a commercial tool supports this for force but uses the current value of the variable. So it is actually acting like a constant at the time the force is executed. This is confusing unless you are using it in the context of a loop where you are forcing many different elements. Since a procedural or continuous assign do not allow non-constant selects I believe force should not either.

I will be converting these to failures and adding them to the slang does not report a failure list.

[caryr]

One more slang issue. I'm down to four items I am working on and I am fairly sure three of the four are Icarus bugs and the last one is subjective and will require a little more thought.

The slang issue is shown in br961 which has a module scope level parameter being used to declare a function return width and input width and the input has the same name as the parameter. I believe this is allowed since the parameter is only used before the input is actually declared. After declaration it is then shadowed by the input. Commercial tools accept this without warning.

[MikePopoloski]

Yes, that one looks like a slang bug. It's finding the input port with the lookup instead of the parameter.

[caryr]

Okay, I think this is the last of the slang bugs in ivtest. If you could send me a direct email when you have time I have some things I would like to discuss separate from sv-tests.

[caryr]

Here is the complete list of new items relative to the list I initially provided:

br961                       - New Slang bug: Module param and function input have same name
undef_lval_select3b         - New Slang bug: non-constant l-value selects are not valid
array_lval_select4b         - New Slang bug: non-constant l-value selects are not valid
undef_lval_select3b         - New Slang bug: non-constant l-value selects are not valid
undef_lval_select3c         - New Slang bug: non-constant l-value selects are not valid
undef_lval_select4b         - New Slang bug: non-constant l-value selects are not valid
undef_lval_select4c         - New Slang bug: non-constant l-value selects are not valid
automatic_error3            - New Slang bug: cannot use automatic variables in delay_or_event_control
automatic_error11           - New Slang bug: cannot use automatic variables in $monitor
automatic_error12           - New Slang bug: cannot use automatic variables in $strobe
automatic_error13           - New Slang bug: cannot use automatic variables in $fstrobe
check_constant_11           - New Slang bug: cannot have a variable Lval in a CA
check_constant_16           - New Slang bug: cannot have a variable Lval in a CA
enum_test8                  - New Slang bug: enum expr width does not need to match enum width
br967                       - New Slang bug: a repeat with a real value
pr2794144                   - New Slang bug: cannot have adjacent unary operators
program3b                   - New Slang bug: cannot have a NBA in a final block

[MikePopoloski]

Thanks, I'll send you an email soon. I already have some fixes up but don't know when sv-tests will update the runners again.

[caryr]

Yah, Icarus has a huge number of fails as well because the runner version is significantly out of date.

[MikePopoloski]

@caryr I was looking at the restrictions for procedural continuous assignments and I think ivtests is using them incorrectly in some places. The LRM says

The left-hand side of the assignment in the assign statement shall be a singular variable reference or a concatenation of variables. It shall not be a bit-select or a part-select of a variable.

Indeed, commercial sims I tried all disallow even constant selects of variables, including of unpacked arrays. I think ivtests assumes it's ok to do constant selects of variables?

[caryr]

I'll need to double check, but that is certainly possible. Thanks for pointing this out.

FYI I still have two tests that I am deciding how they should be dealt with and also need to create a sv-tests specific list generation to get the test list build correctly. There are also a couple tests in the exclude list that I still need to looks at, but overall things should be fairly clean and documented in either ivtest in the Icarus specific list or the sv-tests exclude list for defacto standard items.

[caryr]

Yes, the standard text appears to be perfectly clear on this across all versions of the standard. Of course if you look at the BNF it implies this is allowed so I assume someone was looking at the BNF instead of the text or making some other assumption when this was originally implemented. I will check the commercial tool I use and if it agrees I will update Icarus to report this as an error and update the impacted tests in ivtest accordingly.

[MikePopoloski]

@caryr Here's the remaining list of tests that are failing with slang. The majority are, I believe, invalid tests as opposed to slang bugs.

slang bugs

regress-sv_program3b_iv - fixed in slang upstream
regress-sv_sv_default_port_value3_iv - slang can't simulate
regress-sv_sv_timeunit_prec3b_iv - slang doesn't support nested modules yet
regress-sv_sv_timeunit_prec4b_iv - slang doesn't support nested modules yet
regress-synth_dffsynth8_iv - slang can't simulate
regress-vlg_br1015a_iv - slang can't simulate
regress-vlg_contrib82_iv - slang does not yet support explicit ports
regress-vlg_pr377_iv - slang does not yet support explicit ports
regress-vlg_par_mismatch_iv - slang doesn't support specify blocks
regress-vlg_pr707_iv - fixed upstream
regress-vlg_pr1723367_iv - fixed upstream
regress-vlg_pr3306516_iv - slang doesn't support specify blocks
regress-vlg_pr3309391_iv - slang doesn't support specify blocks
regress-vlg_uwire_fail_iv - slang does not check drivers on uwires

test bugs

regress-fsv_array_lval_select3a_iv - bit select in procedural continuous assignment
regress-vlg_assign32E_iv - bit select in procedural continuous assignment
regress-vlg_assign_deassign_pv_iv - bit select in procedural continuous assignment
regress-vlg_force_lval_part_iv - bit select in procedural continuous assignment
regress-vlg_force_release_reg_pv_iv - bit select in procedural continuous assignment
regress-vlg_pr1832097b_iv - bit select in procedural continuous assignment
regress-vlg_pr2943394_iv - bit select in procedural continuous assignment

regress-sv_sformatf_iv - too many arguments passed to $sformatf
regress-vlg_format_iv - missing argument to $display

regress-vlg_array_word_check_iv - dumpvars with bit select -- I don't think this is actually allowed, commercial tools error on this
regress-vlg_dump_memword_iv - dumpvars with bit select -- I don't think this is actually allowed, commercial tools error on this
regress-vlg_vcddup_iv - tries to dumpvars on nets which I don't think is allowed?

regress-sv_sv_port_default14_iv - not sure why this is marked "should fail"
regress-synth_br995_iv - not sure why this is marked "should fail"
regress-synth_br_gh306a_iv - not sure why this is marked "should fail"
regress-synth_br_gh306b_iv - not sure why this is marked "should fail"
regress-synth_case5synfail_iv - not sure why this is marked "should fail"
regress-synth_casesynth8_iv - not sure why this is marked "should fail"
regress-vlg_br1027a_iv - not sure why this is marked "should fail"
regress-vlg_br1027c_iv - not sure why this is marked "should fail"
regress-vlg_br1027e_iv - not sure why this is marked "should fail"
regress-vlg_br_gh25a_iv - not sure why this is marked "should fail"
regress-vlg_br_gh25b_iv - not sure why this is marked "should fail"
regress-vlg_check_constant_3_iv - not sure why this is marked "should fail"
regress-vlg_function4_iv - not sure why this is marked "should fail"
regress-vlg_readmemh5_iv - not sure why this is marked "should fail"
regress-vlg_pr1704013_iv - not sure why this is marked "should fail"

regress-vlg_br_gh157_iv - defparam targets a localparam, which is illegal
regress-vlg_scoped_events_iv - defparam targets a localparam, which is illegal

regress-vlg_br_ml20150606_iv - redeclares ANSI ports as separate variables, which is illegal

regress-vlg_pr938b_iv - named port connections for UDPs are not allowed in the spec and major tools disallow this
regress-vlg_udp_dff_iv - named port connections for UDPs are not allowed in the spec and major tools disallow this

unclear

regress-vlg_scope2b_iv - unclear, need to investigate more
regress-vlg_indef_width_concat_iv - concat with unsized literal is indeed disallowed but a lot of code out there does it and major tools allow it, so slang only warns. Thoughts on this?
regress-vlg_no_timescale_in_module_iv - I can't find anything that actually disallows this, and major tools do allow it so I'm not sure this should fail.

[caryr]

Thanks Mike, I think some of this is because I still have not had time to build an appropriate test list generation script (e.g. there are synthesis/Verilog failures that are legal SystemVerilog). The current method of just loading the regression files is not correct. I created a special override file that addresses all this for ivtests, but svtests does not currently understand overrides as defined in ivtests. Hopefully I will find some time before too long to get back to this, but with a part in the lab another going to manufacturing and defining the next generation, I'm busy right now.

The `timescale behavior is likely because of the following from Verilog 1364-2005 "This directive specifies the time unit and time precision of the modules that follow it." This looks to be updated to "This directive specifies the time unit and time precision of the design elements that follow it." for SystemVerilog. To me this wording implies the directive must be outside a module/design element, but I agree it is not explicit like it is for some of the other directives. Also allowing this inside a module would imply different regions of the module could have a different unit/precision and that is not consistent with the VPI.

[MikePopoloski]

My interpretation is that a `timescale inside a module would not change the timescale of that module itself, but it could change the timescale of other modules (or other design elements such as interfaces, programs, etc) declared later in the file.

[caryr]

In the context of nested design elements that makes some sense, but it is sure to confuse a user who has code that uses a delay element after the directive, but gets the delay characteristics that match the timeunit/precision in effect when the module keyword (design element) was first encountered. I'll look into this, but it would have been much kinder to the end user to just handle this with the SystemVerilog timeunit/timeprecision statements instead of allowing a time scale directive inside a design element. This is one case where it would have been better not to have complete symmetry between the timescale directive and the timeunit/timeprecision statements. Ah, what fun!

[MikePopoloski]

I think we should be in a good state here now. slang has 11 remaining tests failing and they all appear to be deficiencies in slang itself and not the tests.