From 76e5ec487d1711c16c50daa4130d1ff6c88dd4cd Mon Sep 17 00:00:00 2001 From: Mike Popoloski Date: Tue, 24 Dec 2024 12:16:40 -0500 Subject: [PATCH] Enforce missing rules for virtual interfaces 1. Virtual interfaces can't have hierarchical references to objects outside the interface 2. Virtual interfaces can't have interface ports --- include/slang/ast/Compilation.h | 22 ++++- .../slang/ast/expressions/MiscExpressions.h | 6 +- scripts/diagnostics.txt | 3 + source/ast/Compilation.cpp | 41 +++++++++ source/ast/ElabVisitors.h | 25 ++++-- source/ast/Expression.cpp | 8 +- source/ast/expressions/MiscExpressions.cpp | 25 ++++-- source/ast/symbols/InstanceSymbols.cpp | 4 +- source/ast/types/AllTypes.cpp | 1 + tests/unittests/ast/InterfaceTests.cpp | 83 +++++++++++++++++++ tests/unittests/ast/ParameterTests.cpp | 2 +- 11 files changed, 193 insertions(+), 27 deletions(-) diff --git a/include/slang/ast/Compilation.h b/include/slang/ast/Compilation.h index 6fb5705cf..88b329e7e 100644 --- a/include/slang/ast/Compilation.h +++ b/include/slang/ast/Compilation.h @@ -34,6 +34,7 @@ class ConfigBlockSymbol; class DefinitionSymbol; class Expression; class GenericClassDefSymbol; +class InstanceSymbol; class InterfacePortSymbol; class MethodPrototypeSymbol; class ModportSymbol; @@ -328,6 +329,9 @@ class SLANG_EXPORT Compilation : public BumpAllocator { /// Gets all of the diagnostics produced during compilation. const Diagnostics& getAllDiagnostics(); + /// Queries if any errors have been issued on any scope within this compilation. + bool hasIssuedErrors() const { return numErrors > 0; }; + /// @} /// @name Utility and convenience methods /// @{ @@ -588,6 +592,13 @@ class SLANG_EXPORT Compilation : public BumpAllocator { const Expression& firstExpr, const Symbol& secondSym, DriverBitRange secondRange, const Expression& secondExpr); + /// Notes the existence of the given hierarchical reference, which is used, + /// among other things, to ensure we perform instance caching correctly. + void noteHierarchicalReference(const Scope& scope, const HierarchicalReference& ref); + + /// Notes the existence of a virtual interface type declaration for the given instance. + void noteVirtualIfaceInstance(const InstanceSymbol& instance); + /// Adds a set of diagnostics to the compilation's list of semantic diagnostics. void addDiagnostics(const Diagnostics& diagnostics); @@ -723,14 +734,12 @@ class SLANG_EXPORT Compilation : public BumpAllocator { /// Creates an empty ImplicitTypeSyntax object. const syntax::ImplicitTypeSyntax& createEmptyTypeSyntax(SourceLocation loc); - /// Queries if any errors have been issued on any scope within this compilation. - bool hasIssuedErrors() const { return numErrors > 0; }; - /// @} private: friend class Lookup; friend class Scope; + friend class DiagnosticVisitor; // Collected information about a resolved bind directive. struct ResolvedBind { @@ -764,6 +773,7 @@ class SLANG_EXPORT Compilation : public BumpAllocator { ResolvedBind& resolvedBind); void checkBindTargetParams(const syntax::BindDirectiveSyntax& syntax, const Scope& scope, const ResolvedBind& resolvedBind); + void checkVirtualIfaceInstance(const InstanceSymbol& instance); std::pair resolveConfigRule(const Scope& scope, const ConfigRule& rule) const; std::pair resolveConfigRules( @@ -839,6 +849,9 @@ class SLANG_EXPORT Compilation : public BumpAllocator { // Map from pointers (to symbols, statements, expressions) to their associated attributes. flat_hash_map> attributeMap; + // Map from instance bodies to hierarchical references that extend up through them. + flat_hash_map> hierRefMap; + struct SyntaxMetadata { const syntax::SyntaxTree* tree = nullptr; const NetType* defaultNetType = nullptr; @@ -860,6 +873,9 @@ class SLANG_EXPORT Compilation : public BumpAllocator { // A list of libraries that control the order in which we search for cell bindings. std::vector defaultLiblist; + // A list of instances that have been created by virtual interface type declarations. + std::vector virtualInterfaceInstances; + // A map from class name + decl name + scope to out-of-block declarations. These get // registered when we find the initial declaration and later get used when we see // the class prototype. The value also includes a boolean indicating whether anything diff --git a/include/slang/ast/expressions/MiscExpressions.h b/include/slang/ast/expressions/MiscExpressions.h index dacc9acf6..ac4cd5f9b 100644 --- a/include/slang/ast/expressions/MiscExpressions.h +++ b/include/slang/ast/expressions/MiscExpressions.h @@ -75,8 +75,8 @@ class SLANG_EXPORT HierarchicalValueExpression : public ValueExpressionBase { /// Information about the hierarchical reference. HierarchicalReference ref; - HierarchicalValueExpression(const ValueSymbol& symbol, const HierarchicalReference& ref, - SourceRange sourceRange); + HierarchicalValueExpression(const Scope& scope, const ValueSymbol& symbol, + const HierarchicalReference& ref, SourceRange sourceRange); ConstantValue evalImpl(EvalContext& context) const; @@ -138,7 +138,7 @@ class SLANG_EXPORT ArbitrarySymbolExpression : public Expression { /// if this expression was created via hierarchical reference. HierarchicalReference hierRef; - ArbitrarySymbolExpression(const Symbol& symbol, const Type& type, + ArbitrarySymbolExpression(const Scope& scope, const Symbol& symbol, const Type& type, const HierarchicalReference* hierRef, SourceRange sourceRange); ConstantValue evalImpl(EvalContext&) const { return nullptr; } diff --git a/scripts/diagnostics.txt b/scripts/diagnostics.txt index 813bfc7d4..7f7352d42 100644 --- a/scripts/diagnostics.txt +++ b/scripts/diagnostics.txt @@ -1126,8 +1126,11 @@ error ConfigParamsIgnored "parameter overrides provided by config rule are ignor error MultipleTopDupName "more than one top module specified with the name '{}'" error NetAliasSelf "cannot alias a net to itself" error MultipleNetAlias "cannot specify an alias between the same bits of the same nets more than once" +error VirtualIfaceHierRef "interface containing hierarchical reference to object outside its body cannot be used in a virtual interface" +error VirtualIfaceIfacePort "interface with ports that reference other interfaces cannot be used in a virtual interface" note NoteAliasedTo "which is aliased to" note NoteAliasDeclaration "previous alias declaration here" +note NoteHierarchicalRef "hierarchical reference here" warning unused-def UnusedDefinition "{} definition is unused" warning unused-net UnusedNet "unused net '{}'" warning unused-implicit-net UnusedImplicitNet "implicit net '{}' is unused elsewhere" diff --git a/source/ast/Compilation.cpp b/source/ast/Compilation.cpp index ac425911a..26fee0c65 100644 --- a/source/ast/Compilation.cpp +++ b/source/ast/Compilation.cpp @@ -1254,6 +1254,26 @@ void Compilation::noteNetAlias(const Scope& scope, const Symbol& firstSym, } } +void Compilation::noteHierarchicalReference(const Scope& initialScope, + const HierarchicalReference& ref) { + // For now, we're only interested in upward names that cross + // through instance boundaries. + SLANG_ASSERT(ref.expr); + auto currScope = &initialScope; + for (size_t i = 0; i < ref.upwardCount; i++) { + auto& sym = currScope->asSymbol(); + if (sym.kind == SymbolKind::InstanceBody) + hierRefMap[&sym].push_back(&ref); + + currScope = sym.getHierarchicalParent(); + SLANG_ASSERT(currScope); + } +} + +void Compilation::noteVirtualIfaceInstance(const InstanceSymbol& symbol) { + virtualInterfaceInstances.push_back(&symbol); +} + const Expression* Compilation::getDefaultDisable(const Scope& scope) const { auto curr = &scope; while (true) { @@ -2197,6 +2217,27 @@ void Compilation::checkBindTargetParams(const syntax::BindDirectiveSyntax& synta } } +void Compilation::checkVirtualIfaceInstance(const InstanceSymbol& instance) { + auto body = instance.getCanonicalBody(); + if (!body) + body = &instance.body; + + if (auto it = hierRefMap.find(body); it != hierRefMap.end()) { + auto& diag = body->addDiag(diag::VirtualIfaceHierRef, instance.location); + for (auto ref : it->second) + diag.addNote(diag::NoteHierarchicalRef, ref->expr->sourceRange); + } + + Diagnostic* portDiag = nullptr; + for (auto port : body->getPortList()) { + if (port->kind == SymbolKind::InterfacePort) { + if (!portDiag) + portDiag = &body->addDiag(diag::VirtualIfaceIfacePort, instance.location); + portDiag->addNote(diag::NoteDeclarationHere, port->location); + } + } +} + void Compilation::resolveDefParamsAndBinds() { TimeTraceScope timeScope("resolveDefParamsAndBinds"sv, ""sv); diff --git a/source/ast/ElabVisitors.h b/source/ast/ElabVisitors.h index ec10a5ee6..ff45cc927 100644 --- a/source/ast/ElabVisitors.h +++ b/source/ast/ElabVisitors.h @@ -380,12 +380,11 @@ struct DiagnosticVisitor : public ASTVisitor { // in other instances to facilitate downstream consumers in not needing to recreate // this duplication detection logic again. SLANG_ASSERT(symbol.getCanonicalBody() == nullptr); - if (!compilation.hasFlag(CompilationFlags::DisableInstanceCaching)) { - auto [it, inserted] = instanceCache.try_emplace(symbol, &symbol.body); - if (!inserted) { - symbol.setCanonicalBody(*it->second); + auto [it, inserted] = instanceCache.try_emplace(symbol, &symbol.body); + if (!inserted) { + symbol.setCanonicalBody(*it->second); + if (!compilation.hasFlag(CompilationFlags::DisableInstanceCaching)) return; - } } if (visitInstances) @@ -474,9 +473,19 @@ struct DiagnosticVisitor : public ASTVisitor { void finalize() { // Once everything has been visited, go back over and check things that might - // have been influenced by visiting later symbols. Unfortunately visiting - // a specialization can trigger more specializations to be made for the - // same or other generic class, so we need to be careful here when iterating. + // have been influenced by visiting later symbols. + while (!compilation.virtualInterfaceInstances.empty()) { + auto vii = compilation.virtualInterfaceInstances; + compilation.virtualInterfaceInstances.clear(); + + for (auto inst : vii) { + inst->visit(*this); + compilation.checkVirtualIfaceInstance(*inst); + } + } + + // Visiting a specialization can trigger more specializations to be made for the + // same or other generic classes, so we need to be careful here when iterating. SmallSet visitedSpecs; SmallVector toVisit; bool didSomething; diff --git a/source/ast/Expression.cpp b/source/ast/Expression.cpp index e7e7af1b6..f23e4ecec 100644 --- a/source/ast/Expression.cpp +++ b/source/ast/Expression.cpp @@ -1423,8 +1423,9 @@ Expression* Expression::tryBindInterfaceRef(const ASTContext& context, if (symbol->kind == SymbolKind::UninstantiatedDef || (symbol->kind == SymbolKind::Variable && symbol->as().getType().isError())) { - return comp.emplace(*origSymbol, comp.getErrorType(), - nullptr, syntax.sourceRange()); + return comp.emplace(*context.scope, *origSymbol, + comp.getErrorType(), nullptr, + syntax.sourceRange()); } if (isInterfacePort && !origSymbol->name.empty()) { @@ -1498,7 +1499,8 @@ Expression* Expression::tryBindInterfaceRef(const ASTContext& context, } auto hierRef = HierarchicalReference::fromLookup(comp, result); - return comp.emplace(*origSymbol, *type, &hierRef, sourceRange); + return comp.emplace(*context.scope, *origSymbol, *type, &hierRef, + sourceRange); } void Expression::findPotentiallyImplicitNets( diff --git a/source/ast/expressions/MiscExpressions.cpp b/source/ast/expressions/MiscExpressions.cpp index d55940e4f..90d6a9af2 100644 --- a/source/ast/expressions/MiscExpressions.cpp +++ b/source/ast/expressions/MiscExpressions.cpp @@ -137,7 +137,8 @@ Expression& ValueExpressionBase::fromSymbol(const ASTContext& context, const Sym (symbol.kind == SymbolKind::ConstraintBlock && constraintAllowed) || (symbol.kind == SymbolKind::Coverpoint && flags.has(ASTFlags::AllowCoverpoint))) { // Special case for event expressions and constraint block built-in methods. - return *comp.emplace(symbol, comp.getVoidType(), hierRef, + return *comp.emplace(*context.scope, symbol, + comp.getVoidType(), hierRef, sourceRange); } @@ -174,10 +175,13 @@ Expression& ValueExpressionBase::fromSymbol(const ASTContext& context, const Sym } Expression* result; - if (hierRef && hierRef->target) - result = comp.emplace(value, *hierRef, sourceRange); - else + if (hierRef && hierRef->target) { + result = comp.emplace(*context.scope, value, *hierRef, + sourceRange); + } + else { result = comp.emplace(value, sourceRange); + } if (isUnbounded) result->type = &comp.getUnboundedType(); @@ -476,12 +480,15 @@ bool NamedValueExpression::checkConstant(EvalContext& context) const { return true; } -HierarchicalValueExpression::HierarchicalValueExpression(const ValueSymbol& symbol, +HierarchicalValueExpression::HierarchicalValueExpression(const Scope& scope, + const ValueSymbol& symbol, const HierarchicalReference& ref, SourceRange sourceRange) : ValueExpressionBase(ExpressionKind::HierarchicalValue, symbol, sourceRange), ref(ref) { SLANG_ASSERT(ref.target == &symbol); this->ref.expr = this; + + scope.getCompilation().noteHierarchicalReference(scope, this->ref); } ConstantValue HierarchicalValueExpression::evalImpl(EvalContext& context) const { @@ -545,7 +552,8 @@ void TypeReferenceExpression::serializeTo(ASTSerializer& serializer) const { serializer.write("targetType", targetType); } -ArbitrarySymbolExpression::ArbitrarySymbolExpression(const Symbol& symbol, const Type& type, +ArbitrarySymbolExpression::ArbitrarySymbolExpression(const Scope& scope, const Symbol& symbol, + const Type& type, const HierarchicalReference* hierRef, SourceRange sourceRange) : Expression(ExpressionKind::ArbitrarySymbol, type, sourceRange), symbol(&symbol) { @@ -553,6 +561,7 @@ ArbitrarySymbolExpression::ArbitrarySymbolExpression(const Symbol& symbol, const if (hierRef && hierRef->target) { this->hierRef = *hierRef; this->hierRef.expr = this; + scope.getCompilation().noteHierarchicalReference(scope, this->hierRef); } } @@ -572,8 +581,8 @@ Expression& ArbitrarySymbolExpression::fromSyntax(Compilation& comp, const NameS comp.noteReference(*symbol, context.flags.has(ASTFlags::LValue)); auto hierRef = HierarchicalReference::fromLookup(comp, result); - return *comp.emplace(*symbol, comp.getVoidType(), &hierRef, - syntax.sourceRange()); + return *comp.emplace(*context.scope, *symbol, comp.getVoidType(), + &hierRef, syntax.sourceRange()); } void ArbitrarySymbolExpression::serializeTo(ASTSerializer& serializer) const { diff --git a/source/ast/symbols/InstanceSymbols.cpp b/source/ast/symbols/InstanceSymbols.cpp index 81b4d38d5..b16767160 100644 --- a/source/ast/symbols/InstanceSymbols.cpp +++ b/source/ast/symbols/InstanceSymbols.cpp @@ -1305,7 +1305,9 @@ static const AssertionExpr* bindUnknownPortConn(const ASTContext& context, auto hierRef = HierarchicalReference::fromLookup(comp, result); auto hre = comp.emplace( - *symbol, comp.getVoidType(), &hierRef, syntax.sourceRange()); + *context.scope, *symbol, comp.getVoidType(), &hierRef, + syntax.sourceRange()); + return comp.emplace(*hre, std::nullopt); } } diff --git a/source/ast/types/AllTypes.cpp b/source/ast/types/AllTypes.cpp index ee80811f2..8a2ca43c0 100644 --- a/source/ast/types/AllTypes.cpp +++ b/source/ast/types/AllTypes.cpp @@ -1144,6 +1144,7 @@ const Type& VirtualInterfaceType::fromSyntax(const ASTContext& context, auto loc = syntax.name.location(); auto& iface = InstanceSymbol::createVirtual(context, loc, def->as(), syntax.parameters); + comp.noteVirtualIfaceInstance(iface); const ModportSymbol* modport = nullptr; std::string_view modportName = syntax.modport ? syntax.modport->member.valueText() : ""sv; diff --git a/tests/unittests/ast/InterfaceTests.cpp b/tests/unittests/ast/InterfaceTests.cpp index de246dcba..41d573760 100644 --- a/tests/unittests/ast/InterfaceTests.cpp +++ b/tests/unittests/ast/InterfaceTests.cpp @@ -749,3 +749,86 @@ endmodule compilation.addSyntaxTree(tree); NO_COMPILATION_ERRORS; } + +TEST_CASE("Virtual interface declaration errors") { + auto tree = SyntaxTree::fromText(R"( +localparam type requestType = byte; +localparam type responseType = int; + +interface I; + wire r; + modport ii(input r); +endinterface + +module testMod#(N=16); + wire clk, rst; + I i(); + + allIfc#(N) allInst(clk, rst, i, i.ii); + + virtual allIfc#(N) allInst1; + sliceIfc sliceInst(); + virtual sliceIfc sliceInst1; +endmodule:testMod + +interface automatic allIfc#(N=1)(input clk, rst, I i, I.ii i1); + var requestType Requests[N]; + var responseType Responses[N]; + + function requestType requestRead(int index); + return Requests[index]; + endfunction + + function void responseWrite(int index, responseType response); + Responses[index] <= response; + endfunction + + modport clientMp(output Requests, input Responses, + input clk, rst); + modport serverMp(input Requests, output Responses, + import requestRead, responseWrite, + input clk, rst); +endinterface:allIfc + +interface automatic sliceIfc#(I=0)(); + interface II(); + logic reset; + endinterface + + II ii(); + wire reset = ii.reset; + + I i(); + allIfc allInst(.clk(), .rst(), .i(i), .i1(i.ii)); + + var requestType request; + var responseType response; + + assign allInst.Requests[I] = request; + assign response = allInst.Responses[I]; + + function void requestWrite(requestType req); + request <= req; + endfunction + + function responseType responseRead(); + return response; + endfunction + + wire clk = testMod.clk; // invalid + wire rst = testMod.rst; // invalid + + modport clientMp(output request, input response, + import requestWrite, responseRead, + input clk, rst); +endinterface:sliceIfc +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + + auto& diags = compilation.getAllDiagnostics(); + REQUIRE(diags.size() == 2); + CHECK(diags[0].code == diag::VirtualIfaceIfacePort); + CHECK(diags[1].code == diag::VirtualIfaceHierRef); +} diff --git a/tests/unittests/ast/ParameterTests.cpp b/tests/unittests/ast/ParameterTests.cpp index b9a27cbf5..f074e2021 100644 --- a/tests/unittests/ast/ParameterTests.cpp +++ b/tests/unittests/ast/ParameterTests.cpp @@ -630,7 +630,7 @@ endmodule auto& diags = compilation.getAllDiagnostics(); REQUIRE(diags.size() == 5); CHECK(diags[0].code == diag::RecursiveDefinition); - CHECK(diags[1].code == diag::RecursiveDefinition); + CHECK(diags[1].code == diag::ConstEvalParamCycle); CHECK(diags[2].code == diag::ConstEvalParamCycle); CHECK(diags[3].code == diag::ConstEvalIdUsedInCEBeforeDecl); CHECK(diags[4].code == diag::ConstEvalFunctionIdentifiersMustBeLocal);