MikePopoloski / slang

SystemVerilog compiler and language services
MIT License
558 stars 122 forks source link

module identifier inside`$asserton`/`$assertoff` case #912

Closed likeamahoney closed 3 months ago

likeamahoney commented 3 months ago

Slang errors with "'m' is a module definition but is used as a value" diag on such sample of code:

sample.sv:

module top();
m m1();
endmodule

module m();
initial begin
$assertoff(0, m);
end
endmodule

It occurs when a top module not specified or specified with just a top. For example:

./slang ./sample.sv --top top

In case of both modules are specified as top module it elaborates design without any error:

./slang ./sample.sv --top top --top m

Is this correct behavior?

jcurtiss8086 commented 3 months ago

I think this is correct behavior based on the BNF. The 2nd argument for the assertion tasks must be a hierarchical-identifier. So $assertoff(0, m1) would pass.

assert_control_task ::=
assert_task [ ( levels [ , list_of_scopes_or_assertions ] ) ] ;
| assert_action_task [ ( levels [ , list_of_scopes_or_assertions ] ) ] ;
| $assertcontrol ( control_type [ , [ assertion_type ] [ , [ directive_type ] [ , [ levels ]
[ , list_of_scopes_or_assertions ] ] ] ] ) ;
assert_task ::=
$asserton
| $assertoff
| $assertkill
assert_action_task ::=
$assertpasson
| $assertpassoff
| $assertfailon
| $assertfailoff
| $assertnonvacuouson
| $assertvacuousoff
list_of_scopes_or_assertions ::= scope_or_assertion { , scope_or_assertion }
scope_or_assertion ::= hierarchical_identifier

However if the example is changed so that both the definition-name and instance-name are the same, then I think it probably should pass when searching upwards in the hierarchy for an instance match, but it gets the same error.

module top();
    m m();
endmodule

module m();
    initial begin
       $assertoff(0, m); // fails
       $assertoff(0, top.m); // passes
    end
endmodule
likeamahoney commented 3 months ago

I think this is correct behavior based on the BNF. The 2nd argument for the assertion tasks must be a hierarchical-identifier. So $assertoff(0, m1) would pass.

As it can be seen at 23.6 clause of SystemVerilog LRM simple identifier is also a hierarchical_identifier:

hierarchical_identifier ::= [ $root . ] { identifier constant_bit_select . } identifier

So my example should also pass, because m is simple identifier that can be resolved from context as module m type.

I also suppose that it should be no difference between module types arguments resolving at assert_tasks in case if all modules specified as tops like - --top top --top m or not.

MikePopoloski commented 3 months ago

The difference if you pass --top m is that you have an actual scope named "m", whereas otherwise there are no scopes with that name. Instead you're expecting (I think) to make use of Upward Name Resolution (see 23.8) where it's allowed to match a scope if the module declaration name matches, not just the scope name. The weird thing here is the lack of a dot -- it's not clear in the LRM that you can ever have the upward name resolution apply without a dot, but in the case of the assert control tasks it seems all the major simulators are allowing it so I can change slang to match.

likeamahoney commented 3 months ago

The difference if you pass --top m is that you have an actual scope named "m", whereas otherwise there are no scopes with that name. Instead you're expecting (I think) to make use of Upward Name Resolution (see 23.8) where it's allowed to match a scope if the module declaration name matches, not just the scope name. The weird thing here is the lack of a dot -- it's not clear in the LRM that you can ever have the upward name resolution apply without a dot, but in the case of the assert control tasks it seems all the major simulators are allowing it so I can change slang to match.

It would be great to fix that because many projects use different variant of $assertof notations. For example github searching results - https://github.com/search?q=%24assertoff%280+lang%3ASystemVerilog+&type=code

It can be wrapped with --compat vcs option because it supported by many tools except Verilator.

MikePopoloski commented 3 months ago

Fixed in 3699daa54d2d6ca1591dac32df377d58c4459f0c