Skip to content

Commit

Permalink
Add warnings for unused config rules
Browse files Browse the repository at this point in the history
  • Loading branch information
MikePopoloski committed Mar 3, 2024
1 parent 9903447 commit 5e5fdfd
Show file tree
Hide file tree
Showing 7 changed files with 98 additions and 11 deletions.
8 changes: 8 additions & 0 deletions include/slang/ast/symbols/CompilationUnitSymbols.h
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,9 @@ struct SLANG_EXPORT ConfigRule {

/// The source range where this rule was declared.
SourceRange sourceRange;

/// A flag that marks whether this rule has been used during elaboration.
mutable bool isUsed = false;
};

/// Contains information about a resolved configuration rule
Expand Down Expand Up @@ -283,6 +286,9 @@ class SLANG_EXPORT ConfigBlockSymbol : public Symbol, public Scope {
ConfigRule* rule = nullptr;
};

/// A flag that marks whether this config block has been used during elaboration.
mutable bool isUsed = false;

ConfigBlockSymbol(Compilation& compilation, std::string_view name, SourceLocation loc) :
Symbol(SymbolKind::ConfigBlock, name, loc), Scope(compilation, this) {}

Expand Down Expand Up @@ -310,6 +316,8 @@ class SLANG_EXPORT ConfigBlockSymbol : public Symbol, public Scope {
return instanceOverrides;
}

void checkUnusedRules() const;

static ConfigBlockSymbol& fromSyntax(const Scope& scope,
const syntax::ConfigDeclarationSyntax& syntax);

Expand Down
6 changes: 4 additions & 2 deletions scripts/diagnostics.txt
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,8 @@ warning invalid-pulsestyle InvalidPulseStyle "invalid use of {} for '{}' since i
warning implicit-port-type-mismatch ImplicitNamedPortTypeMismatch "implicit named port '{}' of type {} connects to value of inequivalent type {}"
warning unknown-library WarnUnknownLibrary "unknown library '{}'"
warning dup-config-rule DupConfigRule "duplicate config rule for {} -- ignoring this one"
warning unused-config-cell UnusedConfigCell "unused cell config rule"
warning unused-config-instance UnusedConfigInstance "unused config rule -- instance path does not exist in the design"

subsystem Expressions
error BadUnaryExpression "invalid operand type {} to unary expression"
Expand Down Expand Up @@ -1066,7 +1068,7 @@ group default = { real-underflow real-overflow vector-overflow int-overflow unco
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 unknown-library
dup-config-rule }
dup-config-rule unused-config-cell unused-config-instance }

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
Expand All @@ -1082,4 +1084,4 @@ group conversion = { width-trunc width-expand port-width-trunc port-width-expand
group unused = { unused-def unused-net unused-implicit-net unused-variable undriven-net unassigned-variable
unused-but-set-net unused-but-set-variable unused-port undriven-port unused-argument
unused-parameter unused-type-parameter unused-typedef unused-genvar unused-assertion-decl
unused-but-set-port }
unused-but-set-port unused-config-cell unused-config-instance }
26 changes: 26 additions & 0 deletions scripts/warning_docs.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1162,3 +1162,29 @@ config cfg;
instance d.foo use baz;
endconfig
```

-Wunused-config-cell
A configuration cell rule is unused, which may mean that there's a typo in the target name.
@options --top=cfg
```
config cfg;
design top;
cell foo liblist l;
endconfig

module top;
endmodule
```

-Wunused-config-instance
A configuration instance rule is unused, which means that target path does not exist in the design.
@options --top=cfg
```
config cfg;
design top;
instance top.foo liblist l;
endconfig

module top;
endmodule
```
14 changes: 14 additions & 0 deletions source/ast/Compilation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,7 @@ const RootSymbol& Compilation::getRoot(bool skipDefParamsAndBinds) {
}

if (foundConf) {
foundConf->isUsed = true;
for (auto& cell : foundConf->getTopCells()) {
selectedConfigs.emplace(foundConf);
topDefs.push_back({&cell.definition, foundConf, cell.rule});
Expand Down Expand Up @@ -1350,6 +1351,16 @@ const Diagnostics& Compilation::getSemanticDiagnostics() {
}
}

// Report on unused config rules.
if (!configBlocks.empty()) {
for (auto& [name, confList] : configBlocks) {
for (auto config : confList) {
if (config->isUsed)
config->checkUnusedRules();
}
}
}

if (!hasFlag(CompilationFlags::AllowTopLevelIfacePorts)) {
// Top level instances cannot have interface or ref ports.
for (auto inst : getRoot().topInstances) {
Expand Down Expand Up @@ -2292,6 +2303,7 @@ auto findDefByLib(TDefList& defList, const SourceLibrary& target)
std::pair<Compilation::DefinitionLookupResult, bool> Compilation::resolveConfigRule(
const Scope& scope, const ConfigRule& rule) const {

rule.isUsed = true;
auto& id = rule.useCell;
SLANG_ASSERT(!id.name.empty());

Expand Down Expand Up @@ -2324,6 +2336,7 @@ std::pair<Compilation::DefinitionLookupResult, bool> Compilation::resolveConfigR
if (auto configIt = configBlocks.find(id.name); configIt != configBlocks.end()) {
auto result = findDefByLib(configIt->second, *overrideLib);
if (result) {
result->isUsed = true;
auto topCells = result->getTopCells();
if (topCells.size() != 1) {
auto syntax = result->getSyntax();
Expand Down Expand Up @@ -2373,6 +2386,7 @@ std::pair<Compilation::DefinitionLookupResult, bool> Compilation::resolveConfigR
if (auto& id = rule->useCell; !id.name.empty())
return resolveConfigRule(scope, *rule);

rule->isUsed = true;
if (rule->liblist)
liblist = *rule->liblist;
}
Expand Down
28 changes: 28 additions & 0 deletions source/ast/symbols/CompilationUnitSymbols.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,34 @@ ConfigBlockSymbol& ConfigBlockSymbol::fromSyntax(const Scope& scope,
return *result;
}

static void checkRuleUnused(const Scope& scope, const ConfigRule& rule, bool isCell) {
if (!rule.isUsed) {
scope.addDiag(isCell ? diag::UnusedConfigCell : diag::UnusedConfigInstance,
rule.sourceRange);
}
}

static void checkNodeUnused(const Scope& scope, const ConfigBlockSymbol::InstanceOverride& node) {
if (node.rule)
checkRuleUnused(scope, *node.rule, false);

for (auto& [name, child] : node.childNodes)
checkNodeUnused(scope, child);
}

void ConfigBlockSymbol::checkUnusedRules() const {
for (auto& [_, cellOverride] : getCellOverrides()) {
if (cellOverride.defaultRule)
checkRuleUnused(*this, *cellOverride.defaultRule, true);

for (auto& [lib, rule] : cellOverride.specificLibRules)
checkRuleUnused(*this, *rule, true);
}

for (auto& [_, instOverride] : getInstanceOverrides())
checkNodeUnused(*this, instOverride);
}

void ConfigBlockSymbol::resolve() const {
SLANG_ASSERT(!resolved);
resolved = true;
Expand Down
8 changes: 6 additions & 2 deletions source/ast/symbols/InstanceSymbols.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -273,8 +273,11 @@ InstanceSymbol& InstanceSymbol::createDefault(Compilation& comp, const Definitio

if (configBlock) {
auto rc = comp.emplace<ResolvedConfig>(*configBlock, result);
if (configRule && configRule->liblist)
rc->liblist = *configRule->liblist;
if (configRule) {
configRule->isUsed = true;
if (configRule->liblist)
rc->liblist = *configRule->liblist;
}
result.resolvedConfig = rc;
}

Expand Down Expand Up @@ -527,6 +530,7 @@ void InstanceSymbol::fromSyntax(Compilation& comp, const HierarchyInstantiationS
if (confRule) {
SLANG_ASSERT(resolvedConfig);
auto rc = comp.emplace<ResolvedConfig>(*resolvedConfig);
confRule->isUsed = true;
if (confRule->liblist)
rc->liblist = *confRule->liblist;
localConfig = rc;
Expand Down
19 changes: 12 additions & 7 deletions tests/unittests/ast/ConfigTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -550,15 +550,17 @@ endmodule
compilation.addSyntaxTree(tree);

auto& diags = compilation.getAllDiagnostics();
REQUIRE(diags.size() == 8);
REQUIRE(diags.size() == 10);
CHECK(diags[0].code == diag::ConfigDupTop);
CHECK(diags[1].code == diag::WarnUnknownLibrary);
CHECK(diags[2].code == diag::DupConfigRule);
CHECK(diags[2].code == diag::UnusedConfigCell);
CHECK(diags[3].code == diag::DupConfigRule);
CHECK(diags[4].code == diag::ConfigInstanceWrongTop);
CHECK(diags[5].code == diag::ConfigParamsIgnored);
CHECK(diags[6].code == diag::WarnUnknownLibrary);
CHECK(diags[7].code == diag::NestedConfigMultipleTops);
CHECK(diags[4].code == diag::UnusedConfigInstance);
CHECK(diags[5].code == diag::DupConfigRule);
CHECK(diags[6].code == diag::ConfigInstanceWrongTop);
CHECK(diags[7].code == diag::ConfigParamsIgnored);
CHECK(diags[8].code == diag::WarnUnknownLibrary);
CHECK(diags[9].code == diag::NestedConfigMultipleTops);
}

TEST_CASE("Config rules with param overrides") {
Expand Down Expand Up @@ -786,7 +788,10 @@ endconfig
Compilation compilation(options);
compilation.addSyntaxTree(tree2);
compilation.addSyntaxTree(tree1);
NO_COMPILATION_ERRORS;

auto& diags = compilation.getAllDiagnostics();
REQUIRE(diags.size() == 1);
CHECK(diags[0].code == diag::UnusedConfigCell);

auto getParam = [&](std::string_view name) {
auto& param = compilation.getRoot().lookupName<ParameterSymbol>(name);
Expand Down

0 comments on commit 5e5fdfd

Please sign in to comment.