From 8f20a15ca3a1bbf4993677e2ffe804e322127f8b Mon Sep 17 00:00:00 2001 From: Dave McEwan Date: Fri, 10 Nov 2023 14:51:32 +0000 Subject: [PATCH 01/13] V2k1 Rename `non_ansi_module` to `module_nonansi_forbidden` --- MANUAL.md | 167 +++++++++--------- build.rs | 5 + md/manual-introduction.md | 2 +- md/ruleset-DaveMcEwan-design.md | 2 +- md/ruleset-designintent.md | 2 +- md/ruleset-verifintent.md | 2 +- ...explanation-interface_port_with_modport.md | 2 +- ...s-explanation-module_nonansi_forbidden.md} | 11 +- ...es-explanation-re_forbidden_module_ansi.md | 2 +- ...explanation-re_forbidden_module_nonansi.md | 2 +- ...les-explanation-re_required_module_ansi.md | 2 +- ...-explanation-re_required_module_nonansi.md | 2 +- rulesets/DaveMcEwan-design.toml | 2 +- rulesets/designintent.toml | 2 +- rulesets/verifintent.toml | 2 +- ..._module.rs => module_nonansi_forbidden.rs} | 6 +- ..._module.sv => module_nonansi_forbidden.sv} | 0 ..._module.sv => module_nonansi_forbidden.sv} | 0 18 files changed, 116 insertions(+), 97 deletions(-) rename md/{syntaxrules-explanation-non_ansi_module.md => syntaxrules-explanation-module_nonansi_forbidden.md} (61%) rename src/syntaxrules/{non_ansi_module.rs => module_nonansi_forbidden.rs} (87%) rename testcases/syntaxrules/fail/{non_ansi_module.sv => module_nonansi_forbidden.sv} (100%) rename testcases/syntaxrules/pass/{non_ansi_module.sv => module_nonansi_forbidden.sv} (100%) diff --git a/MANUAL.md b/MANUAL.md index 17e8d89b..e21b3a8c 100644 --- a/MANUAL.md +++ b/MANUAL.md @@ -155,7 +155,7 @@ prefix_label = "lab_" style_indent = true [syntaxrules] -non_ansi_module = true +module_nonansi_forbidden = true keyword_forbidden_wire_reg = true ``` @@ -1946,7 +1946,7 @@ requires that each interface port includes a modport identifier. See also: - **inout_with_tri** - Useful companion rule. - **input_with_var** - Useful companion rule. -- **non_ansi_module** - Useful companion rule. +- **module_nonansi_forbidden** - Useful companion rule. - **output_with_var** - Useful companion rule. The most relevant clauses of IEEE1800-2017 are: @@ -3111,6 +3111,84 @@ The most relevant clauses of IEEE1800-2017 are: +* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + +## Syntax Rule: `module_nonansi_forbidden` + +### Hint + +Declare `module` header in ANSI style. + +### Reason + +Non-ANSI module headers are visually noisy and error-prone. + +### Pass Example (1 of 3) +```systemverilog +module M // An ANSI module has ports declared in the module header. + ( input a + , output b + ); +endmodule +``` + +### Pass Example (2 of 3) +```systemverilog +module M; // A module with no ports is also ANSI. +endmodule +``` + +### Pass Example (3 of 3) +```systemverilog +module M // Declaring ports in the header with default direction (inout) + ( a // also specifies an ANSI module. + , b + ); +endmodule +``` + +### Fail Example (1 of 1) +```systemverilog +module M + ( a + , b + ); + input a; // Declaring ports outside the module header declaration + output b; // makes this a non-ANSI module. +endmodule +``` + +### Explanation + +There are two ways to declare a module header in SystemVerilog: +1. ANSI style - newer, neater, more succinct, mostly compatible with + IEEE1364-2001 (as long as you don't use `localparam`s for ports). +2. non-ANSI style - additionally compatible with older Verilog (IEEE1364-1995). + +Examples of both styles are given in IEEE1364-2001 (e.g. pages 180 vs 182) and +IEEE1800-2017 (e.g. pages 702 vs 700). + +The non-ANSI style separates the declaration of ports, their direction, and +their datatype. +In addition to requiring more text, and visual noise, to convey the same +information, the non-ANSI style encourages simple coding mistakes where +essential attributes may be forgotten. +This rule requires that module headers are declared using the ANSI style. + +See also: +- **module_ansi_forbidden** - For consistency in IEEE1364-2001 (compatibility + with non-overridable parameters, i.e. `localparam`, in port declarations, + or compatibility with IEEE1364-1995. + +The most relevant clauses of IEEE1364-2001 are: +- 12.1 Modules +- 12.2 Overriding module parameter values + +The most relevant clauses of IEEE1800-2017 are: +- 23.2 Module definitions + + + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * ## Syntax Rule: `multiline_for_begin` @@ -3286,77 +3364,6 @@ The most relevant clauses of IEEE1800-2017 are: -* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * - -## Syntax Rule: `non_ansi_module` - -### Hint - -Declare `module` header in ANSI style. - -### Reason - -Non-ANSI module headers are visually noisy and error-prone. - -### Pass Example (1 of 3) -```systemverilog -module M // An ANSI module has ports declared in the module header. - ( input a - , output b - ); -endmodule -``` - -### Pass Example (2 of 3) -```systemverilog -module M; // A module with no ports is also ANSI. -endmodule -``` - -### Pass Example (3 of 3) -```systemverilog -module M // Declaring ports in the header with default direction (inout) - ( a // also specifies an ANSI module. - , b - ); -endmodule -``` - -### Fail Example (1 of 1) -```systemverilog -module M - ( a - , b - ); - input a; // Declaring ports outside the module header declaration - output b; // makes this a non-ANSI module. -endmodule -``` - -### Explanation - -There are two ways to declare a module header in SystemVerilog: -1. ANSI style - newer, neater, more succinct, compatible with IEEE1364-2001. -2. non-ANSI style - additionally compatible with older Verilog (IEEE1364-1995). - -Examples of both styles are given in IEEE1364-2001 (e.g. pages 180 vs 182) and -IEEE1800-2017 (e.g. pages 702 vs 700). - -The non-ANSI style separates the declaration of ports, their direction, and -their datatype. -In addition to requiring more text, and visual noise, to convey the same -information, the non-ANSI style encourages simple coding mistakes where -essential attributes may be forgotten. -This rule requires that module headers are declared using the ANSI style. - -See also: -- No related rules. - -The most relevant clauses of IEEE1800-2017 are: -- 23.2 Module definitions - - - * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * ## Syntax Rule: `non_blocking_assignment_in_always_comb` @@ -5800,7 +5807,7 @@ See also: - **prefix_module** - **uppercamelcase_module** - **lowercamelcase_module** -- **non_ansi_module** +- **module_nonansi_forbidden** @@ -5852,7 +5859,7 @@ See also: - **prefix_module** - **uppercamelcase_module** - **lowercamelcase_module** -- **non_ansi_module** +- **module_nonansi_forbidden** @@ -7027,7 +7034,7 @@ See also: - **prefix_module** - **uppercamelcase_module** - **lowercamelcase_module** -- **non_ansi_module** +- **module_nonansi_forbidden** @@ -7079,7 +7086,7 @@ See also: - **prefix_module** - **uppercamelcase_module** - **lowercamelcase_module** -- **non_ansi_module** +- **module_nonansi_forbidden** @@ -10225,7 +10232,7 @@ syntaxrules.default_nettype_none = true syntaxrules.function_same_as_system_function = true syntaxrules.keyword_forbidden_always = true syntaxrules.keyword_forbidden_wire_reg = true -syntaxrules.non_ansi_module = true +syntaxrules.module_nonansi_forbidden = true ``` Generally, elaboration-time constants (`parameter`, `localparam`) should be @@ -10969,7 +10976,7 @@ syntaxrules.default_nettype_none = true syntaxrules.function_same_as_system_function = true syntaxrules.keyword_forbidden_always = true syntaxrules.keyword_forbidden_wire_reg = true -syntaxrules.non_ansi_module = true +syntaxrules.module_nonansi_forbidden = true ``` When synthesised into a netlist, generate blocks should have labels so that @@ -11446,7 +11453,7 @@ syntaxrules.action_block_with_side_effect = true syntaxrules.default_nettype_none = true syntaxrules.function_same_as_system_function = true syntaxrules.keyword_forbidden_wire_reg = true -syntaxrules.non_ansi_module = true +syntaxrules.module_nonansi_forbidden = true ``` Generally, elaboration-time constant (`parameter`, `localparam`) should be diff --git a/build.rs b/build.rs index e4e98f46..8189b2ad 100644 --- a/build.rs +++ b/build.rs @@ -54,6 +54,11 @@ const RENAMED_SYNTAXRULES: &[(&str, &str, &str)] = &[ "keyword_required_generate", "KeywordRequiredGenerate", ), + ( + "non_ansi_module", + "module_nonansi_forbidden", + "ModuleNonansiForbidden", + ), ]; fn write_rules_rs( diff --git a/md/manual-introduction.md b/md/manual-introduction.md index 80e3e48c..9b7da746 100644 --- a/md/manual-introduction.md +++ b/md/manual-introduction.md @@ -155,7 +155,7 @@ prefix_label = "lab_" style_indent = true [syntaxrules] -non_ansi_module = true +module_nonansi_forbidden = true keyword_forbidden_wire_reg = true ``` diff --git a/md/ruleset-DaveMcEwan-design.md b/md/ruleset-DaveMcEwan-design.md index c98c2a55..a5529822 100644 --- a/md/ruleset-DaveMcEwan-design.md +++ b/md/ruleset-DaveMcEwan-design.md @@ -375,7 +375,7 @@ syntaxrules.default_nettype_none = true syntaxrules.function_same_as_system_function = true syntaxrules.keyword_forbidden_always = true syntaxrules.keyword_forbidden_wire_reg = true -syntaxrules.non_ansi_module = true +syntaxrules.module_nonansi_forbidden = true ``` Generally, elaboration-time constants (`parameter`, `localparam`) should be diff --git a/md/ruleset-designintent.md b/md/ruleset-designintent.md index 26391b9a..5b301c8c 100644 --- a/md/ruleset-designintent.md +++ b/md/ruleset-designintent.md @@ -32,7 +32,7 @@ syntaxrules.default_nettype_none = true syntaxrules.function_same_as_system_function = true syntaxrules.keyword_forbidden_always = true syntaxrules.keyword_forbidden_wire_reg = true -syntaxrules.non_ansi_module = true +syntaxrules.module_nonansi_forbidden = true ``` When synthesised into a netlist, generate blocks should have labels so that diff --git a/md/ruleset-verifintent.md b/md/ruleset-verifintent.md index 58e55b20..1a0b69c8 100644 --- a/md/ruleset-verifintent.md +++ b/md/ruleset-verifintent.md @@ -30,7 +30,7 @@ syntaxrules.action_block_with_side_effect = true syntaxrules.default_nettype_none = true syntaxrules.function_same_as_system_function = true syntaxrules.keyword_forbidden_wire_reg = true -syntaxrules.non_ansi_module = true +syntaxrules.module_nonansi_forbidden = true ``` Generally, elaboration-time constant (`parameter`, `localparam`) should be diff --git a/md/syntaxrules-explanation-interface_port_with_modport.md b/md/syntaxrules-explanation-interface_port_with_modport.md index 018f56b3..7dfdb13a 100644 --- a/md/syntaxrules-explanation-interface_port_with_modport.md +++ b/md/syntaxrules-explanation-interface_port_with_modport.md @@ -20,7 +20,7 @@ requires that each interface port includes a modport identifier. See also: - **inout_with_tri** - Useful companion rule. - **input_with_var** - Useful companion rule. -- **non_ansi_module** - Useful companion rule. +- **module_nonansi_forbidden** - Useful companion rule. - **output_with_var** - Useful companion rule. The most relevant clauses of IEEE1800-2017 are: diff --git a/md/syntaxrules-explanation-non_ansi_module.md b/md/syntaxrules-explanation-module_nonansi_forbidden.md similarity index 61% rename from md/syntaxrules-explanation-non_ansi_module.md rename to md/syntaxrules-explanation-module_nonansi_forbidden.md index cfc0a9b4..555b33b5 100644 --- a/md/syntaxrules-explanation-non_ansi_module.md +++ b/md/syntaxrules-explanation-module_nonansi_forbidden.md @@ -1,5 +1,6 @@ There are two ways to declare a module header in SystemVerilog: -1. ANSI style - newer, neater, more succinct, compatible with IEEE1364-2001. +1. ANSI style - newer, neater, more succinct, mostly compatible with + IEEE1364-2001 (as long as you don't use `localparam`s for ports). 2. non-ANSI style - additionally compatible with older Verilog (IEEE1364-1995). Examples of both styles are given in IEEE1364-2001 (e.g. pages 180 vs 182) and @@ -13,7 +14,13 @@ essential attributes may be forgotten. This rule requires that module headers are declared using the ANSI style. See also: -- No related rules. +- **module_ansi_forbidden** - For consistency in IEEE1364-2001 (compatibility + with non-overridable parameters, i.e. `localparam`, in port declarations, + or compatibility with IEEE1364-1995. + +The most relevant clauses of IEEE1364-2001 are: +- 12.1 Modules +- 12.2 Overriding module parameter values The most relevant clauses of IEEE1800-2017 are: - 23.2 Module definitions diff --git a/md/syntaxrules-explanation-re_forbidden_module_ansi.md b/md/syntaxrules-explanation-re_forbidden_module_ansi.md index d7e805a7..eb0e8af0 100644 --- a/md/syntaxrules-explanation-re_forbidden_module_ansi.md +++ b/md/syntaxrules-explanation-re_forbidden_module_ansi.md @@ -12,4 +12,4 @@ See also: - **prefix_module** - **uppercamelcase_module** - **lowercamelcase_module** -- **non_ansi_module** +- **module_nonansi_forbidden** diff --git a/md/syntaxrules-explanation-re_forbidden_module_nonansi.md b/md/syntaxrules-explanation-re_forbidden_module_nonansi.md index b7eeaeee..05ccb3f8 100644 --- a/md/syntaxrules-explanation-re_forbidden_module_nonansi.md +++ b/md/syntaxrules-explanation-re_forbidden_module_nonansi.md @@ -14,4 +14,4 @@ See also: - **prefix_module** - **uppercamelcase_module** - **lowercamelcase_module** -- **non_ansi_module** +- **module_nonansi_forbidden** diff --git a/md/syntaxrules-explanation-re_required_module_ansi.md b/md/syntaxrules-explanation-re_required_module_ansi.md index 0162a9c8..012d73fd 100644 --- a/md/syntaxrules-explanation-re_required_module_ansi.md +++ b/md/syntaxrules-explanation-re_required_module_ansi.md @@ -12,4 +12,4 @@ See also: - **prefix_module** - **uppercamelcase_module** - **lowercamelcase_module** -- **non_ansi_module** +- **module_nonansi_forbidden** diff --git a/md/syntaxrules-explanation-re_required_module_nonansi.md b/md/syntaxrules-explanation-re_required_module_nonansi.md index f8326c39..db52cfef 100644 --- a/md/syntaxrules-explanation-re_required_module_nonansi.md +++ b/md/syntaxrules-explanation-re_required_module_nonansi.md @@ -14,4 +14,4 @@ See also: - **prefix_module** - **uppercamelcase_module** - **lowercamelcase_module** -- **non_ansi_module** +- **module_nonansi_forbidden** diff --git a/rulesets/DaveMcEwan-design.toml b/rulesets/DaveMcEwan-design.toml index 2491cfe8..65b94010 100644 --- a/rulesets/DaveMcEwan-design.toml +++ b/rulesets/DaveMcEwan-design.toml @@ -44,7 +44,7 @@ syntaxrules.default_nettype_none = true syntaxrules.function_same_as_system_function = true syntaxrules.keyword_forbidden_always = true syntaxrules.keyword_forbidden_wire_reg = true -syntaxrules.non_ansi_module = true +syntaxrules.module_nonansi_forbidden = true syntaxrules.localparam_type_twostate = true syntaxrules.parameter_type_twostate = true syntaxrules.localparam_explicit_type = true diff --git a/rulesets/designintent.toml b/rulesets/designintent.toml index 8c673059..79560d33 100644 --- a/rulesets/designintent.toml +++ b/rulesets/designintent.toml @@ -14,7 +14,7 @@ syntaxrules.default_nettype_none = true syntaxrules.function_same_as_system_function = true syntaxrules.keyword_forbidden_always = true syntaxrules.keyword_forbidden_wire_reg = true -syntaxrules.non_ansi_module = true +syntaxrules.module_nonansi_forbidden = true syntaxrules.generate_case_with_label = true syntaxrules.generate_for_with_label = true syntaxrules.generate_if_with_label = true diff --git a/rulesets/verifintent.toml b/rulesets/verifintent.toml index 73e97c79..03d980d5 100644 --- a/rulesets/verifintent.toml +++ b/rulesets/verifintent.toml @@ -8,7 +8,7 @@ syntaxrules.action_block_with_side_effect = true syntaxrules.default_nettype_none = true syntaxrules.function_same_as_system_function = true syntaxrules.keyword_forbidden_wire_reg = true -syntaxrules.non_ansi_module = true +syntaxrules.module_nonansi_forbidden = true syntaxrules.localparam_type_twostate = true syntaxrules.parameter_type_twostate = true syntaxrules.localparam_explicit_type = true diff --git a/src/syntaxrules/non_ansi_module.rs b/src/syntaxrules/module_nonansi_forbidden.rs similarity index 87% rename from src/syntaxrules/non_ansi_module.rs rename to src/syntaxrules/module_nonansi_forbidden.rs index 817e1786..94d16edd 100644 --- a/src/syntaxrules/non_ansi_module.rs +++ b/src/syntaxrules/module_nonansi_forbidden.rs @@ -3,9 +3,9 @@ use crate::linter::{SyntaxRule, SyntaxRuleResult}; use sv_parser::{NodeEvent, RefNode, SyntaxTree}; #[derive(Default)] -pub struct NonAnsiModule; +pub struct ModuleNonansiForbidden; -impl SyntaxRule for NonAnsiModule { +impl SyntaxRule for ModuleNonansiForbidden { fn check( &mut self, _syntax_tree: &SyntaxTree, @@ -25,7 +25,7 @@ impl SyntaxRule for NonAnsiModule { } fn name(&self) -> String { - String::from("non_ansi_module") + String::from("module_nonansi_forbidden") } fn hint(&self, _option: &ConfigOption) -> String { diff --git a/testcases/syntaxrules/fail/non_ansi_module.sv b/testcases/syntaxrules/fail/module_nonansi_forbidden.sv similarity index 100% rename from testcases/syntaxrules/fail/non_ansi_module.sv rename to testcases/syntaxrules/fail/module_nonansi_forbidden.sv diff --git a/testcases/syntaxrules/pass/non_ansi_module.sv b/testcases/syntaxrules/pass/module_nonansi_forbidden.sv similarity index 100% rename from testcases/syntaxrules/pass/non_ansi_module.sv rename to testcases/syntaxrules/pass/module_nonansi_forbidden.sv From ef2aa6d7dbfda8042d0782d723b97d54bad6d3b0 Mon Sep 17 00:00:00 2001 From: Dave McEwan Date: Fri, 10 Nov 2023 15:34:43 +0000 Subject: [PATCH 02/13] V2k1 New rule `module_ansi_forbidden` --- MANUAL.md | 92 +++++++++++++++++++ ...rules-explanation-module_ansi_forbidden.md | 33 +++++++ src/syntaxrules/module_ansi_forbidden.rs | 45 +++++++++ .../syntaxrules/fail/module_ansi_forbidden.sv | 15 +++ .../syntaxrules/pass/module_ansi_forbidden.sv | 10 ++ 5 files changed, 195 insertions(+) create mode 100644 md/syntaxrules-explanation-module_ansi_forbidden.md create mode 100644 src/syntaxrules/module_ansi_forbidden.rs create mode 100644 testcases/syntaxrules/fail/module_ansi_forbidden.sv create mode 100644 testcases/syntaxrules/pass/module_ansi_forbidden.sv diff --git a/MANUAL.md b/MANUAL.md index e21b3a8c..edc27eb5 100644 --- a/MANUAL.md +++ b/MANUAL.md @@ -3111,6 +3111,98 @@ The most relevant clauses of IEEE1800-2017 are: +* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + +## Syntax Rule: `module_ansi_forbidden` + +### Hint + +Declare `module` header in non-ANSI style. + +### Reason + +Only SystemVerilog, not Verilog, allows `localparam` in ANSI module header. + +### Pass Example (1 of 2) +```systemverilog +module M + ( a + , b + ); + input a; // Declaring ports outside the module header declaration + output b; // makes this a non-ANSI module. +endmodule +``` + +### Pass Example (2 of 2) +```systemverilog +module M; // A module with no portlist is ANSI, but allowed. +endmodule +``` + +### Fail Example (1 of 3) +```systemverilog +module M // An ANSI module has ports declared in the module header. + ( input a + , output b + ); +endmodule +``` + +### Fail Example (2 of 3) +```systemverilog +module M // Declaring ports in the header with default direction (inout) + ( a // also specifies an ANSI module where directions are not given + , b // later. + ); +endmodule +``` + +### Fail Example (3 of 3) +```systemverilog +module M + (); // A module with an empty portlist is ANSI. +endmodule +``` + +### Explanation + +There are two ways to declare a module header in SystemVerilog: +1. ANSI style - newer, neater, more succinct, mostly compatible with + IEEE1364-2001 (as long as you don't use `localparam`s for ports). +2. non-ANSI style - additionally compatible with older Verilog (IEEE1364-1995). + +Examples of both styles are given in IEEE1364-2001 (e.g. pages 180 vs 182) and +IEEE1800-2017 (e.g. pages 702 vs 700). + +The non-ANSI style separates the declaration of ports, their direction, and +their datatype. +While requiring more text, and visual noise, to convey the same information, +the non-ANSI style allows non-overridable parameters, i.e. `localparam`, to be +used in port declarations. +If only `parameter` is used instead, as allowed in IEEE1364, an instance may +inadvertently override a parameter, thus causing difficult-to-debug issues. + +This rule requires that module headers are declared using the non-ANSI style. +It is recommended only to use this rule where compatibility with IEEE1364 is +required. +By forbidding the ANSI style, this rule requires that module declarations are +written in a consistent manner, which facilitates easier review and prevents +easily overlooked issues before they become problems. + +See also: +- **module_nonansi_forbidden** - For safer usability where compatibility with + Verilog is not required. + +The most relevant clauses of IEEE1364-2001 are: +- 12.1 Modules +- 12.2 Overriding module parameter values + +The most relevant clauses of IEEE1800-2017 are: +- 23.2 Module definitions + + + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * ## Syntax Rule: `module_nonansi_forbidden` diff --git a/md/syntaxrules-explanation-module_ansi_forbidden.md b/md/syntaxrules-explanation-module_ansi_forbidden.md new file mode 100644 index 00000000..78f94a9f --- /dev/null +++ b/md/syntaxrules-explanation-module_ansi_forbidden.md @@ -0,0 +1,33 @@ +There are two ways to declare a module header in SystemVerilog: +1. ANSI style - newer, neater, more succinct, mostly compatible with + IEEE1364-2001 (as long as you don't use `localparam`s for ports). +2. non-ANSI style - additionally compatible with older Verilog (IEEE1364-1995). + +Examples of both styles are given in IEEE1364-2001 (e.g. pages 180 vs 182) and +IEEE1800-2017 (e.g. pages 702 vs 700). + +The non-ANSI style separates the declaration of ports, their direction, and +their datatype. +While requiring more text, and visual noise, to convey the same information, +the non-ANSI style allows non-overridable parameters, i.e. `localparam`, to be +used in port declarations. +If only `parameter` is used instead, as allowed in IEEE1364, an instance may +inadvertently override a parameter, thus causing difficult-to-debug issues. + +This rule requires that module headers are declared using the non-ANSI style. +It is recommended only to use this rule where compatibility with IEEE1364 is +required. +By forbidding the ANSI style, this rule requires that module declarations are +written in a consistent manner, which facilitates easier review and prevents +easily overlooked issues before they become problems. + +See also: +- **module_nonansi_forbidden** - For safer usability where compatibility with + Verilog is not required. + +The most relevant clauses of IEEE1364-2001 are: +- 12.1 Modules +- 12.2 Overriding module parameter values + +The most relevant clauses of IEEE1800-2017 are: +- 23.2 Module definitions diff --git a/src/syntaxrules/module_ansi_forbidden.rs b/src/syntaxrules/module_ansi_forbidden.rs new file mode 100644 index 00000000..96f62617 --- /dev/null +++ b/src/syntaxrules/module_ansi_forbidden.rs @@ -0,0 +1,45 @@ +use crate::config::ConfigOption; +use crate::linter::{SyntaxRule, SyntaxRuleResult}; +use sv_parser::{NodeEvent, RefNode, SyntaxTree}; + +#[derive(Default)] +pub struct ModuleAnsiForbidden; + +impl SyntaxRule for ModuleAnsiForbidden { + fn check( + &mut self, + _syntax_tree: &SyntaxTree, + event: &NodeEvent, + _option: &ConfigOption, + ) -> SyntaxRuleResult { + let node = match event { + NodeEvent::Enter(x) => x, + NodeEvent::Leave(_) => { + return SyntaxRuleResult::Pass; + } + }; + match node { + RefNode::ModuleAnsiHeader(x) => { + let (_, _, _, _, _, _, ports, _) = &x.nodes; + if let Some(_) = ports { + SyntaxRuleResult::Fail + } else { + SyntaxRuleResult::Pass + } + } + _ => SyntaxRuleResult::Pass, + } + } + + fn name(&self) -> String { + String::from("module_ansi_forbidden") + } + + fn hint(&self, _option: &ConfigOption) -> String { + String::from("Declare `module` header in non-ANSI style.") + } + + fn reason(&self) -> String { + String::from("Only SystemVerilog, not Verilog, allows `localparam` in ANSI module header.") + } +} diff --git a/testcases/syntaxrules/fail/module_ansi_forbidden.sv b/testcases/syntaxrules/fail/module_ansi_forbidden.sv new file mode 100644 index 00000000..c9503051 --- /dev/null +++ b/testcases/syntaxrules/fail/module_ansi_forbidden.sv @@ -0,0 +1,15 @@ +module M // An ANSI module has ports declared in the module header. + ( input a + , output b + ); +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M // Declaring ports in the header with default direction (inout) + ( a // also specifies an ANSI module where directions are not given + , b // later. + ); +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M + (); // A module with an empty portlist is ANSI. +endmodule diff --git a/testcases/syntaxrules/pass/module_ansi_forbidden.sv b/testcases/syntaxrules/pass/module_ansi_forbidden.sv new file mode 100644 index 00000000..499f396f --- /dev/null +++ b/testcases/syntaxrules/pass/module_ansi_forbidden.sv @@ -0,0 +1,10 @@ +module M + ( a + , b + ); + input a; // Declaring ports outside the module header declaration + output b; // makes this a non-ANSI module. +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; // A module with no portlist is ANSI, but allowed. +endmodule From 107350a279a1285c17e1f0355de630fdeb2c0e0e Mon Sep 17 00:00:00 2001 From: Dave McEwan Date: Fri, 10 Nov 2023 16:16:55 +0000 Subject: [PATCH 03/13] V2k1 New rule `operator_self_assignment` --- MANUAL.md | 127 ++++++++++++++++++ ...es-explanation-operator_self_assignment.md | 17 +++ src/syntaxrules/operator_self_assignment.rs | 61 +++++++++ .../fail/operator_self_assignment.sv | 47 +++++++ .../pass/operator_self_assignment.sv | 5 + 5 files changed, 257 insertions(+) create mode 100644 md/syntaxrules-explanation-operator_self_assignment.md create mode 100644 src/syntaxrules/operator_self_assignment.rs create mode 100644 testcases/syntaxrules/fail/operator_self_assignment.sv create mode 100644 testcases/syntaxrules/pass/operator_self_assignment.sv diff --git a/MANUAL.md b/MANUAL.md index edc27eb5..ff5343ec 100644 --- a/MANUAL.md +++ b/MANUAL.md @@ -3568,6 +3568,133 @@ The most relevant clauses of IEEE1800-2017 are: +* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + +## Syntax Rule: `operator_self_assignment` + +### Hint + +Use `=` with a binary operator instead of a self-assignment operator. + +### Reason + +Only SystemVerilog, not Verilog, allows self-assignment operators. + +### Pass Example (1 of 1) +```systemverilog +module M; + always @* + if (a == b) // Logical-equality operator is not an assignment. + z = y; // Simple assignment operator is allowed. +endmodule +``` + +### Fail Example (1 of 12) +```systemverilog +module M; + always @* z += y; // Addition `z = z + y` +endmodule +``` + +### Fail Example (2 of 12) +```systemverilog +module M; + always @* z -= y; // Subtraction `z = z - y` +endmodule +``` + +### Fail Example (3 of 12) +```systemverilog +module M; + always @* z *= y; // Multiplication `z = z * y` +endmodule +``` + +### Fail Example (4 of 12) +```systemverilog +module M; + always @* z /= y; // Division `z = z / y` +endmodule +``` + +### Fail Example (5 of 12) +```systemverilog +module M; + always @* z %= y; // Modulo `z = z % y` +endmodule +``` + +### Fail Example (6 of 12) +```systemverilog +module M; + always @* z &= y; // Bitwise AND `z = z & y` +endmodule +``` + +### Fail Example (7 of 12) +```systemverilog +module M; + always @* z |= y; // Bitwise OR `z = z | y` +endmodule +``` + +### Fail Example (8 of 12) +```systemverilog +module M; + always @* z ^= y; // Bitwise XOR `z = z ^ y` +endmodule +``` + +### Fail Example (9 of 12) +```systemverilog +module M; + always @* z <<= y; // Logical left shift `z = z << y` +endmodule +``` + +### Fail Example (10 of 12) +```systemverilog +module M; + always @* z >>= y; // Logical right shift `z = z >> y` +endmodule +``` + +### Fail Example (11 of 12) +```systemverilog +module M; + always @* z <<<= y; // Arithmetic left shift `z = z <<< y` +endmodule +``` + +### Fail Example (12 of 12) +```systemverilog +module M; + always @* z >>>= y; // Arithmetic right shift `z = z >>> y` +endmodule +``` + +### Explanation + +Self-assignment operators (`+=`, `-=`, `*=`, `/=`, `%=`, `&=`, `|=`, `^=`, +`<<=`, `>>=`, `<<<=`, and `>>>=`) are part of SystemVerilog (IEEE1800), but not +Verilog (IEEE1364). + +This rule allows only simple assigment (using `=`) to encourage backwards +compatibility with Verilog. + +See also: +- **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. + +The most relevant clauses of IEEE1364-2001 are: +- 4.1 Operators +- 9.2.1 Blocking procedural assignments + +The most relevant clauses of IEEE1800-2017 are: +- 10.4.1 Blocking procedural assignments +- 11.4.1 Assignment operators + + + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * ## Syntax Rule: `output_with_var` diff --git a/md/syntaxrules-explanation-operator_self_assignment.md b/md/syntaxrules-explanation-operator_self_assignment.md new file mode 100644 index 00000000..740152e7 --- /dev/null +++ b/md/syntaxrules-explanation-operator_self_assignment.md @@ -0,0 +1,17 @@ +Self-assignment operators (`+=`, `-=`, `*=`, `/=`, `%=`, `&=`, `|=`, `^=`, +`<<=`, `>>=`, `<<<=`, and `>>>=`) are part of SystemVerilog (IEEE1800), but not +Verilog (IEEE1364). + +This rule allows only simple assigment (using `=`) to encourage backwards +compatibility with Verilog. + +See also: +- **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. + +The most relevant clauses of IEEE1364-2001 are: +- 4.1 Operators +- 9.2.1 Blocking procedural assignments + +The most relevant clauses of IEEE1800-2017 are: +- 10.4.1 Blocking procedural assignments +- 11.4.1 Assignment operators diff --git a/src/syntaxrules/operator_self_assignment.rs b/src/syntaxrules/operator_self_assignment.rs new file mode 100644 index 00000000..1682ddc1 --- /dev/null +++ b/src/syntaxrules/operator_self_assignment.rs @@ -0,0 +1,61 @@ +use crate::config::ConfigOption; +use crate::linter::{SyntaxRule, SyntaxRuleResult}; +use sv_parser::{unwrap_node, unwrap_locate, Locate, NodeEvent, RefNode, SyntaxTree}; + +#[derive(Default)] +pub struct OperatorSelfAssignment; + +impl SyntaxRule for OperatorSelfAssignment { + fn check( + &mut self, + syntax_tree: &SyntaxTree, + event: &NodeEvent, + _option: &ConfigOption, + ) -> SyntaxRuleResult { + let node = match event { + NodeEvent::Enter(x) => x, + NodeEvent::Leave(_) => { + return SyntaxRuleResult::Pass; + } + }; + + match node { + RefNode::AssignmentOperator(x) => { + let loc: Option<&Locate> = match unwrap_node!(*x, Symbol) { + Some(RefNode::Symbol(symbol_)) => { + unwrap_locate!(symbol_) + } + _ => None, + }; + + if let Some(loc) = loc { + let s = syntax_tree.get_str(loc).unwrap(); + + // Only Verilog-compatible assignment `=` is a single ASCII character. + // assignment_operator ::= + // = | += | -= | *= | /= | %= | &= | |= | ^= | <<= | >>= | <<<= | >>>= + if 1 < s.len() { + SyntaxRuleResult::Fail + } else { + SyntaxRuleResult::Pass + } + } else { + SyntaxRuleResult::Pass + } + } + _ => SyntaxRuleResult::Pass, + } + } + + fn name(&self) -> String { + String::from("operator_self_assignment") + } + + fn hint(&self, _option: &ConfigOption) -> String { + String::from("Use `=` with a binary operator instead of a self-assignment operator.") + } + + fn reason(&self) -> String { + String::from("Only SystemVerilog, not Verilog, allows self-assignment operators.") + } +} diff --git a/testcases/syntaxrules/fail/operator_self_assignment.sv b/testcases/syntaxrules/fail/operator_self_assignment.sv new file mode 100644 index 00000000..76a86eac --- /dev/null +++ b/testcases/syntaxrules/fail/operator_self_assignment.sv @@ -0,0 +1,47 @@ +module M; + always @* z += y; // Addition `z = z + y` +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @* z -= y; // Subtraction `z = z - y` +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @* z *= y; // Multiplication `z = z * y` +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @* z /= y; // Division `z = z / y` +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @* z %= y; // Modulo `z = z % y` +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @* z &= y; // Bitwise AND `z = z & y` +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @* z |= y; // Bitwise OR `z = z | y` +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @* z ^= y; // Bitwise XOR `z = z ^ y` +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @* z <<= y; // Logical left shift `z = z << y` +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @* z >>= y; // Logical right shift `z = z >> y` +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @* z <<<= y; // Arithmetic left shift `z = z <<< y` +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @* z >>>= y; // Arithmetic right shift `z = z >>> y` +endmodule diff --git a/testcases/syntaxrules/pass/operator_self_assignment.sv b/testcases/syntaxrules/pass/operator_self_assignment.sv new file mode 100644 index 00000000..25a7f7ed --- /dev/null +++ b/testcases/syntaxrules/pass/operator_self_assignment.sv @@ -0,0 +1,5 @@ +module M; + always @* + if (a == b) // Logical-equality operator is not an assignment. + z = y; // Simple assignment operator is allowed. +endmodule From a19363dc49b327ace3d70761ab4b2aa28f21b0df Mon Sep 17 00:00:00 2001 From: Dave McEwan Date: Fri, 10 Nov 2023 17:00:46 +0000 Subject: [PATCH 04/13] V2k1 New rules `keyword_forbidden_always_comb`, `keyword_forbidden_always_ff`, `keyword_forbidden_always_latch` --- MANUAL.md | 165 ++++++++++++++++++ ...planation-keyword_forbidden_always_comb.md | 18 ++ ...explanation-keyword_forbidden_always_ff.md | 18 ++ ...lanation-keyword_forbidden_always_latch.md | 18 ++ ...es-explanation-operator_self_assignment.md | 3 + .../keyword_forbidden_always_comb.rs | 38 ++++ .../keyword_forbidden_always_ff.rs | 38 ++++ .../keyword_forbidden_always_latch.rs | 38 ++++ .../fail/keyword_forbidden_always_comb.sv | 3 + .../fail/keyword_forbidden_always_ff.sv | 4 + .../fail/keyword_forbidden_always_latch.sv | 5 + .../pass/keyword_forbidden_always_comb.sv | 3 + .../pass/keyword_forbidden_always_ff.sv | 4 + .../pass/keyword_forbidden_always_latch.sv | 11 ++ 14 files changed, 366 insertions(+) create mode 100644 md/syntaxrules-explanation-keyword_forbidden_always_comb.md create mode 100644 md/syntaxrules-explanation-keyword_forbidden_always_ff.md create mode 100644 md/syntaxrules-explanation-keyword_forbidden_always_latch.md create mode 100644 src/syntaxrules/keyword_forbidden_always_comb.rs create mode 100644 src/syntaxrules/keyword_forbidden_always_ff.rs create mode 100644 src/syntaxrules/keyword_forbidden_always_latch.rs create mode 100644 testcases/syntaxrules/fail/keyword_forbidden_always_comb.sv create mode 100644 testcases/syntaxrules/fail/keyword_forbidden_always_ff.sv create mode 100644 testcases/syntaxrules/fail/keyword_forbidden_always_latch.sv create mode 100644 testcases/syntaxrules/pass/keyword_forbidden_always_comb.sv create mode 100644 testcases/syntaxrules/pass/keyword_forbidden_always_ff.sv create mode 100644 testcases/syntaxrules/pass/keyword_forbidden_always_latch.sv diff --git a/MANUAL.md b/MANUAL.md index ff5343ec..93864d4a 100644 --- a/MANUAL.md +++ b/MANUAL.md @@ -2036,6 +2036,168 @@ The most relevant clauses of IEEE1800-2017 are: +* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + +## Syntax Rule: `keyword_forbidden_always_comb` + +### Hint + +Use `always @*` instead of `always_comb`. + +### Reason + +Only SystemVerilog, not Verilog, has `always_comb`. + +### Pass Example (1 of 1) +```systemverilog +module M; + always @* z = x + y; +endmodule +``` + +### Fail Example (1 of 1) +```systemverilog +module M; + always_comb z = x + y; +endmodule +``` + +### Explanation + +The keywords `always_comb`, `always_ff`, and `always_latch` were added to +SystemVerilog (IEEE1800) to require extra safety checks at compile time. +Verilog (IEEE1364) only has `always`, which can describe equivalent behaviour +but without the compile-time checks. +This rule requires `always @*` to be used instead of `always_comb` for +backwards compatibility with Verilog. + +See also: +- **keyword_forbidden_always_ff** - Suggested companion rule. +- **keyword_forbidden_always_latch** - Suggested companion rule. +- **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **operator_self_assignment** - Suggested companion rule. + +The most relevant clauses of IEEE1364-2001 are: +- 9.9 Structured procedures + +The most relevant clauses of IEEE1800-2017 are: +- 9.2 Structured procedures + + + +* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + +## Syntax Rule: `keyword_forbidden_always_ff` + +### Hint + +Use `always @(posedge clk)` instead of `always_ff @(posedge clk)`. + +### Reason + +Only SystemVerilog, not Verilog, has `always_ff`. + +### Pass Example (1 of 1) +```systemverilog +module M; + always @(posedge clk) + d <= q; +endmodule +``` + +### Fail Example (1 of 1) +```systemverilog +module M; + always_ff @(posedge clk) + d <= q; +endmodule +``` + +### Explanation + +The keywords `always_comb`, `always_ff`, and `always_latch` were added to +SystemVerilog (IEEE1800) to require extra safety checks at compile time. +Verilog (IEEE1364) only has `always`, which can describe equivalent behaviour +but without the compile-time checks. +This rule requires something like `always @(posedge clk)` to be used instead of +`always_ff @(posedge clk)` for backwards compatibility with Verilog. + +See also: +- **keyword_forbidden_always_comb** - Suggested companion rule. +- **keyword_forbidden_always_latch** - Suggested companion rule. +- **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **operator_self_assignment** - Suggested companion rule. + +The most relevant clauses of IEEE1364-2001 are: +- 9.9 Structured procedures + +The most relevant clauses of IEEE1800-2017 are: +- 9.2 Structured procedures + + + +* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + +## Syntax Rule: `keyword_forbidden_always_latch` + +### Hint + +Use `always @*` or `always @(en)` instead of `always_latch`. + +### Reason + +Only SystemVerilog, not Verilog, has `always_latch`. + +### Pass Example (1 of 2) +```systemverilog +module M; + always @* + if (en) + d <= q; +endmodule +``` + +### Pass Example (2 of 2) +```systemverilog +module M; + always @(en) + if (en) + d <= q; +endmodule +``` + +### Fail Example (1 of 1) +```systemverilog +module M; + always_latch + if (en) + d <= q; +endmodule +``` + +### Explanation + +The keywords `always_comb`, `always_ff`, and `always_latch` were added to +SystemVerilog (IEEE1800) to require extra safety checks at compile time. +Verilog (IEEE1364) only has `always`, which can describe equivalent behaviour +but without the compile-time checks. +This rule requires `always @*` or something like `always @(en)` to be used +instead of `always_latch` for backwards compatibility with Verilog. + +See also: +- **keyword_forbidden_always_comb** - Suggested companion rule. +- **keyword_forbidden_always_ff** - Suggested companion rule. +- **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **operator_self_assignment** - Suggested companion rule. + +The most relevant clauses of IEEE1364-2001 are: +- 9.9 Structured procedures + +The most relevant clauses of IEEE1800-2017 are: +- 9.2 Structured procedures + + + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * ## Syntax Rule: `keyword_forbidden_generate` @@ -3684,6 +3846,9 @@ compatibility with Verilog. See also: - **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **keyword_forbidden_always_comb** - Suggested companion rule. +- **keyword_forbidden_always_ff** - Suggested companion rule. +- **keyword_forbidden_always_latch** - Suggested companion rule. The most relevant clauses of IEEE1364-2001 are: - 4.1 Operators diff --git a/md/syntaxrules-explanation-keyword_forbidden_always_comb.md b/md/syntaxrules-explanation-keyword_forbidden_always_comb.md new file mode 100644 index 00000000..e28c7540 --- /dev/null +++ b/md/syntaxrules-explanation-keyword_forbidden_always_comb.md @@ -0,0 +1,18 @@ +The keywords `always_comb`, `always_ff`, and `always_latch` were added to +SystemVerilog (IEEE1800) to require extra safety checks at compile time. +Verilog (IEEE1364) only has `always`, which can describe equivalent behaviour +but without the compile-time checks. +This rule requires `always @*` to be used instead of `always_comb` for +backwards compatibility with Verilog. + +See also: +- **keyword_forbidden_always_ff** - Suggested companion rule. +- **keyword_forbidden_always_latch** - Suggested companion rule. +- **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **operator_self_assignment** - Suggested companion rule. + +The most relevant clauses of IEEE1364-2001 are: +- 9.9 Structured procedures + +The most relevant clauses of IEEE1800-2017 are: +- 9.2 Structured procedures diff --git a/md/syntaxrules-explanation-keyword_forbidden_always_ff.md b/md/syntaxrules-explanation-keyword_forbidden_always_ff.md new file mode 100644 index 00000000..1dd40d20 --- /dev/null +++ b/md/syntaxrules-explanation-keyword_forbidden_always_ff.md @@ -0,0 +1,18 @@ +The keywords `always_comb`, `always_ff`, and `always_latch` were added to +SystemVerilog (IEEE1800) to require extra safety checks at compile time. +Verilog (IEEE1364) only has `always`, which can describe equivalent behaviour +but without the compile-time checks. +This rule requires something like `always @(posedge clk)` to be used instead of +`always_ff @(posedge clk)` for backwards compatibility with Verilog. + +See also: +- **keyword_forbidden_always_comb** - Suggested companion rule. +- **keyword_forbidden_always_latch** - Suggested companion rule. +- **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **operator_self_assignment** - Suggested companion rule. + +The most relevant clauses of IEEE1364-2001 are: +- 9.9 Structured procedures + +The most relevant clauses of IEEE1800-2017 are: +- 9.2 Structured procedures diff --git a/md/syntaxrules-explanation-keyword_forbidden_always_latch.md b/md/syntaxrules-explanation-keyword_forbidden_always_latch.md new file mode 100644 index 00000000..450554ac --- /dev/null +++ b/md/syntaxrules-explanation-keyword_forbidden_always_latch.md @@ -0,0 +1,18 @@ +The keywords `always_comb`, `always_ff`, and `always_latch` were added to +SystemVerilog (IEEE1800) to require extra safety checks at compile time. +Verilog (IEEE1364) only has `always`, which can describe equivalent behaviour +but without the compile-time checks. +This rule requires `always @*` or something like `always @(en)` to be used +instead of `always_latch` for backwards compatibility with Verilog. + +See also: +- **keyword_forbidden_always_comb** - Suggested companion rule. +- **keyword_forbidden_always_ff** - Suggested companion rule. +- **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **operator_self_assignment** - Suggested companion rule. + +The most relevant clauses of IEEE1364-2001 are: +- 9.9 Structured procedures + +The most relevant clauses of IEEE1800-2017 are: +- 9.2 Structured procedures diff --git a/md/syntaxrules-explanation-operator_self_assignment.md b/md/syntaxrules-explanation-operator_self_assignment.md index 740152e7..b003f489 100644 --- a/md/syntaxrules-explanation-operator_self_assignment.md +++ b/md/syntaxrules-explanation-operator_self_assignment.md @@ -7,6 +7,9 @@ compatibility with Verilog. See also: - **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **keyword_forbidden_always_comb** - Suggested companion rule. +- **keyword_forbidden_always_ff** - Suggested companion rule. +- **keyword_forbidden_always_latch** - Suggested companion rule. The most relevant clauses of IEEE1364-2001 are: - 4.1 Operators diff --git a/src/syntaxrules/keyword_forbidden_always_comb.rs b/src/syntaxrules/keyword_forbidden_always_comb.rs new file mode 100644 index 00000000..2673cd28 --- /dev/null +++ b/src/syntaxrules/keyword_forbidden_always_comb.rs @@ -0,0 +1,38 @@ +use crate::config::ConfigOption; +use crate::linter::{SyntaxRule, SyntaxRuleResult}; +use sv_parser::{AlwaysKeyword, NodeEvent, RefNode, SyntaxTree}; + +#[derive(Default)] +pub struct KeywordForbiddenAlwaysComb; + +impl SyntaxRule for KeywordForbiddenAlwaysComb { + fn check( + &mut self, + _syntax_tree: &SyntaxTree, + event: &NodeEvent, + _option: &ConfigOption, + ) -> SyntaxRuleResult { + let node = match event { + NodeEvent::Enter(x) => x, + NodeEvent::Leave(_) => { + return SyntaxRuleResult::Pass; + } + }; + match node { + RefNode::AlwaysKeyword(AlwaysKeyword::AlwaysComb(_)) => SyntaxRuleResult::Fail, + _ => SyntaxRuleResult::Pass, + } + } + + fn name(&self) -> String { + String::from("keyword_forbidden_always_comb") + } + + fn hint(&self, _option: &ConfigOption) -> String { + String::from("Use `always @*` instead of `always_comb`.") + } + + fn reason(&self) -> String { + String::from("Only SystemVerilog, not Verilog, has `always_comb`.") + } +} diff --git a/src/syntaxrules/keyword_forbidden_always_ff.rs b/src/syntaxrules/keyword_forbidden_always_ff.rs new file mode 100644 index 00000000..b586ea4d --- /dev/null +++ b/src/syntaxrules/keyword_forbidden_always_ff.rs @@ -0,0 +1,38 @@ +use crate::config::ConfigOption; +use crate::linter::{SyntaxRule, SyntaxRuleResult}; +use sv_parser::{AlwaysKeyword, NodeEvent, RefNode, SyntaxTree}; + +#[derive(Default)] +pub struct KeywordForbiddenAlwaysFf; + +impl SyntaxRule for KeywordForbiddenAlwaysFf { + fn check( + &mut self, + _syntax_tree: &SyntaxTree, + event: &NodeEvent, + _option: &ConfigOption, + ) -> SyntaxRuleResult { + let node = match event { + NodeEvent::Enter(x) => x, + NodeEvent::Leave(_) => { + return SyntaxRuleResult::Pass; + } + }; + match node { + RefNode::AlwaysKeyword(AlwaysKeyword::AlwaysFf(_)) => SyntaxRuleResult::Fail, + _ => SyntaxRuleResult::Pass, + } + } + + fn name(&self) -> String { + String::from("keyword_forbidden_always_ff") + } + + fn hint(&self, _option: &ConfigOption) -> String { + String::from("Use `always @(posedge clk)` instead of `always_ff @(posedge clk)`.") + } + + fn reason(&self) -> String { + String::from("Only SystemVerilog, not Verilog, has `always_ff`.") + } +} diff --git a/src/syntaxrules/keyword_forbidden_always_latch.rs b/src/syntaxrules/keyword_forbidden_always_latch.rs new file mode 100644 index 00000000..5df55a01 --- /dev/null +++ b/src/syntaxrules/keyword_forbidden_always_latch.rs @@ -0,0 +1,38 @@ +use crate::config::ConfigOption; +use crate::linter::{SyntaxRule, SyntaxRuleResult}; +use sv_parser::{AlwaysKeyword, NodeEvent, RefNode, SyntaxTree}; + +#[derive(Default)] +pub struct KeywordForbiddenAlwaysLatch; + +impl SyntaxRule for KeywordForbiddenAlwaysLatch { + fn check( + &mut self, + _syntax_tree: &SyntaxTree, + event: &NodeEvent, + _option: &ConfigOption, + ) -> SyntaxRuleResult { + let node = match event { + NodeEvent::Enter(x) => x, + NodeEvent::Leave(_) => { + return SyntaxRuleResult::Pass; + } + }; + match node { + RefNode::AlwaysKeyword(AlwaysKeyword::AlwaysLatch(_)) => SyntaxRuleResult::Fail, + _ => SyntaxRuleResult::Pass, + } + } + + fn name(&self) -> String { + String::from("keyword_forbidden_always_latch") + } + + fn hint(&self, _option: &ConfigOption) -> String { + String::from("Use `always @*` or `always @(en)` instead of `always_latch`.") + } + + fn reason(&self) -> String { + String::from("Only SystemVerilog, not Verilog, has `always_latch`.") + } +} diff --git a/testcases/syntaxrules/fail/keyword_forbidden_always_comb.sv b/testcases/syntaxrules/fail/keyword_forbidden_always_comb.sv new file mode 100644 index 00000000..9ed037ba --- /dev/null +++ b/testcases/syntaxrules/fail/keyword_forbidden_always_comb.sv @@ -0,0 +1,3 @@ +module M; + always_comb z = x + y; +endmodule diff --git a/testcases/syntaxrules/fail/keyword_forbidden_always_ff.sv b/testcases/syntaxrules/fail/keyword_forbidden_always_ff.sv new file mode 100644 index 00000000..35bd9f29 --- /dev/null +++ b/testcases/syntaxrules/fail/keyword_forbidden_always_ff.sv @@ -0,0 +1,4 @@ +module M; + always_ff @(posedge clk) + d <= q; +endmodule diff --git a/testcases/syntaxrules/fail/keyword_forbidden_always_latch.sv b/testcases/syntaxrules/fail/keyword_forbidden_always_latch.sv new file mode 100644 index 00000000..0989992b --- /dev/null +++ b/testcases/syntaxrules/fail/keyword_forbidden_always_latch.sv @@ -0,0 +1,5 @@ +module M; + always_latch + if (en) + d <= q; +endmodule diff --git a/testcases/syntaxrules/pass/keyword_forbidden_always_comb.sv b/testcases/syntaxrules/pass/keyword_forbidden_always_comb.sv new file mode 100644 index 00000000..617a7269 --- /dev/null +++ b/testcases/syntaxrules/pass/keyword_forbidden_always_comb.sv @@ -0,0 +1,3 @@ +module M; + always @* z = x + y; +endmodule diff --git a/testcases/syntaxrules/pass/keyword_forbidden_always_ff.sv b/testcases/syntaxrules/pass/keyword_forbidden_always_ff.sv new file mode 100644 index 00000000..ee69ff55 --- /dev/null +++ b/testcases/syntaxrules/pass/keyword_forbidden_always_ff.sv @@ -0,0 +1,4 @@ +module M; + always @(posedge clk) + d <= q; +endmodule diff --git a/testcases/syntaxrules/pass/keyword_forbidden_always_latch.sv b/testcases/syntaxrules/pass/keyword_forbidden_always_latch.sv new file mode 100644 index 00000000..b77a3b7a --- /dev/null +++ b/testcases/syntaxrules/pass/keyword_forbidden_always_latch.sv @@ -0,0 +1,11 @@ +module M; + always @* + if (en) + d <= q; +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @(en) + if (en) + d <= q; +endmodule From 7e1180a7ca194e26102286ff43fdcb130ccda9c2 Mon Sep 17 00:00:00 2001 From: Dave McEwan Date: Fri, 10 Nov 2023 17:25:53 +0000 Subject: [PATCH 05/13] V2k1 New rule `operator_incdec` --- MANUAL.md | 139 ++++++++++++++++++ ...planation-keyword_forbidden_always_comb.md | 1 + ...explanation-keyword_forbidden_always_ff.md | 1 + ...lanation-keyword_forbidden_always_latch.md | 1 + md/syntaxrules-explanation-operator_incdec.md | 22 +++ ...es-explanation-operator_self_assignment.md | 1 + src/syntaxrules/operator_incdec.rs | 39 +++++ testcases/syntaxrules/fail/operator_incdec.sv | 29 ++++ testcases/syntaxrules/pass/operator_incdec.sv | 29 ++++ 9 files changed, 262 insertions(+) create mode 100644 md/syntaxrules-explanation-operator_incdec.md create mode 100644 src/syntaxrules/operator_incdec.rs create mode 100644 testcases/syntaxrules/fail/operator_incdec.sv create mode 100644 testcases/syntaxrules/pass/operator_incdec.sv diff --git a/MANUAL.md b/MANUAL.md index 93864d4a..db357301 100644 --- a/MANUAL.md +++ b/MANUAL.md @@ -2075,6 +2075,7 @@ See also: - **keyword_forbidden_always_ff** - Suggested companion rule. - **keyword_forbidden_always_latch** - Suggested companion rule. - **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **operator_incdec** - Suggested companion rule. - **operator_self_assignment** - Suggested companion rule. The most relevant clauses of IEEE1364-2001 are: @@ -2126,6 +2127,7 @@ See also: - **keyword_forbidden_always_comb** - Suggested companion rule. - **keyword_forbidden_always_latch** - Suggested companion rule. - **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **operator_incdec** - Suggested companion rule. - **operator_self_assignment** - Suggested companion rule. The most relevant clauses of IEEE1364-2001 are: @@ -2188,6 +2190,7 @@ See also: - **keyword_forbidden_always_comb** - Suggested companion rule. - **keyword_forbidden_always_ff** - Suggested companion rule. - **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **operator_incdec** - Suggested companion rule. - **operator_self_assignment** - Suggested companion rule. The most relevant clauses of IEEE1364-2001 are: @@ -3730,6 +3733,141 @@ The most relevant clauses of IEEE1800-2017 are: +* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + +## Syntax Rule: `operator_incdec` + +### Hint + +Use `=` with a `+` or `-` instead of an increment or decrement operator. + +### Reason + +Only SystemVerilog, not Verilog, has increment and decrement operators. + +### Pass Example (1 of 6) +```systemverilog +module M; + always @(posedge clk) z = z - 1; +endmodule +``` + +### Pass Example (2 of 6) +```systemverilog +module M; + always @(posedge clk) z = z + 1; +endmodule +``` + +### Pass Example (3 of 6) +```systemverilog +module M; + always @* z = z - 1; +endmodule +``` + +### Pass Example (4 of 6) +```systemverilog +module M; + always @* z = z + 1; +endmodule +``` + +### Pass Example (5 of 6) +```systemverilog +module M; + genvar i; + for (i = 4; i >= 0; i = i - 1) begin + assign z[i] = y[i] + x[i]; + end +endmodule +``` + +### Pass Example (6 of 6) +```systemverilog +module M; + genvar i; + for (i = 0; i < 5; i = i + 1) begin + assign z[i] = y[i] + x[i]; + end +endmodule +``` + +### Fail Example (1 of 6) +```systemverilog +module M; + always @(posedge clk) z--; +endmodule +``` + +### Fail Example (2 of 6) +```systemverilog +module M; + always @(posedge clk) z++; +endmodule +``` + +### Fail Example (3 of 6) +```systemverilog +module M; + always @* z = x + y--; +endmodule +``` + +### Fail Example (4 of 6) +```systemverilog +module M; + always @* z = x + y++; +endmodule +``` + +### Fail Example (5 of 6) +```systemverilog +module M; + genvar i; + for (i = 4; i >= 0; i--) begin + assign z[i] = y[i] + x[i]; + end +endmodule +``` + +### Fail Example (6 of 6) +```systemverilog +module M; + genvar i; + for (i = 0; i < 5; i++) begin + assign z[i] = y[i] + x[i]; + end +endmodule +``` + +### Explanation + +Increment and decrement operators (`++` and `--`) are part of SystemVerilog +(IEEE1800), but not Verilog (IEEE1364). + +This rule allows only binary operators with simple assigments (`foo = foo + 1`) +to encourage backwards compatibility with Verilog. + +See also: +- **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **keyword_forbidden_always_comb** - Suggested companion rule. +- **keyword_forbidden_always_ff** - Suggested companion rule. +- **keyword_forbidden_always_latch** - Suggested companion rule. +- **operator_self_assignment** - Suggested companion rule. + +The most relevant clauses of IEEE1364-2001 are: +- 4.1 Operators +- 9.2.1 Blocking procedural assignments +- 12.1.3.2 generate-loop + +The most relevant clauses of IEEE1800-2017 are: +- 10.4.1 Blocking procedural assignments +- 11.4.2 Increment and decrement operators +- 27.4 Loop generate constructs + + + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * ## Syntax Rule: `operator_self_assignment` @@ -3849,6 +3987,7 @@ See also: - **keyword_forbidden_always_comb** - Suggested companion rule. - **keyword_forbidden_always_ff** - Suggested companion rule. - **keyword_forbidden_always_latch** - Suggested companion rule. +- **operator_incdec** - Suggested companion rule. The most relevant clauses of IEEE1364-2001 are: - 4.1 Operators diff --git a/md/syntaxrules-explanation-keyword_forbidden_always_comb.md b/md/syntaxrules-explanation-keyword_forbidden_always_comb.md index e28c7540..7f5151f6 100644 --- a/md/syntaxrules-explanation-keyword_forbidden_always_comb.md +++ b/md/syntaxrules-explanation-keyword_forbidden_always_comb.md @@ -9,6 +9,7 @@ See also: - **keyword_forbidden_always_ff** - Suggested companion rule. - **keyword_forbidden_always_latch** - Suggested companion rule. - **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **operator_incdec** - Suggested companion rule. - **operator_self_assignment** - Suggested companion rule. The most relevant clauses of IEEE1364-2001 are: diff --git a/md/syntaxrules-explanation-keyword_forbidden_always_ff.md b/md/syntaxrules-explanation-keyword_forbidden_always_ff.md index 1dd40d20..2c57e5c4 100644 --- a/md/syntaxrules-explanation-keyword_forbidden_always_ff.md +++ b/md/syntaxrules-explanation-keyword_forbidden_always_ff.md @@ -9,6 +9,7 @@ See also: - **keyword_forbidden_always_comb** - Suggested companion rule. - **keyword_forbidden_always_latch** - Suggested companion rule. - **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **operator_incdec** - Suggested companion rule. - **operator_self_assignment** - Suggested companion rule. The most relevant clauses of IEEE1364-2001 are: diff --git a/md/syntaxrules-explanation-keyword_forbidden_always_latch.md b/md/syntaxrules-explanation-keyword_forbidden_always_latch.md index 450554ac..dc368cc5 100644 --- a/md/syntaxrules-explanation-keyword_forbidden_always_latch.md +++ b/md/syntaxrules-explanation-keyword_forbidden_always_latch.md @@ -9,6 +9,7 @@ See also: - **keyword_forbidden_always_comb** - Suggested companion rule. - **keyword_forbidden_always_ff** - Suggested companion rule. - **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **operator_incdec** - Suggested companion rule. - **operator_self_assignment** - Suggested companion rule. The most relevant clauses of IEEE1364-2001 are: diff --git a/md/syntaxrules-explanation-operator_incdec.md b/md/syntaxrules-explanation-operator_incdec.md new file mode 100644 index 00000000..819900dd --- /dev/null +++ b/md/syntaxrules-explanation-operator_incdec.md @@ -0,0 +1,22 @@ +Increment and decrement operators (`++` and `--`) are part of SystemVerilog +(IEEE1800), but not Verilog (IEEE1364). + +This rule allows only binary operators with simple assigments (`foo = foo + 1`) +to encourage backwards compatibility with Verilog. + +See also: +- **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **keyword_forbidden_always_comb** - Suggested companion rule. +- **keyword_forbidden_always_ff** - Suggested companion rule. +- **keyword_forbidden_always_latch** - Suggested companion rule. +- **operator_self_assignment** - Suggested companion rule. + +The most relevant clauses of IEEE1364-2001 are: +- 4.1 Operators +- 9.2.1 Blocking procedural assignments +- 12.1.3.2 generate-loop + +The most relevant clauses of IEEE1800-2017 are: +- 10.4.1 Blocking procedural assignments +- 11.4.2 Increment and decrement operators +- 27.4 Loop generate constructs diff --git a/md/syntaxrules-explanation-operator_self_assignment.md b/md/syntaxrules-explanation-operator_self_assignment.md index b003f489..7899f143 100644 --- a/md/syntaxrules-explanation-operator_self_assignment.md +++ b/md/syntaxrules-explanation-operator_self_assignment.md @@ -10,6 +10,7 @@ See also: - **keyword_forbidden_always_comb** - Suggested companion rule. - **keyword_forbidden_always_ff** - Suggested companion rule. - **keyword_forbidden_always_latch** - Suggested companion rule. +- **operator_incdec** - Suggested companion rule. The most relevant clauses of IEEE1364-2001 are: - 4.1 Operators diff --git a/src/syntaxrules/operator_incdec.rs b/src/syntaxrules/operator_incdec.rs new file mode 100644 index 00000000..714b5c33 --- /dev/null +++ b/src/syntaxrules/operator_incdec.rs @@ -0,0 +1,39 @@ +use crate::config::ConfigOption; +use crate::linter::{SyntaxRule, SyntaxRuleResult}; +use sv_parser::{NodeEvent, RefNode, SyntaxTree}; + +#[derive(Default)] +pub struct OperatorIncdec; + +impl SyntaxRule for OperatorIncdec { + fn check( + &mut self, + _syntax_tree: &SyntaxTree, + event: &NodeEvent, + _option: &ConfigOption, + ) -> SyntaxRuleResult { + let node = match event { + NodeEvent::Enter(x) => x, + NodeEvent::Leave(_) => { + return SyntaxRuleResult::Pass; + } + }; + + match node { + RefNode::IncOrDecOperator(_) => SyntaxRuleResult::Fail, + _ => SyntaxRuleResult::Pass, + } + } + + fn name(&self) -> String { + String::from("operator_incdec") + } + + fn hint(&self, _option: &ConfigOption) -> String { + String::from("Use `=` with a `+` or `-` instead of an increment or decrement operator.") + } + + fn reason(&self) -> String { + String::from("Only SystemVerilog, not Verilog, has increment and decrement operators.") + } +} diff --git a/testcases/syntaxrules/fail/operator_incdec.sv b/testcases/syntaxrules/fail/operator_incdec.sv new file mode 100644 index 00000000..809d6bd5 --- /dev/null +++ b/testcases/syntaxrules/fail/operator_incdec.sv @@ -0,0 +1,29 @@ +module M; + always @(posedge clk) z--; +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @(posedge clk) z++; +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @* z = x + y--; +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @* z = x + y++; +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + genvar i; + for (i = 4; i >= 0; i--) begin + assign z[i] = y[i] + x[i]; + end +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + genvar i; + for (i = 0; i < 5; i++) begin + assign z[i] = y[i] + x[i]; + end +endmodule diff --git a/testcases/syntaxrules/pass/operator_incdec.sv b/testcases/syntaxrules/pass/operator_incdec.sv new file mode 100644 index 00000000..0b412701 --- /dev/null +++ b/testcases/syntaxrules/pass/operator_incdec.sv @@ -0,0 +1,29 @@ +module M; + always @(posedge clk) z = z - 1; +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @(posedge clk) z = z + 1; +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @* z = z - 1; +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @* z = z + 1; +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + genvar i; + for (i = 4; i >= 0; i = i - 1) begin + assign z[i] = y[i] + x[i]; + end +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + genvar i; + for (i = 0; i < 5; i = i + 1) begin + assign z[i] = y[i] + x[i]; + end +endmodule From 5854860feec753be4317c40dd3423e1bdb60ca28 Mon Sep 17 00:00:00 2001 From: Dave McEwan Date: Fri, 10 Nov 2023 20:05:22 +0000 Subject: [PATCH 06/13] V2k1 New rules `blocking_assignment_in_always_at_edge`, `non_blocking_assignment_in_always_no_edge` --- MANUAL.md | 180 +++++++++++++++++- ...n-blocking_assignment_in_always_at_edge.md | 35 ++++ ...n_blocking_assignment_in_always_no_edge.md | 35 ++++ .../blocking_assignment_in_always_at_edge.rs | 54 ++++++ ...n_blocking_assignment_in_always_no_edge.rs | 53 ++++++ .../blocking_assignment_in_always_at_edge.sv | 11 ++ .../fail/blocking_assignment_in_always_ff.sv | 6 +- ...n_blocking_assignment_in_always_no_edge.sv | 7 + .../blocking_assignment_in_always_at_edge.sv | 11 ++ ...n_blocking_assignment_in_always_no_edge.sv | 7 + 10 files changed, 389 insertions(+), 10 deletions(-) create mode 100644 md/syntaxrules-explanation-blocking_assignment_in_always_at_edge.md create mode 100644 md/syntaxrules-explanation-non_blocking_assignment_in_always_no_edge.md create mode 100644 src/syntaxrules/blocking_assignment_in_always_at_edge.rs create mode 100644 src/syntaxrules/non_blocking_assignment_in_always_no_edge.rs create mode 100644 testcases/syntaxrules/fail/blocking_assignment_in_always_at_edge.sv create mode 100644 testcases/syntaxrules/fail/non_blocking_assignment_in_always_no_edge.sv create mode 100644 testcases/syntaxrules/pass/blocking_assignment_in_always_at_edge.sv create mode 100644 testcases/syntaxrules/pass/non_blocking_assignment_in_always_no_edge.sv diff --git a/MANUAL.md b/MANUAL.md index db357301..b362122d 100644 --- a/MANUAL.md +++ b/MANUAL.md @@ -665,6 +665,100 @@ The most relevant clauses of IEEE1800-2017 are: +* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + +## Syntax Rule: `blocking_assignment_in_always_at_edge` + +### Hint + +Do not use blocking assignments within edge-sensitive `always`. + +### Reason + +Blocking assignment in `always_ff` may cause undefined event ordering. + +### Pass Example (1 of 3) +```systemverilog +module M; + always @(posedge clk) q <= d; +endmodule +``` + +### Pass Example (2 of 3) +```systemverilog +module M; + always @(negedge clk) q <= d; +endmodule +``` + +### Pass Example (3 of 3) +```systemverilog +module M; + always @(edge clk) q <= d; +endmodule +``` + +### Fail Example (1 of 3) +```systemverilog +module M; + always @(posedge clk) q = d; +endmodule +``` + +### Fail Example (2 of 3) +```systemverilog +module M; + always @(negedge clk) q = d; +endmodule +``` + +### Fail Example (3 of 3) +```systemverilog +module M; + always @(edge clk) q = d; +endmodule +``` + +### Explanation + +Simulator event ordering between blocking and non-blocking assignments +is undefined, so observed behavior is simulator-dependent. +Edge-sensitive (usually clocked) processes like, `always @(posedge clk)` should +only contain non-blocking assignments in order for sampling and variable +evaluation to operate in a defined order, e.g. `q <= d;`, not `q = d;`. + +For SystemVerilog (IEEE1800) code, the keyword `always_ff` (or `always_latch`) +should be used instead of the general purpose `always` to take advantage of +extra compile-time checks. +For code which must be compatible with Verilog (IEEE1364), `always` is the only +option. +Therefore, this rule `reg` assignments to be compatible with Verilog like this +(in conjunction with **non_blocking_assignment_in_always_no_edge**): +```verilog +always @(posedge clk) q <= d; // Clocked to reg (flip-flop) +always @* a = b + c; // Combinational to reg (logic gates) +assign d = e + f; // Combinational to wire (logic gates) +``` + +See also: +- **non_blocking_assignment_in_always_no_edge** - Useful companion rule. +- **blocking_assignment_in_always_ff** - Similar rule, suggested as alternative + for SystemVerilog code, but not Verilog. +- **blocking_assignment_in_always_latch** - Useful companion rule for + SystemVerilog, but not Verilog. +- **non_blocking_assignment_in_always_comb** - Useful companion rule for + SystemVerilog, but not Verilog. + +The most relevant clauses of IEEE1800-2017 are: +- 4.9.3 Blocking assignment +- 4.9.4 Non-blocking assignment +- 9.4.2 Event control +- 10.4.1 Blocking procedural assignments +- 10.4.2 Nonblocking procedural assignments +- 16.5.1 Sampling + + + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * ## Syntax Rule: `blocking_assignment_in_always_ff` @@ -691,11 +785,7 @@ endmodule ### Fail Example (1 of 1) ```systemverilog module M; -/* svlint off blocking_assignment_in_always_ff */ -always_ff @(posedge clk) q1 = d; // Control comments avoid failure. -/* svlint on blocking_assignment_in_always_ff */ - -always_ff @(posedge clk) q2 = d; // Failure. + always_ff @(posedge clk) q = d; // Failure. endmodule ``` @@ -3675,6 +3765,86 @@ The most relevant clauses of IEEE1800-2017 are: +* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + +## Syntax Rule: `non_blocking_assignment_in_always_no_edge` + +### Hint + +Remove non-blocking assignment in combinational `always`. + +### Reason + +Scheduling between blocking and non-blocking assignments is non-deterministic. + +### Pass Example (1 of 2) +```systemverilog +module M; + always @* a = b + c; +endmodule +``` + +### Pass Example (2 of 2) +```systemverilog +module M; + always @(*) a = b + c; +endmodule +``` + +### Fail Example (1 of 2) +```systemverilog +module M; + always @* a <= b + c; +endmodule +``` + +### Fail Example (2 of 2) +```systemverilog +module M; + always @(*) a <= b + c; +endmodule +``` + +### Explanation + +Simulator event ordering between blocking and non-blocking assignments +is undefined, so observed behavior is simulator-dependent. +Value-sensitive (usually combinatorial) processes like, `always @*` +should only contain blocking assignments in order for sampling and variable +evaluation to operate in a defined order, e.g. `a = b;`, not `a <= b;`. + +For SystemVerilog (IEEE1800) code, the keyword `always_comb` should be used +instead of the general purpose `always` to take advantage of extra compile-time +checks. +For code which must be compatible with Verilog (IEEE1364), `always` is the only +option. +Therefore, this rule `reg` assignments to be compatible with Verilog like this +(in conjunction with **blocking_assignment_in_always_at_edge**): +```verilog +always @(posedge clk) q <= d; // Clocked to reg (flip-flop) +always @* a = b + c; // Combinational to reg (logic gates) +assign d = e + f; // Combinational to wire (logic gates) +``` + +See also: +- **non_blocking_assignment_in_always_at_edge** - Useful companion rule. +- **blocking_assignment_in_always_ff** - Similar rule, suggested as alternative + for SystemVerilog code, but not Verilog. +- **blocking_assignment_in_always_latch** - Useful companion rule for + SystemVerilog, but not Verilog. +- **non_blocking_assignment_in_always_comb** - Useful companion rule for + SystemVerilog, but not Verilog. + +The most relevant clauses of IEEE1800-2017 are: +- 4.9.3 Blocking assignment +- 4.9.4 Non-blocking assignment +- 9.4.2 Event control +- 10.4.1 Blocking procedural assignments +- 10.4.2 Nonblocking procedural assignments +- 16.5.1 Sampling + + + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * ## Syntax Rule: `operator_case_equality` diff --git a/md/syntaxrules-explanation-blocking_assignment_in_always_at_edge.md b/md/syntaxrules-explanation-blocking_assignment_in_always_at_edge.md new file mode 100644 index 00000000..e66130a0 --- /dev/null +++ b/md/syntaxrules-explanation-blocking_assignment_in_always_at_edge.md @@ -0,0 +1,35 @@ +Simulator event ordering between blocking and non-blocking assignments +is undefined, so observed behavior is simulator-dependent. +Edge-sensitive (usually clocked) processes like, `always @(posedge clk)` should +only contain non-blocking assignments in order for sampling and variable +evaluation to operate in a defined order, e.g. `q <= d;`, not `q = d;`. + +For SystemVerilog (IEEE1800) code, the keyword `always_ff` (or `always_latch`) +should be used instead of the general purpose `always` to take advantage of +extra compile-time checks. +For code which must be compatible with Verilog (IEEE1364), `always` is the only +option. +Therefore, this rule `reg` assignments to be compatible with Verilog like this +(in conjunction with **non_blocking_assignment_in_always_no_edge**): +```verilog +always @(posedge clk) q <= d; // Clocked to reg (flip-flop) +always @* a = b + c; // Combinational to reg (logic gates) +assign d = e + f; // Combinational to wire (logic gates) +``` + +See also: +- **non_blocking_assignment_in_always_no_edge** - Useful companion rule. +- **blocking_assignment_in_always_ff** - Similar rule, suggested as alternative + for SystemVerilog code, but not Verilog. +- **blocking_assignment_in_always_latch** - Useful companion rule for + SystemVerilog, but not Verilog. +- **non_blocking_assignment_in_always_comb** - Useful companion rule for + SystemVerilog, but not Verilog. + +The most relevant clauses of IEEE1800-2017 are: +- 4.9.3 Blocking assignment +- 4.9.4 Non-blocking assignment +- 9.4.2 Event control +- 10.4.1 Blocking procedural assignments +- 10.4.2 Nonblocking procedural assignments +- 16.5.1 Sampling diff --git a/md/syntaxrules-explanation-non_blocking_assignment_in_always_no_edge.md b/md/syntaxrules-explanation-non_blocking_assignment_in_always_no_edge.md new file mode 100644 index 00000000..012a8ee7 --- /dev/null +++ b/md/syntaxrules-explanation-non_blocking_assignment_in_always_no_edge.md @@ -0,0 +1,35 @@ +Simulator event ordering between blocking and non-blocking assignments +is undefined, so observed behavior is simulator-dependent. +Value-sensitive (usually combinatorial) processes like, `always @*` +should only contain blocking assignments in order for sampling and variable +evaluation to operate in a defined order, e.g. `a = b;`, not `a <= b;`. + +For SystemVerilog (IEEE1800) code, the keyword `always_comb` should be used +instead of the general purpose `always` to take advantage of extra compile-time +checks. +For code which must be compatible with Verilog (IEEE1364), `always` is the only +option. +Therefore, this rule `reg` assignments to be compatible with Verilog like this +(in conjunction with **blocking_assignment_in_always_at_edge**): +```verilog +always @(posedge clk) q <= d; // Clocked to reg (flip-flop) +always @* a = b + c; // Combinational to reg (logic gates) +assign d = e + f; // Combinational to wire (logic gates) +``` + +See also: +- **non_blocking_assignment_in_always_at_edge** - Useful companion rule. +- **blocking_assignment_in_always_ff** - Similar rule, suggested as alternative + for SystemVerilog code, but not Verilog. +- **blocking_assignment_in_always_latch** - Useful companion rule for + SystemVerilog, but not Verilog. +- **non_blocking_assignment_in_always_comb** - Useful companion rule for + SystemVerilog, but not Verilog. + +The most relevant clauses of IEEE1800-2017 are: +- 4.9.3 Blocking assignment +- 4.9.4 Non-blocking assignment +- 9.4.2 Event control +- 10.4.1 Blocking procedural assignments +- 10.4.2 Nonblocking procedural assignments +- 16.5.1 Sampling diff --git a/src/syntaxrules/blocking_assignment_in_always_at_edge.rs b/src/syntaxrules/blocking_assignment_in_always_at_edge.rs new file mode 100644 index 00000000..99d49faa --- /dev/null +++ b/src/syntaxrules/blocking_assignment_in_always_at_edge.rs @@ -0,0 +1,54 @@ +use crate::config::ConfigOption; +use crate::linter::{SyntaxRule, SyntaxRuleResult}; +use sv_parser::{unwrap_node, AlwaysKeyword, NodeEvent, RefNode, SyntaxTree}; + +#[derive(Default)] +pub struct BlockingAssignmentInAlwaysAtEdge; + +impl SyntaxRule for BlockingAssignmentInAlwaysAtEdge { + fn check( + &mut self, + _syntax_tree: &SyntaxTree, + event: &NodeEvent, + _option: &ConfigOption, + ) -> SyntaxRuleResult { + let node = match event { + NodeEvent::Enter(x) => x, + NodeEvent::Leave(_) => { + return SyntaxRuleResult::Pass; + } + }; + + match node { + RefNode::AlwaysConstruct(x) => { + let (t, x) = &x.nodes; + match t { + AlwaysKeyword::Always(_) => { + let edge = unwrap_node!(x, EdgeIdentifier); + let blocking_assignment = unwrap_node!(x, BlockingAssignment); + let variable_assignment = unwrap_node!(x, VariableDeclAssignment); + if edge.is_some() && (blocking_assignment.is_some() || variable_assignment.is_some()) { + SyntaxRuleResult::Fail + } else { + SyntaxRuleResult::Pass + } + } + _ => SyntaxRuleResult::Pass, + } + } + _ => SyntaxRuleResult::Pass, + } + } + + fn name(&self) -> String { + String::from("blocking_assignment_in_always_at_edge") + } + + fn hint(&self, _option: &ConfigOption) -> String { + String::from("Do not use blocking assignments within edge-sensitive `always`.") + } + + fn reason(&self) -> String { + String::from("Blocking assignment in `always_ff` may cause undefined event ordering.") + } +} diff --git a/src/syntaxrules/non_blocking_assignment_in_always_no_edge.rs b/src/syntaxrules/non_blocking_assignment_in_always_no_edge.rs new file mode 100644 index 00000000..9290380d --- /dev/null +++ b/src/syntaxrules/non_blocking_assignment_in_always_no_edge.rs @@ -0,0 +1,53 @@ +use crate::config::ConfigOption; +use crate::linter::{SyntaxRule, SyntaxRuleResult}; +use sv_parser::{unwrap_node, AlwaysKeyword, NodeEvent, RefNode, SyntaxTree}; + +#[derive(Default)] +pub struct NonBlockingAssignmentInAlwaysNoEdge; + +impl SyntaxRule for NonBlockingAssignmentInAlwaysNoEdge { + fn check( + &mut self, + _syntax_tree: &SyntaxTree, + event: &NodeEvent, + _option: &ConfigOption, + ) -> SyntaxRuleResult { + let node = match event { + NodeEvent::Enter(x) => x, + NodeEvent::Leave(_) => { + return SyntaxRuleResult::Pass; + } + }; + + match node { + RefNode::AlwaysConstruct(x) => { + let (t, x) = &x.nodes; + match t { + AlwaysKeyword::Always(_) => { + let edge = unwrap_node!(x, EdgeIdentifier); + let nonblocking_assignment = unwrap_node!(x, NonblockingAssignment); + if edge.is_none() && nonblocking_assignment.is_some() { + SyntaxRuleResult::Fail + } else { + SyntaxRuleResult::Pass + } + } + _ => SyntaxRuleResult::Pass, + } + } + _ => SyntaxRuleResult::Pass, + } + } + + fn name(&self) -> String { + String::from("non_blocking_assignment_in_always_no_edge") + } + + fn hint(&self, _option: &ConfigOption) -> String { + String::from("Remove non-blocking assignment in combinational `always`.") + } + + fn reason(&self) -> String { + String::from("Scheduling between blocking and non-blocking assignments is non-deterministic.") + } +} diff --git a/testcases/syntaxrules/fail/blocking_assignment_in_always_at_edge.sv b/testcases/syntaxrules/fail/blocking_assignment_in_always_at_edge.sv new file mode 100644 index 00000000..67d46454 --- /dev/null +++ b/testcases/syntaxrules/fail/blocking_assignment_in_always_at_edge.sv @@ -0,0 +1,11 @@ +module M; + always @(posedge clk) q = d; +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @(negedge clk) q = d; +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @(edge clk) q = d; +endmodule diff --git a/testcases/syntaxrules/fail/blocking_assignment_in_always_ff.sv b/testcases/syntaxrules/fail/blocking_assignment_in_always_ff.sv index 0998910c..c22b5344 100644 --- a/testcases/syntaxrules/fail/blocking_assignment_in_always_ff.sv +++ b/testcases/syntaxrules/fail/blocking_assignment_in_always_ff.sv @@ -1,7 +1,3 @@ module M; -/* svlint off blocking_assignment_in_always_ff */ -always_ff @(posedge clk) q1 = d; // Control comments avoid failure. -/* svlint on blocking_assignment_in_always_ff */ - -always_ff @(posedge clk) q2 = d; // Failure. + always_ff @(posedge clk) q = d; // Failure. endmodule diff --git a/testcases/syntaxrules/fail/non_blocking_assignment_in_always_no_edge.sv b/testcases/syntaxrules/fail/non_blocking_assignment_in_always_no_edge.sv new file mode 100644 index 00000000..10ce89d8 --- /dev/null +++ b/testcases/syntaxrules/fail/non_blocking_assignment_in_always_no_edge.sv @@ -0,0 +1,7 @@ +module M; + always @* a <= b + c; +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @(*) a <= b + c; +endmodule diff --git a/testcases/syntaxrules/pass/blocking_assignment_in_always_at_edge.sv b/testcases/syntaxrules/pass/blocking_assignment_in_always_at_edge.sv new file mode 100644 index 00000000..c88c3480 --- /dev/null +++ b/testcases/syntaxrules/pass/blocking_assignment_in_always_at_edge.sv @@ -0,0 +1,11 @@ +module M; + always @(posedge clk) q <= d; +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @(negedge clk) q <= d; +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @(edge clk) q <= d; +endmodule diff --git a/testcases/syntaxrules/pass/non_blocking_assignment_in_always_no_edge.sv b/testcases/syntaxrules/pass/non_blocking_assignment_in_always_no_edge.sv new file mode 100644 index 00000000..5ae5dce9 --- /dev/null +++ b/testcases/syntaxrules/pass/non_blocking_assignment_in_always_no_edge.sv @@ -0,0 +1,7 @@ +module M; + always @* a = b + c; +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @(*) a = b + c; +endmodule From 557af8aff3665094d51e6c3ec49a3a919efb7488 Mon Sep 17 00:00:00 2001 From: Dave McEwan Date: Fri, 10 Nov 2023 20:20:25 +0000 Subject: [PATCH 07/13] V2k1 Rename `level_sensitive_always` to `general_always_no_edge` --- MANUAL.md | 180 +++++++++--------- build.rs | 5 + md/ruleset-DaveMcEwan-design.md | 2 +- md/ruleset-designintent.md | 2 +- md/ruleset-simsynth.md | 2 +- ...s-explanation-eventlist_comma_always_ff.md | 2 +- md/syntaxrules-explanation-eventlist_or.md | 2 +- ...les-explanation-general_always_no_edge.md} | 2 +- ...es-explanation-keyword_forbidden_always.md | 6 +- rulesets/DaveMcEwan-design.toml | 2 +- rulesets/designintent.toml | 2 +- rulesets/simsynth.toml | 2 +- ...ve_always.rs => general_always_no_edge.rs} | 10 +- ...ve_always.sv => general_always_no_edge.sv} | 0 ...ve_always.sv => general_always_no_edge.sv} | 0 15 files changed, 112 insertions(+), 107 deletions(-) rename md/{syntaxrules-explanation-level_sensitive_always.md => syntaxrules-explanation-general_always_no_edge.md} (96%) rename src/syntaxrules/{level_sensitive_always.rs => general_always_no_edge.rs} (82%) rename testcases/syntaxrules/fail/{level_sensitive_always.sv => general_always_no_edge.sv} (100%) rename testcases/syntaxrules/pass/{level_sensitive_always.sv => general_always_no_edge.sv} (100%) diff --git a/MANUAL.md b/MANUAL.md index b362122d..c5731e9a 100644 --- a/MANUAL.md +++ b/MANUAL.md @@ -1205,7 +1205,7 @@ This rule only applies to event expressions in `always_ff` processes. See also: - **eventlist_or** - Mutually exclusive rule. - **blocking_assignment_in_always_ff** - Useful companion rule. -- **level_sensitive_always** - Useful companion rule. +- **general_always_no_edge** - Useful companion rule. - **style_keyword_1space** - Useful companion rule. The most relevant clauses of IEEE1800-2017 are: @@ -1332,7 +1332,7 @@ processes. See also: - **eventlist_comma_always_ff** - Mutually exclusive rule. - **blocking_assignment_in_always_ff** - Useful companion rule. -- **level_sensitive_always** - Useful companion rule. +- **general_always_no_edge** - Useful companion rule. - **style_keyword_commaleading** - Useful companion rule. The most relevant clauses of IEEE1800-2017 are: @@ -1676,6 +1676,88 @@ The most relevant clauses of IEEE1800-2017 are: +* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + +## Syntax Rule: `general_always_no_edge` + +### Hint + +Replace general-purpose `always` with `always_comb`. + +### Reason + +General-purpose `always` cannot detect combinatorial/stateful mistakes. + +### Pass Example (1 of 1) +```systemverilog +module M; + always_comb begin + end + always @(posedge a) begin + end +endmodule +``` + +### Fail Example (1 of 2) +```systemverilog +module M; + always @* begin // No sensitivity list. + end +endmodule +``` + +### Fail Example (2 of 2) +```systemverilog +module M; + always @ (a or b) begin // No sensitivity to posedge, negedge, or edge. + end +endmodule +``` + +### Explanation + +In Verilog (IEEE1364), there are two language constructs which can be used to +model combinatorial logic: +1. Continuous assignment to `wire` signals is specified with the `assign` + keyword. +2. `reg` signals are assigned to with an `always` block, which is evaluated + whenever anything in the sensitivity list changes value. + +The `always` keyword can also be used for modelling sequential logic by +including the edge of a signal in the sensitivity list. + +The semantics of these keywords in SystemVerilog are compatible with Verilog, +but additional keywords (`always_comb`, `always_ff`, and `always_latch`) should +be used to clarify intent of digital designs. +The `always_*` keywords have slightly different semantics which are beneficial +for synthesizable designs: +1. `always_*` processes require compiler checks that any signals driven on the + LHS are not driven by any other process, i.e. `always_*` cannot infer + multi-driven or tri-state logic. +2. `always_comb` processes require a compiler check that the process does not + infer state. +3. `always_ff` processes require a compiler check that the process does infer + state. + +This rule requires that general-purpose `always` blocks have an explicit +sensitivity list which includes at least one edge, thus forcing the use of +`assign` or `always_comb` to specify combinatorial logic. +It is possible to construct a full-featured testbench where all `always` blocks +meet that requriment. +The alternative rule **keyword_forbidden_always** has similar reasoning but is +more strict, completely forbidding the use of general-purpose `always` blocks. +It is appropriate to use **keyword_forbidden_always** on synthesizable design +code, but on verification code use **general_always_no_edge** instead. + +See also: +- **keyword_forbidden_always** - Alternative rule. + +The most relevant clauses of IEEE1800-2017 are: +- 9.2.2 Always procedures +- 9.5 Process execution threads + + + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * ## Syntax Rule: `genvar_declaration_in_loop` @@ -2105,17 +2187,17 @@ process is a valid and useful way of scheduling events. Therefore, this rule is intended only for synthesizable design code, not for testbench code. -The alternative rule **level_sensitive_always** has similar reasoning but is +The alternative rule **general_always_no_edge** has similar reasoning but is slightly relaxed, requiring that `always` blocks have an explicit sensitivity list including an edge. It is possible to construct a full-featured testbench where all `always` blocks meet that requriment. Therefore, it is appropriate to use **keyword_forbidden_always** on synthesizable design code, but on verification code use -**level_sensitive_always** instead. +**general_always_no_edge** instead. See also: -- **level_sensitive_always** - Alternative rule. +- **general_always_no_edge** - Alternative rule. - **sequential_block_in_always_comb** - Useful companion rule. - **sequential_block_in_always_if** - Useful companion rule. - **sequential_block_in_always_latch** - Useful companion rule. @@ -2763,88 +2845,6 @@ The most relevant clauses of IEEE1800-2017 are: -* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * - -## Syntax Rule: `level_sensitive_always` - -### Hint - -Replace level-sensitive `always` with `always_comb`. - -### Reason - -Level-sensitive `always` cannot detect combinatorial/stateful (non-)blocking mistakes. - -### Pass Example (1 of 1) -```systemverilog -module M; - always_comb begin - end - always @(posedge a) begin - end -endmodule -``` - -### Fail Example (1 of 2) -```systemverilog -module M; - always @* begin // No sensitivity list. - end -endmodule -``` - -### Fail Example (2 of 2) -```systemverilog -module M; - always @ (a or b) begin // No sensitivity to posedge, negedge, or edge. - end -endmodule -``` - -### Explanation - -In Verilog (IEEE1364), there are two language constructs which can be used to -model combinatorial logic: -1. Continuous assignment to `wire` signals is specified with the `assign` - keyword. -2. `reg` signals are assigned to with an `always` block, which is evaluated - whenever anything in the sensitivity list changes value. - -The `always` keyword can also be used for modelling sequential logic by -including the edge of a signal in the sensitivity list. - -The semantics of these keywords in SystemVerilog are compatible with Verilog, -but additional keywords (`always_comb`, `always_ff`, and `always_latch`) should -be used to clarify intent of digital designs. -The `always_*` keywords have slightly different semantics which are beneficial -for synthesizable designs: -1. `always_*` processes require compiler checks that any signals driven on the - LHS are not driven by any other process, i.e. `always_*` cannot infer - multi-driven or tri-state logic. -2. `always_comb` processes require a compiler check that the process does not - infer state. -3. `always_ff` processes require a compiler check that the process does infer - state. - -This rule requires that general-purpose `always` blocks have an explicit -sensitivity list which includes at least one edge, thus forcing the use of -`assign` or `always_comb` to specify combinatorial logic. -It is possible to construct a full-featured testbench where all `always` blocks -meet that requriment. -The alternative rule **keyword_forbidden_always** has similar reasoning but is -more strict, completely forbidding the use of general-purpose `always` blocks. -It is appropriate to use **keyword_forbidden_always** on synthesizable design -code, but on verification code use **level_sensitive_always** instead. - -See also: -- **keyword_forbidden_always** - Alternative rule. - -The most relevant clauses of IEEE1800-2017 are: -- 9.2.2 Always procedures -- 9.5 Process execution threads - - - * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * ## Syntax Rule: `localparam_explicit_type` @@ -10916,7 +10916,7 @@ syntaxrules.function_with_automatic = true syntaxrules.keyword_forbidden_priority = true syntaxrules.keyword_forbidden_unique = true syntaxrules.keyword_forbidden_unique0 = true -syntaxrules.level_sensitive_always = true +syntaxrules.general_always_no_edge = true syntaxrules.operator_case_equality = true # Common to **ruleset-designintent**. @@ -11652,7 +11652,7 @@ syntaxrules.function_with_automatic = true syntaxrules.keyword_forbidden_priority = true syntaxrules.keyword_forbidden_unique = true syntaxrules.keyword_forbidden_unique0 = true -syntaxrules.level_sensitive_always = true # Redundant with keyword_forbidden_always. +syntaxrules.general_always_no_edge = true # Redundant with keyword_forbidden_always. syntaxrules.operator_case_equality = true ``` @@ -11805,7 +11805,7 @@ syntaxrules.function_with_automatic = true syntaxrules.keyword_forbidden_priority = true syntaxrules.keyword_forbidden_unique = true syntaxrules.keyword_forbidden_unique0 = true -syntaxrules.level_sensitive_always = true +syntaxrules.general_always_no_edge = true syntaxrules.operator_case_equality = true ``` diff --git a/build.rs b/build.rs index 8189b2ad..5761149f 100644 --- a/build.rs +++ b/build.rs @@ -59,6 +59,11 @@ const RENAMED_SYNTAXRULES: &[(&str, &str, &str)] = &[ "module_nonansi_forbidden", "ModuleNonansiForbidden", ), + ( + "level_sensitive_always", + "general_always_no_edge", + "GeneralAlwaysNoEdge", + ), ]; fn write_rules_rs( diff --git a/md/ruleset-DaveMcEwan-design.md b/md/ruleset-DaveMcEwan-design.md index a5529822..2343de11 100644 --- a/md/ruleset-DaveMcEwan-design.md +++ b/md/ruleset-DaveMcEwan-design.md @@ -366,7 +366,7 @@ syntaxrules.function_with_automatic = true syntaxrules.keyword_forbidden_priority = true syntaxrules.keyword_forbidden_unique = true syntaxrules.keyword_forbidden_unique0 = true -syntaxrules.level_sensitive_always = true +syntaxrules.general_always_no_edge = true syntaxrules.operator_case_equality = true # Common to **ruleset-designintent**. diff --git a/md/ruleset-designintent.md b/md/ruleset-designintent.md index 5b301c8c..8b128624 100644 --- a/md/ruleset-designintent.md +++ b/md/ruleset-designintent.md @@ -15,7 +15,7 @@ syntaxrules.function_with_automatic = true syntaxrules.keyword_forbidden_priority = true syntaxrules.keyword_forbidden_unique = true syntaxrules.keyword_forbidden_unique0 = true -syntaxrules.level_sensitive_always = true # Redundant with keyword_forbidden_always. +syntaxrules.general_always_no_edge = true # Redundant with keyword_forbidden_always. syntaxrules.operator_case_equality = true ``` diff --git a/md/ruleset-simsynth.md b/md/ruleset-simsynth.md index 7bdd9a29..ed76646b 100644 --- a/md/ruleset-simsynth.md +++ b/md/ruleset-simsynth.md @@ -16,7 +16,7 @@ syntaxrules.function_with_automatic = true syntaxrules.keyword_forbidden_priority = true syntaxrules.keyword_forbidden_unique = true syntaxrules.keyword_forbidden_unique0 = true -syntaxrules.level_sensitive_always = true +syntaxrules.general_always_no_edge = true syntaxrules.operator_case_equality = true ``` diff --git a/md/syntaxrules-explanation-eventlist_comma_always_ff.md b/md/syntaxrules-explanation-eventlist_comma_always_ff.md index 83320055..6d3762ef 100644 --- a/md/syntaxrules-explanation-eventlist_comma_always_ff.md +++ b/md/syntaxrules-explanation-eventlist_comma_always_ff.md @@ -33,7 +33,7 @@ This rule only applies to event expressions in `always_ff` processes. See also: - **eventlist_or** - Mutually exclusive rule. - **blocking_assignment_in_always_ff** - Useful companion rule. -- **level_sensitive_always** - Useful companion rule. +- **general_always_no_edge** - Useful companion rule. - **style_keyword_1space** - Useful companion rule. The most relevant clauses of IEEE1800-2017 are: diff --git a/md/syntaxrules-explanation-eventlist_or.md b/md/syntaxrules-explanation-eventlist_or.md index 7da148b0..4c54d2c5 100644 --- a/md/syntaxrules-explanation-eventlist_or.md +++ b/md/syntaxrules-explanation-eventlist_or.md @@ -31,7 +31,7 @@ processes. See also: - **eventlist_comma_always_ff** - Mutually exclusive rule. - **blocking_assignment_in_always_ff** - Useful companion rule. -- **level_sensitive_always** - Useful companion rule. +- **general_always_no_edge** - Useful companion rule. - **style_keyword_commaleading** - Useful companion rule. The most relevant clauses of IEEE1800-2017 are: diff --git a/md/syntaxrules-explanation-level_sensitive_always.md b/md/syntaxrules-explanation-general_always_no_edge.md similarity index 96% rename from md/syntaxrules-explanation-level_sensitive_always.md rename to md/syntaxrules-explanation-general_always_no_edge.md index f03f0ab2..6e60d031 100644 --- a/md/syntaxrules-explanation-level_sensitive_always.md +++ b/md/syntaxrules-explanation-general_always_no_edge.md @@ -29,7 +29,7 @@ meet that requriment. The alternative rule **keyword_forbidden_always** has similar reasoning but is more strict, completely forbidding the use of general-purpose `always` blocks. It is appropriate to use **keyword_forbidden_always** on synthesizable design -code, but on verification code use **level_sensitive_always** instead. +code, but on verification code use **general_always_no_edge** instead. See also: - **keyword_forbidden_always** - Alternative rule. diff --git a/md/syntaxrules-explanation-keyword_forbidden_always.md b/md/syntaxrules-explanation-keyword_forbidden_always.md index 78c4167f..587859fb 100644 --- a/md/syntaxrules-explanation-keyword_forbidden_always.md +++ b/md/syntaxrules-explanation-keyword_forbidden_always.md @@ -28,17 +28,17 @@ process is a valid and useful way of scheduling events. Therefore, this rule is intended only for synthesizable design code, not for testbench code. -The alternative rule **level_sensitive_always** has similar reasoning but is +The alternative rule **general_always_no_edge** has similar reasoning but is slightly relaxed, requiring that `always` blocks have an explicit sensitivity list including an edge. It is possible to construct a full-featured testbench where all `always` blocks meet that requriment. Therefore, it is appropriate to use **keyword_forbidden_always** on synthesizable design code, but on verification code use -**level_sensitive_always** instead. +**general_always_no_edge** instead. See also: -- **level_sensitive_always** - Alternative rule. +- **general_always_no_edge** - Alternative rule. - **sequential_block_in_always_comb** - Useful companion rule. - **sequential_block_in_always_if** - Useful companion rule. - **sequential_block_in_always_latch** - Useful companion rule. diff --git a/rulesets/DaveMcEwan-design.toml b/rulesets/DaveMcEwan-design.toml index 65b94010..ac2ee404 100644 --- a/rulesets/DaveMcEwan-design.toml +++ b/rulesets/DaveMcEwan-design.toml @@ -35,7 +35,7 @@ syntaxrules.function_with_automatic = true syntaxrules.keyword_forbidden_priority = true syntaxrules.keyword_forbidden_unique = true syntaxrules.keyword_forbidden_unique0 = true -syntaxrules.level_sensitive_always = true +syntaxrules.general_always_no_edge = true syntaxrules.operator_case_equality = true # Common to **ruleset-designintent**. diff --git a/rulesets/designintent.toml b/rulesets/designintent.toml index 79560d33..f791d286 100644 --- a/rulesets/designintent.toml +++ b/rulesets/designintent.toml @@ -7,7 +7,7 @@ syntaxrules.function_with_automatic = true syntaxrules.keyword_forbidden_priority = true syntaxrules.keyword_forbidden_unique = true syntaxrules.keyword_forbidden_unique0 = true -syntaxrules.level_sensitive_always = true # Redundant with keyword_forbidden_always. +syntaxrules.general_always_no_edge = true # Redundant with keyword_forbidden_always. syntaxrules.operator_case_equality = true syntaxrules.action_block_with_side_effect = true syntaxrules.default_nettype_none = true diff --git a/rulesets/simsynth.toml b/rulesets/simsynth.toml index 6ac0b087..cbd65301 100644 --- a/rulesets/simsynth.toml +++ b/rulesets/simsynth.toml @@ -7,5 +7,5 @@ syntaxrules.function_with_automatic = true syntaxrules.keyword_forbidden_priority = true syntaxrules.keyword_forbidden_unique = true syntaxrules.keyword_forbidden_unique0 = true -syntaxrules.level_sensitive_always = true +syntaxrules.general_always_no_edge = true syntaxrules.operator_case_equality = true diff --git a/src/syntaxrules/level_sensitive_always.rs b/src/syntaxrules/general_always_no_edge.rs similarity index 82% rename from src/syntaxrules/level_sensitive_always.rs rename to src/syntaxrules/general_always_no_edge.rs index e62b2d8f..a68d8f93 100644 --- a/src/syntaxrules/level_sensitive_always.rs +++ b/src/syntaxrules/general_always_no_edge.rs @@ -3,9 +3,9 @@ use crate::linter::{SyntaxRule, SyntaxRuleResult}; use sv_parser::{unwrap_node, AlwaysKeyword, NodeEvent, RefNode, SyntaxTree}; #[derive(Default)] -pub struct LevelSensitiveAlways; +pub struct GeneralAlwaysNoEdge; -impl SyntaxRule for LevelSensitiveAlways { +impl SyntaxRule for GeneralAlwaysNoEdge { fn check( &mut self, _syntax_tree: &SyntaxTree, @@ -39,14 +39,14 @@ impl SyntaxRule for LevelSensitiveAlways { } fn name(&self) -> String { - String::from("level_sensitive_always") + String::from("general_always_no_edge") } fn hint(&self, _option: &ConfigOption) -> String { - String::from("Replace level-sensitive `always` with `always_comb`.") + String::from("Replace general-purpose `always` with `always_comb`.") } fn reason(&self) -> String { - String::from("Level-sensitive `always` cannot detect combinatorial/stateful (non-)blocking mistakes.") + String::from("General-purpose `always` cannot detect combinatorial/stateful mistakes.") } } diff --git a/testcases/syntaxrules/fail/level_sensitive_always.sv b/testcases/syntaxrules/fail/general_always_no_edge.sv similarity index 100% rename from testcases/syntaxrules/fail/level_sensitive_always.sv rename to testcases/syntaxrules/fail/general_always_no_edge.sv diff --git a/testcases/syntaxrules/pass/level_sensitive_always.sv b/testcases/syntaxrules/pass/general_always_no_edge.sv similarity index 100% rename from testcases/syntaxrules/pass/level_sensitive_always.sv rename to testcases/syntaxrules/pass/general_always_no_edge.sv From 584c36f03cb29879d964aa996e904269d933f8b5 Mon Sep 17 00:00:00 2001 From: Dave McEwan Date: Fri, 10 Nov 2023 21:28:45 +0000 Subject: [PATCH 08/13] V2k1 New rule `general_always_level_sensitive` --- MANUAL.md | 92 ++++++++++++++++++- ...lanation-general_always_level_sensitive.md | 39 ++++++++ ...ules-explanation-general_always_no_edge.md | 1 + ...es-explanation-keyword_forbidden_always.md | 1 + .../general_always_level_sensitive.rs | 55 +++++++++++ .../fail/general_always_level_sensitive.sv | 9 ++ .../fail/general_always_no_edge.sv | 2 +- .../pass/general_always_level_sensitive.sv | 9 ++ 8 files changed, 206 insertions(+), 2 deletions(-) create mode 100644 md/syntaxrules-explanation-general_always_level_sensitive.md create mode 100644 src/syntaxrules/general_always_level_sensitive.rs create mode 100644 testcases/syntaxrules/fail/general_always_level_sensitive.sv create mode 100644 testcases/syntaxrules/pass/general_always_level_sensitive.sv diff --git a/MANUAL.md b/MANUAL.md index c5731e9a..cb90bf12 100644 --- a/MANUAL.md +++ b/MANUAL.md @@ -1676,6 +1676,94 @@ The most relevant clauses of IEEE1800-2017 are: +* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + +## Syntax Rule: `general_always_level_sensitive` + +### Hint + +Replace general-purpose `always @(...no edge...)` with `always @*`. + +### Reason + +General-purpose `always` cannot detect combinatorial/stateful mistakes. + +### Pass Example (1 of 2) +```systemverilog +module M; + always @* // Sensitive to `b` and `c`. + a = b + c; +endmodule +``` + +### Pass Example (2 of 2) +```systemverilog +module M; + always @(posedge clk) // Sensitive to edges of `clk`. + q <= d; +endmodule +``` + +### Fail Example (1 of 2) +```systemverilog +module M; + always @(b) // Missing sensitivity to `c`. + a = b + c; +endmodule +``` + +### Fail Example (2 of 2) +```systemverilog +module M; + always @(a or b) // Correct sensitivity list, but error prone. + a = b + c; +endmodule +``` + +### Explanation + +This rule is specific to code which must be compatible with Verilog, not +only SystemVerilog. + +In Verilog (IEEE1364), there are two language constructs which can be used to +model combinatorial logic: +1. Continuous assignment to `wire` signals is specified with the `assign` + keyword. +2. `reg` signals are assigned to with an `always` block, which is evaluated + whenever anything in the sensitivity list changes value. + +To ensure that the process correctly sensitive to changes on all driving +signals, `always @*` should be used instead of providing an explicit +sensitivity list like `always @(a or b or c)`. +The `always` keyword can also be used for modelling sequential logic by +including the edge of a signal in the sensitivity list. +Providing an explicit sensitivity list is prone to two mistakes: +1. Forgetting to add a driver to the list, e.g. `always @(b) a = b + c;` + instead of `always @(b or c) a = b + c;`. +2. Forgetting to add and edge specifier, e.g. `always @(clk) q <= d;` instead + of `always @(posedge clk) q <= d;`. + That makes the process level-sensitive, instead of the edge-sensitive. + +This rule requires that general-purpose `always` blocks with an explicit +sensitivity list which include at least one edge. +Combinational logic should use the Kleen-star notation, +e.g. `always @* a = b + c;` + +See also: +- **keyword_forbidden_always** - Related rule forbidding general-purpose + `always`, only applicable for SystemVerilog code. +- **general_always_no_edge** - Related rule forbidding purely combinational + logic in `always` processes. + While this is straightforward to use with SystemVerilog, this might be overly + restrictive for Verilog because all combinational variables must be driven + with `assign`. + +The most relevant clauses of IEEE1800-2017 are: +- 9.2.2 Always procedures +- 9.5 Process execution threads + + + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * ## Syntax Rule: `general_always_no_edge` @@ -1709,7 +1797,7 @@ endmodule ### Fail Example (2 of 2) ```systemverilog module M; - always @ (a or b) begin // No sensitivity to posedge, negedge, or edge. + always @(a or b) begin // No sensitivity to posedge, negedge, or edge. end endmodule ``` @@ -1751,6 +1839,7 @@ code, but on verification code use **general_always_no_edge** instead. See also: - **keyword_forbidden_always** - Alternative rule. +- **general_always_no_edge** - Similar rule that allows `always @*`. The most relevant clauses of IEEE1800-2017 are: - 9.2.2 Always procedures @@ -2198,6 +2287,7 @@ synthesizable design code, but on verification code use See also: - **general_always_no_edge** - Alternative rule. +- **general_always_level_sensitive** - Alternative rule. - **sequential_block_in_always_comb** - Useful companion rule. - **sequential_block_in_always_if** - Useful companion rule. - **sequential_block_in_always_latch** - Useful companion rule. diff --git a/md/syntaxrules-explanation-general_always_level_sensitive.md b/md/syntaxrules-explanation-general_always_level_sensitive.md new file mode 100644 index 00000000..269e9826 --- /dev/null +++ b/md/syntaxrules-explanation-general_always_level_sensitive.md @@ -0,0 +1,39 @@ +This rule is specific to code which must be compatible with Verilog, not +only SystemVerilog. + +In Verilog (IEEE1364), there are two language constructs which can be used to +model combinatorial logic: +1. Continuous assignment to `wire` signals is specified with the `assign` + keyword. +2. `reg` signals are assigned to with an `always` block, which is evaluated + whenever anything in the sensitivity list changes value. + +To ensure that the process correctly sensitive to changes on all driving +signals, `always @*` should be used instead of providing an explicit +sensitivity list like `always @(a or b or c)`. +The `always` keyword can also be used for modelling sequential logic by +including the edge of a signal in the sensitivity list. +Providing an explicit sensitivity list is prone to two mistakes: +1. Forgetting to add a driver to the list, e.g. `always @(b) a = b + c;` + instead of `always @(b or c) a = b + c;`. +2. Forgetting to add and edge specifier, e.g. `always @(clk) q <= d;` instead + of `always @(posedge clk) q <= d;`. + That makes the process level-sensitive, instead of the edge-sensitive. + +This rule requires that general-purpose `always` blocks with an explicit +sensitivity list which include at least one edge. +Combinational logic should use the Kleen-star notation, +e.g. `always @* a = b + c;` + +See also: +- **keyword_forbidden_always** - Related rule forbidding general-purpose + `always`, only applicable for SystemVerilog code. +- **general_always_no_edge** - Related rule forbidding purely combinational + logic in `always` processes. + While this is straightforward to use with SystemVerilog, this might be overly + restrictive for Verilog because all combinational variables must be driven + with `assign`. + +The most relevant clauses of IEEE1800-2017 are: +- 9.2.2 Always procedures +- 9.5 Process execution threads diff --git a/md/syntaxrules-explanation-general_always_no_edge.md b/md/syntaxrules-explanation-general_always_no_edge.md index 6e60d031..5c4e6d48 100644 --- a/md/syntaxrules-explanation-general_always_no_edge.md +++ b/md/syntaxrules-explanation-general_always_no_edge.md @@ -33,6 +33,7 @@ code, but on verification code use **general_always_no_edge** instead. See also: - **keyword_forbidden_always** - Alternative rule. +- **general_always_no_edge** - Similar rule that allows `always @*`. The most relevant clauses of IEEE1800-2017 are: - 9.2.2 Always procedures diff --git a/md/syntaxrules-explanation-keyword_forbidden_always.md b/md/syntaxrules-explanation-keyword_forbidden_always.md index 587859fb..5583c572 100644 --- a/md/syntaxrules-explanation-keyword_forbidden_always.md +++ b/md/syntaxrules-explanation-keyword_forbidden_always.md @@ -39,6 +39,7 @@ synthesizable design code, but on verification code use See also: - **general_always_no_edge** - Alternative rule. +- **general_always_level_sensitive** - Alternative rule. - **sequential_block_in_always_comb** - Useful companion rule. - **sequential_block_in_always_if** - Useful companion rule. - **sequential_block_in_always_latch** - Useful companion rule. diff --git a/src/syntaxrules/general_always_level_sensitive.rs b/src/syntaxrules/general_always_level_sensitive.rs new file mode 100644 index 00000000..296f08cd --- /dev/null +++ b/src/syntaxrules/general_always_level_sensitive.rs @@ -0,0 +1,55 @@ +use crate::config::ConfigOption; +use crate::linter::{SyntaxRule, SyntaxRuleResult}; +use sv_parser::{unwrap_node, AlwaysKeyword, NodeEvent, RefNode, SyntaxTree}; + +#[derive(Default)] +pub struct GeneralAlwaysLevelSensitive; + +impl SyntaxRule for GeneralAlwaysLevelSensitive { + fn check( + &mut self, + _syntax_tree: &SyntaxTree, + event: &NodeEvent, + _option: &ConfigOption, + ) -> SyntaxRuleResult { + let node = match event { + NodeEvent::Enter(x) => x, + NodeEvent::Leave(_) => { + return SyntaxRuleResult::Pass; + } + }; + match node { + RefNode::AlwaysConstruct(x) => { + let (t, x) = &x.nodes; + match t { + AlwaysKeyword::Always(_) => { + let c = unwrap_node!(x, EventControlEventExpression); + if let Some(x) = c { + if let Some(_) = unwrap_node!(x, EdgeIdentifier) { + SyntaxRuleResult::Pass + } else { + SyntaxRuleResult::Fail + } + } else { + SyntaxRuleResult::Pass + } + } + _ => SyntaxRuleResult::Pass, + } + } + _ => SyntaxRuleResult::Pass, + } + } + + fn name(&self) -> String { + String::from("general_always_level_sensitive") + } + + fn hint(&self, _option: &ConfigOption) -> String { + String::from("Replace general-purpose `always @(...no edge...)` with `always @*`.") + } + + fn reason(&self) -> String { + String::from("General-purpose `always` cannot detect combinatorial/stateful mistakes.") + } +} diff --git a/testcases/syntaxrules/fail/general_always_level_sensitive.sv b/testcases/syntaxrules/fail/general_always_level_sensitive.sv new file mode 100644 index 00000000..183ceb91 --- /dev/null +++ b/testcases/syntaxrules/fail/general_always_level_sensitive.sv @@ -0,0 +1,9 @@ +module M; + always @(b) // Missing sensitivity to `c`. + a = b + c; +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @(a or b) // Correct sensitivity list, but error prone. + a = b + c; +endmodule diff --git a/testcases/syntaxrules/fail/general_always_no_edge.sv b/testcases/syntaxrules/fail/general_always_no_edge.sv index ad7f77f9..39161f49 100644 --- a/testcases/syntaxrules/fail/general_always_no_edge.sv +++ b/testcases/syntaxrules/fail/general_always_no_edge.sv @@ -4,6 +4,6 @@ module M; endmodule //////////////////////////////////////////////////////////////////////////////// module M; - always @ (a or b) begin // No sensitivity to posedge, negedge, or edge. + always @(a or b) begin // No sensitivity to posedge, negedge, or edge. end endmodule diff --git a/testcases/syntaxrules/pass/general_always_level_sensitive.sv b/testcases/syntaxrules/pass/general_always_level_sensitive.sv new file mode 100644 index 00000000..9ed28422 --- /dev/null +++ b/testcases/syntaxrules/pass/general_always_level_sensitive.sv @@ -0,0 +1,9 @@ +module M; + always @* // Sensitive to `b` and `c`. + a = b + c; +endmodule +//////////////////////////////////////////////////////////////////////////////// +module M; + always @(posedge clk) // Sensitive to edges of `clk`. + q <= d; +endmodule From a7b6164c9c2dc6860f5c65be870ee53ca4ec2f57 Mon Sep 17 00:00:00 2001 From: Dave McEwan Date: Fri, 10 Nov 2023 23:38:54 +0000 Subject: [PATCH 09/13] V2k1 Minor typo fixes --- MANUAL.md | 14 +++++++------- md/ruleset-designintent.md | 2 +- md/syntaxrules-explanation-case_default.md | 2 +- ...yntaxrules-explanation-explicit_case_default.md | 6 +++--- md/syntaxrules-explanation-explicit_if_else.md | 4 ++-- rulesets/designintent.toml | 2 +- 6 files changed, 15 insertions(+), 15 deletions(-) diff --git a/MANUAL.md b/MANUAL.md index cb90bf12..b34afb47 100644 --- a/MANUAL.md +++ b/MANUAL.md @@ -943,7 +943,7 @@ When `foo` is non-zero, this example may be interpreted in at least two ways: See also: - **explicit_case_default** - Useful companion rule. - **explicit_if_else** - Useful companion rule. -- **legacy_always** - Useful companion rule. +- **keyword_forbidden_always** - Useful companion rule. - **sequential_block_in_always_comb** - Useful companion rule. The most relevant clauses of IEEE1800-2017 are: @@ -1399,7 +1399,7 @@ endmodule ### Explanation -The reasoning behind this rule are different between combinatial constructs +The reasoning behind this is are different between combinatial constructs (`always_comb`, `always @*`) vs sequential constructs (`always_ff`, `always_latch`). The reasoning behind this rule is equivalent to that of **explicit_if_else**. @@ -1420,12 +1420,12 @@ and clear through some useful redundancy. NOTE: The legacy keyword `always` can infer both combinational and sequential constructs in the same block, which can be confusing and should be avoided. -Use of the legacy keyword can be detected with the rule **legacy_always**. +Use of the legacy keyword can be detected with the rule **keyword_forbidden_always**. See also: - **case_default** - Useful companion rule. - **explicit_if_else** - Useful companion rule. -- **legacy_always** - Useful companion rule. +- **keyword_forbidden_always** - Useful companion rule. - **sequential_block_in_always_comb** - Useful companion rule. - **sequential_block_in_always_ff** - Useful companion rule. - **sequential_block_in_always_latch** - Useful companion rule. @@ -1504,11 +1504,11 @@ and clear through some useful redundancy. NOTE: The legacy keyword `always` can infer both combinational and sequential constructs in the same block, which can be confusing and should be avoided. -Use of the legacy keyword can be detected with the rule **legacy_always**. +Use of the legacy keyword can be detected with the rule **keyword_forbidden_always**. See also: - **explicit_case_default** - Useful companion rule. -- **legacy_always** - Useful companion rule. +- **keyword_forbidden_always** - Useful companion rule. - **sequential_block_in_always_comb** - Useful companion rule. - **sequential_block_in_always_ff** - Useful companion rule. - **sequential_block_in_always_latch** - Useful companion rule. @@ -11742,7 +11742,7 @@ syntaxrules.function_with_automatic = true syntaxrules.keyword_forbidden_priority = true syntaxrules.keyword_forbidden_unique = true syntaxrules.keyword_forbidden_unique0 = true -syntaxrules.general_always_no_edge = true # Redundant with keyword_forbidden_always. +#syntaxrules.general_always_no_edge = true # Redundant with keyword_forbidden_always. syntaxrules.operator_case_equality = true ``` diff --git a/md/ruleset-designintent.md b/md/ruleset-designintent.md index 8b128624..0ae1b7ad 100644 --- a/md/ruleset-designintent.md +++ b/md/ruleset-designintent.md @@ -15,7 +15,7 @@ syntaxrules.function_with_automatic = true syntaxrules.keyword_forbidden_priority = true syntaxrules.keyword_forbidden_unique = true syntaxrules.keyword_forbidden_unique0 = true -syntaxrules.general_always_no_edge = true # Redundant with keyword_forbidden_always. +#syntaxrules.general_always_no_edge = true # Redundant with keyword_forbidden_always. syntaxrules.operator_case_equality = true ``` diff --git a/md/syntaxrules-explanation-case_default.md b/md/syntaxrules-explanation-case_default.md index e0f5d267..f0aa688b 100644 --- a/md/syntaxrules-explanation-case_default.md +++ b/md/syntaxrules-explanation-case_default.md @@ -19,7 +19,7 @@ When `foo` is non-zero, this example may be interpreted in at least two ways: See also: - **explicit_case_default** - Useful companion rule. - **explicit_if_else** - Useful companion rule. -- **legacy_always** - Useful companion rule. +- **keyword_forbidden_always** - Useful companion rule. - **sequential_block_in_always_comb** - Useful companion rule. The most relevant clauses of IEEE1800-2017 are: diff --git a/md/syntaxrules-explanation-explicit_case_default.md b/md/syntaxrules-explanation-explicit_case_default.md index 0e6001b6..48653b90 100644 --- a/md/syntaxrules-explanation-explicit_case_default.md +++ b/md/syntaxrules-explanation-explicit_case_default.md @@ -1,4 +1,4 @@ -The reasoning behind this rule are different between combinatial constructs +The reasoning behind this is are different between combinatial constructs (`always_comb`, `always @*`) vs sequential constructs (`always_ff`, `always_latch`). The reasoning behind this rule is equivalent to that of **explicit_if_else**. @@ -19,12 +19,12 @@ and clear through some useful redundancy. NOTE: The legacy keyword `always` can infer both combinational and sequential constructs in the same block, which can be confusing and should be avoided. -Use of the legacy keyword can be detected with the rule **legacy_always**. +Use of the legacy keyword can be detected with the rule **keyword_forbidden_always**. See also: - **case_default** - Useful companion rule. - **explicit_if_else** - Useful companion rule. -- **legacy_always** - Useful companion rule. +- **keyword_forbidden_always** - Useful companion rule. - **sequential_block_in_always_comb** - Useful companion rule. - **sequential_block_in_always_ff** - Useful companion rule. - **sequential_block_in_always_latch** - Useful companion rule. diff --git a/md/syntaxrules-explanation-explicit_if_else.md b/md/syntaxrules-explanation-explicit_if_else.md index bd91059f..9a2f9bf3 100644 --- a/md/syntaxrules-explanation-explicit_if_else.md +++ b/md/syntaxrules-explanation-explicit_if_else.md @@ -17,11 +17,11 @@ and clear through some useful redundancy. NOTE: The legacy keyword `always` can infer both combinational and sequential constructs in the same block, which can be confusing and should be avoided. -Use of the legacy keyword can be detected with the rule **legacy_always**. +Use of the legacy keyword can be detected with the rule **keyword_forbidden_always**. See also: - **explicit_case_default** - Useful companion rule. -- **legacy_always** - Useful companion rule. +- **keyword_forbidden_always** - Useful companion rule. - **sequential_block_in_always_comb** - Useful companion rule. - **sequential_block_in_always_ff** - Useful companion rule. - **sequential_block_in_always_latch** - Useful companion rule. diff --git a/rulesets/designintent.toml b/rulesets/designintent.toml index f791d286..ba04a851 100644 --- a/rulesets/designintent.toml +++ b/rulesets/designintent.toml @@ -7,7 +7,7 @@ syntaxrules.function_with_automatic = true syntaxrules.keyword_forbidden_priority = true syntaxrules.keyword_forbidden_unique = true syntaxrules.keyword_forbidden_unique0 = true -syntaxrules.general_always_no_edge = true # Redundant with keyword_forbidden_always. +#syntaxrules.general_always_no_edge = true # Redundant with keyword_forbidden_always. syntaxrules.operator_case_equality = true syntaxrules.action_block_with_side_effect = true syntaxrules.default_nettype_none = true From 183d62103bf4ff70555a8d39c72a5374969a4023 Mon Sep 17 00:00:00 2001 From: Dave McEwan Date: Fri, 10 Nov 2023 23:43:48 +0000 Subject: [PATCH 10/13] V2k1 New ruleset `designintentV2001` --- MANUAL.md | 130 ++++++++++++++++++++++++++ md/ruleset-designintentV2001.md | 125 +++++++++++++++++++++++++ rulesets/designintentV2001.toml | 34 +++++++ rulesets/svlint-designintentV2001 | 32 +++++++ rulesets/svlint-designintentV2001.cmd | 7 ++ rulesets/svls-designintentV2001 | 15 +++ rulesets/svls-designintentV2001.cmd | 7 ++ 7 files changed, 350 insertions(+) create mode 100644 md/ruleset-designintentV2001.md create mode 100644 rulesets/designintentV2001.toml create mode 100755 rulesets/svlint-designintentV2001 create mode 100644 rulesets/svlint-designintentV2001.cmd create mode 100755 rulesets/svls-designintentV2001 create mode 100644 rulesets/svls-designintentV2001.cmd diff --git a/MANUAL.md b/MANUAL.md index b34afb47..d0554ac8 100644 --- a/MANUAL.md +++ b/MANUAL.md @@ -11848,6 +11848,136 @@ syntaxrules.interface_port_with_modport = true +* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + +## Ruleset: *designintentV2001* + +This ruleset has the same aims as **ruleset-designintent** but with the +additional aim of only allowing code which is backwards compatible with +IEEE1364-2001 (Verilog). +Note that IEEE1364-2001 is not the most recent version (IEEE1364-2005), which +was released in the same year as the first version of SystemVerilog +(IEEE1800-2005). + +Firstly, let's forbid some things which are only in SystemVerilog, but not +Verilog. +```toml +syntaxrules.keyword_forbidden_always_comb = true +syntaxrules.keyword_forbidden_always_ff = true +syntaxrules.keyword_forbidden_always_latch = true +syntaxrules.keyword_forbidden_priority = true +syntaxrules.keyword_forbidden_unique = true +syntaxrules.keyword_forbidden_unique0 = true +#syntaxrules.keyword_forbidden_logic = true # TODO +syntaxrules.operator_incdec = true +syntaxrules.operator_self_assignment = true +``` + +Next, let's use some of the rules in common with **ruleset-simsynth**. +```toml +syntaxrules.enum_with_type = true +syntaxrules.function_with_automatic = true +syntaxrules.operator_case_equality = true +syntaxrules.action_block_with_side_effect = true +syntaxrules.default_nettype_none = true +syntaxrules.function_same_as_system_function = true +``` + +Verilog does allow both ANSI and non-ANSI forms of module declaration, but +there is a crucial difference for the ANSI form: +Only `parameter`s are allowed in the list of parameter ports, not +`localparam`s, meaning that derived parameters are overridable. +In the following example, there is no way of preventing `PTR_W` from being +overridden to something incorrect, risking some frustration and wasted time +when non-obvious effects cause issues later. +```verilog +module M + #(parameter integer WIDTH = 123 + , parameter integer PTR_W = clogb2(WIDTH) + ) + ( input wire [WIDTH-1:0] i_data + , output wire [PTR_W-1:0] o_pointer + ); +``` +However, using the non-ANSI form allows `PTR_W` to be specified as +`localparam`, thus preventing overrides and the resulting confusion, i.e: +```verilog +module M + ( i_data + , o_pointer + ); + + parameter integer WIDTH = 123; + localparam integer PTR_W = clogb2(WIDTH); + + input wire [WIDTH-1:0] i_data; + output wire [PTR_W-1:0] o_pointer; +``` +While this only affects modules which use derived parameters in the port +declarations, a consistent style is generally easier to work with. +For these reasons, the non-ANSI form is required. +```toml +syntaxrules.module_ansi_forbidden = true +``` + +SystemVerilog introduced several keywords which greatly help to clarify intent, +but these are unavailable. +Instead of `always_ff @(posedge clk)` and `always_comb`, we can use +`always @(posedge clk)` and `always @*`. +That means only the form like `always @(a or b)`, i.e. no edge sensitivities, +can be forbidden. +```toml +syntaxrules.general_always_level_sensitive = true +``` +On the same theme, guidelines around blocking vs non-blocking assignments also +need to be altered, but keeping the same general intention. +Clocked `always` processes should only use non-blocking assignment `<=`, and +combinatorial `always` processes should only use blocking assignment `=`. +```toml +syntaxrules.blocking_assignment_in_always_at_edge = true +syntaxrules.non_blocking_assignment_in_always_no_edge = true +``` + +Verilog doesn't have the same distinction between 2-state and 4-state types as +SystemVerilog, e.g. `int` and `integer`, but requiring some type is still a +good idea. +```toml +syntaxrules.localparam_explicit_type = true +syntaxrules.parameter_explicit_type = true +syntaxrules.parameter_default_value = true +syntaxrules.parameter_in_generate = true +``` + +In IEEE1364-2001, the use of `generate` and `endgenerate` is mandatory, but +optional in IEEE1364-2005. +For more compatibility, these keywords are required by this ruleset, as are +`genvar` declarations outside their generate `for` statements. +The enablements of these rules are swapped in **ruleset-designintent** to +reduce visual noise in SystemVerilog. +```toml +syntaxrules.genvar_declaration_in_loop = false +syntaxrules.genvar_declaration_out_loop = true +syntaxrules.keyword_forbidden_generate = false +syntaxrules.keyword_required_generate = true +``` + +Unlike the in the richer language of SystemVerilog, forbidding sequential +blocks (between `begin` and `end`) and sequential loops (`for` under `always`) +is probably too restrictive for Verilog. +Indeed, there is little point in using `always @*` instead of `assign` if +`begin` and `end` are forbidden - in SystemVerilog, `always_comb` provides +extra compile-time checks that `assign` does not. +```toml +#syntaxrules.loop_statement_in_always = true # Not implemented. +#syntaxrules.sequential_block_in_always = true # Not implemented. +syntaxrules.case_default = true # Applies in functions. +syntaxrules.explicit_case_default = true # Applies under `always`. +syntaxrules.explicit_if_else = true +syntaxrules.multiline_for_begin = true +syntaxrules.multiline_if_begin = true +``` + + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * ## Ruleset: *parseonly* diff --git a/md/ruleset-designintentV2001.md b/md/ruleset-designintentV2001.md new file mode 100644 index 00000000..3a8e4875 --- /dev/null +++ b/md/ruleset-designintentV2001.md @@ -0,0 +1,125 @@ + +This ruleset has the same aims as **ruleset-designintent** but with the +additional aim of only allowing code which is backwards compatible with +IEEE1364-2001 (Verilog). +Note that IEEE1364-2001 is not the most recent version (IEEE1364-2005), which +was released in the same year as the first version of SystemVerilog +(IEEE1800-2005). + +Firstly, let's forbid some things which are only in SystemVerilog, but not +Verilog. +```toml +syntaxrules.keyword_forbidden_always_comb = true +syntaxrules.keyword_forbidden_always_ff = true +syntaxrules.keyword_forbidden_always_latch = true +syntaxrules.keyword_forbidden_priority = true +syntaxrules.keyword_forbidden_unique = true +syntaxrules.keyword_forbidden_unique0 = true +#syntaxrules.keyword_forbidden_logic = true # TODO +syntaxrules.operator_incdec = true +syntaxrules.operator_self_assignment = true +``` + +Next, let's use some of the rules in common with **ruleset-simsynth**. +```toml +syntaxrules.enum_with_type = true +syntaxrules.function_with_automatic = true +syntaxrules.operator_case_equality = true +syntaxrules.action_block_with_side_effect = true +syntaxrules.default_nettype_none = true +syntaxrules.function_same_as_system_function = true +``` + +Verilog does allow both ANSI and non-ANSI forms of module declaration, but +there is a crucial difference for the ANSI form: +Only `parameter`s are allowed in the list of parameter ports, not +`localparam`s, meaning that derived parameters are overridable. +In the following example, there is no way of preventing `PTR_W` from being +overridden to something incorrect, risking some frustration and wasted time +when non-obvious effects cause issues later. +```verilog +module M + #(parameter integer WIDTH = 123 + , parameter integer PTR_W = clogb2(WIDTH) + ) + ( input wire [WIDTH-1:0] i_data + , output wire [PTR_W-1:0] o_pointer + ); +``` +However, using the non-ANSI form allows `PTR_W` to be specified as +`localparam`, thus preventing overrides and the resulting confusion, i.e: +```verilog +module M + ( i_data + , o_pointer + ); + + parameter integer WIDTH = 123; + localparam integer PTR_W = clogb2(WIDTH); + + input wire [WIDTH-1:0] i_data; + output wire [PTR_W-1:0] o_pointer; +``` +While this only affects modules which use derived parameters in the port +declarations, a consistent style is generally easier to work with. +For these reasons, the non-ANSI form is required. +```toml +syntaxrules.module_ansi_forbidden = true +``` + +SystemVerilog introduced several keywords which greatly help to clarify intent, +but these are unavailable. +Instead of `always_ff @(posedge clk)` and `always_comb`, we can use +`always @(posedge clk)` and `always @*`. +That means only the form like `always @(a or b)`, i.e. no edge sensitivities, +can be forbidden. +```toml +syntaxrules.general_always_level_sensitive = true +``` +On the same theme, guidelines around blocking vs non-blocking assignments also +need to be altered, but keeping the same general intention. +Clocked `always` processes should only use non-blocking assignment `<=`, and +combinatorial `always` processes should only use blocking assignment `=`. +```toml +syntaxrules.blocking_assignment_in_always_at_edge = true +syntaxrules.non_blocking_assignment_in_always_no_edge = true +``` + +Verilog doesn't have the same distinction between 2-state and 4-state types as +SystemVerilog, e.g. `int` and `integer`, but requiring some type is still a +good idea. +```toml +syntaxrules.localparam_explicit_type = true +syntaxrules.parameter_explicit_type = true +syntaxrules.parameter_default_value = true +syntaxrules.parameter_in_generate = true +``` + +In IEEE1364-2001, the use of `generate` and `endgenerate` is mandatory, but +optional in IEEE1364-2005. +For more compatibility, these keywords are required by this ruleset, as are +`genvar` declarations outside their generate `for` statements. +The enablements of these rules are swapped in **ruleset-designintent** to +reduce visual noise in SystemVerilog. +```toml +syntaxrules.genvar_declaration_in_loop = false +syntaxrules.genvar_declaration_out_loop = true +syntaxrules.keyword_forbidden_generate = false +syntaxrules.keyword_required_generate = true +``` + +Unlike the in the richer language of SystemVerilog, forbidding sequential +blocks (between `begin` and `end`) and sequential loops (`for` under `always`) +is probably too restrictive for Verilog. +Indeed, there is little point in using `always @*` instead of `assign` if +`begin` and `end` are forbidden - in SystemVerilog, `always_comb` provides +extra compile-time checks that `assign` does not. +```toml +#syntaxrules.loop_statement_in_always = true # Not implemented. +#syntaxrules.sequential_block_in_always = true # Not implemented. +syntaxrules.case_default = true # Applies in functions. +syntaxrules.explicit_case_default = true # Applies under `always`. +syntaxrules.explicit_if_else = true +syntaxrules.multiline_for_begin = true +syntaxrules.multiline_if_begin = true +``` diff --git a/rulesets/designintentV2001.toml b/rulesets/designintentV2001.toml new file mode 100644 index 00000000..dafeb0e6 --- /dev/null +++ b/rulesets/designintentV2001.toml @@ -0,0 +1,34 @@ +syntaxrules.keyword_forbidden_always_comb = true +syntaxrules.keyword_forbidden_always_ff = true +syntaxrules.keyword_forbidden_always_latch = true +syntaxrules.keyword_forbidden_priority = true +syntaxrules.keyword_forbidden_unique = true +syntaxrules.keyword_forbidden_unique0 = true +#syntaxrules.keyword_forbidden_logic = true # TODO +syntaxrules.operator_incdec = true +syntaxrules.operator_self_assignment = true +syntaxrules.enum_with_type = true +syntaxrules.function_with_automatic = true +syntaxrules.operator_case_equality = true +syntaxrules.action_block_with_side_effect = true +syntaxrules.default_nettype_none = true +syntaxrules.function_same_as_system_function = true +syntaxrules.module_ansi_forbidden = true +syntaxrules.general_always_level_sensitive = true +syntaxrules.blocking_assignment_in_always_at_edge = true +syntaxrules.non_blocking_assignment_in_always_no_edge = true +syntaxrules.localparam_explicit_type = true +syntaxrules.parameter_explicit_type = true +syntaxrules.parameter_default_value = true +syntaxrules.parameter_in_generate = true +syntaxrules.genvar_declaration_in_loop = false +syntaxrules.genvar_declaration_out_loop = true +syntaxrules.keyword_forbidden_generate = false +syntaxrules.keyword_required_generate = true +#syntaxrules.loop_statement_in_always = true # Not implemented. +#syntaxrules.sequential_block_in_always = true # Not implemented. +syntaxrules.case_default = true # Applies in functions. +syntaxrules.explicit_case_default = true # Applies under `always`. +syntaxrules.explicit_if_else = true +syntaxrules.multiline_for_begin = true +syntaxrules.multiline_if_begin = true diff --git a/rulesets/svlint-designintentV2001 b/rulesets/svlint-designintentV2001 new file mode 100755 index 00000000..b1dc5ad1 --- /dev/null +++ b/rulesets/svlint-designintentV2001 @@ -0,0 +1,32 @@ +#!/usr/bin/env sh +set -e + +# If flag/options are given that don't use the ruleset config, simply run +# svlint with the given arguments. +NONRULESET="-h|--help|-V|--version" +NONRULESET="${NONRULESET}|--dump-filelist|--shell-completion" +NONRULESET="${NONRULESET}|-E|--preprocess-only" +NONRULESET="${NONRULESET}|--config-example|--config-update|--example|--update" +if printf "%b\n" " $*" | grep -Eq " (${NONRULESET})"; +then + svlint $* + exit $? +fi + +SVLINT_CONFIG="$(dirname $(command -v svlint-designintentV2001))/designintentV2001.toml" + +# Delete ANSI control sequences that begin with ESC and (usually) end with m. +# Delete ASCII control characters except line feed ('\n' = 0o12 = 10 = 0x0A). +SANS_CONTROL="| sed -e 's/\\o33\\[[0-9;]*[mGKHF]//g'" +SANS_CONTROL="${SANS_CONTROL} | tr -d '[\\000-\\011\\013-\\037\\177]'" + +# Combine the above output sanitization fragments into variables which can be +# evaluated and processed with xargs, e.g: +# eval "${SVFILES}" | xargs -I {} sh -c "grep foo {};" +# NOTE: Creating a variable with the result (instead of the command) would lead +# to undefined behavior where the list of file paths exceeds 2MiB. +SVFILES="svlint --dump-filelist=files $* ${SANS_CONTROL}" +SVINCDIRS="svlint --dump-filelist=incdirs $* ${SANS_CONTROL}" + + +env SVLINT_CONFIG="${SVLINT_CONFIG}" svlint $* diff --git a/rulesets/svlint-designintentV2001.cmd b/rulesets/svlint-designintentV2001.cmd new file mode 100644 index 00000000..9319f52c --- /dev/null +++ b/rulesets/svlint-designintentV2001.cmd @@ -0,0 +1,7 @@ + +@echo off +for /f %%E in ('where.exe svlint-designintentV2001') do ( + set "SVLINT_CONFIG=%%~dpEdesignintentV2001.toml" +) +svlint %* + diff --git a/rulesets/svls-designintentV2001 b/rulesets/svls-designintentV2001 new file mode 100755 index 00000000..9d8c23aa --- /dev/null +++ b/rulesets/svls-designintentV2001 @@ -0,0 +1,15 @@ +#!/usr/bin/env sh +set -e + +# If flag/options are given that don't use the ruleset config, simply run +# svls with the given arguments. +NONRULESET="-h|--help|-V|--version" +if printf "%b\n" " $*" | grep -Eq " (${NONRULESET})"; +then + svls $* + exit $? +fi + +SVLINT_CONFIG="$(dirname $(command -v svls-designintentV2001))/designintentV2001.toml" + +env SVLINT_CONFIG="${SVLINT_CONFIG}" svls $* diff --git a/rulesets/svls-designintentV2001.cmd b/rulesets/svls-designintentV2001.cmd new file mode 100644 index 00000000..fb2ae2c8 --- /dev/null +++ b/rulesets/svls-designintentV2001.cmd @@ -0,0 +1,7 @@ + +@echo off +for /f %%E in ('where.exe svls-designintentV2001') do ( + set "SVLINT_CONFIG=%%~dpEdesignintentV2001.toml" +) +svls %* + From 85e0585e86434476b4eb56c143675e42fd38a696 Mon Sep 17 00:00:00 2001 From: Dave McEwan Date: Fri, 10 Nov 2023 23:48:14 +0000 Subject: [PATCH 11/13] V2k1 More minor typo fixes ("delimet" -> "delimit") --- MANUAL.md | 8 ++++---- testcases/syntaxrules/fail/generate_for_with_label.sv | 2 +- testcases/syntaxrules/fail/generate_if_with_label.sv | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/MANUAL.md b/MANUAL.md index d0554ac8..e5d5625f 100644 --- a/MANUAL.md +++ b/MANUAL.md @@ -5271,7 +5271,7 @@ endmodule ### Fail Example (1 of 3) ```systemverilog module M; - for (genvar i=0; i < 10; i++) // No begin/end delimeters. + for (genvar i=0; i < 10; i++) // No begin/end delimiters. assign a[i] = i; endmodule ``` @@ -5357,7 +5357,7 @@ endmodule ### Fail Example (1 of 8) ```systemverilog module M; - if (x) // No begin/end delimeters. + if (x) // No begin/end delimiters. assign a = 0; // if condition. else if (x) begin: l_def assign a = 1; @@ -5372,7 +5372,7 @@ endmodule module M; if (x) begin: l_abc assign a = 0; - end else if (x) // No begin/end delimeters. + end else if (x) // No begin/end delimiters. assign a = 1; // else-if condition. else begin: l_hij assign a = 2; @@ -5385,7 +5385,7 @@ module M; assign a = 0; end else if (x) begin: l_def assign a = 1; - end else // No begin/end delimeters. + end else // No begin/end delimiters. assign a = 2; // else condition endmodule ``` diff --git a/testcases/syntaxrules/fail/generate_for_with_label.sv b/testcases/syntaxrules/fail/generate_for_with_label.sv index d2021926..5d20a6b7 100644 --- a/testcases/syntaxrules/fail/generate_for_with_label.sv +++ b/testcases/syntaxrules/fail/generate_for_with_label.sv @@ -1,5 +1,5 @@ module M; - for (genvar i=0; i < 10; i++) // No begin/end delimeters. + for (genvar i=0; i < 10; i++) // No begin/end delimiters. assign a[i] = i; endmodule //////////////////////////////////////////////////////////////////////////////// diff --git a/testcases/syntaxrules/fail/generate_if_with_label.sv b/testcases/syntaxrules/fail/generate_if_with_label.sv index adf001a0..53576cd7 100644 --- a/testcases/syntaxrules/fail/generate_if_with_label.sv +++ b/testcases/syntaxrules/fail/generate_if_with_label.sv @@ -1,5 +1,5 @@ module M; - if (x) // No begin/end delimeters. + if (x) // No begin/end delimiters. assign a = 0; // if condition. else if (x) begin: l_def assign a = 1; @@ -11,7 +11,7 @@ endmodule module M; if (x) begin: l_abc assign a = 0; - end else if (x) // No begin/end delimeters. + end else if (x) // No begin/end delimiters. assign a = 1; // else-if condition. else begin: l_hij assign a = 2; @@ -24,7 +24,7 @@ module M; assign a = 0; end else if (x) begin: l_def assign a = 1; - end else // No begin/end delimeters. + end else // No begin/end delimiters. assign a = 2; // else condition endmodule //////////////////////////////////////////////////////////////////////////////// From 7d320e43a537094abb4c988b41b979caa3be7b8e Mon Sep 17 00:00:00 2001 From: Dave McEwan Date: Sat, 11 Nov 2023 00:09:48 +0000 Subject: [PATCH 12/13] V2k1 New rule `keyword_forbidden_logic` --- MANUAL.md | 63 ++++++++++++++++++- md/ruleset-designintentV2001.md | 2 +- ...planation-keyword_forbidden_always_comb.md | 1 + ...explanation-keyword_forbidden_always_ff.md | 1 + ...lanation-keyword_forbidden_always_latch.md | 1 + ...les-explanation-keyword_forbidden_logic.md | 24 +++++++ md/syntaxrules-explanation-operator_incdec.md | 1 + ...es-explanation-operator_self_assignment.md | 1 + rulesets/designintentV2001.toml | 2 +- src/syntaxrules/keyword_forbidden_logic.rs | 38 +++++++++++ .../fail/keyword_forbidden_logic.sv | 3 + .../pass/keyword_forbidden_logic.sv | 4 ++ 12 files changed, 138 insertions(+), 3 deletions(-) create mode 100644 md/syntaxrules-explanation-keyword_forbidden_logic.md create mode 100644 src/syntaxrules/keyword_forbidden_logic.rs create mode 100644 testcases/syntaxrules/fail/keyword_forbidden_logic.sv create mode 100644 testcases/syntaxrules/pass/keyword_forbidden_logic.sv diff --git a/MANUAL.md b/MANUAL.md index e5d5625f..a0d4841c 100644 --- a/MANUAL.md +++ b/MANUAL.md @@ -2336,6 +2336,7 @@ backwards compatibility with Verilog. See also: - **keyword_forbidden_always_ff** - Suggested companion rule. - **keyword_forbidden_always_latch** - Suggested companion rule. +- **keyword_forbidden_logic** - Suggested companion rule. - **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. - **operator_incdec** - Suggested companion rule. - **operator_self_assignment** - Suggested companion rule. @@ -2388,6 +2389,7 @@ This rule requires something like `always @(posedge clk)` to be used instead of See also: - **keyword_forbidden_always_comb** - Suggested companion rule. - **keyword_forbidden_always_latch** - Suggested companion rule. +- **keyword_forbidden_logic** - Suggested companion rule. - **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. - **operator_incdec** - Suggested companion rule. - **operator_self_assignment** - Suggested companion rule. @@ -2451,6 +2453,7 @@ instead of `always_latch` for backwards compatibility with Verilog. See also: - **keyword_forbidden_always_comb** - Suggested companion rule. - **keyword_forbidden_always_ff** - Suggested companion rule. +- **keyword_forbidden_logic** - Suggested companion rule. - **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. - **operator_incdec** - Suggested companion rule. - **operator_self_assignment** - Suggested companion rule. @@ -2516,6 +2519,62 @@ The most relevant clauses of IEEE1800-2017 are: +* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + +## Syntax Rule: `keyword_forbidden_logic` + +### Hint + +Replace `logic` keywords with `wire` or `reg`. + +### Reason + +Only SystemVerilog, not Verilog, has `logic`. + +### Pass Example (1 of 1) +```systemverilog +module M; + wire a; + reg b; +endmodule +``` + +### Fail Example (1 of 1) +```systemverilog +module M; + logic a; +endmodule +``` + +### Explanation + +The datatype `logic` was added to SystemVerilog (IEEE1800) to clarify +designer's intent, mostly replacing `wire` and fully replacing `reg`. +Verilog (IEEE1364) only has the `reg` bit-vector variable (and the various type +of nets). +This rule forbids `logic` for backwards compatibility with Verilog. + +See also: +- **keyword_forbidden_always_comb** - Suggested companion rule. +- **keyword_forbidden_always_ff** - Suggested companion rule. +- **keyword_forbidden_always_latch** - Suggested companion rule. +- **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **operator_incdec** - Suggested companion rule. +- **operator_self_assignment** - Suggested companion rule. + +The most relevant clauses of IEEE1364-2001 are: +- 3.2 Nets and variables +- 3.3 Vectors +- 3.7 Nets types +- 3.8 regs + +The most relevant clauses of IEEE1800-2017 are: +- 6.5 Nets and variables +- 6.5 Vector declarations +- 6.11 Integer data types + + + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * ## Syntax Rule: `keyword_forbidden_priority` @@ -4114,6 +4173,7 @@ See also: - **keyword_forbidden_always_comb** - Suggested companion rule. - **keyword_forbidden_always_ff** - Suggested companion rule. - **keyword_forbidden_always_latch** - Suggested companion rule. +- **keyword_forbidden_logic** - Suggested companion rule. - **operator_self_assignment** - Suggested companion rule. The most relevant clauses of IEEE1364-2001 are: @@ -4247,6 +4307,7 @@ See also: - **keyword_forbidden_always_comb** - Suggested companion rule. - **keyword_forbidden_always_ff** - Suggested companion rule. - **keyword_forbidden_always_latch** - Suggested companion rule. +- **keyword_forbidden_logic** - Suggested companion rule. - **operator_incdec** - Suggested companion rule. The most relevant clauses of IEEE1364-2001 are: @@ -11868,7 +11929,7 @@ syntaxrules.keyword_forbidden_always_latch = true syntaxrules.keyword_forbidden_priority = true syntaxrules.keyword_forbidden_unique = true syntaxrules.keyword_forbidden_unique0 = true -#syntaxrules.keyword_forbidden_logic = true # TODO +syntaxrules.keyword_forbidden_logic = true syntaxrules.operator_incdec = true syntaxrules.operator_self_assignment = true ``` diff --git a/md/ruleset-designintentV2001.md b/md/ruleset-designintentV2001.md index 3a8e4875..2da58a15 100644 --- a/md/ruleset-designintentV2001.md +++ b/md/ruleset-designintentV2001.md @@ -15,7 +15,7 @@ syntaxrules.keyword_forbidden_always_latch = true syntaxrules.keyword_forbidden_priority = true syntaxrules.keyword_forbidden_unique = true syntaxrules.keyword_forbidden_unique0 = true -#syntaxrules.keyword_forbidden_logic = true # TODO +syntaxrules.keyword_forbidden_logic = true syntaxrules.operator_incdec = true syntaxrules.operator_self_assignment = true ``` diff --git a/md/syntaxrules-explanation-keyword_forbidden_always_comb.md b/md/syntaxrules-explanation-keyword_forbidden_always_comb.md index 7f5151f6..c999c0b7 100644 --- a/md/syntaxrules-explanation-keyword_forbidden_always_comb.md +++ b/md/syntaxrules-explanation-keyword_forbidden_always_comb.md @@ -8,6 +8,7 @@ backwards compatibility with Verilog. See also: - **keyword_forbidden_always_ff** - Suggested companion rule. - **keyword_forbidden_always_latch** - Suggested companion rule. +- **keyword_forbidden_logic** - Suggested companion rule. - **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. - **operator_incdec** - Suggested companion rule. - **operator_self_assignment** - Suggested companion rule. diff --git a/md/syntaxrules-explanation-keyword_forbidden_always_ff.md b/md/syntaxrules-explanation-keyword_forbidden_always_ff.md index 2c57e5c4..2664958f 100644 --- a/md/syntaxrules-explanation-keyword_forbidden_always_ff.md +++ b/md/syntaxrules-explanation-keyword_forbidden_always_ff.md @@ -8,6 +8,7 @@ This rule requires something like `always @(posedge clk)` to be used instead of See also: - **keyword_forbidden_always_comb** - Suggested companion rule. - **keyword_forbidden_always_latch** - Suggested companion rule. +- **keyword_forbidden_logic** - Suggested companion rule. - **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. - **operator_incdec** - Suggested companion rule. - **operator_self_assignment** - Suggested companion rule. diff --git a/md/syntaxrules-explanation-keyword_forbidden_always_latch.md b/md/syntaxrules-explanation-keyword_forbidden_always_latch.md index dc368cc5..054b1b20 100644 --- a/md/syntaxrules-explanation-keyword_forbidden_always_latch.md +++ b/md/syntaxrules-explanation-keyword_forbidden_always_latch.md @@ -8,6 +8,7 @@ instead of `always_latch` for backwards compatibility with Verilog. See also: - **keyword_forbidden_always_comb** - Suggested companion rule. - **keyword_forbidden_always_ff** - Suggested companion rule. +- **keyword_forbidden_logic** - Suggested companion rule. - **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. - **operator_incdec** - Suggested companion rule. - **operator_self_assignment** - Suggested companion rule. diff --git a/md/syntaxrules-explanation-keyword_forbidden_logic.md b/md/syntaxrules-explanation-keyword_forbidden_logic.md new file mode 100644 index 00000000..248b0ac8 --- /dev/null +++ b/md/syntaxrules-explanation-keyword_forbidden_logic.md @@ -0,0 +1,24 @@ +The datatype `logic` was added to SystemVerilog (IEEE1800) to clarify +designer's intent, mostly replacing `wire` and fully replacing `reg`. +Verilog (IEEE1364) only has the `reg` bit-vector variable (and the various type +of nets). +This rule forbids `logic` for backwards compatibility with Verilog. + +See also: +- **keyword_forbidden_always_comb** - Suggested companion rule. +- **keyword_forbidden_always_ff** - Suggested companion rule. +- **keyword_forbidden_always_latch** - Suggested companion rule. +- **module_ansi_forbidden** - Useful companion rule for Verilog compatibility. +- **operator_incdec** - Suggested companion rule. +- **operator_self_assignment** - Suggested companion rule. + +The most relevant clauses of IEEE1364-2001 are: +- 3.2 Nets and variables +- 3.3 Vectors +- 3.7 Nets types +- 3.8 regs + +The most relevant clauses of IEEE1800-2017 are: +- 6.5 Nets and variables +- 6.5 Vector declarations +- 6.11 Integer data types diff --git a/md/syntaxrules-explanation-operator_incdec.md b/md/syntaxrules-explanation-operator_incdec.md index 819900dd..9a7a63b3 100644 --- a/md/syntaxrules-explanation-operator_incdec.md +++ b/md/syntaxrules-explanation-operator_incdec.md @@ -9,6 +9,7 @@ See also: - **keyword_forbidden_always_comb** - Suggested companion rule. - **keyword_forbidden_always_ff** - Suggested companion rule. - **keyword_forbidden_always_latch** - Suggested companion rule. +- **keyword_forbidden_logic** - Suggested companion rule. - **operator_self_assignment** - Suggested companion rule. The most relevant clauses of IEEE1364-2001 are: diff --git a/md/syntaxrules-explanation-operator_self_assignment.md b/md/syntaxrules-explanation-operator_self_assignment.md index 7899f143..6ec0e6d1 100644 --- a/md/syntaxrules-explanation-operator_self_assignment.md +++ b/md/syntaxrules-explanation-operator_self_assignment.md @@ -10,6 +10,7 @@ See also: - **keyword_forbidden_always_comb** - Suggested companion rule. - **keyword_forbidden_always_ff** - Suggested companion rule. - **keyword_forbidden_always_latch** - Suggested companion rule. +- **keyword_forbidden_logic** - Suggested companion rule. - **operator_incdec** - Suggested companion rule. The most relevant clauses of IEEE1364-2001 are: diff --git a/rulesets/designintentV2001.toml b/rulesets/designintentV2001.toml index dafeb0e6..b675d5f7 100644 --- a/rulesets/designintentV2001.toml +++ b/rulesets/designintentV2001.toml @@ -4,7 +4,7 @@ syntaxrules.keyword_forbidden_always_latch = true syntaxrules.keyword_forbidden_priority = true syntaxrules.keyword_forbidden_unique = true syntaxrules.keyword_forbidden_unique0 = true -#syntaxrules.keyword_forbidden_logic = true # TODO +syntaxrules.keyword_forbidden_logic = true syntaxrules.operator_incdec = true syntaxrules.operator_self_assignment = true syntaxrules.enum_with_type = true diff --git a/src/syntaxrules/keyword_forbidden_logic.rs b/src/syntaxrules/keyword_forbidden_logic.rs new file mode 100644 index 00000000..a6190834 --- /dev/null +++ b/src/syntaxrules/keyword_forbidden_logic.rs @@ -0,0 +1,38 @@ +use crate::config::ConfigOption; +use crate::linter::{SyntaxRule, SyntaxRuleResult}; +use sv_parser::{IntegerVectorType, NodeEvent, RefNode, SyntaxTree}; + +#[derive(Default)] +pub struct KeywordForbiddenLogic; + +impl SyntaxRule for KeywordForbiddenLogic { + fn check( + &mut self, + _syntax_tree: &SyntaxTree, + event: &NodeEvent, + _option: &ConfigOption, + ) -> SyntaxRuleResult { + let node = match event { + NodeEvent::Enter(x) => x, + NodeEvent::Leave(_) => { + return SyntaxRuleResult::Pass; + } + }; + match node { + RefNode::IntegerVectorType(IntegerVectorType::Logic(_)) => SyntaxRuleResult::Fail, + _ => SyntaxRuleResult::Pass, + } + } + + fn name(&self) -> String { + String::from("keyword_forbidden_logic") + } + + fn hint(&self, _option: &ConfigOption) -> String { + String::from("Replace `logic` keywords with `wire` or `reg`.") + } + + fn reason(&self) -> String { + String::from("Only SystemVerilog, not Verilog, has `logic`.") + } +} diff --git a/testcases/syntaxrules/fail/keyword_forbidden_logic.sv b/testcases/syntaxrules/fail/keyword_forbidden_logic.sv new file mode 100644 index 00000000..31e04a7c --- /dev/null +++ b/testcases/syntaxrules/fail/keyword_forbidden_logic.sv @@ -0,0 +1,3 @@ +module M; + logic a; +endmodule diff --git a/testcases/syntaxrules/pass/keyword_forbidden_logic.sv b/testcases/syntaxrules/pass/keyword_forbidden_logic.sv new file mode 100644 index 00000000..0951a62b --- /dev/null +++ b/testcases/syntaxrules/pass/keyword_forbidden_logic.sv @@ -0,0 +1,4 @@ +module M; + wire a; + reg b; +endmodule From b1382e94088e31b1f5616abead6f8fd18e2589dc Mon Sep 17 00:00:00 2001 From: Dave McEwan Date: Sat, 11 Nov 2023 21:13:39 +0000 Subject: [PATCH 13/13] More minor typo fixes - behaviour -> behavior (prefer US spellings) - be encourage -> be encouraged --- MANUAL.md | 10 +++++----- md/manual-rulesets.md | 4 ++-- ...xrules-explanation-keyword_forbidden_always_comb.md | 2 +- ...taxrules-explanation-keyword_forbidden_always_ff.md | 2 +- ...rules-explanation-keyword_forbidden_always_latch.md | 2 +- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/MANUAL.md b/MANUAL.md index a0d4841c..4628fa10 100644 --- a/MANUAL.md +++ b/MANUAL.md @@ -2328,7 +2328,7 @@ endmodule The keywords `always_comb`, `always_ff`, and `always_latch` were added to SystemVerilog (IEEE1800) to require extra safety checks at compile time. -Verilog (IEEE1364) only has `always`, which can describe equivalent behaviour +Verilog (IEEE1364) only has `always`, which can describe equivalent behavior but without the compile-time checks. This rule requires `always @*` to be used instead of `always_comb` for backwards compatibility with Verilog. @@ -2381,7 +2381,7 @@ endmodule The keywords `always_comb`, `always_ff`, and `always_latch` were added to SystemVerilog (IEEE1800) to require extra safety checks at compile time. -Verilog (IEEE1364) only has `always`, which can describe equivalent behaviour +Verilog (IEEE1364) only has `always`, which can describe equivalent behavior but without the compile-time checks. This rule requires something like `always @(posedge clk)` to be used instead of `always_ff @(posedge clk)` for backwards compatibility with Verilog. @@ -2445,7 +2445,7 @@ endmodule The keywords `always_comb`, `always_ff`, and `always_latch` were added to SystemVerilog (IEEE1800) to require extra safety checks at compile time. -Verilog (IEEE1364) only has `always`, which can describe equivalent behaviour +Verilog (IEEE1364) only has `always`, which can describe equivalent behavior but without the compile-time checks. This rule requires `always @*` or something like `always @(en)` to be used instead of `always_latch` for backwards compatibility with Verilog. @@ -10622,8 +10622,8 @@ If instead the `--config` option was used in wrapper scripts, this could lead to confusion where TOML files exist elsewhere in the hierarchy. It isn't essential for all ruleset scripts to be POSIX compliant, but POSIX -compliance should be encourage because it allows for consistent behavior across -the widest range of systems. +compliance should be encouraged because it allows for consistent behavior +across the widest range of systems. The utilities used in the POSIX wrappers are specified in the current POSIX standard (IEEE1003.1-2017, Volume 3: Shell and Utilities). Some resources related to these components: diff --git a/md/manual-rulesets.md b/md/manual-rulesets.md index 90292522..3adaf11f 100644 --- a/md/manual-rulesets.md +++ b/md/manual-rulesets.md @@ -137,8 +137,8 @@ If instead the `--config` option was used in wrapper scripts, this could lead to confusion where TOML files exist elsewhere in the hierarchy. It isn't essential for all ruleset scripts to be POSIX compliant, but POSIX -compliance should be encourage because it allows for consistent behavior across -the widest range of systems. +compliance should be encouraged because it allows for consistent behavior +across the widest range of systems. The utilities used in the POSIX wrappers are specified in the current POSIX standard (IEEE1003.1-2017, Volume 3: Shell and Utilities). Some resources related to these components: diff --git a/md/syntaxrules-explanation-keyword_forbidden_always_comb.md b/md/syntaxrules-explanation-keyword_forbidden_always_comb.md index c999c0b7..dc3f526d 100644 --- a/md/syntaxrules-explanation-keyword_forbidden_always_comb.md +++ b/md/syntaxrules-explanation-keyword_forbidden_always_comb.md @@ -1,6 +1,6 @@ The keywords `always_comb`, `always_ff`, and `always_latch` were added to SystemVerilog (IEEE1800) to require extra safety checks at compile time. -Verilog (IEEE1364) only has `always`, which can describe equivalent behaviour +Verilog (IEEE1364) only has `always`, which can describe equivalent behavior but without the compile-time checks. This rule requires `always @*` to be used instead of `always_comb` for backwards compatibility with Verilog. diff --git a/md/syntaxrules-explanation-keyword_forbidden_always_ff.md b/md/syntaxrules-explanation-keyword_forbidden_always_ff.md index 2664958f..a729619a 100644 --- a/md/syntaxrules-explanation-keyword_forbidden_always_ff.md +++ b/md/syntaxrules-explanation-keyword_forbidden_always_ff.md @@ -1,6 +1,6 @@ The keywords `always_comb`, `always_ff`, and `always_latch` were added to SystemVerilog (IEEE1800) to require extra safety checks at compile time. -Verilog (IEEE1364) only has `always`, which can describe equivalent behaviour +Verilog (IEEE1364) only has `always`, which can describe equivalent behavior but without the compile-time checks. This rule requires something like `always @(posedge clk)` to be used instead of `always_ff @(posedge clk)` for backwards compatibility with Verilog. diff --git a/md/syntaxrules-explanation-keyword_forbidden_always_latch.md b/md/syntaxrules-explanation-keyword_forbidden_always_latch.md index 054b1b20..2ec9d25b 100644 --- a/md/syntaxrules-explanation-keyword_forbidden_always_latch.md +++ b/md/syntaxrules-explanation-keyword_forbidden_always_latch.md @@ -1,6 +1,6 @@ The keywords `always_comb`, `always_ff`, and `always_latch` were added to SystemVerilog (IEEE1800) to require extra safety checks at compile time. -Verilog (IEEE1364) only has `always`, which can describe equivalent behaviour +Verilog (IEEE1364) only has `always`, which can describe equivalent behavior but without the compile-time checks. This rule requires `always @*` or something like `always @(en)` to be used instead of `always_latch` for backwards compatibility with Verilog.