From 4951cb8ae0251a0f7de2484d4595afc595f0cbf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joel=20Sol=C3=A9=20Casal=C3=A9?= Date: Wed, 26 Jun 2024 15:42:03 +0200 Subject: [PATCH 01/11] New slang-tidy check: AlwaysCombNamed This new check enforces that each always_comb block is named --- tools/tidy/CMakeLists.txt | 4 +- tools/tidy/include/TidyDiags.h | 1 + tools/tidy/src/TidyConfig.cpp | 1 + tools/tidy/src/style/AlwaysCombNamed.cpp | 58 +++++++++++++++++ tools/tidy/tests/AlwaysCombNamedTest.cpp | 82 ++++++++++++++++++++++++ tools/tidy/tests/CMakeLists.txt | 4 +- 6 files changed, 148 insertions(+), 2 deletions(-) create mode 100644 tools/tidy/src/style/AlwaysCombNamed.cpp create mode 100644 tools/tidy/tests/AlwaysCombNamedTest.cpp diff --git a/tools/tidy/CMakeLists.txt b/tools/tidy/CMakeLists.txt index aad57dbd2..4e035a690 100644 --- a/tools/tidy/CMakeLists.txt +++ b/tools/tidy/CMakeLists.txt @@ -20,7 +20,9 @@ add_library( src/synthesis/XilinxDoNotCareValues.cpp src/synthesis/CastSignedIndex.cpp src/style/NoDotStarInPortConnection.cpp - src/style/NoImplicitPortNameInPortConnection.cpp) + src/style/NoImplicitPortNameInPortConnection.cpp + src/style/AlwaysCombNamed.cpp + ) target_include_directories(slang_tidy_obj_lib PUBLIC include ../../include) target_link_libraries(slang_tidy_obj_lib PUBLIC slang::slang) diff --git a/tools/tidy/include/TidyDiags.h b/tools/tidy/include/TidyDiags.h index 4befa9b1c..fcd4c059e 100644 --- a/tools/tidy/include/TidyDiags.h +++ b/tools/tidy/include/TidyDiags.h @@ -24,5 +24,6 @@ inline constexpr DiagCode XilinxDoNotCareValues(DiagSubsystem::Tidy, 9); inline constexpr DiagCode CastSignedIndex(DiagSubsystem::Tidy, 10); inline constexpr DiagCode NoDotStarInPortConnection(DiagSubsystem::Tidy, 11); inline constexpr DiagCode NoImplicitPortNameInPortConnection(DiagSubsystem::Tidy, 12); +inline constexpr DiagCode AlwaysCombBlockNamed(DiagSubsystem::Tidy, 13); } // namespace slang::diag diff --git a/tools/tidy/src/TidyConfig.cpp b/tools/tidy/src/TidyConfig.cpp index 1332615be..b3413d1f3 100644 --- a/tools/tidy/src/TidyConfig.cpp +++ b/tools/tidy/src/TidyConfig.cpp @@ -26,6 +26,7 @@ TidyConfig::TidyConfig() { styleChecks.emplace("OnlyANSIPortDecl", CheckStatus::ENABLED); styleChecks.emplace("NoDotStarInPortConnection", CheckStatus::ENABLED); styleChecks.emplace("NoImplicitPortNameInPortConnection", CheckStatus::ENABLED); + styleChecks.emplace("AlwaysCombBlockNamed", CheckStatus::ENABLED); checkKinds.insert({slang::TidyKind::Style, styleChecks}); auto synthesisChecks = std::unordered_map(); diff --git a/tools/tidy/src/style/AlwaysCombNamed.cpp b/tools/tidy/src/style/AlwaysCombNamed.cpp new file mode 100644 index 000000000..275364cf9 --- /dev/null +++ b/tools/tidy/src/style/AlwaysCombNamed.cpp @@ -0,0 +1,58 @@ +// SPDX-FileCopyrightText: Michael Popoloski +// SPDX-License-Identifier: MIT + +#include "ASTHelperVisitors.h" +#include "TidyDiags.h" +#include "fmt/color.h" + +#include "slang/syntax/AllSyntax.h" + +using namespace slang; +using namespace slang::ast; + +namespace always_comb_named { +struct MainVisitor : public TidyVisitor, ASTVisitor { + explicit MainVisitor(Diagnostics& diagnostics) : TidyVisitor(diagnostics) {} + + void handle(const ProceduralBlockSymbol& symbol) { + + NEEDS_SKIP_SYMBOL(symbol) + + if (symbol.procedureKind != ProceduralBlockKind::AlwaysComb || + symbol.getBody().kind != StatementKind::Block) + return; + + bool isUnnamed = (symbol.getBody().as().blockSymbol == nullptr || + symbol.getBody().as().blockSymbol->name.empty()); + + if (isUnnamed) { + diags.add(diag::AlwaysCombBlockNamed, symbol.location); + } + } +}; +} // namespace always_comb_named + +using namespace always_comb_named; +class AlwaysCombBlockNamed : public TidyCheck { +public: + [[maybe_unused]] explicit AlwaysCombBlockNamed(TidyKind kind) : TidyCheck(kind) {} + + bool check(const ast::RootSymbol& root) override { + MainVisitor visitor(diagnostics); + root.visit(visitor); + if (!diagnostics.empty()) + return false; + return true; + } + + DiagCode diagCode() const override { return diag::AlwaysCombBlockNamed; } + DiagnosticSeverity diagSeverity() const override { return DiagnosticSeverity::Warning; } + std::string diagString() const override { return "definition of an unnamed always_comb block"; } + std::string name() const override { return "AlwaysCombBlockNamed"; } + std::string description() const override { return shortDescription(); } + std::string shortDescription() const override { + return "Enforces that each always_comb block is named"; + } +}; + +REGISTER(AlwaysCombBlockNamed, AlwaysCombBlockNamed, TidyKind::Style) diff --git a/tools/tidy/tests/AlwaysCombNamedTest.cpp b/tools/tidy/tests/AlwaysCombNamedTest.cpp new file mode 100644 index 000000000..d974607e6 --- /dev/null +++ b/tools/tidy/tests/AlwaysCombNamedTest.cpp @@ -0,0 +1,82 @@ +// SPDX-FileCopyrightText: Michael Popoloski +// SPDX-License-Identifier: MIT + +#include "Test.h" +#include "TidyFactory.h" + +TEST_CASE("AlwaysCombBlockNamed: Unnamed always_comb block") { + auto tree = SyntaxTree::fromText(R"( +module top (); + logic a, b; + always_comb begin + a = b; + end +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("AlwaysCombBlockNamed"); + bool result = visitor->check(root); + CHECK_FALSE(result); +} + +TEST_CASE("AlwaysCombBlockNamed: Named always_comb block") { + auto tree = SyntaxTree::fromText(R"( +module top (); + logic a, b; + always_comb begin : named_comb2 + a = b; + end +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("AlwaysCombBlockNamed"); + bool result = visitor->check(root); + CHECK(result); +} + +TEST_CASE("AlwaysCombBlockNamed: Unnamed simple always_comb block") { + auto tree = SyntaxTree::fromText(R"( +module add_or_sub + #(parameter N = 4) + ( + input logic [N-1:0] x_i, y_i, + input logic add, + output logic [N-1:0] z_o + ); + + always_comb + if (add) + z = x + y; + else + z = x - y; +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("AlwaysCombBlockNamed"); + bool result = visitor->check(root); + CHECK(result); +} diff --git a/tools/tidy/tests/CMakeLists.txt b/tools/tidy/tests/CMakeLists.txt index cbd7dc30d..205404112 100644 --- a/tools/tidy/tests/CMakeLists.txt +++ b/tools/tidy/tests/CMakeLists.txt @@ -20,7 +20,9 @@ add_executable( XilinxDoNotCareValuesTest.cpp CastSignedIndexTest.cpp NoDotStarInPortConnectionTest.cpp - NoImplicitPortNameInPortConnectionTest.cpp) + NoImplicitPortNameInPortConnectionTest.cpp + AlwaysCombNamedTest.cpp + ) target_link_libraries(tidy_unittests PRIVATE Catch2::Catch2 slang_tidy_obj_lib) target_compile_definitions(tidy_unittests PRIVATE UNITTESTS) From ce4d6a8907d6440d40a98aed5c51c80831a87ecc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joel=20Sol=C3=A9=20Casal=C3=A9?= Date: Wed, 26 Jun 2024 15:45:15 +0200 Subject: [PATCH 02/11] New slang-tidy check: GenerateNamed This new check enforces that each generate block is named. This also is applied to the for block --- tools/tidy/CMakeLists.txt | 1 + tools/tidy/include/TidyDiags.h | 1 + tools/tidy/src/TidyConfig.cpp | 1 + tools/tidy/src/style/GenerateNamed.cpp | 63 +++++++++ tools/tidy/tests/CMakeLists.txt | 1 + tools/tidy/tests/GenerateNamedTest.cpp | 178 +++++++++++++++++++++++++ 6 files changed, 245 insertions(+) create mode 100644 tools/tidy/src/style/GenerateNamed.cpp create mode 100644 tools/tidy/tests/GenerateNamedTest.cpp diff --git a/tools/tidy/CMakeLists.txt b/tools/tidy/CMakeLists.txt index 4e035a690..3e4e4d29f 100644 --- a/tools/tidy/CMakeLists.txt +++ b/tools/tidy/CMakeLists.txt @@ -22,6 +22,7 @@ add_library( src/style/NoDotStarInPortConnection.cpp src/style/NoImplicitPortNameInPortConnection.cpp src/style/AlwaysCombNamed.cpp + src/style/GenerateNamed.cpp ) target_include_directories(slang_tidy_obj_lib PUBLIC include ../../include) diff --git a/tools/tidy/include/TidyDiags.h b/tools/tidy/include/TidyDiags.h index fcd4c059e..8bfe0d4fc 100644 --- a/tools/tidy/include/TidyDiags.h +++ b/tools/tidy/include/TidyDiags.h @@ -25,5 +25,6 @@ inline constexpr DiagCode CastSignedIndex(DiagSubsystem::Tidy, 10); inline constexpr DiagCode NoDotStarInPortConnection(DiagSubsystem::Tidy, 11); inline constexpr DiagCode NoImplicitPortNameInPortConnection(DiagSubsystem::Tidy, 12); inline constexpr DiagCode AlwaysCombBlockNamed(DiagSubsystem::Tidy, 13); +inline constexpr DiagCode GenerateNamed(DiagSubsystem::Tidy, 14); } // namespace slang::diag diff --git a/tools/tidy/src/TidyConfig.cpp b/tools/tidy/src/TidyConfig.cpp index b3413d1f3..13066e06e 100644 --- a/tools/tidy/src/TidyConfig.cpp +++ b/tools/tidy/src/TidyConfig.cpp @@ -27,6 +27,7 @@ TidyConfig::TidyConfig() { styleChecks.emplace("NoDotStarInPortConnection", CheckStatus::ENABLED); styleChecks.emplace("NoImplicitPortNameInPortConnection", CheckStatus::ENABLED); styleChecks.emplace("AlwaysCombBlockNamed", CheckStatus::ENABLED); + styleChecks.emplace("GenerateNamed", CheckStatus::ENABLED); checkKinds.insert({slang::TidyKind::Style, styleChecks}); auto synthesisChecks = std::unordered_map(); diff --git a/tools/tidy/src/style/GenerateNamed.cpp b/tools/tidy/src/style/GenerateNamed.cpp new file mode 100644 index 000000000..14f6ad471 --- /dev/null +++ b/tools/tidy/src/style/GenerateNamed.cpp @@ -0,0 +1,63 @@ +// SPDX-FileCopyrightText: Michael Popoloski +// SPDX-License-Identifier: MIT + +#include "ASTHelperVisitors.h" +#include "TidyDiags.h" +#include "fmt/color.h" + +#include "slang/syntax/AllSyntax.h" + +using namespace slang; +using namespace slang::ast; + +namespace generate_named { +struct MainVisitor : public TidyVisitor, ASTVisitor { + explicit MainVisitor(Diagnostics& diagnostics) : TidyVisitor(diagnostics) {} + + void handle(const GenerateBlockSymbol& symbol) { + + NEEDS_SKIP_SYMBOL(symbol) + + if (!symbol.getParentScope()) + return; + + auto& parSymbol = symbol.getParentScope()->asSymbol(); + + bool isUnnamed = symbol.name.empty(); + if (parSymbol.kind == SymbolKind::GenerateBlockArray) { + if (symbol.constructIndex != 0) + return; + isUnnamed = parSymbol.name.empty(); + } + + if (isUnnamed) { + diags.add(diag::GenerateNamed, symbol.location); + } + } +}; +} // namespace generate_named + +using namespace generate_named; +class GenerateNamed : public TidyCheck { +public: + [[maybe_unused]] explicit GenerateNamed(TidyKind kind) : TidyCheck(kind) {} + + bool check(const ast::RootSymbol& root) override { + MainVisitor visitor(diagnostics); + root.visit(visitor); + if (!diagnostics.empty()) + return false; + return true; + } + + DiagCode diagCode() const override { return diag::GenerateNamed; } + DiagnosticSeverity diagSeverity() const override { return DiagnosticSeverity::Warning; } + std::string diagString() const override { return "definition of an unnamed generate block"; } + std::string name() const override { return "GenerateNamed"; } + std::string description() const override { return shortDescription(); } + std::string shortDescription() const override { + return "Enforces that each generate block is named"; + } +}; + +REGISTER(GenerateNamed, GenerateNamed, TidyKind::Style) diff --git a/tools/tidy/tests/CMakeLists.txt b/tools/tidy/tests/CMakeLists.txt index 205404112..daf45888b 100644 --- a/tools/tidy/tests/CMakeLists.txt +++ b/tools/tidy/tests/CMakeLists.txt @@ -22,6 +22,7 @@ add_executable( NoDotStarInPortConnectionTest.cpp NoImplicitPortNameInPortConnectionTest.cpp AlwaysCombNamedTest.cpp + GenerateNamedTest.cpp ) target_link_libraries(tidy_unittests PRIVATE Catch2::Catch2 slang_tidy_obj_lib) diff --git a/tools/tidy/tests/GenerateNamedTest.cpp b/tools/tidy/tests/GenerateNamedTest.cpp new file mode 100644 index 000000000..ff58a8f46 --- /dev/null +++ b/tools/tidy/tests/GenerateNamedTest.cpp @@ -0,0 +1,178 @@ +// SPDX-FileCopyrightText: Michael Popoloski +// SPDX-License-Identifier: MIT + +#include "Test.h" +#include "TidyFactory.h" + +TEST_CASE("GenerateNamed: Unnamed generate block") { + auto tree = SyntaxTree::fromText(R"( +module eq_n + #( parameter N =4) + ( + input logic [N -1:0] a , b , + output logic eq + ) ; + logic [N -1:0] tmp ; + generate begin + genvar i ; + for ( i = 0; i < N ; i = i + 1) + xnor gen_u ( tmp [ i ] , a [ i ] , b [ i ]) ; + end + endgenerate + + assign eq = & tmp ; +endmodule + +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("GenerateNamed"); + bool result = visitor->check(root); + CHECK_FALSE(result); +} + +TEST_CASE("GenerateNamed: Named generate block") { + auto tree = SyntaxTree::fromText(R"( +module eq_n + #( parameter N =4) + ( + input logic [N -1:0] a , b , + output logic eq + ) ; + logic [N -1:0] tmp ; + generate begin : gen_named + genvar i ; + for ( i = 0; i < N ; i = i + 1) + xnor gen_u ( tmp [ i ] , a [ i ] , b [ i ]) ; + end + endgenerate + + assign eq = & tmp ; +endmodule + +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("GenerateNamed"); + bool result = visitor->check(root); + CHECK(result); +} + +TEST_CASE("GenerateNamed: Unnamed simple generate block") { + auto tree = SyntaxTree::fromText(R"( +module eq_n + #( parameter N =4) + ( + input logic [N -1:0] a , b , + output logic eq + ) ; + logic [N -1:0] tmp ; + generate + genvar i ; + for ( i = 0; i < N ; i = i + 1) + xnor gen_u ( tmp [ i ] , a [ i ] , b [ i ]) ; + endgenerate + + assign eq = & tmp ; +endmodule + +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("GenerateNamed"); + bool result = visitor->check(root); + CHECK_FALSE(result); +} + +TEST_CASE("GenerateNamed: Unnamed for block") { + auto tree = SyntaxTree::fromText(R"( +module full_adder( + input a, b, cin, + output sum, cout +); + + assign {sum, cout} = {a^b^cin, ((a & b) | (b & cin) | (a & cin))}; +endmodule + +module ripple_carry_adder #(parameter SIZE = 4) ( + input [SIZE-1:0] A, B, + input Cin, + output [SIZE-1:0] S, Cout); + + full_adder fa0(A[0], B[0], Cin, S[0], Cout[0]); + + for(genvar g = 1; gcheck(root); + CHECK_FALSE(result); +} + +TEST_CASE("GenerateNamed: Named for block") { + auto tree = SyntaxTree::fromText(R"( +module full_adder( + input a, b, cin, + output sum, cout +); + + assign {sum, cout} = {a^b^cin, ((a & b) | (b & cin) | (a & cin))}; +endmodule + +module ripple_carry_adder #(parameter SIZE = 4) ( + input [SIZE-1:0] A, B, + input Cin, + output [SIZE-1:0] S, Cout); + + full_adder fa0(A[0], B[0], Cin, S[0], Cout[0]); + + for(genvar g = 1; gcheck(root); + CHECK(result); +} From 7cf2f959d56062e7d2e05a2893d7ae6878f7de98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joel=20Sol=C3=A9=20Casal=C3=A9?= Date: Wed, 26 Jun 2024 15:52:03 +0200 Subject: [PATCH 03/11] New slang-tidy check: NoDotVarInPortConnection This check ensures that no module instantiation uses the sintax '.var' in the port connection, and ensures that '.var(var)' is used instead --- tools/tidy/CMakeLists.txt | 1 + tools/tidy/include/TidyDiags.h | 1 + tools/tidy/src/TidyConfig.cpp | 1 + .../src/style/NoDotVarInPortConnection.cpp | 75 ++++++++++++++++++ tools/tidy/tests/CMakeLists.txt | 1 + .../tests/NoDotVarInPortConnectionTest.cpp | 77 +++++++++++++++++++ 6 files changed, 156 insertions(+) create mode 100644 tools/tidy/src/style/NoDotVarInPortConnection.cpp create mode 100644 tools/tidy/tests/NoDotVarInPortConnectionTest.cpp diff --git a/tools/tidy/CMakeLists.txt b/tools/tidy/CMakeLists.txt index 3e4e4d29f..4a22273af 100644 --- a/tools/tidy/CMakeLists.txt +++ b/tools/tidy/CMakeLists.txt @@ -23,6 +23,7 @@ add_library( src/style/NoImplicitPortNameInPortConnection.cpp src/style/AlwaysCombNamed.cpp src/style/GenerateNamed.cpp + src/style/NoDotVarInPortConnection.cpp ) target_include_directories(slang_tidy_obj_lib PUBLIC include ../../include) diff --git a/tools/tidy/include/TidyDiags.h b/tools/tidy/include/TidyDiags.h index 8bfe0d4fc..77bd921c1 100644 --- a/tools/tidy/include/TidyDiags.h +++ b/tools/tidy/include/TidyDiags.h @@ -26,5 +26,6 @@ inline constexpr DiagCode NoDotStarInPortConnection(DiagSubsystem::Tidy, 11); inline constexpr DiagCode NoImplicitPortNameInPortConnection(DiagSubsystem::Tidy, 12); inline constexpr DiagCode AlwaysCombBlockNamed(DiagSubsystem::Tidy, 13); inline constexpr DiagCode GenerateNamed(DiagSubsystem::Tidy, 14); +inline constexpr DiagCode NoDotVarInPortConnection(DiagSubsystem::Tidy, 15); } // namespace slang::diag diff --git a/tools/tidy/src/TidyConfig.cpp b/tools/tidy/src/TidyConfig.cpp index 13066e06e..834fde09b 100644 --- a/tools/tidy/src/TidyConfig.cpp +++ b/tools/tidy/src/TidyConfig.cpp @@ -28,6 +28,7 @@ TidyConfig::TidyConfig() { styleChecks.emplace("NoImplicitPortNameInPortConnection", CheckStatus::ENABLED); styleChecks.emplace("AlwaysCombBlockNamed", CheckStatus::ENABLED); styleChecks.emplace("GenerateNamed", CheckStatus::ENABLED); + styleChecks.emplace("NoDotVarInPortConnection", CheckStatus::ENABLED); checkKinds.insert({slang::TidyKind::Style, styleChecks}); auto synthesisChecks = std::unordered_map(); diff --git a/tools/tidy/src/style/NoDotVarInPortConnection.cpp b/tools/tidy/src/style/NoDotVarInPortConnection.cpp new file mode 100644 index 000000000..71689cee2 --- /dev/null +++ b/tools/tidy/src/style/NoDotVarInPortConnection.cpp @@ -0,0 +1,75 @@ +// SPDX-FileCopyrightText: Michael Popoloski +// SPDX-License-Identifier: MIT + +#include "ASTHelperVisitors.h" +#include "TidyDiags.h" + +#include "slang/syntax/AllSyntax.h" +#include "slang/syntax/SyntaxVisitor.h" + +using namespace slang; +using namespace slang::ast; +using namespace slang::syntax; + +namespace no_dot_var_in_port_connection { + +struct PortConnectionVisitor : public SyntaxVisitor { + void handle(const NamedPortConnectionSyntax& port) { + if (!port.expr) + foundPorts.push_back(&port); + } + +public: + std::vector foundPorts; +}; + +struct MainVisitor : public TidyVisitor, ASTVisitor { + explicit MainVisitor(Diagnostics& diagnostics) : TidyVisitor(diagnostics) {} + + void handle(const InstanceBodySymbol& symbol) { + NEEDS_SKIP_SYMBOL(symbol) + if (!symbol.getSyntax()) + return; + + PortConnectionVisitor visitor; + symbol.getSyntax()->visit(visitor); + + for (const auto port : visitor.foundPorts) + diags.add(diag::NoDotVarInPortConnection, port->name.location()) + << port->toString() << port->toString() << port->name.valueText(); + } +}; +} // namespace no_dot_var_in_port_connection + +using namespace no_dot_var_in_port_connection; + +class NoDotVarInPortConnection : public TidyCheck { +public: + [[maybe_unused]] explicit NoDotVarInPortConnection(TidyKind kind) : TidyCheck(kind) {} + + bool check(const RootSymbol& root) override { + MainVisitor visitor(diagnostics); + root.visit(visitor); + if (!diagnostics.empty()) + return false; + return true; + } + + DiagCode diagCode() const override { return diag::NoDotVarInPortConnection; } + + std::string diagString() const override { + return "use of '{}' in port connection list, consider using '{}({})' instead"; + } + + DiagnosticSeverity diagSeverity() const override { return DiagnosticSeverity::Warning; } + + std::string name() const override { return "NoDotVarInPortConnection"; } + + std::string description() const override { return shortDescription(); } + + std::string shortDescription() const override { + return "Checks if in a module instantiation any port is connected using .var syntax."; + } +}; + +REGISTER(NoDotVarInPortConnection, NoDotVarInPortConnection, TidyKind::Style) diff --git a/tools/tidy/tests/CMakeLists.txt b/tools/tidy/tests/CMakeLists.txt index daf45888b..1456e77ce 100644 --- a/tools/tidy/tests/CMakeLists.txt +++ b/tools/tidy/tests/CMakeLists.txt @@ -23,6 +23,7 @@ add_executable( NoImplicitPortNameInPortConnectionTest.cpp AlwaysCombNamedTest.cpp GenerateNamedTest.cpp + NoDotVarInPortConnectionTest.cpp ) target_link_libraries(tidy_unittests PRIVATE Catch2::Catch2 slang_tidy_obj_lib) diff --git a/tools/tidy/tests/NoDotVarInPortConnectionTest.cpp b/tools/tidy/tests/NoDotVarInPortConnectionTest.cpp new file mode 100644 index 000000000..612f13491 --- /dev/null +++ b/tools/tidy/tests/NoDotVarInPortConnectionTest.cpp @@ -0,0 +1,77 @@ +// SPDX-FileCopyrightText: Michael Popoloski +// SPDX-License-Identifier: MIT + +#include "Test.h" +#include "TidyFactory.h" + +TEST_CASE("NoDotVarInPortConnection: Use of dot var in module port connection") { + auto tree = SyntaxTree::fromText(R"( +module test (input clk, input rst); +endmodule + +module top (); + logic clk, rst; + test t (.clk(clk), .rst); +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("NoDotVarInPortConnection"); + bool result = visitor->check(root); + CHECK_FALSE(result); +} + +TEST_CASE("NoDotVarInPortConnection: Module port connection port by port") { + auto tree = SyntaxTree::fromText(R"( +module test (input clk, input rst); +endmodule + +module top (); + logic clk, rst; + test t (.clk(clk), .rst(rst)); +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("NoDotVarInPortConnection"); + bool result = visitor->check(root); + CHECK(result); +} + +TEST_CASE("NoDotVarInPortConnection: Use of dot star in module port connection") { + auto tree = SyntaxTree::fromText(R"( +module test (input clk, input rst); +endmodule + +module top (); + logic clk, rst; + test t (.*); +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("NoDotVarInPortConnection"); + bool result = visitor->check(root); + CHECK(result); +} From 9163a7454ba80ddbf5f6e26b2cb887631a8bde5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joel=20Sol=C3=A9=20Casal=C3=A9?= Date: Wed, 26 Jun 2024 15:58:29 +0200 Subject: [PATCH 04/11] Fix in the warning location of a check Now, when '.*' is used with the NoDotStarInPortConnection check enabled, the warning will be located where the '.*' character is, instead of the location of the module that was being called. --- tools/tidy/src/style/NoDotStarInPortConnection.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/tidy/src/style/NoDotStarInPortConnection.cpp b/tools/tidy/src/style/NoDotStarInPortConnection.cpp index cbd3e090f..226822a76 100644 --- a/tools/tidy/src/style/NoDotStarInPortConnection.cpp +++ b/tools/tidy/src/style/NoDotStarInPortConnection.cpp @@ -14,10 +14,10 @@ using namespace slang::syntax; namespace no_dot_start_in_port_connection { struct PortConnectionVisitor : public SyntaxVisitor { - void handle(const WildcardPortConnectionSyntax&) { found = true; } + void handle(const WildcardPortConnectionSyntax& port) { foundPorts.push_back(&port); } public: - bool found{false}; + std::vector foundPorts; }; struct MainVisitor : public TidyVisitor, ASTVisitor { @@ -27,8 +27,8 @@ struct MainVisitor : public TidyVisitor, ASTVisitor { NEEDS_SKIP_SYMBOL(symbol) PortConnectionVisitor visitor; symbol.getSyntax()->visit(visitor); - if (visitor.found) - diags.add(diag::NoDotStarInPortConnection, symbol.location); + for (const auto port : visitor.foundPorts) + diags.add(diag::NoDotStarInPortConnection, port->star.location()); } }; } // namespace no_dot_start_in_port_connection From a9500687985f60d6407c3387a1c6caaf380d42dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joel=20Sol=C3=A9=20Casal=C3=A9?= Date: Wed, 26 Jun 2024 16:03:05 +0200 Subject: [PATCH 05/11] New slang-tidy check: NoLegacyGenerate This new check enforces that no generate block is used in the code, since it is an old system verilog syntax --- tools/tidy/CMakeLists.txt | 1 + tools/tidy/include/TidyDiags.h | 1 + tools/tidy/src/TidyConfig.cpp | 1 + tools/tidy/src/style/NoLegacyGenerate.cpp | 65 +++++++++++ tools/tidy/tests/CMakeLists.txt | 1 + tools/tidy/tests/NoLegacyGenerateTest.cpp | 128 ++++++++++++++++++++++ 6 files changed, 197 insertions(+) create mode 100644 tools/tidy/src/style/NoLegacyGenerate.cpp create mode 100644 tools/tidy/tests/NoLegacyGenerateTest.cpp diff --git a/tools/tidy/CMakeLists.txt b/tools/tidy/CMakeLists.txt index 4a22273af..42f8ed2a6 100644 --- a/tools/tidy/CMakeLists.txt +++ b/tools/tidy/CMakeLists.txt @@ -24,6 +24,7 @@ add_library( src/style/AlwaysCombNamed.cpp src/style/GenerateNamed.cpp src/style/NoDotVarInPortConnection.cpp + src/style/NoLegacyGenerate.cpp ) target_include_directories(slang_tidy_obj_lib PUBLIC include ../../include) diff --git a/tools/tidy/include/TidyDiags.h b/tools/tidy/include/TidyDiags.h index 77bd921c1..8a636c924 100644 --- a/tools/tidy/include/TidyDiags.h +++ b/tools/tidy/include/TidyDiags.h @@ -27,5 +27,6 @@ inline constexpr DiagCode NoImplicitPortNameInPortConnection(DiagSubsystem::Tidy inline constexpr DiagCode AlwaysCombBlockNamed(DiagSubsystem::Tidy, 13); inline constexpr DiagCode GenerateNamed(DiagSubsystem::Tidy, 14); inline constexpr DiagCode NoDotVarInPortConnection(DiagSubsystem::Tidy, 15); +inline constexpr DiagCode NoLegacyGenerate(DiagSubsystem::Tidy, 16); } // namespace slang::diag diff --git a/tools/tidy/src/TidyConfig.cpp b/tools/tidy/src/TidyConfig.cpp index 834fde09b..1ae5680c5 100644 --- a/tools/tidy/src/TidyConfig.cpp +++ b/tools/tidy/src/TidyConfig.cpp @@ -29,6 +29,7 @@ TidyConfig::TidyConfig() { styleChecks.emplace("AlwaysCombBlockNamed", CheckStatus::ENABLED); styleChecks.emplace("GenerateNamed", CheckStatus::ENABLED); styleChecks.emplace("NoDotVarInPortConnection", CheckStatus::ENABLED); + styleChecks.emplace("NoLegacyGenerate", CheckStatus::ENABLED); checkKinds.insert({slang::TidyKind::Style, styleChecks}); auto synthesisChecks = std::unordered_map(); diff --git a/tools/tidy/src/style/NoLegacyGenerate.cpp b/tools/tidy/src/style/NoLegacyGenerate.cpp new file mode 100644 index 000000000..2a880f941 --- /dev/null +++ b/tools/tidy/src/style/NoLegacyGenerate.cpp @@ -0,0 +1,65 @@ +// SPDX-FileCopyrightText: Michael Popoloski +// SPDX-License-Identifier: MIT + +#include "ASTHelperVisitors.h" +#include "TidyDiags.h" +#include "fmt/color.h" + +#include "slang/syntax/AllSyntax.h" +#include "slang/syntax/SyntaxVisitor.h" + +using namespace slang; +using namespace slang::ast; +using namespace slang::syntax; + +namespace no_legacy_generate { + +struct GenerateVisitor : public SyntaxVisitor { + void handle(const GenerateRegionSyntax& syntax) { foundGenerate.push_back(&syntax); } + +public: + std::vector foundGenerate; +}; + +struct MainVisitor : public TidyVisitor, ASTVisitor { + explicit MainVisitor(Diagnostics& diagnostics) : TidyVisitor(diagnostics) {} + + void handle(const InstanceBodySymbol& symbol) { + NEEDS_SKIP_SYMBOL(symbol) + if (!symbol.getSyntax()) + return; + + GenerateVisitor visitor; + symbol.getSyntax()->visit(visitor); + + for (const auto& syntax : visitor.foundGenerate) { + diags.add(diag::NoLegacyGenerate, syntax->keyword.location()); + } + } +}; +} // namespace no_legacy_generate + +using namespace no_legacy_generate; +class NoLegacyGenerate : public TidyCheck { +public: + [[maybe_unused]] explicit NoLegacyGenerate(TidyKind kind) : TidyCheck(kind) {} + + bool check(const ast::RootSymbol& root) override { + MainVisitor visitor(diagnostics); + root.visit(visitor); + if (!diagnostics.empty()) + return false; + return true; + } + + DiagCode diagCode() const override { return diag::NoLegacyGenerate; } + DiagnosticSeverity diagSeverity() const override { return DiagnosticSeverity::Warning; } + std::string diagString() const override { return "usage of generate block is deprecated"; } + std::string name() const override { return "NoLegacyGenerate"; } + std::string description() const override { return shortDescription(); } + std::string shortDescription() const override { + return "Enforces that no generate block is declared since it is deprecated"; + } +}; + +REGISTER(NoLegacyGenerate, NoLegacyGenerate, TidyKind::Style) diff --git a/tools/tidy/tests/CMakeLists.txt b/tools/tidy/tests/CMakeLists.txt index 1456e77ce..9dc38f2b2 100644 --- a/tools/tidy/tests/CMakeLists.txt +++ b/tools/tidy/tests/CMakeLists.txt @@ -24,6 +24,7 @@ add_executable( AlwaysCombNamedTest.cpp GenerateNamedTest.cpp NoDotVarInPortConnectionTest.cpp + NoLegacyGenerateTest.cpp ) target_link_libraries(tidy_unittests PRIVATE Catch2::Catch2 slang_tidy_obj_lib) diff --git a/tools/tidy/tests/NoLegacyGenerateTest.cpp b/tools/tidy/tests/NoLegacyGenerateTest.cpp new file mode 100644 index 000000000..c7ec41654 --- /dev/null +++ b/tools/tidy/tests/NoLegacyGenerateTest.cpp @@ -0,0 +1,128 @@ +// SPDX-FileCopyrightText: Michael Popoloski +// SPDX-License-Identifier: MIT + +#include "Test.h" +#include "TidyFactory.h" + +TEST_CASE("NoLegacyGenerate: For loop inside generate block") { + auto tree = SyntaxTree::fromText(R"( +module eq_n + #( parameter N =4) + ( + input logic [N -1:0] a , b , + output logic eq + ) ; + logic [N -1:0] tmp ; + generate begin : named_generate + genvar i ; + for ( i = 0; i < N ; i = i + 1) + xnor gen_u ( tmp [ i ] , a [ i ] , b [ i ]) ; + end + endgenerate +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("NoLegacyGenerate"); + bool result = visitor->check(root); + CHECK_FALSE(result); +} + +TEST_CASE("NoLegacyGenerate: For loop inside unnamed generate block") { + auto tree = SyntaxTree::fromText(R"( +module eq_n + #( parameter N =4) + ( + input logic [N -1:0] a , b , + output logic eq + ) ; + logic [N -1:0] tmp ; + generate + genvar i ; + for ( i = 0; i < N ; i = i + 1) + xnor gen_u ( tmp [ i ] , a [ i ] , b [ i ]) ; + endgenerate +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("NoLegacyGenerate"); + bool result = visitor->check(root); + CHECK_FALSE(result); +} + +TEST_CASE("NoLegacyGenerate: For loop, no generate block") { + auto tree = SyntaxTree::fromText(R"( +module eq_n + #( parameter N =4) + ( + input logic [N -1:0] a , b , + output logic eq + ) ; + logic [N -1:0] tmp ; + for (genvar i = 0; i < N ; i = i + 1) + xnor gen_u ( tmp [ i ] , a [ i ] , b [ i ]) ; +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("NoLegacyGenerate"); + bool result = visitor->check(root); + CHECK(result); +} + +TEST_CASE("NoLegacyGenerate: For loop inside conditional block") { + auto tree = SyntaxTree::fromText(R"( +module eq_n + #( + parameter N = 4, + parameter cond = 0 + ) + ( + input logic [N -1:0] a , b , + output logic eq + ) ; + logic [N -1:0] tmp ; + if (cond) begin + for (genvar i = 0; i < N ; i = i + 1) begin : foo_name + xnor gen_u ( tmp [ i ] , a [ i ] , b [ i ]) ; + end + end + +endmodule + +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("NoLegacyGenerate"); + bool result = visitor->check(root); + CHECK(result); +} From debdf3c18ccab6cb607a086b4e02467754d50d7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joel=20Sol=C3=A9=20Casal=C3=A9?= Date: Wed, 26 Jun 2024 16:53:33 +0200 Subject: [PATCH 06/11] New check: AlwaysFFAssignmentOusideConditional This new synthesis check ensures that if there is a conditional block with reset, all the assignments must be either inside the if or else statements. --- tools/tidy/CMakeLists.txt | 1 + tools/tidy/include/TidyDiags.h | 1 + tools/tidy/src/TidyConfig.cpp | 1 + .../AlwaysFFAssignmentOutsideConditional.cpp | 111 ++++++++++++++++++ ...waysFFAssignmentOutsideConditionalTest.cpp | 67 +++++++++++ tools/tidy/tests/CMakeLists.txt | 1 + 6 files changed, 182 insertions(+) create mode 100644 tools/tidy/src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp create mode 100644 tools/tidy/tests/AlwaysFFAssignmentOutsideConditionalTest.cpp diff --git a/tools/tidy/CMakeLists.txt b/tools/tidy/CMakeLists.txt index 42f8ed2a6..5239924a1 100644 --- a/tools/tidy/CMakeLists.txt +++ b/tools/tidy/CMakeLists.txt @@ -25,6 +25,7 @@ add_library( src/style/GenerateNamed.cpp src/style/NoDotVarInPortConnection.cpp src/style/NoLegacyGenerate.cpp + src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp ) target_include_directories(slang_tidy_obj_lib PUBLIC include ../../include) diff --git a/tools/tidy/include/TidyDiags.h b/tools/tidy/include/TidyDiags.h index 8a636c924..a9aa09c18 100644 --- a/tools/tidy/include/TidyDiags.h +++ b/tools/tidy/include/TidyDiags.h @@ -28,5 +28,6 @@ inline constexpr DiagCode AlwaysCombBlockNamed(DiagSubsystem::Tidy, 13); inline constexpr DiagCode GenerateNamed(DiagSubsystem::Tidy, 14); inline constexpr DiagCode NoDotVarInPortConnection(DiagSubsystem::Tidy, 15); inline constexpr DiagCode NoLegacyGenerate(DiagSubsystem::Tidy, 16); +inline constexpr DiagCode AlwaysFFAssignmentOutsideConditional(DiagSubsystem::Tidy, 17); } // namespace slang::diag diff --git a/tools/tidy/src/TidyConfig.cpp b/tools/tidy/src/TidyConfig.cpp index 1ae5680c5..6e2f629be 100644 --- a/tools/tidy/src/TidyConfig.cpp +++ b/tools/tidy/src/TidyConfig.cpp @@ -38,6 +38,7 @@ TidyConfig::TidyConfig() { synthesisChecks.emplace("RegisterHasNoReset", CheckStatus::ENABLED); synthesisChecks.emplace("XilinxDoNotCareValues", CheckStatus::ENABLED); synthesisChecks.emplace("CastSignedIndex", CheckStatus::ENABLED); + synthesisChecks.emplace("AlwaysFFAssignmentOutsideConditional", CheckStatus::ENABLED); checkKinds.insert({slang::TidyKind::Synthesis, synthesisChecks}); } diff --git a/tools/tidy/src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp b/tools/tidy/src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp new file mode 100644 index 000000000..b389a0ee0 --- /dev/null +++ b/tools/tidy/src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp @@ -0,0 +1,111 @@ +// SPDX-FileCopyrightText: Michael Popoloski +// SPDX-License-Identifier: MIT + +#include "ASTHelperVisitors.h" +#include "TidyDiags.h" +#include "TidyFactory.h" +#include "fmt/color.h" + +#include "slang/syntax/AllSyntax.h" + +using namespace slang; +using namespace slang::ast; + +namespace always_ff_assignment_outside_conditional { +struct AlwaysFFVisitor : public ASTVisitor { + explicit AlwaysFFVisitor(const std::string_view name, const std::string_view resetName) : + name(name), resetName(resetName){}; + + void handle(const ConditionalStatement& statement) { + // Early return, if there's no else clause on the conditional statement + if (!statement.ifFalse) { + return; + } + + // Collect all the identifiers in the conditions + CollectIdentifiers collectIdentifiersVisitor; + for (const auto& condition : statement.conditions) { + condition.expr->visit(collectIdentifiersVisitor); + } + + // Check if one of the identifiers is a reset + const auto isReset = + std::ranges::any_of(collectIdentifiersVisitor.identifiers, [this](auto id) { + return id.find(resetName) != std::string_view::npos; + }); + + condStatmenentWithReset = isReset; + } + + void handle(const AssignmentExpression& expression) { + if (LookupLhsIdentifier::hasIdentifier(name, expression)) { + assignedOutsideIfWithReset = true; + errorLocation = expression.left().syntax->getFirstToken().location(); + } + } + + bool hasError() { return condStatmenentWithReset && assignedOutsideIfWithReset; } + + const std::string_view name; + const std::string_view resetName; + bool condStatmenentWithReset = false; + bool assignedOutsideIfWithReset = false; + SourceLocation errorLocation = SourceLocation(); +}; + +struct MainVisitor : public TidyVisitor, ASTVisitor { + explicit MainVisitor(Diagnostics& diagnostics) : TidyVisitor(diagnostics) {} + + void handle(const VariableSymbol& symbol) { + NEEDS_SKIP_SYMBOL(symbol) + if (symbol.drivers().empty()) + return; + + auto firstDriver = *symbol.drivers().begin(); + if (firstDriver && firstDriver->isInAlwaysFFBlock()) { + AlwaysFFVisitor visitor(symbol.name, config.getCheckConfigs().resetName); + firstDriver->containingSymbol->visit(visitor); + if (visitor.hasError()) { + diags.add(diag::RegisterNotAssignedOnReset, visitor.errorLocation) << symbol.name; + } + } + } +}; +} // namespace always_ff_assignment_outside_conditional + +using namespace always_ff_assignment_outside_conditional; + +class AlwaysFFAssignmentOutsideConditional : public TidyCheck { +public: + explicit AlwaysFFAssignmentOutsideConditional(TidyKind kind) : TidyCheck(kind) {} + + bool check(const RootSymbol& root) override { + MainVisitor visitor(diagnostics); + root.visit(visitor); + if (!diagnostics.empty()) + return false; + return true; + } + + DiagCode diagCode() const override { return diag::RegisterNotAssignedOnReset; } + + std::string diagString() const override { + return "register '{}' has an assignment outside a conditional block with reset. Consider " + "moving the register to an always_ff that has no reset or moving the assignment " + "inside the conditional block"; + } + + DiagnosticSeverity diagSeverity() const override { return DiagnosticSeverity::Warning; } + + std::string name() const override { return "AlwaysFFAssignmentOutsideConditional"; } + + std::string description() const override { return shortDescription(); } + + std::string shortDescription() const override { + return "A register in an always_ff with an assignment outside a conditional block with a " + "reset signal on its sensitivity list"; + } +}; + +REGISTER(AlwaysFFAssignmentOutsideConditional, AlwaysFFAssignmentOutsideConditional, + TidyKind::Synthesis) diff --git a/tools/tidy/tests/AlwaysFFAssignmentOutsideConditionalTest.cpp b/tools/tidy/tests/AlwaysFFAssignmentOutsideConditionalTest.cpp new file mode 100644 index 000000000..8c21d03f6 --- /dev/null +++ b/tools/tidy/tests/AlwaysFFAssignmentOutsideConditionalTest.cpp @@ -0,0 +1,67 @@ +// SPDX-FileCopyrightText: Michael Popoloski +// SPDX-License-Identifier: MIT + +#include "Test.h" +#include "TidyFactory.h" + +TEST_CASE("AlwaysFFAssignmentOutsideConditional: Assignment inside always_ff and outside if " + "statement with reset") { + auto tree = SyntaxTree::fromText(R"( +module top; + logic clk_i; + logic rst_ni; + logic foo, bar; + int a; + + always_ff @(posedge clk_i or negedge rst_ni) begin + foo <= bar; + if(rst_ni) a <= '0; + else a <= a +1; + end +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("AlwaysFFAssignmentOutsideConditional"); + bool result = visitor->check(root); + CHECK_FALSE(result); +} + +TEST_CASE("AlwaysFFAssignmentOutsideConditional: All assignments inside either if or else " + "statements inside the always_ff block") { + auto tree = SyntaxTree::fromText(R"( +module top; + logic clk_i; + logic rst_ni; + logic a, b; + + always_ff @(posedge clk_i or negedge rst_ni) begin + if (~rst_ni) begin + a <= '0; + end else begin + a <= 1'b1; + b <= 1'b1; + end + end +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("AlwaysFFAssignmentOutsideConditional"); + bool result = visitor->check(root); + CHECK(result); +} diff --git a/tools/tidy/tests/CMakeLists.txt b/tools/tidy/tests/CMakeLists.txt index 9dc38f2b2..5cf2aedbf 100644 --- a/tools/tidy/tests/CMakeLists.txt +++ b/tools/tidy/tests/CMakeLists.txt @@ -25,6 +25,7 @@ add_executable( GenerateNamedTest.cpp NoDotVarInPortConnectionTest.cpp NoLegacyGenerateTest.cpp + AlwaysFFAssignmentOutsideConditionalTest.cpp ) target_link_libraries(tidy_unittests PRIVATE Catch2::Catch2 slang_tidy_obj_lib) From e486634310c5f08ba3927fe656aa53ba94dd97c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joel=20Sol=C3=A9=20Casal=C3=A9?= Date: Mon, 1 Jul 2024 14:26:24 +0200 Subject: [PATCH 07/11] New slang-tidy check: UnusedSensitiveSignal This new check verifies that every signal inside the sensitivity list of an 'always' or 'always_ff' block is used inside the block statement. The only exception is the clock signal --- tools/tidy/CMakeLists.txt | 1 + tools/tidy/include/ASTHelperVisitors.h | 63 +++++++++++-- tools/tidy/include/TidyDiags.h | 1 + tools/tidy/src/ASTHelperVisitors.cpp | 6 ++ tools/tidy/src/TidyConfig.cpp | 1 + .../src/synthesis/UnusedSensitiveSignal.cpp | 92 +++++++++++++++++++ tools/tidy/tests/CMakeLists.txt | 1 + .../tidy/tests/UnusedSensitiveSignalTest.cpp | 92 +++++++++++++++++++ 8 files changed, 251 insertions(+), 6 deletions(-) create mode 100644 tools/tidy/src/synthesis/UnusedSensitiveSignal.cpp create mode 100644 tools/tidy/tests/UnusedSensitiveSignalTest.cpp diff --git a/tools/tidy/CMakeLists.txt b/tools/tidy/CMakeLists.txt index 5239924a1..7be0a4c77 100644 --- a/tools/tidy/CMakeLists.txt +++ b/tools/tidy/CMakeLists.txt @@ -26,6 +26,7 @@ add_library( src/style/NoDotVarInPortConnection.cpp src/style/NoLegacyGenerate.cpp src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp + src/synthesis/UnusedSensitiveSignal.cpp ) target_include_directories(slang_tidy_obj_lib PUBLIC include ../../include) diff --git a/tools/tidy/include/ASTHelperVisitors.h b/tools/tidy/include/ASTHelperVisitors.h index bb2b1c949..5951f11d5 100644 --- a/tools/tidy/include/ASTHelperVisitors.h +++ b/tools/tidy/include/ASTHelperVisitors.h @@ -20,6 +20,10 @@ // Function that tries to get the name of the variable in an expression std::optional getIdentifier(const slang::ast::Expression& expr); +// Function that tries to get the source location of an expression +std::optional getExpressionSourceLocation( + const slang::ast::Expression& expr); + struct TidyVisitor { protected: explicit TidyVisitor(slang::Diagnostics& diagnostics) : @@ -52,6 +56,46 @@ struct CollectIdentifiers : public slang::ast::ASTVisitor identifiers; }; +/// ASTVisitor that will try to find the provided name in the identifiers under a node +struct LookupIdentifier : public slang::ast::ASTVisitor { + explicit LookupIdentifier(const std::string_view& name, const bool exactMatching = true) : + name(name), exactMatching(exactMatching) {} + + void handle(const slang::ast::NamedValueExpression& expression) { + if (hasIdentifier(name, exactMatching, expression)) { + _found = true; + _foundLocation = getExpressionSourceLocation(expression); + } + } + + // Checks if the symbol reference of the expression has the provided name + static bool hasIdentifier(const std::string_view& name, const bool exactMatching, + const slang::ast::NamedValueExpression& expression) { + auto symbol = expression.getSymbolReference(); + + return symbol && exactMatching ? symbol->name == name + : symbol->name.find(name) != std::string_view::npos; + } + + // Retrieves whether the identifier has been found + [[nodiscard]] bool found() const { return _found; } + + // Retrieves the identifier location in case it has been found + std::optional foundLocation() const { return _foundLocation; } + + // Resets the state of the visitor, so it can be used again without having to create a new one + void reset() { + _found = false; + _foundLocation = {}; + } + +private: + const std::string_view name; + const bool exactMatching; + bool _found = false; + std::optional _foundLocation; +}; + /// ASTVisitor that will collect all LHS assignment symbols under a node struct CollectLHSSymbols : public slang::ast::ASTVisitor { void handle(const slang::ast::AssignmentExpression& expression) { @@ -67,7 +111,10 @@ struct LookupLhsIdentifier : public slang::ast::ASTVisitor foundLocation() const { return _foundLocation; } + // Resets the state of the visitor, so it can be used again without having to create a new one - void reset() { _found = false; } + void reset() { + _found = false; + _foundLocation = {}; + } private: const std::string_view name; bool _found = false; + std::optional _foundLocation; }; diff --git a/tools/tidy/include/TidyDiags.h b/tools/tidy/include/TidyDiags.h index a9aa09c18..442b81fb9 100644 --- a/tools/tidy/include/TidyDiags.h +++ b/tools/tidy/include/TidyDiags.h @@ -29,5 +29,6 @@ inline constexpr DiagCode GenerateNamed(DiagSubsystem::Tidy, 14); inline constexpr DiagCode NoDotVarInPortConnection(DiagSubsystem::Tidy, 15); inline constexpr DiagCode NoLegacyGenerate(DiagSubsystem::Tidy, 16); inline constexpr DiagCode AlwaysFFAssignmentOutsideConditional(DiagSubsystem::Tidy, 17); +inline constexpr DiagCode UnusedSensitiveSignal(DiagSubsystem::Tidy, 18); } // namespace slang::diag diff --git a/tools/tidy/src/ASTHelperVisitors.cpp b/tools/tidy/src/ASTHelperVisitors.cpp index 8b140d7fa..5937d22ef 100644 --- a/tools/tidy/src/ASTHelperVisitors.cpp +++ b/tools/tidy/src/ASTHelperVisitors.cpp @@ -2,6 +2,7 @@ // SPDX-License-Identifier: MIT #include "ASTHelperVisitors.h" +#include "slang/syntax/AllSyntax.h" std::optional getIdentifier(const slang::ast::Expression& expr) { const slang::ast::Symbol* symbol = nullptr; @@ -19,3 +20,8 @@ std::optional getIdentifier(const slang::ast::Expression& expr } return {}; } + +std::optional getExpressionSourceLocation( + const slang::ast::Expression& expr) { + return expr.syntax->getFirstToken().location(); +} diff --git a/tools/tidy/src/TidyConfig.cpp b/tools/tidy/src/TidyConfig.cpp index 6e2f629be..b28efc4b5 100644 --- a/tools/tidy/src/TidyConfig.cpp +++ b/tools/tidy/src/TidyConfig.cpp @@ -39,6 +39,7 @@ TidyConfig::TidyConfig() { synthesisChecks.emplace("XilinxDoNotCareValues", CheckStatus::ENABLED); synthesisChecks.emplace("CastSignedIndex", CheckStatus::ENABLED); synthesisChecks.emplace("AlwaysFFAssignmentOutsideConditional", CheckStatus::ENABLED); + synthesisChecks.emplace("UnusedSensitiveSignal", CheckStatus::ENABLED); checkKinds.insert({slang::TidyKind::Synthesis, synthesisChecks}); } diff --git a/tools/tidy/src/synthesis/UnusedSensitiveSignal.cpp b/tools/tidy/src/synthesis/UnusedSensitiveSignal.cpp new file mode 100644 index 000000000..3b58b6d59 --- /dev/null +++ b/tools/tidy/src/synthesis/UnusedSensitiveSignal.cpp @@ -0,0 +1,92 @@ +// SPDX-FileCopyrightText: Michael Popoloski +// SPDX-License-Identifier: MIT + +#include "ASTHelperVisitors.h" +#include "TidyDiags.h" +#include "fmt/color.h" +#include + +#include "slang/syntax/AllSyntax.h" + +using namespace slang; +using namespace slang::ast; + +namespace unused_sensitive_signal { + +struct MainVisitor : public TidyVisitor, ASTVisitor { + explicit MainVisitor(Diagnostics& diagnostics) : TidyVisitor(diagnostics) {} + + struct CollectAllIdentifiers + : public slang::ast::ASTVisitor { + void handle(const slang::ast::NamedValueExpression& expression) { + if (auto* symbol = expression.getSymbolReference(); symbol && expression.syntax) { + identifiers.push_back( + std::make_pair(symbol->name, getExpressionSourceLocation(expression).value())); + } + } + std::vector> identifiers; + }; + + void handle(const ProceduralBlockSymbol& block) { + NEEDS_SKIP_SYMBOL(block) + + if (block.procedureKind != ProceduralBlockKind::AlwaysFF && + block.procedureKind != ProceduralBlockKind::Always) + return; + + CollectAllIdentifiers stmtIdVisitor, timingIdVisitor; + block.getBody().as().stmt.as().visitStmts(stmtIdVisitor); + block.getBody().as().timing.visit(timingIdVisitor); + + auto compare = [](const auto& a, const auto& b) { return a.first < b.first; }; + + std::sort(timingIdVisitor.identifiers.begin(), timingIdVisitor.identifiers.end(), compare); + std::sort(stmtIdVisitor.identifiers.begin(), stmtIdVisitor.identifiers.end(), compare); + + std::vector> unusedSignals; + + std::set_difference(timingIdVisitor.identifiers.begin(), timingIdVisitor.identifiers.end(), + stmtIdVisitor.identifiers.begin(), stmtIdVisitor.identifiers.end(), + std::back_inserter(unusedSignals), compare); + + for (auto signal : unusedSignals) { + if (signal.first != config.getCheckConfigs().clkName) + diags.add(diag::UnusedSensitiveSignal, signal.second) << signal.first; + } + } +}; +} // namespace unused_sensitive_signal + +using namespace unused_sensitive_signal; + +class UnusedSensitiveSignal : public TidyCheck { +public: + [[maybe_unused]] explicit UnusedSensitiveSignal(TidyKind kind) : TidyCheck(kind) {} + + bool check(const RootSymbol& root) override { + MainVisitor visitor(diagnostics); + root.visit(visitor); + return diagnostics.empty(); + } + + DiagCode diagCode() const override { return diag::UnusedSensitiveSignal; } + + std::string diagString() const override { + return "the signal '{}' is in the sensitivity list of a procedural block but is never " + "referenced in the statement. Consider using the signal inside the procedural block " + "or removing it from the sensitivity list"; + } + + DiagnosticSeverity diagSeverity() const override { return DiagnosticSeverity::Warning; } + + std::string name() const override { return "UnusedSensitiveSignal"; } + + std::string description() const override { return shortDescription(); } + + std::string shortDescription() const override { + return "A signal inside the sensitivity list is never used in the statement of the " + "procedural block."; + } +}; + +REGISTER(UnusedSensitiveSignal, UnusedSensitiveSignal, TidyKind::Synthesis) diff --git a/tools/tidy/tests/CMakeLists.txt b/tools/tidy/tests/CMakeLists.txt index 5cf2aedbf..ba8338905 100644 --- a/tools/tidy/tests/CMakeLists.txt +++ b/tools/tidy/tests/CMakeLists.txt @@ -26,6 +26,7 @@ add_executable( NoDotVarInPortConnectionTest.cpp NoLegacyGenerateTest.cpp AlwaysFFAssignmentOutsideConditionalTest.cpp + UnusedSensitiveSignalTest.cpp ) target_link_libraries(tidy_unittests PRIVATE Catch2::Catch2 slang_tidy_obj_lib) diff --git a/tools/tidy/tests/UnusedSensitiveSignalTest.cpp b/tools/tidy/tests/UnusedSensitiveSignalTest.cpp new file mode 100644 index 000000000..3be7eee70 --- /dev/null +++ b/tools/tidy/tests/UnusedSensitiveSignalTest.cpp @@ -0,0 +1,92 @@ +// SPDX-FileCopyrightText: Michael Popoloski +// SPDX-License-Identifier: MIT + +#include "Test.h" +#include "TidyFactory.h" + +TEST_CASE("UnusedSensitiveSignal: Unused sensitive signal in always block") { + auto tree = SyntaxTree::fromText(R"( +module d_ff_en +( + input int a_i, b_i, c_i, + + output int sum_o +) ; +always @ (a_i , b_i, c_i ) begin + sum_o = a_i + b_i ; +end +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("UnusedSensitiveSignal"); + bool result = visitor->check(root); + CHECK_FALSE(result); +} + +TEST_CASE("UnusedSensitiveSignal: Unused sensitive signal in always_ff block") { + auto tree = SyntaxTree::fromText(R"( +module d_ff_en +( + input logic clk_i, rst_i, en_i, c_i, d_i, + + output logic q_o +) ; +always_ff @ (posedge clk_i, posedge rst_i, c_i) begin + if (rst_i) + q_o <= '0; + else if (en_i) + q_o <= d_i; + end +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("UnusedSensitiveSignal"); + bool result = visitor->check(root); + CHECK_FALSE(result); +} + +TEST_CASE("UnusedSensitiveSignal: All sensitive signal in always_ff block have been used") { + auto tree = SyntaxTree::fromText(R"( +module d_ff_en +( + input logic clk_i, rst_i, en_i, c_i, d_i, + + output logic q_o +) ; +always_ff @ (posedge clk_i, posedge rst_i) begin + if (rst_i) + q_o <= '0; + else if (en_i) + q_o <= d_i; + end +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("UnusedSensitiveSignal"); + bool result = visitor->check(root); + CHECK(result); +} \ No newline at end of file From aaddd1c0d8660f39b20ea8898a759b2c1636383c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joel=20Sol=C3=A9=20Casal=C3=A9?= Date: Mon, 1 Jul 2024 14:28:23 +0200 Subject: [PATCH 08/11] Fix slang-tidy synthesis checks * slang-tidy fix OnlyAssignedOnReset and RegisterHasNoReset: This commit will ensure that both checks will no longer be ignoring the configuration setting 'resetIsActiveHigh' and the possible negation of the reset signal. Two test cases have also been added to consider this bug. * The error location of the warnings generated by the OnlyAssignedOnReset and RegisterHasNoReset are now more precise and target the variable instance where the error is found instead of the variable declaration. --- .../AlwaysFFAssignmentOutsideConditional.cpp | 36 ++++----- .../src/synthesis/OnlyAssignedOnReset.cpp | 76 +++++++++++++------ .../tidy/src/synthesis/RegisterHasNoReset.cpp | 74 ++++++++++++------ tools/tidy/tests/OnlyAssignedOnResetTest.cpp | 36 +++++++++ tools/tidy/tests/RegisterHasNoResetTest.cpp | 34 +++++++++ 5 files changed, 193 insertions(+), 63 deletions(-) diff --git a/tools/tidy/src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp b/tools/tidy/src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp index b389a0ee0..ca0d13c11 100644 --- a/tools/tidy/src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp +++ b/tools/tidy/src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp @@ -22,35 +22,35 @@ struct AlwaysFFVisitor : public ASTVisitor { return; } - // Collect all the identifiers in the conditions - CollectIdentifiers collectIdentifiersVisitor; + // Check if there is a reset in the conditional statement + LookupIdentifier lookupIdentifierVisitor(resetName, false); for (const auto& condition : statement.conditions) { - condition.expr->visit(collectIdentifiersVisitor); - } - - // Check if one of the identifiers is a reset - const auto isReset = - std::ranges::any_of(collectIdentifiersVisitor.identifiers, [this](auto id) { - return id.find(resetName) != std::string_view::npos; - }); + condition.expr->visit(lookupIdentifierVisitor); - condStatmenentWithReset = isReset; + if (lookupIdentifierVisitor.found()) { + condStatmenentWithReset = true; + return; + } + } } void handle(const AssignmentExpression& expression) { if (LookupLhsIdentifier::hasIdentifier(name, expression)) { - assignedOutsideIfWithReset = true; - errorLocation = expression.left().syntax->getFirstToken().location(); + assignedOutsideIfReset = true; + errorLocation = getExpressionSourceLocation(expression); } } - bool hasError() { return condStatmenentWithReset && assignedOutsideIfWithReset; } + bool hasError() { return condStatmenentWithReset && assignedOutsideIfReset; } + + std::optional getErrorLocation() const { return errorLocation; } +private: const std::string_view name; const std::string_view resetName; bool condStatmenentWithReset = false; - bool assignedOutsideIfWithReset = false; - SourceLocation errorLocation = SourceLocation(); + bool assignedOutsideIfReset = false; + std::optional errorLocation; }; struct MainVisitor : public TidyVisitor, ASTVisitor { @@ -66,7 +66,9 @@ struct MainVisitor : public TidyVisitor, ASTVisitor { AlwaysFFVisitor visitor(symbol.name, config.getCheckConfigs().resetName); firstDriver->containingSymbol->visit(visitor); if (visitor.hasError()) { - diags.add(diag::RegisterNotAssignedOnReset, visitor.errorLocation) << symbol.name; + diags.add(diag::RegisterNotAssignedOnReset, + visitor.getErrorLocation().value_or(symbol.location)) + << symbol.name; } } } diff --git a/tools/tidy/src/synthesis/OnlyAssignedOnReset.cpp b/tools/tidy/src/synthesis/OnlyAssignedOnReset.cpp index 921105207..271aee191 100644 --- a/tools/tidy/src/synthesis/OnlyAssignedOnReset.cpp +++ b/tools/tidy/src/synthesis/OnlyAssignedOnReset.cpp @@ -12,8 +12,10 @@ using namespace slang::ast; namespace only_assigned_on_reset { struct AlwaysFFVisitor : public ASTVisitor { - explicit AlwaysFFVisitor(const std::string_view name, const std::string_view resetName) : - name(name), resetName(resetName) {}; + explicit AlwaysFFVisitor(const std::string_view name, const std::string_view resetName, + const bool resetIsActiveHigh) : + name(name), + resetName(resetName), resetIsActiveHigh(resetIsActiveHigh){}; void handle(const ConditionalStatement& statement) { // Early return, if there's no else clause on the conditional statement @@ -21,27 +23,46 @@ struct AlwaysFFVisitor : public ASTVisitor { return; } - // Collect all the identifiers in the conditions - CollectIdentifiers collectIdentifiersVisitor; + // Check if there is a reset in the conditional statement and if the signal is negated + bool isReset = false; + bool isResetNeg = false; + + LookupIdentifier lookupIdentifierVisitor(resetName, false); for (const auto& condition : statement.conditions) { - condition.expr->visit(collectIdentifiersVisitor); + condition.expr->visit(lookupIdentifierVisitor); + isReset = lookupIdentifierVisitor.found(); + + if (isReset) { + if (condition.expr->kind == ExpressionKind::UnaryOp) { + auto op = condition.expr->as().op; + isResetNeg = (op == UnaryOperator::BitwiseNot) || + (op == UnaryOperator::LogicalNot); + } + break; + } } - // Check if one of the identifiers is a reset - const auto isReset = - std::ranges::any_of(collectIdentifiersVisitor.identifiers, [this](auto id) { - return id.find(resetName) != std::string_view::npos; - }); - - if (isReset) { - LookupLhsIdentifier visitor(name); - statement.ifTrue.visit(visitor); - if (visitor.found()) { - visitor.reset(); - statement.ifFalse->visit(visitor); - if (!visitor.found()) { - correctlyAssignedOnIfReset = true; - } + if (!isReset) + return; + + auto resetStatement = &statement.ifTrue; + auto noResetStatement = statement.ifFalse; + + bool swapStatements = isResetNeg ? !resetIsActiveHigh : resetIsActiveHigh; + + if (swapStatements) { + std::swap(resetStatement, noResetStatement); + } + + LookupLhsIdentifier visitor(name); + resetStatement->visit(visitor); + if (visitor.found()) { + auto foundLocation = visitor.foundLocation(); + visitor.reset(); + noResetStatement->visit(visitor); + if (!visitor.found()) { + correctlyAssignedOnIfReset = true; + errorLocation = foundLocation; } } } @@ -54,27 +75,34 @@ struct AlwaysFFVisitor : public ASTVisitor { bool hasError() { return correctlyAssignedOnIfReset && !assignedOutsideIfReset; } + std::optional getErrorLocation() const { return errorLocation; } + +private: const std::string_view name; const std::string_view resetName; + const bool resetIsActiveHigh; bool correctlyAssignedOnIfReset = false; bool assignedOutsideIfReset = false; + std::optional errorLocation; }; -struct MainVisitor : public TidyVisitor, ASTVisitor { +struct MainVisitor : public TidyVisitor, ASTVisitor { explicit MainVisitor(Diagnostics& diagnostics) : TidyVisitor(diagnostics) {} void handle(const VariableSymbol& symbol) { NEEDS_SKIP_SYMBOL(symbol) - if (symbol.drivers().empty()) return; auto firstDriver = *symbol.drivers().begin(); if (firstDriver && firstDriver->isInAlwaysFFBlock()) { - AlwaysFFVisitor visitor(symbol.name, config.getCheckConfigs().resetName); + auto& configs = config.getCheckConfigs(); + AlwaysFFVisitor visitor(symbol.name, configs.resetName, configs.resetIsActiveHigh); firstDriver->containingSymbol->visit(visitor); if (visitor.hasError()) { - diags.add(diag::OnlyAssignedOnReset, symbol.location) << symbol.name; + diags.add(diag::RegisterNotAssignedOnReset, + visitor.getErrorLocation().value_or(symbol.location)) + << symbol.name; } } } diff --git a/tools/tidy/src/synthesis/RegisterHasNoReset.cpp b/tools/tidy/src/synthesis/RegisterHasNoReset.cpp index 40989faff..c28acc017 100644 --- a/tools/tidy/src/synthesis/RegisterHasNoReset.cpp +++ b/tools/tidy/src/synthesis/RegisterHasNoReset.cpp @@ -13,8 +13,10 @@ using namespace slang::ast; namespace register_has_no_reset { struct AlwaysFFVisitor : public ASTVisitor { - explicit AlwaysFFVisitor(const std::string_view name, const std::string_view resetName) : - name(name), resetName(resetName) {}; + explicit AlwaysFFVisitor(const std::string_view name, const std::string_view resetName, + const bool resetIsActiveHigh) : + name(name), + resetName(resetName), resetIsActiveHigh(resetIsActiveHigh){}; void handle(const ConditionalStatement& statement) { // Early return, if there's no else clause on the conditional statement @@ -22,27 +24,46 @@ struct AlwaysFFVisitor : public ASTVisitor { return; } - // Collect all the identifiers in the conditions - CollectIdentifiers collectIdentifiersVisitor; + // Check if there is a reset in the conditional statement and if the signal is negated + bool isReset = false; + bool isResetNeg = false; + + LookupIdentifier lookupIdentifierVisitor(resetName, false); for (const auto& condition : statement.conditions) { - condition.expr->visit(collectIdentifiersVisitor); + condition.expr->visit(lookupIdentifierVisitor); + isReset = lookupIdentifierVisitor.found(); + + if (isReset) { + if (condition.expr->kind == ExpressionKind::UnaryOp) { + auto op = condition.expr->as().op; + isResetNeg = (op == UnaryOperator::BitwiseNot) || + (op == UnaryOperator::LogicalNot); + } + break; + } } - // Check if one of the identifiers is a reset - const auto isReset = - std::ranges::any_of(collectIdentifiersVisitor.identifiers, [this](auto id) { - return id.find(resetName) != std::string_view::npos; - }); - - if (isReset) { - LookupLhsIdentifier visitor(name); - statement.ifFalse->visit(visitor); - if (visitor.found()) { - visitor.reset(); - statement.ifTrue.visit(visitor); - if (!visitor.found()) { - correctlyAssignedOnIfReset = true; - } + if (!isReset) + return; + + auto resetStatement = &statement.ifTrue; + auto noResetStatement = statement.ifFalse; + + bool swapStatements = isResetNeg ? !resetIsActiveHigh : resetIsActiveHigh; + + if (swapStatements) { + std::swap(resetStatement, noResetStatement); + } + + LookupLhsIdentifier visitor(name); + noResetStatement->visit(visitor); + if (visitor.found()) { + auto foundLocation = visitor.foundLocation(); + visitor.reset(); + resetStatement->visit(visitor); + if (!visitor.found()) { + correctlyAssignedOnIfReset = true; + errorLocation = foundLocation; } } } @@ -55,10 +76,16 @@ struct AlwaysFFVisitor : public ASTVisitor { bool hasError() { return correctlyAssignedOnIfReset && !assignedOutsideIfReset; } + std::optional getErrorLocation() const { return errorLocation; } + +private: const std::string_view name; const std::string_view resetName; + const bool resetIsActiveHigh; + bool correctlyAssignedOnIfReset = false; bool assignedOutsideIfReset = false; + std::optional errorLocation; }; struct MainVisitor : public TidyVisitor, ASTVisitor { @@ -71,10 +98,13 @@ struct MainVisitor : public TidyVisitor, ASTVisitor { auto firstDriver = *symbol.drivers().begin(); if (firstDriver && firstDriver->isInAlwaysFFBlock()) { - AlwaysFFVisitor visitor(symbol.name, config.getCheckConfigs().resetName); + auto& configs = config.getCheckConfigs(); + AlwaysFFVisitor visitor(symbol.name, configs.resetName, configs.resetIsActiveHigh); firstDriver->containingSymbol->visit(visitor); if (visitor.hasError()) { - diags.add(diag::RegisterNotAssignedOnReset, symbol.location) << symbol.name; + diags.add(diag::RegisterNotAssignedOnReset, + visitor.getErrorLocation().value_or(symbol.location)) + << symbol.name; } } } diff --git a/tools/tidy/tests/OnlyAssignedOnResetTest.cpp b/tools/tidy/tests/OnlyAssignedOnResetTest.cpp index afb8c1bac..9844e3270 100644 --- a/tools/tidy/tests/OnlyAssignedOnResetTest.cpp +++ b/tools/tidy/tests/OnlyAssignedOnResetTest.cpp @@ -239,3 +239,39 @@ endmodule bool result = visitor->check(root); CHECK_FALSE(result); } + +TEST_CASE("OnlyAssignedOnReset: Struct only assigned on reset, reset signal inversed") { + auto tree = SyntaxTree::fromText(R"( +module top; + logic clk_i; + logic rst_ni; + struct { + logic a; + logic b; + logic c; + } data; + + always_ff @(posedge clk_i or negedge rst_ni) begin + if (rst_ni) begin + end + else begin + data.a <= 1'b0; + data.b <= 1'b0; + data.c <= 1'b0; + end + end +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("OnlyAssignedOnReset"); + bool result = visitor->check(root); + CHECK_FALSE(result); +} diff --git a/tools/tidy/tests/RegisterHasNoResetTest.cpp b/tools/tidy/tests/RegisterHasNoResetTest.cpp index 38d532d39..853d760e5 100644 --- a/tools/tidy/tests/RegisterHasNoResetTest.cpp +++ b/tools/tidy/tests/RegisterHasNoResetTest.cpp @@ -236,3 +236,37 @@ endmodule bool result = visitor->check(root); CHECK_FALSE(result); } + +TEST_CASE("RegisterHasNoReset: Struct not assigned on reset, reset signal inverted") { + auto tree = SyntaxTree::fromText(R"( +module top; + logic clk_i; + logic rst_ni; + struct { + logic a; + logic b; + logic c; + } data; + + always_ff @(posedge clk_i or negedge rst_ni) begin + if (rst_ni) begin + data.a <= 1'b1; + end + else begin + end + end +endmodule +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + compilation.getAllDiagnostics(); + auto& root = compilation.getRoot(); + + TidyConfig config; + Registry::setConfig(config); + Registry::setSourceManager(compilation.getSourceManager()); + auto visitor = Registry::create("RegisterHasNoReset"); + bool result = visitor->check(root); + CHECK_FALSE(result); +} From 334e5ca50a9665073b175ff71b24b2e4e4bdfee3 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 1 Jul 2024 12:29:33 +0000 Subject: [PATCH 09/11] style: pre-commit fixes --- tools/tidy/CMakeLists.txt | 3 +-- tools/tidy/src/ASTHelperVisitors.cpp | 1 + .../AlwaysFFAssignmentOutsideConditional.cpp | 2 +- tools/tidy/src/synthesis/OnlyAssignedOnReset.cpp | 3 +-- tools/tidy/src/synthesis/RegisterHasNoReset.cpp | 3 +-- tools/tidy/tests/AlwaysCombNamedTest.cpp | 2 +- tools/tidy/tests/CMakeLists.txt | 3 +-- tools/tidy/tests/GenerateNamedTest.cpp | 12 ++++++------ tools/tidy/tests/UnusedSensitiveSignalTest.cpp | 8 ++++---- 9 files changed, 17 insertions(+), 20 deletions(-) diff --git a/tools/tidy/CMakeLists.txt b/tools/tidy/CMakeLists.txt index 7be0a4c77..c0836439d 100644 --- a/tools/tidy/CMakeLists.txt +++ b/tools/tidy/CMakeLists.txt @@ -26,8 +26,7 @@ add_library( src/style/NoDotVarInPortConnection.cpp src/style/NoLegacyGenerate.cpp src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp - src/synthesis/UnusedSensitiveSignal.cpp - ) + src/synthesis/UnusedSensitiveSignal.cpp) target_include_directories(slang_tidy_obj_lib PUBLIC include ../../include) target_link_libraries(slang_tidy_obj_lib PUBLIC slang::slang) diff --git a/tools/tidy/src/ASTHelperVisitors.cpp b/tools/tidy/src/ASTHelperVisitors.cpp index 5937d22ef..537c8e9e3 100644 --- a/tools/tidy/src/ASTHelperVisitors.cpp +++ b/tools/tidy/src/ASTHelperVisitors.cpp @@ -2,6 +2,7 @@ // SPDX-License-Identifier: MIT #include "ASTHelperVisitors.h" + #include "slang/syntax/AllSyntax.h" std::optional getIdentifier(const slang::ast::Expression& expr) { diff --git a/tools/tidy/src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp b/tools/tidy/src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp index ca0d13c11..60552d3da 100644 --- a/tools/tidy/src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp +++ b/tools/tidy/src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp @@ -14,7 +14,7 @@ using namespace slang::ast; namespace always_ff_assignment_outside_conditional { struct AlwaysFFVisitor : public ASTVisitor { explicit AlwaysFFVisitor(const std::string_view name, const std::string_view resetName) : - name(name), resetName(resetName){}; + name(name), resetName(resetName) {}; void handle(const ConditionalStatement& statement) { // Early return, if there's no else clause on the conditional statement diff --git a/tools/tidy/src/synthesis/OnlyAssignedOnReset.cpp b/tools/tidy/src/synthesis/OnlyAssignedOnReset.cpp index 271aee191..f63706350 100644 --- a/tools/tidy/src/synthesis/OnlyAssignedOnReset.cpp +++ b/tools/tidy/src/synthesis/OnlyAssignedOnReset.cpp @@ -14,8 +14,7 @@ namespace only_assigned_on_reset { struct AlwaysFFVisitor : public ASTVisitor { explicit AlwaysFFVisitor(const std::string_view name, const std::string_view resetName, const bool resetIsActiveHigh) : - name(name), - resetName(resetName), resetIsActiveHigh(resetIsActiveHigh){}; + name(name), resetName(resetName), resetIsActiveHigh(resetIsActiveHigh) {}; void handle(const ConditionalStatement& statement) { // Early return, if there's no else clause on the conditional statement diff --git a/tools/tidy/src/synthesis/RegisterHasNoReset.cpp b/tools/tidy/src/synthesis/RegisterHasNoReset.cpp index c28acc017..4d9acc24a 100644 --- a/tools/tidy/src/synthesis/RegisterHasNoReset.cpp +++ b/tools/tidy/src/synthesis/RegisterHasNoReset.cpp @@ -15,8 +15,7 @@ namespace register_has_no_reset { struct AlwaysFFVisitor : public ASTVisitor { explicit AlwaysFFVisitor(const std::string_view name, const std::string_view resetName, const bool resetIsActiveHigh) : - name(name), - resetName(resetName), resetIsActiveHigh(resetIsActiveHigh){}; + name(name), resetName(resetName), resetIsActiveHigh(resetIsActiveHigh) {}; void handle(const ConditionalStatement& statement) { // Early return, if there's no else clause on the conditional statement diff --git a/tools/tidy/tests/AlwaysCombNamedTest.cpp b/tools/tidy/tests/AlwaysCombNamedTest.cpp index d974607e6..01b1ea319 100644 --- a/tools/tidy/tests/AlwaysCombNamedTest.cpp +++ b/tools/tidy/tests/AlwaysCombNamedTest.cpp @@ -59,7 +59,7 @@ module add_or_sub input logic add, output logic [N-1:0] z_o ); - + always_comb if (add) z = x + y; diff --git a/tools/tidy/tests/CMakeLists.txt b/tools/tidy/tests/CMakeLists.txt index ba8338905..ebcefa278 100644 --- a/tools/tidy/tests/CMakeLists.txt +++ b/tools/tidy/tests/CMakeLists.txt @@ -26,8 +26,7 @@ add_executable( NoDotVarInPortConnectionTest.cpp NoLegacyGenerateTest.cpp AlwaysFFAssignmentOutsideConditionalTest.cpp - UnusedSensitiveSignalTest.cpp - ) + UnusedSensitiveSignalTest.cpp) target_link_libraries(tidy_unittests PRIVATE Catch2::Catch2 slang_tidy_obj_lib) target_compile_definitions(tidy_unittests PRIVATE UNITTESTS) diff --git a/tools/tidy/tests/GenerateNamedTest.cpp b/tools/tidy/tests/GenerateNamedTest.cpp index ff58a8f46..25bfe0322 100644 --- a/tools/tidy/tests/GenerateNamedTest.cpp +++ b/tools/tidy/tests/GenerateNamedTest.cpp @@ -111,17 +111,17 @@ module full_adder( input a, b, cin, output sum, cout ); - + assign {sum, cout} = {a^b^cin, ((a & b) | (b & cin) | (a & cin))}; endmodule module ripple_carry_adder #(parameter SIZE = 4) ( - input [SIZE-1:0] A, B, + input [SIZE-1:0] A, B, input Cin, output [SIZE-1:0] S, Cout); full_adder fa0(A[0], B[0], Cin, S[0], Cout[0]); - + for(genvar g = 1; gcheck(root); CHECK(result); -} \ No newline at end of file +} From 6703cc68c7d2f26801551b694a4cf32185a0a22e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joel=20Sol=C3=A9=20Casal=C3=A9?= Date: Mon, 1 Jul 2024 16:16:16 +0200 Subject: [PATCH 10/11] Remove redundant condition in slang-tidy --- tools/tidy/src/style/AlwaysCombNamed.cpp | 4 +--- tools/tidy/src/style/AlwaysCombNonBlocking.cpp | 4 +--- tools/tidy/src/style/AlwaysFFBlocking.cpp | 4 +--- tools/tidy/src/style/EnforceModuleInstantiationPrefix.cpp | 4 +--- tools/tidy/src/style/EnforcePortSuffix.cpp | 4 +--- tools/tidy/src/style/GenerateNamed.cpp | 4 +--- tools/tidy/src/style/NoDotStarInPortConnection.cpp | 4 +--- tools/tidy/src/style/NoDotVarInPortConnection.cpp | 4 +--- tools/tidy/src/style/NoImplicitPortNameInPortConnection.cpp | 4 +--- tools/tidy/src/style/NoLegacyGenerate.cpp | 4 +--- tools/tidy/src/style/NoOldAlwaysSyntax.cpp | 4 +--- tools/tidy/src/style/OnlyANSIPortDecl.cpp | 4 +--- .../src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp | 4 +--- tools/tidy/src/synthesis/NoLatchesOnDesign.cpp | 4 +--- tools/tidy/src/synthesis/OnlyAssignedOnReset.cpp | 4 +--- tools/tidy/src/synthesis/RegisterHasNoReset.cpp | 4 +--- 16 files changed, 16 insertions(+), 48 deletions(-) diff --git a/tools/tidy/src/style/AlwaysCombNamed.cpp b/tools/tidy/src/style/AlwaysCombNamed.cpp index 275364cf9..b3e6c0c7d 100644 --- a/tools/tidy/src/style/AlwaysCombNamed.cpp +++ b/tools/tidy/src/style/AlwaysCombNamed.cpp @@ -40,9 +40,7 @@ class AlwaysCombBlockNamed : public TidyCheck { bool check(const ast::RootSymbol& root) override { MainVisitor visitor(diagnostics); root.visit(visitor); - if (!diagnostics.empty()) - return false; - return true; + return diagnostics.empty(); } DiagCode diagCode() const override { return diag::AlwaysCombBlockNamed; } diff --git a/tools/tidy/src/style/AlwaysCombNonBlocking.cpp b/tools/tidy/src/style/AlwaysCombNonBlocking.cpp index ec8715b56..83534cd14 100644 --- a/tools/tidy/src/style/AlwaysCombNonBlocking.cpp +++ b/tools/tidy/src/style/AlwaysCombNonBlocking.cpp @@ -40,9 +40,7 @@ class AlwaysCombNonBlocking : public TidyCheck { bool check(const ast::RootSymbol& root) override { MainVisitor visitor(diagnostics); root.visit(visitor); - if (!diagnostics.empty()) - return false; - return true; + return diagnostics.empty(); } DiagCode diagCode() const override { return diag::AlwaysCombNonBlocking; } diff --git a/tools/tidy/src/style/AlwaysFFBlocking.cpp b/tools/tidy/src/style/AlwaysFFBlocking.cpp index 91e73c7e9..05d3112b5 100644 --- a/tools/tidy/src/style/AlwaysFFBlocking.cpp +++ b/tools/tidy/src/style/AlwaysFFBlocking.cpp @@ -51,9 +51,7 @@ class AlwaysFFBlocking : public TidyCheck { bool check(const ast::RootSymbol& root) override { MainVisitor visitor(diagnostics); root.visit(visitor); - if (!diagnostics.empty()) - return false; - return true; + return diagnostics.empty(); } DiagCode diagCode() const override { return diag::AlwaysFFBlocking; } diff --git a/tools/tidy/src/style/EnforceModuleInstantiationPrefix.cpp b/tools/tidy/src/style/EnforceModuleInstantiationPrefix.cpp index 74e772c36..e71aeb35b 100644 --- a/tools/tidy/src/style/EnforceModuleInstantiationPrefix.cpp +++ b/tools/tidy/src/style/EnforceModuleInstantiationPrefix.cpp @@ -38,9 +38,7 @@ class EnforceModuleInstantiationPrefix : public TidyCheck { bool check(const ast::RootSymbol& root) override { MainVisitor visitor(diagnostics); root.visit(visitor); - if (!diagnostics.empty()) - return false; - return true; + return diagnostics.empty(); } DiagCode diagCode() const override { return diag::EnforceModuleInstantiationPrefix; } diff --git a/tools/tidy/src/style/EnforcePortSuffix.cpp b/tools/tidy/src/style/EnforcePortSuffix.cpp index a95f1bcff..ac806f81c 100644 --- a/tools/tidy/src/style/EnforcePortSuffix.cpp +++ b/tools/tidy/src/style/EnforcePortSuffix.cpp @@ -61,9 +61,7 @@ class EnforcePortSuffix : public TidyCheck { bool check(const ast::RootSymbol& root) override { MainVisitor visitor(diagnostics); root.visit(visitor); - if (!diagnostics.empty()) - return false; - return true; + return diagnostics.empty(); } DiagCode diagCode() const override { return diag::EnforcePortSuffix; } diff --git a/tools/tidy/src/style/GenerateNamed.cpp b/tools/tidy/src/style/GenerateNamed.cpp index 14f6ad471..a2af51d37 100644 --- a/tools/tidy/src/style/GenerateNamed.cpp +++ b/tools/tidy/src/style/GenerateNamed.cpp @@ -45,9 +45,7 @@ class GenerateNamed : public TidyCheck { bool check(const ast::RootSymbol& root) override { MainVisitor visitor(diagnostics); root.visit(visitor); - if (!diagnostics.empty()) - return false; - return true; + return diagnostics.empty(); } DiagCode diagCode() const override { return diag::GenerateNamed; } diff --git a/tools/tidy/src/style/NoDotStarInPortConnection.cpp b/tools/tidy/src/style/NoDotStarInPortConnection.cpp index 226822a76..03e8105b2 100644 --- a/tools/tidy/src/style/NoDotStarInPortConnection.cpp +++ b/tools/tidy/src/style/NoDotStarInPortConnection.cpp @@ -42,9 +42,7 @@ class NoDotStarInPortConnection : public TidyCheck { bool check(const RootSymbol& root) override { MainVisitor visitor(diagnostics); root.visit(visitor); - if (!diagnostics.empty()) - return false; - return true; + return diagnostics.empty(); } DiagCode diagCode() const override { return diag::NoDotStarInPortConnection; } diff --git a/tools/tidy/src/style/NoDotVarInPortConnection.cpp b/tools/tidy/src/style/NoDotVarInPortConnection.cpp index 71689cee2..9716c58f4 100644 --- a/tools/tidy/src/style/NoDotVarInPortConnection.cpp +++ b/tools/tidy/src/style/NoDotVarInPortConnection.cpp @@ -50,9 +50,7 @@ class NoDotVarInPortConnection : public TidyCheck { bool check(const RootSymbol& root) override { MainVisitor visitor(diagnostics); root.visit(visitor); - if (!diagnostics.empty()) - return false; - return true; + return diagnostics.empty(); } DiagCode diagCode() const override { return diag::NoDotVarInPortConnection; } diff --git a/tools/tidy/src/style/NoImplicitPortNameInPortConnection.cpp b/tools/tidy/src/style/NoImplicitPortNameInPortConnection.cpp index c587fc76e..1bbb291be 100644 --- a/tools/tidy/src/style/NoImplicitPortNameInPortConnection.cpp +++ b/tools/tidy/src/style/NoImplicitPortNameInPortConnection.cpp @@ -46,9 +46,7 @@ class NoImplicitPortNameInPortConnection : public TidyCheck { bool check(const RootSymbol& root) override { MainVisitor visitor(diagnostics); root.visit(visitor); - if (!diagnostics.empty()) - return false; - return true; + return diagnostics.empty(); } DiagCode diagCode() const override { return diag::NoImplicitPortNameInPortConnection; } diff --git a/tools/tidy/src/style/NoLegacyGenerate.cpp b/tools/tidy/src/style/NoLegacyGenerate.cpp index 2a880f941..0fda1ed86 100644 --- a/tools/tidy/src/style/NoLegacyGenerate.cpp +++ b/tools/tidy/src/style/NoLegacyGenerate.cpp @@ -47,9 +47,7 @@ class NoLegacyGenerate : public TidyCheck { bool check(const ast::RootSymbol& root) override { MainVisitor visitor(diagnostics); root.visit(visitor); - if (!diagnostics.empty()) - return false; - return true; + return diagnostics.empty(); } DiagCode diagCode() const override { return diag::NoLegacyGenerate; } diff --git a/tools/tidy/src/style/NoOldAlwaysSyntax.cpp b/tools/tidy/src/style/NoOldAlwaysSyntax.cpp index 31549daf1..ae89c0ad9 100644 --- a/tools/tidy/src/style/NoOldAlwaysSyntax.cpp +++ b/tools/tidy/src/style/NoOldAlwaysSyntax.cpp @@ -48,9 +48,7 @@ class NoOldAlwaysSyntax : public TidyCheck { bool check(const RootSymbol& root) override { MainVisitor visitor(diagnostics); root.visit(visitor); - if (!diagnostics.empty()) - return false; - return true; + return diagnostics.empty(); } DiagCode diagCode() const override { return diag::NoOldAlwaysSyntax; } diff --git a/tools/tidy/src/style/OnlyANSIPortDecl.cpp b/tools/tidy/src/style/OnlyANSIPortDecl.cpp index 461aa050e..78e79bb98 100644 --- a/tools/tidy/src/style/OnlyANSIPortDecl.cpp +++ b/tools/tidy/src/style/OnlyANSIPortDecl.cpp @@ -31,9 +31,7 @@ class OnlyANSIPortDecl : public TidyCheck { bool check(const ast::RootSymbol& root) override { MainVisitor visitor(diagnostics); root.visit(visitor); - if (!diagnostics.empty()) - return false; - return true; + return diagnostics.empty(); } DiagCode diagCode() const override { return diag::OnlyANSIPortDecl; } diff --git a/tools/tidy/src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp b/tools/tidy/src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp index ca0d13c11..dcb10d155 100644 --- a/tools/tidy/src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp +++ b/tools/tidy/src/synthesis/AlwaysFFAssignmentOutsideConditional.cpp @@ -84,9 +84,7 @@ class AlwaysFFAssignmentOutsideConditional : public TidyCheck { bool check(const RootSymbol& root) override { MainVisitor visitor(diagnostics); root.visit(visitor); - if (!diagnostics.empty()) - return false; - return true; + return diagnostics.empty(); } DiagCode diagCode() const override { return diag::RegisterNotAssignedOnReset; } diff --git a/tools/tidy/src/synthesis/NoLatchesOnDesign.cpp b/tools/tidy/src/synthesis/NoLatchesOnDesign.cpp index b93f984ed..4a22018f8 100644 --- a/tools/tidy/src/synthesis/NoLatchesOnDesign.cpp +++ b/tools/tidy/src/synthesis/NoLatchesOnDesign.cpp @@ -37,9 +37,7 @@ class NoLatchesOnDesign : public TidyCheck { bool check(const RootSymbol& root) override { MainVisitor visitor(diagnostics); root.visit(visitor); - if (!diagnostics.empty()) - return false; - return true; + return diagnostics.empty(); } DiagCode diagCode() const override { return diag::NoLatchesOnDesign; } diff --git a/tools/tidy/src/synthesis/OnlyAssignedOnReset.cpp b/tools/tidy/src/synthesis/OnlyAssignedOnReset.cpp index 271aee191..229cb948c 100644 --- a/tools/tidy/src/synthesis/OnlyAssignedOnReset.cpp +++ b/tools/tidy/src/synthesis/OnlyAssignedOnReset.cpp @@ -117,9 +117,7 @@ class OnlyAssignedOnReset : public TidyCheck { bool check(const RootSymbol& root) override { MainVisitor visitor(diagnostics); root.visit(visitor); - if (!diagnostics.empty()) - return false; - return true; + return diagnostics.empty(); } DiagCode diagCode() const override { return diag::OnlyAssignedOnReset; } diff --git a/tools/tidy/src/synthesis/RegisterHasNoReset.cpp b/tools/tidy/src/synthesis/RegisterHasNoReset.cpp index c28acc017..5161ac092 100644 --- a/tools/tidy/src/synthesis/RegisterHasNoReset.cpp +++ b/tools/tidy/src/synthesis/RegisterHasNoReset.cpp @@ -120,9 +120,7 @@ class RegisterHasNoReset : public TidyCheck { bool check(const RootSymbol& root) override { MainVisitor visitor(diagnostics); root.visit(visitor); - if (!diagnostics.empty()) - return false; - return true; + return diagnostics.empty(); } DiagCode diagCode() const override { return diag::RegisterNotAssignedOnReset; } From dae936307f0511c52afe58be243685beb40d33cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joel=20Sol=C3=A9=20Casal=C3=A9?= Date: Fri, 12 Jul 2024 10:34:07 +0200 Subject: [PATCH 11/11] Fix tidy-check: UnusedSensitiveSignal Added a simple fix to ensure that the body kind is the correct one when accessing the body attributes --- tools/tidy/src/synthesis/UnusedSensitiveSignal.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/tidy/src/synthesis/UnusedSensitiveSignal.cpp b/tools/tidy/src/synthesis/UnusedSensitiveSignal.cpp index 3b58b6d59..19f2eb5c2 100644 --- a/tools/tidy/src/synthesis/UnusedSensitiveSignal.cpp +++ b/tools/tidy/src/synthesis/UnusedSensitiveSignal.cpp @@ -34,6 +34,10 @@ struct MainVisitor : public TidyVisitor, ASTVisitor { block.procedureKind != ProceduralBlockKind::Always) return; + if (block.getBody().kind != StatementKind::Timed || + block.getBody().as().stmt.kind != StatementKind::Block) + return; + CollectAllIdentifiers stmtIdVisitor, timingIdVisitor; block.getBody().as().stmt.as().visitStmts(stmtIdVisitor); block.getBody().as().timing.visit(timingIdVisitor);