From 618959d1478fef89b724e8d71bc0a7daf6126e0f Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Sat, 5 Feb 2022 10:49:34 +0100 Subject: [PATCH 1/3] Add helper function to emit error when SystemVerilog is requried When encountering a construct that requires SystemVerilog in most cases an error message is generated when SystemVerilog is not enabled and parsing simply continues. Factor the checking and generating of the error message into a helper function. This slightly reduces boiler plate code and gives consistent error messages. Signed-off-by: Lars-Peter Clausen --- parse.y | 107 +++++++++++++++------------------------------------ parse_misc.h | 1 + pform.cc | 22 ++++++----- pform.h | 2 + 4 files changed, 48 insertions(+), 84 deletions(-) diff --git a/parse.y b/parse.y index 4850a0611..20e3b14d2 100644 --- a/parse.y +++ b/parse.y @@ -309,9 +309,8 @@ static void current_task_set_statement(const YYLTYPE&loc, std::vectorset_statement(tmp); @@ -330,9 +329,7 @@ static void current_task_set_statement(const YYLTYPE&loc, std::vectorset_statement(tmp); @@ -368,9 +364,7 @@ static void current_function_set_statement(const YYLTYPE&loc, std::vectorsize() > 1 && !gn_system_verilog()) { - yyerror(@7, "error: Task body with multiple statements requires SystemVerilog."); + if ($7 && $7->size() > 1) { + pform_requires_sv(@7, "Task body with multiple statements"); } delete $7; } @@ -2452,10 +2445,8 @@ tf_port_item /* IEEE1800-2005: A.2.7 */ tmp = pform_make_task_ports(@3, use_port_type, $2, ilist); } if ($4 != 0) { - if (gn_system_verilog()) { + if (pform_requires_sv(@4, "Task/function port with unpacked dimensions")) { pform_set_reg_idx(name, $4); - } else { - yyerror(@4, "error: Task/function port with unpacked dimensions requires SystemVerilog."); } } @@ -2479,10 +2470,7 @@ tf_port_item /* IEEE1800-2005: A.2.7 */ tf_port_item_expr_opt : '=' expression - { if (! gn_system_verilog()) { - yyerror(@1, "error: Task/function default arguments require " - "SystemVerilog."); - } + { pform_requires_sv(@$, "Task/function default argument"); $$ = $2; } | { $$ = 0; } @@ -2582,9 +2570,7 @@ variable_dimension /* IEEE1800-2005: A.2.5 */ | '[' ']' { std::list *tmp = new std::list; pform_range_t index (0,0); - if (!gn_system_verilog()) { - yyerror("error: Dynamic array declaration require SystemVerilog."); - } + pform_requires_sv(@$, "Dynamic array declaration"); tmp->push_back(index); $$ = tmp; } @@ -2592,9 +2578,7 @@ variable_dimension /* IEEE1800-2005: A.2.5 */ { // SystemVerilog queue list *tmp = new std::list; pform_range_t index (new PENull,0); - if (!gn_system_verilog()) { - yyerror("error: Queue declaration require SystemVerilog."); - } + pform_requires_sv(@$, "Queue declaration"); tmp->push_back(index); $$ = tmp; } @@ -2602,9 +2586,7 @@ variable_dimension /* IEEE1800-2005: A.2.5 */ { // SystemVerilog queue with a max size list *tmp = new std::list; pform_range_t index (new PENull,$4); - if (!gn_system_verilog()) { - yyerror("error: Queue declarations require SystemVerilog."); - } + pform_requires_sv(@$, "Queue declaration"); tmp->push_back(index); $$ = tmp; } @@ -2612,10 +2594,8 @@ variable_dimension /* IEEE1800-2005: A.2.5 */ variable_lifetime : lifetime - { if (!gn_system_verilog()) { - yyerror(@1, "error: overriding the default variable lifetime " - "requires SystemVerilog."); - } else if ($1 != pform_peek_scope()->default_lifetime) { + { if (pform_requires_sv(@1, "Overriding default variable lifetime") && + $1 != pform_peek_scope()->default_lifetime) { yyerror(@1, "sorry: overriding the default variable lifetime " "is not yet supported."); } @@ -3913,9 +3893,7 @@ expr_primary FILE_NAME(tmp, @1); delete[]$1; $$ = tmp; - if (!gn_system_verilog()) { - yyerror(@1, "error: Empty function argument list requires SystemVerilog."); - } + pform_requires_sv(@1, "Empty function argument list"); } | implicit_class_handle @@ -4158,24 +4136,22 @@ expr_primary | expr_primary '\'' '(' expression ')' { PExpr*base = $4; - if (gn_system_verilog()) { + if (pform_requires_sv(@1, "Size cast")) { PECastSize*tmp = new PECastSize($1, base); FILE_NAME(tmp, @1); $$ = tmp; } else { - yyerror(@1, "error: Size cast requires SystemVerilog."); $$ = base; } } | simple_type_or_string '\'' '(' expression ')' { PExpr*base = $4; - if (gn_system_verilog()) { + if (pform_requires_sv(@1, "Type cast")) { PECastType*tmp = new PECastType($1, base); FILE_NAME(tmp, @1); $$ = tmp; } else { - yyerror(@1, "error: Type cast requires SystemVerilog."); $$ = base; } } @@ -4409,12 +4385,9 @@ hierarchy_identifier $$ = tmp; } | hierarchy_identifier '[' '$' ']' - { pform_name_t * tmp = $1; + { pform_requires_sv(@3, "Last element expression ($)"); + pform_name_t * tmp = $1; name_component_t&tail = tmp->back(); - if (! gn_system_verilog()) { - yyerror(@3, "error: Last element expression ($) " - "requires SystemVerilog. Try enabling SystemVerilog."); - } index_component_t itmp; itmp.sel = index_component_t::SEL_BIT_LAST; itmp.msb = 0; @@ -4585,9 +4558,7 @@ port_declaration $$ = ptmp; } | attribute_list_opt K_input net_type_opt data_type_or_implicit IDENTIFIER '=' expression - { if (!gn_system_verilog()) { - yyerror("error: Default port values require SystemVerilog."); - } + { pform_requires_sv(@6, "Default port value"); Module::port_t*ptmp; perm_string name = lex_strings.make($5); data_type_t*use_type = $4; @@ -4935,11 +4906,8 @@ module_parameter_port_list_opt module_parameter : parameter param_type parameter_assign | localparam param_type parameter_assign - { if (!gn_system_verilog()) { - yyerror(@1, "error: Local parameters in module parameter " - "port lists requires SystemVerilog."); - } - } + { pform_requires_sv(@1, "Local parameter in module parameter port list"); + } ; module_parameter_port_list @@ -5379,10 +5347,7 @@ module_item | K_function error K_endfunction label_opt { yyerror(@1, "error: I give up on this function definition."); if ($4) { - if (!gn_system_verilog()) { - yyerror(@4, "error: Function end names require " - "SystemVerilog."); - } + pform_requires_sv(@4, "Function end label"); delete[]$4; } yyerrok; @@ -6514,10 +6479,7 @@ statement_item /* This is roughly statement_item in the LRM */ block_item_decls_opt { if (!$2) { if ($4) { - if (! gn_system_verilog()) { - yyerror("error: Variable declaration in unnamed block " - "requires SystemVerilog."); - } + pform_requires_sv(@4, "Variable declaration in unnamed block"); } else { /* If there are no declarations in the scope then just delete it. */ pform_pop_scope(); @@ -6560,10 +6522,7 @@ statement_item /* This is roughly statement_item in the LRM */ { if (!$2) { if ($4) { - if (! gn_system_verilog()) { - yyerror("error: Variable declaration in unnamed block " - "requires SystemVerilog."); - } + pform_requires_sv(@4, "Variable declaration in unnamed block"); } else { /* If there are no declarations in the scope then just delete it. */ pform_pop_scope(); @@ -6847,9 +6806,7 @@ statement_item /* This is roughly statement_item in the LRM */ | hierarchy_identifier K_with '{' constraint_block_item_list_opt '}' ';' { /* ....randomize with { } */ if ($1 && peek_tail_name(*$1) == "randomize") { - if (!gn_system_verilog()) - yyerror(@2, "error: Randomize with constraint requires SystemVerilog."); - else + if (pform_requires_sv(@2, "Randomize with constraint")) yyerror(@2, "sorry: Randomize with constraint not supported."); } else { yyerror(@2, "error: Constraint block can only be applied to randomize method."); diff --git a/parse_misc.h b/parse_misc.h index c7a6faa2e..c20cc0e4e 100644 --- a/parse_misc.h +++ b/parse_misc.h @@ -57,6 +57,7 @@ extern YYLTYPE yylloc; */ extern int VLlex(); extern void VLerror(const char*msg); +extern void VLerror(const YYLTYPE&loc, va_list ap); extern void VLerror(const YYLTYPE&loc, const char*msg, ...) __attribute__((format(printf,2,3))); #define yywarn VLwarn extern void VLwarn(const char*msg); diff --git a/pform.cc b/pform.cc index 4de09e481..4ab111ea0 100644 --- a/pform.cc +++ b/pform.cc @@ -1334,10 +1334,9 @@ void pform_startmodule(const struct vlltype&loc, const char*name, error_count += 1; } - if (lifetime != LexicalScope::INHERITED && !gn_system_verilog()) { - cerr << loc << ": error: Default subroutine lifetimes " - "require SystemVerilog." << endl; - error_count += 1; + + if (lifetime != LexicalScope::INHERITED) { + pform_requires_sv(loc, "Default subroutine lifetime"); } if (gn_system_verilog() && ! pform_cur_module.empty()) { @@ -3138,11 +3137,7 @@ PAssign* pform_compressed_assign_from_inc_dec(const struct vlltype&loc, PExpr*ex PExpr* pform_genvar_inc_dec(const struct vlltype&loc, const char*name, bool inc_flag) { - if (!gn_system_verilog()) { - cerr << loc << ": error: Increment/decrement operators " - "require SystemVerilog." << endl; - error_count += 1; - } + pform_requires_sv(loc, "Increment/decrement operator"); PExpr*lval = new PEIdent(lex_strings.make(name)); PExpr*rval = new PENumber(new verinum(1)); @@ -3760,6 +3755,15 @@ void pform_add_modport_port(const struct vlltype&loc, pform_cur_modport->simple_ports[name] = make_pair(port_type, expr); } +bool pform_requires_sv(const struct vlltype&loc, const char *feature) +{ + if (gn_system_verilog()) + return true; + + VLerror(loc, "error: %s requires SystemVerilog.", feature); + + return false; +} FILE*vl_input = 0; extern void reset_lexor(); diff --git a/pform.h b/pform.h index cfefa09b3..b1b5e7a1d 100644 --- a/pform.h +++ b/pform.h @@ -612,4 +612,6 @@ extern bool allow_timeprec_decl; void pform_put_enum_type_in_scope(enum_type_t*enum_set); +bool pform_requires_sv(const struct vlltype&loc, const char *feature); + #endif /* IVL_pform_H */ From 89e935c210a14aa60f536fc701b20102de2a2ac2 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Sun, 23 Jan 2022 21:32:55 +0100 Subject: [PATCH 2/3] Allow omitting `parameter` in module parameter port list SystemVerilog allows to completely omit the `parameter` keyword in a module parameter port list. This is described in section 6.20.1 ("Parameter declaration syntax") of the LRM (1800-2017). E.g. ``` module a #(X = 10) ... module b #(int Y = 20) ... ``` It also allows to redefine the parameter type without having to have a parameter or localparam before the type. E.g. ``` module a #(parameter int A = 1, real B = 2.0) ... module b #(int X = 3, real Y = 4.0) ... ``` Extend the parser to support this. Note that it is not possible to declare a parameter with an implicit data type this way. E.g. the following is not legal SystemVerilog ``` module a #([3:0] A = 1) ... module b #(int X = 2, signed Y = 3.0) ... ``` Signed-off-by: Lars-Peter Clausen --- parse.y | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/parse.y b/parse.y index 20e3b14d2..d7c1ebe66 100644 --- a/parse.y +++ b/parse.y @@ -653,7 +653,7 @@ static void current_function_set_statement(const YYLTYPE&loc, std::vector variable_decl_assignment %type list_of_variable_decl_assignments -%type data_type data_type_or_implicit data_type_or_implicit_or_void +%type data_type data_type_opt data_type_or_implicit data_type_or_implicit_or_void %type simple_type_or_string let_formal_type %type packed_array_data_type %type ps_type_identifier @@ -1263,6 +1263,11 @@ data_type /* IEEE1800-2005: A.2.2.1 */ } ; +/* Data type or nothing, but not implicit */ +data_type_opt + : data_type { $$ = $1; } + | { $$ = 0; } + /* The data_type_or_implicit rule is a little more complex then the rule documented in the IEEE format syntax in order to allow for signaling the special case that the data_type is completely @@ -1279,7 +1284,7 @@ scalar_vector_opt /*IEEE1800-2005: optional support for packed array */ ; data_type_or_implicit /* IEEE1800-2005: A.2.2.1 */ - : data_type + : data_type_opt { $$ = $1; } | signing dimensions_opt { vector_type_t*tmp = new vector_type_t(IVL_VT_LOGIC, $1, $2); @@ -1293,8 +1298,6 @@ data_type_or_implicit /* IEEE1800-2005: A.2.2.1 */ FILE_NAME(tmp, @2); $$ = tmp; } - | - { $$ = 0; } ; @@ -4912,8 +4915,23 @@ module_parameter module_parameter_port_list : module_parameter + | data_type_opt + { param_data_type = $1; + param_is_local = false; + } + parameter_assign + { pform_requires_sv(@3, "Omitting initial `parameter` in parameter port " + "list"); + } | module_parameter_port_list ',' module_parameter - | module_parameter_port_list ',' parameter_assign + | module_parameter_port_list ',' data_type_opt + { if ($3) { + pform_requires_sv(@3, "Omitting `parameter`/`localparam` before " + "data type in parameter port list"); + param_data_type = $3; + } + } + parameter_assign ; module_item From 7f40e120c823fbf486accbfca059610270f93c0f Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Tue, 8 Feb 2022 10:45:18 +0100 Subject: [PATCH 3/3] Add regression tests for omitting `parameter` in parameter port list SystemVerilog allows to completely omit the `parameter` or `localparam` keyword in the parameter list. Both at the beginning and before redefining the parameter data type. This is not support in Verilog. Add regression tests that check that this is supported when in SystemVerilog mode. It is not valid to use an implicit data type e.g. just `signed` when `parameter` was omitted, add regression tests to check for this as well. Signed-off-by: Lars-Peter Clausen --- ivtest/ivltests/localparam_implicit3.v | 30 +++++++++++++++++++++++ ivtest/ivltests/parameter_omit1.v | 17 +++++++++++++ ivtest/ivltests/parameter_omit2.v | 17 +++++++++++++ ivtest/ivltests/parameter_omit3.v | 17 +++++++++++++ ivtest/ivltests/parameter_omit_invalid1.v | 8 ++++++ ivtest/ivltests/parameter_omit_invalid2.v | 8 ++++++ ivtest/ivltests/parameter_omit_invalid3.v | 8 ++++++ ivtest/regress-fsv.list | 3 +++ ivtest/regress-sv.list | 1 + ivtest/regress-vlg.list | 6 +++++ 10 files changed, 115 insertions(+) create mode 100644 ivtest/ivltests/localparam_implicit3.v create mode 100644 ivtest/ivltests/parameter_omit1.v create mode 100644 ivtest/ivltests/parameter_omit2.v create mode 100644 ivtest/ivltests/parameter_omit3.v create mode 100644 ivtest/ivltests/parameter_omit_invalid1.v create mode 100644 ivtest/ivltests/parameter_omit_invalid2.v create mode 100644 ivtest/ivltests/parameter_omit_invalid3.v diff --git a/ivtest/ivltests/localparam_implicit3.v b/ivtest/ivltests/localparam_implicit3.v new file mode 100644 index 000000000..54ced0465 --- /dev/null +++ b/ivtest/ivltests/localparam_implicit3.v @@ -0,0 +1,30 @@ +// Check that all parameters in a parameter port list after a `localparam` get +// elaborated as localparams, until the next `parameter`. Check that this is the +// case even when the data type of the parameter is redefined. + +module a #( + parameter A = 1, B = 2, + localparam C = 3, real D = 4, + parameter E = 5 +); + +initial begin + if (A == 10 && B == 20 && C == 3 && D == 4 && E == 50) begin + $display("PASSED"); + end else begin + $display("FAILED"); + end +end + +endmodule + +module b; + + a #( + .A(10), + .B(20), + .D(40), // This will cause an error + .E(50) + ) i_a(); + +endmodule diff --git a/ivtest/ivltests/parameter_omit1.v b/ivtest/ivltests/parameter_omit1.v new file mode 100644 index 000000000..1f635eb09 --- /dev/null +++ b/ivtest/ivltests/parameter_omit1.v @@ -0,0 +1,17 @@ +// Tests that it possible to omit the initial `parameter` keyword in a parameter +// port list in SystemVerilog. In Verilog this is not allowed and should result +// in an error. + +module a #(A = 1); + initial begin + if (A == 10) begin + $display("PASSED"); + end else begin + $display("FAILED"); + end + end +endmodule + +module test; + a #(.A(10)) i_a(); +endmodule diff --git a/ivtest/ivltests/parameter_omit2.v b/ivtest/ivltests/parameter_omit2.v new file mode 100644 index 000000000..a2a4d32d1 --- /dev/null +++ b/ivtest/ivltests/parameter_omit2.v @@ -0,0 +1,17 @@ +// Tests that it possible to omit the initial `parameter` keyword in a parameter +// port list in SystemVerilog. In Verilog this is not allowed and should result +// in an error. + +module a #(integer A = 1); + initial begin + if (A == 10) begin + $display("PASSED"); + end else begin + $display("FAILED"); + end + end +endmodule + +module test; + a #(.A(10.1)) i_a(); +endmodule diff --git a/ivtest/ivltests/parameter_omit3.v b/ivtest/ivltests/parameter_omit3.v new file mode 100644 index 000000000..bd7f3e3bf --- /dev/null +++ b/ivtest/ivltests/parameter_omit3.v @@ -0,0 +1,17 @@ +// Tests that it possible to omit the `parameter` keyword in a parameter port +// list before changing the parameter type in SystemVerilog. In Verilog this is +// not allowed and should result in an error. + +module a #(parameter real A = 1.0, integer B = 2); + initial begin + if (A == 10.1 && B == 20) begin + $display("PASSED"); + end else begin + $display("FAILED"); + end + end +endmodule + +module test; + a #(.A(10.1), .B(20)) i_a(); +endmodule diff --git a/ivtest/ivltests/parameter_omit_invalid1.v b/ivtest/ivltests/parameter_omit_invalid1.v new file mode 100644 index 000000000..99afa1024 --- /dev/null +++ b/ivtest/ivltests/parameter_omit_invalid1.v @@ -0,0 +1,8 @@ +// Check that implicit type in a parameter port list without `parameter` +// generates an error. + +module test #([7:0] A = 1); + initial begin + $display("FAILED"); + end +endmodule diff --git a/ivtest/ivltests/parameter_omit_invalid2.v b/ivtest/ivltests/parameter_omit_invalid2.v new file mode 100644 index 000000000..310074353 --- /dev/null +++ b/ivtest/ivltests/parameter_omit_invalid2.v @@ -0,0 +1,8 @@ +// Check that implicit type in a parameter port list without `parameter` +// generates an error. + +module test #(signed A = 1); + initial begin + $display("FAILED"); + end +endmodule diff --git a/ivtest/ivltests/parameter_omit_invalid3.v b/ivtest/ivltests/parameter_omit_invalid3.v new file mode 100644 index 000000000..61346c410 --- /dev/null +++ b/ivtest/ivltests/parameter_omit_invalid3.v @@ -0,0 +1,8 @@ +// Check that declaring changing the parameter type to an implicit type without +// the `parameter` keyword results in an error. + +module test #(parameter real A = 1.0, signed B = 2); + initial begin + $display("FAILED"); + end +endmodule diff --git a/ivtest/regress-fsv.list b/ivtest/regress-fsv.list index 9f129e8f9..a25e451c6 100644 --- a/ivtest/regress-fsv.list +++ b/ivtest/regress-fsv.list @@ -78,6 +78,9 @@ br_gh567 normal ivltests check_constant_3 normal ivltests function4 normal ivltests parameter_in_generate1 normal ivltests +parameter_omit1 normal ivltests +parameter_omit2 normal ivltests +parameter_omit3 normal ivltests pr1963962 normal ivltests gold=pr1963962-fsv.gold pr3015421 CE ivltests gold=pr3015421-fsv.gold resetall normal,-Wtimescale ivltests gold=resetall-fsv.gold diff --git a/ivtest/regress-sv.list b/ivtest/regress-sv.list index fc9d76edf..46a08f5e6 100644 --- a/ivtest/regress-sv.list +++ b/ivtest/regress-sv.list @@ -302,6 +302,7 @@ l_equiv_const normal,-g2005-sv ivltests line_directive normal,-g2009,-I./ivltests ivltests gold=line_directive.gold localparam_implicit normal,-g2005-sv ivltests localparam_implicit2 CE,-g2005-sv ivltests +localparam_implicit3 CE,-g2005-sv ivltests localparam_query normal,-g2005-sv ivltests localparam_type2 normal,-g2009 ivltests logical_short_circuit normal,-g2012 ivltests diff --git a/ivtest/regress-vlg.list b/ivtest/regress-vlg.list index 0b78224c5..950db67b9 100644 --- a/ivtest/regress-vlg.list +++ b/ivtest/regress-vlg.list @@ -695,6 +695,12 @@ param_test4 normal ivltests param_times normal ivltests # param has multiplication. parameter_type normal ivltests gold=parameter_type.gold parameter_in_generate1 CE ivltests +parameter_omit1 CE ivltests +parameter_omit2 CE ivltests +parameter_omit3 CE ivltests +parameter_omit_invalid1 CE ivltests +parameter_omit_invalid2 CE ivltests +parameter_omit_invalid3 CE ivltests patch1268 normal ivltests pca1 normal ivltests # Procedural Continuous Assignment in a mux pic normal contrib pictest gold=pic.gold