Skip to content

Commit

Permalink
Clean up error handling cases for instance config rules
Browse files Browse the repository at this point in the history
  • Loading branch information
MikePopoloski committed Jan 20, 2024
1 parent cd2a5e5 commit aa7a5cb
Show file tree
Hide file tree
Showing 6 changed files with 156 additions and 64 deletions.
3 changes: 3 additions & 0 deletions include/slang/ast/symbols/CompilationUnitSymbols.h
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,9 @@ struct ConfigRule {

/// A specific cell to use for this instance or definition lookup.
ConfigCellId useCell;

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

/// Represents a config block declaration.
Expand Down
7 changes: 7 additions & 0 deletions include/slang/ast/symbols/InstanceSymbols.h
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,13 @@ class SLANG_EXPORT UninstantiatedDefSymbol : public Symbol {
const ASTContext& context, SmallVectorBase<const Symbol*>& results,
SmallVectorBase<const Symbol*>& implicitNets);

static void fromSyntax(Compilation& compilation,
const syntax::HierarchyInstantiationSyntax& syntax,
const syntax::HierarchicalInstanceSyntax* specificInstance,
const ASTContext& context, SmallVectorBase<const Symbol*>& results,
SmallVectorBase<const Symbol*>& implicitNets,
SmallSet<std::string_view, 8>& implicitNetNames, const NetType& netType);

static void fromSyntax(Compilation& compilation,
const syntax::PrimitiveInstantiationSyntax& syntax,
const ASTContext& context, SmallVectorBase<const Symbol*>& results,
Expand Down
1 change: 1 addition & 0 deletions scripts/diagnostics.txt
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ note NoteReferencedHere "referenced here"
note NoteAssignedHere "also assigned here"
note NoteDrivenHere "driven here"
note NoteOriginalAssign "original assignment here"
note NoteConfigRule "from configuration here"
note NoteFromHere2 "from '{}' and '{}'"
error AttributesNotAllowed "attributes are not allowed here"

Expand Down
1 change: 1 addition & 0 deletions source/ast/symbols/CompilationUnitSymbols.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ ConfigBlockSymbol& ConfigBlockSymbol::fromSyntax(const Scope& scope,

auto buildRule = [&](const ConfigRuleClauseSyntax& rule) {
ConfigRule result;
result.sourceRange = rule.parent->sourceRange();
if (rule.kind == SyntaxKind::ConfigUseClause) {
// TODO: handle other parts of this
auto& cuc = rule.as<ConfigUseClauseSyntax>();
Expand Down
170 changes: 106 additions & 64 deletions source/ast/symbols/InstanceSymbols.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -377,59 +377,71 @@ void InstanceSymbol::fromSyntax(Compilation& comp, const HierarchyInstantiationS

const DefinitionSymbol* owningDefinition = nullptr;
const HierarchyOverrideNode* parentOverrideNode = nullptr;
const DefinitionSymbol* explicitDef = nullptr;
SmallSet<std::string_view, 8> implicitNetNames;
auto& netType = context.scope->getDefaultNetType();

// Does a lookup for the definition that was explicitly provided in
// the instantiation syntax node.
auto resolveExplicitDef = [&] {
auto def = comp.getDefinition(defName, *context.scope, syntax.type.range(),
diag::UnknownModule);
// Creates instance symbols -- if specificInstance is provided then only that
// instance will be created, otherwise all instances in the original syntax
// node will be created in one go.
auto createInstances = [&](const Symbol* def,
const HierarchicalInstanceSyntax* specificInstance,
const ConfigRule* configRule) {
if (!def) {
UninstantiatedDefSymbol::fromSyntax(comp, syntax, context, results, implicitNets);
UninstantiatedDefSymbol::fromSyntax(comp, syntax, specificInstance, context, results,
implicitNets, implicitNetNames, netType);
return;
}
else if (def->kind == SymbolKind::Primitive) {

auto addDiag = [&](DiagCode code) -> Diagnostic& {
if (configRule) {
SLANG_ASSERT(specificInstance);
auto& diag = context.addDiag(code, specificInstance->sourceRange());
diag.addNote(diag::NoteConfigRule, configRule->sourceRange);
return diag;
}
else {
auto& diag = context.addDiag(code, syntax.type.range());
if (specificInstance)
diag << specificInstance->sourceRange();
return diag;
}
};

if (def->kind == SymbolKind::Primitive) {
PrimitiveInstanceSymbol::fromSyntax(def->as<PrimitiveSymbol>(), syntax, context,
results, implicitNets);
if (!results.empty()) {
if (!owningDefinition ||
owningDefinition->definitionKind != DefinitionKind::Module || inChecker) {
context.addDiag(diag::InvalidPrimInstanceForParent, syntax.type.range());
addDiag(diag::InvalidPrimInstanceForParent);
}
else if (isFromBind) {
context.addDiag(diag::BindTargetPrimitive, syntax.type.range());
addDiag(diag::BindTargetPrimitive);
}
}
return;
}
else {
explicitDef = &def->as<DefinitionSymbol>();
}
};

// Creates instance symbols -- if specificInstance is provided then only that
// instance will be created, otherwise all instances in the original syntax
// node will be created in one go.
auto createInstances = [&](const DefinitionSymbol& definition,
const HierarchicalInstanceSyntax* specificInstance) {
auto& definition = def->as<DefinitionSymbol>();
definition.noteInstantiated();

if (inChecker) {
context.addDiag(diag::InvalidInstanceForParent, syntax.type.range())
addDiag(diag::InvalidInstanceForParent)
<< definition.getArticleKindString() << "a checker"sv;
}
else if (owningDefinition) {
auto owningKind = owningDefinition->definitionKind;
if (owningKind == DefinitionKind::Program ||
(owningKind == DefinitionKind::Interface &&
definition.definitionKind == DefinitionKind::Module)) {
context.addDiag(diag::InvalidInstanceForParent, syntax.type.range())
<< definition.getArticleKindString()
<< owningDefinition->getArticleKindString();
addDiag(diag::InvalidInstanceForParent) << definition.getArticleKindString()
<< owningDefinition->getArticleKindString();
}
}

if (parentInst && parentInst->isFromBind) {
if (isFromBind) {
context.addDiag(diag::BindUnderBind, syntax.type.range());
addDiag(diag::BindUnderBind);
return;
}

Expand All @@ -438,9 +450,6 @@ void InstanceSymbol::fromSyntax(Compilation& comp, const HierarchyInstantiationS
isFromBind = true;
}

SmallSet<std::string_view, 8> implicitNetNames;
auto& netType = context.scope->getDefaultNetType();

ParameterBuilder paramBuilder(*context.scope, definition.name, definition.parameters);
if (syntax.parameters)
paramBuilder.setAssignments(*syntax.parameters);
Expand Down Expand Up @@ -474,6 +483,7 @@ void InstanceSymbol::fromSyntax(Compilation& comp, const HierarchyInstantiationS
// We need to handle each instance separately, as the config
// rules allow the entire definition and parameter values
// to be overridden on a per-instance basis.
std::optional<const Symbol*> explicitDef;
for (auto instanceSyntax : syntax.instances) {
auto instName = instanceSyntax->decl ? instanceSyntax->decl->name.valueText()
: ""sv;
Expand All @@ -482,38 +492,29 @@ void InstanceSymbol::fromSyntax(Compilation& comp, const HierarchyInstantiationS
overrideIt->second.configRule) {

// We have an override rule, so use it to lookup the def.
auto def = comp.getDefinition(defName, *context.scope,
*overrideIt->second.configRule);
if (!def || def->kind != SymbolKind::Definition) {
// TODO: only error for this specific instance
// TODO: handle primitives
UninstantiatedDefSymbol::fromSyntax(comp, syntax, context, results,
implicitNets);
}
else {
createInstances(def->as<DefinitionSymbol>(), instanceSyntax);
}
auto rule = overrideIt->second.configRule;
auto def = comp.getDefinition(defName, *context.scope, *rule);
createInstances(def, instanceSyntax, rule);
}
else {
// No specific config rule, so use the default lookup behavior.
if (!explicitDef) {
// TODO: only error for this specific instance
resolveExplicitDef();
if (!explicitDef)
return;
explicitDef = comp.getDefinition(defName, *context.scope,
syntax.type.range(),
diag::UnknownModule);
}

createInstances(*explicitDef, instanceSyntax);
createInstances(*explicitDef, instanceSyntax, nullptr);
}
}
return;
}
}
}

resolveExplicitDef();
if (explicitDef)
createInstances(*explicitDef, nullptr);
// Simple case: look up the definition and create all instances in one go.
auto def = comp.getDefinition(defName, *context.scope, syntax.type.range(),
diag::UnknownModule);
createInstances(def, nullptr, nullptr);
}

void InstanceSymbol::fromFixupSyntax(Compilation& comp, const DefinitionSymbol& definition,
Expand Down Expand Up @@ -863,6 +864,24 @@ void InstanceArraySymbol::serializeTo(ASTSerializer& serializer) const {
serializer.write("range", range.toString());
}

template<typename TSyntax>
static void createUninstantiatedDef(Compilation& compilation, const TSyntax& syntax,
const HierarchicalInstanceSyntax* instanceSyntax,
std::string_view moduleName, const ASTContext& context,
std::span<const Expression* const> params,
SmallVectorBase<const Symbol*>& results,
SmallVectorBase<const Symbol*>& implicitNets,
SmallSet<std::string_view, 8>& implicitNetNames,
const NetType& netType) {
createImplicitNets(*instanceSyntax, context, netType, implicitNetNames, implicitNets);

auto [name, loc] = getNameLoc(*instanceSyntax);
auto sym = compilation.emplace<UninstantiatedDefSymbol>(name, loc, moduleName, params);
sym->setSyntax(*instanceSyntax);
sym->setAttributes(*context.scope, syntax.attributes);
results.push_back(sym);
}

template<typename TSyntax>
static void createUninstantiatedDefs(Compilation& compilation, const TSyntax& syntax,
std::string_view moduleName, const ASTContext& context,
Expand All @@ -872,24 +891,15 @@ static void createUninstantiatedDefs(Compilation& compilation, const TSyntax& sy
SmallSet<std::string_view, 8> implicitNetNames;
auto& netType = context.scope->getDefaultNetType();
for (auto instanceSyntax : syntax.instances) {
createImplicitNets(*instanceSyntax, context, netType, implicitNetNames, implicitNets);

auto [name, loc] = getNameLoc(*instanceSyntax);
auto sym = compilation.emplace<UninstantiatedDefSymbol>(name, loc, moduleName, params);
sym->setSyntax(*instanceSyntax);
sym->setAttributes(*context.scope, syntax.attributes);
results.push_back(sym);
createUninstantiatedDef(compilation, syntax, instanceSyntax, moduleName, context, params,
results, implicitNets, implicitNetNames, netType);
}
}

void UninstantiatedDefSymbol::fromSyntax(Compilation& compilation,
const HierarchyInstantiationSyntax& syntax,
const ASTContext& parentContext,
SmallVectorBase<const Symbol*>& results,
SmallVectorBase<const Symbol*>& implicitNets) {
SmallVector<const Expression*> params;
ASTContext context = parentContext.resetFlags(ASTFlags::NonProcedural);
static std::span<const Expression* const> createUninstantiatedParams(
const HierarchyInstantiationSyntax& syntax, const ASTContext& context) {

SmallVector<const Expression*> params;
if (syntax.parameters) {
for (auto expr : syntax.parameters->parameters) {
// Empty expressions are just ignored here.
Expand All @@ -904,9 +914,41 @@ void UninstantiatedDefSymbol::fromSyntax(Compilation& compilation,
}
}

auto paramSpan = params.copy(compilation);
createUninstantiatedDefs(compilation, syntax, syntax.type.valueText(), context, paramSpan,
results, implicitNets);
return params.copy(context.getCompilation());
}

void UninstantiatedDefSymbol::fromSyntax(Compilation& compilation,
const HierarchyInstantiationSyntax& syntax,
const ASTContext& parentContext,
SmallVectorBase<const Symbol*>& results,
SmallVectorBase<const Symbol*>& implicitNets) {
ASTContext context = parentContext.resetFlags(ASTFlags::NonProcedural);
auto params = createUninstantiatedParams(syntax, context);

createUninstantiatedDefs(compilation, syntax, syntax.type.valueText(), context, params, results,
implicitNets);
}

void UninstantiatedDefSymbol::fromSyntax(
Compilation& compilation, const syntax::HierarchyInstantiationSyntax& syntax,
const syntax::HierarchicalInstanceSyntax* specificInstance, const ASTContext& parentContext,
SmallVectorBase<const Symbol*>& results, SmallVectorBase<const Symbol*>& implicitNets,
SmallSet<std::string_view, 8>& implicitNetNames, const NetType& netType) {

ASTContext context = parentContext.resetFlags(ASTFlags::NonProcedural);
auto params = createUninstantiatedParams(syntax, context);

if (specificInstance) {
createUninstantiatedDef(compilation, syntax, specificInstance, syntax.type.valueText(),
context, params, results, implicitNets, implicitNetNames, netType);
}
else {
for (auto instanceSyntax : syntax.instances) {
createUninstantiatedDef(compilation, syntax, instanceSyntax, syntax.type.valueText(),
context, params, results, implicitNets, implicitNetNames,
netType);
}
}
}

void UninstantiatedDefSymbol::fromSyntax(Compilation& compilation,
Expand Down
38 changes: 38 additions & 0 deletions tests/unittests/ast/ConfigTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -334,3 +334,41 @@ endmodule
auto& inst = compilation.getRoot().lookupName<InstanceSymbol>("top.b.f2");
CHECK(inst.getDefinition().name == "bar");
}

TEST_CASE("Config instance override errors") {
auto tree = SyntaxTree::fromText(R"(
config cfg1;
design top;
instance top.b.f2 use bar;
instance top.i.p use foo;
endconfig
module foo;
endmodule
module baz;
foo f1(), f2();
endmodule
module top;
baz b();
I i();
endmodule
program prog;
endprogram
interface I;
prog p(), q();
endinterface
)");
CompilationOptions options;
options.topModules.emplace("cfg1");

Compilation compilation(options);
compilation.addSyntaxTree(tree);

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

0 comments on commit aa7a5cb

Please sign in to comment.