From eee07bfd55080335eecfbffbb8a561f49143dca3 Mon Sep 17 00:00:00 2001 From: Yan Churkin Date: Thu, 29 Feb 2024 22:28:25 +0300 Subject: [PATCH 1/3] Fix static subroutine local variables multiple assigns and reduce MultipleAlwaysAssigns diag severity --- scripts/diagnostics.txt | 2 +- source/ast/expressions/CallExpression.cpp | 3 +- tests/unittests/ast/MemberTests.cpp | 2 +- tests/unittests/ast/SubroutineTests.cpp | 80 +++++++++++++++++++++++ 4 files changed, 83 insertions(+), 4 deletions(-) diff --git a/scripts/diagnostics.txt b/scripts/diagnostics.txt index ade7de63b..ae2bbfdf4 100644 --- a/scripts/diagnostics.txt +++ b/scripts/diagnostics.txt @@ -661,7 +661,6 @@ error InOutVarPortConn "'{}' cannot be connected to 'inout' port (only nets are error InOutUWireConn "'uwire' net '{}' cannot be connected to 'inout' port" error MixedVarAssigns "cannot mix continuous and procedural assignments to variable '{}'" error MultipleContAssigns "cannot have multiple continuous assignments to variable '{}'" -error MultipleAlwaysAssigns "variable '{}' driven by {} procedure cannot be written to by any other process" error MultipleUWireDrivers "'uwire' net '{}' cannot have multiple drivers" error MultipleUDNTDrivers "net '{}' with user-defined nettype '{}' cannot have multiple drivers because it does not specify a resolution function" error InputPortAssign "cannot assign to input port '{}'" @@ -701,6 +700,7 @@ warning sign-conversion SignConversion "implicit conversion changes signedness f warning float-bool-conv FloatBoolConv "implicit conversion from floating point type {} to boolean value" warning int-bool-conv IntBoolConv "implicit conversion from {} to boolean value" warning useless-cast UselessCast "useless cast from {} to the same type" +warning multiple-always-assigns MultipleAlwaysAssigns "variable '{}' driven by {} procedure cannot be written to by any other process" subsystem Statements error ReturnNotInSubroutine "return statement is only valid inside task and function blocks" diff --git a/source/ast/expressions/CallExpression.cpp b/source/ast/expressions/CallExpression.cpp index a0c37d08d..a7e3853ae 100644 --- a/source/ast/expressions/CallExpression.cpp +++ b/source/ast/expressions/CallExpression.cpp @@ -372,8 +372,7 @@ Expression& CallExpression::fromArgs(Compilation& compilation, const Subroutine& // If this subroutine is invoked from a procedure, register drivers for this // particular procedure to detect multiple driver violations. if (!thisClass) { - if (auto proc = context.getProceduralBlock(); - proc && proc->isSingleDriverBlock() && !context.scope->isUninstantiated()) { + if (auto proc = context.getProceduralBlock(); proc && !context.scope->isUninstantiated()) { addSubroutineDrivers(*proc, symbol, *result); } } diff --git a/tests/unittests/ast/MemberTests.cpp b/tests/unittests/ast/MemberTests.cpp index 520668612..5d8c090bf 100644 --- a/tests/unittests/ast/MemberTests.cpp +++ b/tests/unittests/ast/MemberTests.cpp @@ -1915,7 +1915,7 @@ endmodule auto& diags = compilation.getAllDiagnostics(); std::string result = "\n" + report(diags); CHECK(result == R"( -source:14:17: error: variable 'foo' driven by always_comb procedure cannot be written to by any other process +source:14:17: warning: variable 'foo' driven by always_comb procedure cannot be written to by any other process [-Wmultiple-always-assigns] always_comb i.foo = 1; ^~~~~ note: from 'm.n2' and 'm.n1' diff --git a/tests/unittests/ast/SubroutineTests.cpp b/tests/unittests/ast/SubroutineTests.cpp index 21a5336df..fe8794614 100644 --- a/tests/unittests/ast/SubroutineTests.cpp +++ b/tests/unittests/ast/SubroutineTests.cpp @@ -708,3 +708,83 @@ endmodule CHECK(diags[0].code == diag::MultipleAlwaysAssigns); CHECK(diags[1].code == diag::MultipleAlwaysAssigns); } + +TEST_CASE("driver checking applied to static function args (always_ff and always_ff)") { + auto tree = SyntaxTree::fromText(R"( +module m(input clk, input rst); + function logic f(logic a); + logic b; + b = a; + return b; + endfunction + + logic i, j; + always_ff @(posedge clk) begin + i <= f(i); + end + always_ff @(posedge rst) begin + j <= f(j); + end +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + + auto& diags = compilation.getAllDiagnostics(); + REQUIRE(diags.size() == 1); + CHECK(diags[0].code == diag::MultipleAlwaysAssigns); +} + +TEST_CASE("driver checking applied to static function local variables (always_ff and always)") { + auto tree = SyntaxTree::fromText(R"( +module m(input clk, input rst); + function logic f(logic a); + logic b; + b = a; + return b; + endfunction + + logic i, j; + always @(posedge clk) begin + i <= f(i); + end + always_ff @(posedge rst) begin + j <= f(j); + end +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + + auto& diags = compilation.getAllDiagnostics(); + REQUIRE(diags.size() == 1); + CHECK(diags[0].code == diag::MultipleAlwaysAssigns); +} + +TEST_CASE("driver checking applied to static function local variables (always and always)") { + auto tree = SyntaxTree::fromText(R"( +module m(input clk, input rst); + function logic f(logic a); + logic b; + b = a; + return b; + endfunction + + logic i, j; + always @(posedge clk) begin + i <= f(i); + end + always @(posedge rst) begin + j <= f(j); + end +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + + auto& diags = compilation.getAllDiagnostics(); + REQUIRE(diags.size() == 0); +} From b32ad0304fa123fad3892f2757c2d0eedc9c5b30 Mon Sep 17 00:00:00 2001 From: Yan Churkin Date: Thu, 29 Feb 2024 23:13:16 +0300 Subject: [PATCH 2/3] Add docs --- scripts/diagnostics.txt | 2 +- scripts/warning_docs.txt | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/scripts/diagnostics.txt b/scripts/diagnostics.txt index ae2bbfdf4..d946802ab 100644 --- a/scripts/diagnostics.txt +++ b/scripts/diagnostics.txt @@ -1055,7 +1055,7 @@ group default = { real-underflow real-overflow vector-overflow int-overflow unco invalid-pragma-viewport duplicate-definition protect-encoding-bytes invalid-encoding-byte raw-protect-eof protected-envelope specify-param dup-timing-path invalid-pulsestyle negative-timing-limit bad-procedural-force duplicate-defparam implicit-port-type-mismatch - split-distweight-op dpi-pure-task multibit-edge unknown-sys-name } + split-distweight-op dpi-pure-task multibit-edge unknown-sys-name multiple-always-assigns } group extra = { empty-member empty-stmt dup-import pointless-void-cast case-gen-none case-gen-dup unused-result format-real ignored-slice task-ignored width-trunc dup-attr event-const diff --git a/scripts/warning_docs.txt b/scripts/warning_docs.txt index c3b694e73..f63f0ae29 100644 --- a/scripts/warning_docs.txt +++ b/scripts/warning_docs.txt @@ -1137,3 +1137,18 @@ module m; end endmodule ``` + +-Wmultiple-always-assigns +Detects that variables on the left-hand side of assignments within an procedure blocks, +including variables from the contents of a called functions, shall not be written to by any other process. +``` +module m(input clk, input rst); + logic i; + always_ff @(posedge clk) begin + i <= 1; + end + always_ff @(posedge rst) begin + i <= 2; + end +endmodule +``` From 09eb2084d7238a16a88a17f28cb4a73a599d558f Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 2 Mar 2024 18:10:16 +0000 Subject: [PATCH 3/3] style: pre-commit fixes --- scripts/warning_docs.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/warning_docs.txt b/scripts/warning_docs.txt index 36ced389d..b4cda2c30 100644 --- a/scripts/warning_docs.txt +++ b/scripts/warning_docs.txt @@ -1176,4 +1176,4 @@ module m(input clk, input rst); i <= 2; end endmodule -``` \ No newline at end of file +```