diff --git a/include/slang/ast/symbols/InstanceSymbols.h b/include/slang/ast/symbols/InstanceSymbols.h index f00215b31..fd53da36c 100644 --- a/include/slang/ast/symbols/InstanceSymbols.h +++ b/include/slang/ast/symbols/InstanceSymbols.h @@ -10,6 +10,7 @@ #include "slang/ast/ASTContext.h" #include "slang/ast/Scope.h" #include "slang/ast/Symbol.h" +#include "slang/ast/expressions/MiscExpressions.h" #include "slang/numeric/ConstantValue.h" #include "slang/syntax/SyntaxFwd.h" @@ -164,6 +165,10 @@ class SLANG_EXPORT InstanceBodySymbol : public Symbol, public Scope { /// Flags that describe properties of the instance. bitmask flags; + /// Collected source ranges of hierarchical identifiers used inside instance body + /// for later diagnostic. + mutable std::optional> hierIdentifiers; + InstanceBodySymbol(Compilation& compilation, const DefinitionSymbol& definition, const HierarchyOverrideNode* hierarchyOverrideNode, bitmask flags); @@ -184,6 +189,9 @@ class SLANG_EXPORT InstanceBodySymbol : public Symbol, public Scope { bool hasSameType(const InstanceBodySymbol& other) const; + /// Fill hierIdentifiers list. + void collectHierIdents(Compilation& comp) const; + static InstanceBodySymbol& fromDefinition( Compilation& compilation, const DefinitionSymbol& definition, SourceLocation instanceLoc, bitmask flags, const HierarchyOverrideNode* hierarchyOverrideNode, @@ -199,9 +207,28 @@ class SLANG_EXPORT InstanceBodySymbol : public Symbol, public Scope { static bool isKind(SymbolKind kind) { return kind == SymbolKind::InstanceBody; } + static void processHierIdent(const HierarchicalValueExpression& hVE, + const InstanceBodySymbol& ifaceBody, + SmallVector& hierIdents) { + const auto& sym = hVE.symbol; + const auto* parentScope = sym.getParentScope(); + // Checking whether a symbol belongs to the interface definition body + while (parentScope->asSymbol().kind != SymbolKind::CompilationUnit) { + if (const auto symScope = parentScope->asSymbol().as_if(); + symScope && symScope == &ifaceBody) + break; + parentScope = parentScope->asSymbol().getParentScope(); + } + + if (parentScope->asSymbol().kind == SymbolKind::CompilationUnit) + hierIdents.push_back(hVE.sourceRange); + } + private: friend class Scope; + class HierIdentsVisitor; + void setPorts(std::span ports) const { portList = ports; } const DefinitionSymbol& definition; diff --git a/scripts/diagnostics.txt b/scripts/diagnostics.txt index 5433f4e43..65b6727f8 100644 --- a/scripts/diagnostics.txt +++ b/scripts/diagnostics.txt @@ -489,6 +489,7 @@ error TimingCheckEventEdgeRequired "'{}' event argument requires an edge trigger error NoChangeEdgeRequired "trigger for first argument to '$nochange' must be posedge or negedge only" error PathPulseInvalidPathName "PATHPULSE$ path '{}' must be of the form 'source$dest'" error VirtualInterfaceIfaceMember "virtual interfaces cannot be used as the type for interface items" +error VirtualInterfaceHierRef "Declaration of virtual interface should not contain hierarchical references to objects outside its body" error DefparamBadHierarchy "defparam in a hierarchy in or under a generate block, array of instances, or bind instantiation cannot change a parameter value outside that hierarchy" error BindTargetPrimitive "cannot bind a primitive" error BindUnderBind "cannot bind an instance underneath the scope of another bind instance" diff --git a/source/ast/ElabVisitors.h b/source/ast/ElabVisitors.h index bb55f687d..bb07273cf 100644 --- a/source/ast/ElabVisitors.h +++ b/source/ast/ElabVisitors.h @@ -18,7 +18,7 @@ using namespace syntax; // This visitor is used to touch every node in the AST to ensure that all lazily // evaluated members have been realized and we have recorded every diagnostic. -struct DiagnosticVisitor : public ASTVisitor { +struct DiagnosticVisitor : public ASTVisitor { DiagnosticVisitor(Compilation& compilation, const size_t& numErrors, uint32_t errorLimit) : compilation(compilation), numErrors(numErrors), errorLimit(errorLimit) {} @@ -336,8 +336,25 @@ struct DiagnosticVisitor : public ASTVisitor { return; } - if (visitInstances) + if (visitInstances) { + // Collect hierarchical identifiers for current instance + SmallVector hierIdents; + instBodiesHierIdents.push_back(std::make_pair(&symbol.body, hierIdents)); visit(symbol.body); + symbol.body.hierIdentifiers = instBodiesHierIdents.back().second.copy(compilation); + instBodiesHierIdents.pop_back(); + } + } + + void handle(const HierarchicalValueExpression& hVE) { + if (instBodiesHierIdents.empty()) { + visitDefault(hVE); + return; + } + + InstanceBodySymbol::processHierIdent(hVE, *instBodiesHierIdents.back().first, + instBodiesHierIdents.back().second); + visitDefault(hVE); } void handle(const SubroutineSymbol& symbol) { @@ -466,6 +483,10 @@ struct DiagnosticVisitor : public ASTVisitor { SmallVector externIfaceProtos; SmallVector> modportsWithExports; TimingPathMap timingPathMap; + +private: + SmallVector>, 4> + instBodiesHierIdents; }; // This visitor is for finding all defparam directives in the hierarchy. diff --git a/source/ast/symbols/InstanceSymbols.cpp b/source/ast/symbols/InstanceSymbols.cpp index 96290dc28..c0c045588 100644 --- a/source/ast/symbols/InstanceSymbols.cpp +++ b/source/ast/symbols/InstanceSymbols.cpp @@ -1126,6 +1126,28 @@ bool InstanceBodySymbol::hasSameType(const InstanceBodySymbol& other) const { return true; } +class InstanceBodySymbol::HierIdentsVisitor : public ASTVisitor { +public: + SmallVector hierIdentsSR; + + HierIdentsVisitor(const InstanceBodySymbol& ifaceBody) : ifaceBody(ifaceBody){}; + + void handle(const HierarchicalValueExpression& hVE) { + processHierIdent(hVE, ifaceBody, hierIdentsSR); + } + +private: + const InstanceBodySymbol& ifaceBody; +}; + +void InstanceBodySymbol::collectHierIdents(Compilation& comp) const { + if (!hierIdentifiers.has_value()) { + HierIdentsVisitor hIdentsVisitor(*this); + hIdentsVisitor.visit(*this); + hierIdentifiers = hIdentsVisitor.hierIdentsSR.copy(comp); + } +} + void InstanceBodySymbol::serializeTo(ASTSerializer& serializer) const { serializer.writeLink("definition", definition); } diff --git a/source/ast/types/AllTypes.cpp b/source/ast/types/AllTypes.cpp index 15443d09e..f3834e9a3 100644 --- a/source/ast/types/AllTypes.cpp +++ b/source/ast/types/AllTypes.cpp @@ -1128,9 +1128,48 @@ const Type& VirtualInterfaceType::fromSyntax(const ASTContext& context, } auto loc = syntax.name.location(); + auto& defSym = def->as(); auto& iface = InstanceSymbol::createVirtual(context, loc, def->as(), syntax.parameters); + // To control duplicates + SmallSet visited; + // Find cached instance hierarchical identifiers + std::function(const Scope& scope)> findInst = + [&defSym, &findInst, &visited](const Scope& scope) -> std::optional { + for (auto& name : scope.getNameMap()) { + if (auto inst = name.second->as_if()) { + auto instDef = &inst->getDefinition(); + if (instDef == &defSym && inst->body.hierIdentifiers.has_value()) + return inst; + + if (visited.contains(instDef)) + continue; + visited.insert(instDef); + + if (auto founded = findInst(inst->body); founded.has_value()) + return founded; + } + } + return std::nullopt; + }; + + auto& scope = comp.getRoot(); + auto founded = findInst(scope); + const InstanceSymbol* toReport = nullptr; + if (!founded.has_value()) { + iface.body.collectHierIdents(comp); + toReport = &iface; + } + else { + toReport = founded.value(); + } + + for (auto& sR : toReport->body.hierIdentifiers.value()) { + auto& diag = context.addDiag(diag::VirtualInterfaceHierRef, sR); + diag.addNote(diag::NoteDeclarationHere, iface.location); + } + const ModportSymbol* modport = nullptr; std::string_view modportName = syntax.modport ? syntax.modport->member.valueText() : ""sv; if (!modportName.empty() && syntax.modport) { diff --git a/tests/unittests/ast/InterfaceTests.cpp b/tests/unittests/ast/InterfaceTests.cpp index 402ec8a6d..c2586514b 100644 --- a/tests/unittests/ast/InterfaceTests.cpp +++ b/tests/unittests/ast/InterfaceTests.cpp @@ -724,3 +724,127 @@ endmodule REQUIRE(diags.size() == 1); CHECK(diags[0].code == diag::UndeclaredIdentifier); } + +TEST_CASE("Virtual interface declaration errors") { + auto tree = SyntaxTree::fromText(R"( +localparam type requestType = byte; +localparam type responseType = int; + +module testMod#(N=16); + + wire clk, rst; + + allIfc#(N) allInst(clk, rst); + sliceIfc sliceInst(); + virtual sliceIfc sliceInst1; + +endmodule:testMod + +module testMod1#(N=16); + + wire clk, rst; + + allIfc#(N) allInst(clk, rst); + virtual sliceIfc1 sliceInst; + +endmodule:testMod1 + +interface automatic allIfc#(N=1)(input clk, rst); + + 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(); // invalid + wire reset = ii.reset; // valid + + allIfc allInst(.clk(), .rst()); + var requestType request; + var responseType response; + + assign allInst.Requests[I] = request; // invalid + assign response = allInst.Responses[I]; // invalid + + function void requestWrite(requestType req); + request <= req; + endfunction + + function responseType responseRead(); + return response; + endfunction + + wire clk = allInst.clk; // invalid + wire rst = allInst.rst; // invalid + + modport clientMp(output request, input response, + import requestWrite, responseRead, + input clk, rst); + +endinterface:sliceIfc + +interface automatic sliceIfc1#(I=0)(); + interface II(); + logic reset; + endinterface + II ii(); // invalid + wire reset = ii.reset; // valid + + allIfc allInst(.clk(), .rst()); + var requestType request; + var responseType response; + + assign allInst.Requests[I] = request; // invalid + assign response = allInst.Responses[I]; // invalid + + function void requestWrite(requestType req); + request <= req; + endfunction + + function responseType responseRead(); + return response; + endfunction + + wire clk = allInst.clk; // invalid + wire rst = allInst.rst; // invalid + + modport clientMp(output request, input response, + import requestWrite, responseRead, + input clk, rst); + +endinterface:sliceIfc1 +)"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + + auto& diags = compilation.getAllDiagnostics(); + REQUIRE(diags.size() == 8); + CHECK(diags[0].code == diag::VirtualInterfaceHierRef); + CHECK(diags[1].code == diag::VirtualInterfaceHierRef); + CHECK(diags[2].code == diag::VirtualInterfaceHierRef); + CHECK(diags[3].code == diag::VirtualInterfaceHierRef); + CHECK(diags[4].code == diag::VirtualInterfaceHierRef); + CHECK(diags[5].code == diag::VirtualInterfaceHierRef); + CHECK(diags[6].code == diag::VirtualInterfaceHierRef); + CHECK(diags[7].code == diag::VirtualInterfaceHierRef); +}