From 3e5de874ab3f17702553f15ff52b823bcfd49dcd Mon Sep 17 00:00:00 2001 From: Arkadiusz Kozdra Date: Fri, 16 Sep 2022 12:04:24 +0200 Subject: [PATCH 01/11] Parse virtual interfaces as iface refs --- src/V3AstNodeDType.h | 9 +++++++++ src/verilog.y | 30 ++++++++++++++++------------ test_regress/t/t_class_unsup_bad.out | 7 ------- 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/src/V3AstNodeDType.h b/src/V3AstNodeDType.h index c3f4d096e9..ca30090a29 100644 --- a/src/V3AstNodeDType.h +++ b/src/V3AstNodeDType.h @@ -755,6 +755,7 @@ class AstEnumDType final : public AstNodeDType { }; class AstIfaceRefDType final : public AstNodeDType { // Reference to an interface, either for a port, or inside parent cell + // @astgen op1 := paramsp : List[AstPin] private: FileLine* m_modportFileline; // Where modport token was string m_cellName; // "" = no cell, such as when connects to 'input' iface @@ -777,6 +778,14 @@ class AstIfaceRefDType final : public AstNodeDType { , m_cellName{cellName} , m_ifaceName{ifaceName} , m_modportName{modport} {} + AstIfaceRefDType(FileLine* fl, FileLine* modportFl, const string& cellName, + const string& ifaceName, const string& modport, AstPin* paramsp) + : ASTGEN_SUPER_IfaceRefDType(fl) + , m_modportFileline{modportFl} + , m_cellName{cellName} + , m_ifaceName{ifaceName} { + addParamsp(paramsp); + } ASTGEN_MEMBERS_AstIfaceRefDType; // METHODS const char* broken() const override; diff --git a/src/verilog.y b/src/verilog.y index 86ecb76043..eba4bde68c 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -1401,9 +1401,9 @@ port: // ==IEEE: port VARDTYPE(new AstIfaceRefDType($2, $4, "", *$2, *$4)); addNextNull($$, VARDONEP($$,$6,$7)); } | portDirNetE yINTERFACE portSig rangeListE sigAttrListE - { $$ = nullptr; BBUNSUP($2, "Unsupported: virtual or generic interfaces"); } + { $$ = nullptr; BBUNSUP($2, "Unsupported: generic interfaces"); } | portDirNetE yINTERFACE '.' idAny/*modport*/ portSig rangeListE sigAttrListE - { $$ = nullptr; BBUNSUP($2, "Unsupported: virtual or generic interfaces"); } + { $$ = nullptr; BBUNSUP($2, "Unsupported: generic interfaces"); } // // // IEEE: ansi_port_declaration, with [port_direction] removed // // IEEE: [ net_port_header | interface_port_header ] @@ -1955,6 +1955,11 @@ data_type: // ==IEEE: data_type | packageClassScopeE idType parameter_value_assignmentClass packed_dimensionListE { AstRefDType* const refp = new AstRefDType{$2, *$2, $1, $3}; $$ = GRAMMARP->createArray(refp, $4, true); } + // // Rules overlap virtual_interface_declaration + | yVIRTUAL__INTERFACE yINTERFACE data_typeVirtual + { $$ = $3; } + | yVIRTUAL__anyID data_typeVirtual + { $$ = $2; } ; data_typeBasic: // IEEE: part of data_type @@ -1980,23 +1985,22 @@ data_typeNoRef: // ==IEEE: data_type, excluding class_ty { $$ = new AstBasicDType{$1, VBasicDTypeKwd::CHANDLE}; } | yEVENT { $$ = new AstBasicDType{$1, VBasicDTypeKwd::EVENT}; v3Global.setHasEvents(); } - // // Rules overlap virtual_interface_declaration - // // Parameters here are SV2009 - // // IEEE has ['.' modport] but that will conflict with port - // // declarations which decode '.' modport themselves, so - // // instead see data_typeVar - | yVIRTUAL__INTERFACE yINTERFACE id/*interface*/ - { $$ = new AstBasicDType{$1, VBasicDTypeKwd::CHANDLE}; - BBUNSUP($1, "Unsupported: virtual interface"); } - | yVIRTUAL__anyID id/*interface*/ - { $$ = new AstBasicDType{$1, VBasicDTypeKwd::CHANDLE}; - BBUNSUP($1, "Unsupported: virtual data type"); } | type_reference { $$ = $1; } // // IEEE: class_scope: see data_type above // // IEEE: class_type: see data_type above // // IEEE: ps_covergroup: see data_type above ; +data_typeVirtual: // ==IEEE: data_type after yVIRTUAL [ yINTERFACE ] + // // Parameters here are SV2009 + id/*interface*/ { $$ = new AstIfaceRefDType{$1, "", *$1}; } + | id/*interface*/ '.' id/*modport*/ { $$ = new AstIfaceRefDType{$1, $3, "", *$1, *$3}; } + | id/*interface*/ parameter_value_assignmentClass + { $$ = new AstIfaceRefDType{$1, nullptr, "", *$1, "", $2}; } + | id/*interface*/ parameter_value_assignmentClass '.' id/*modport*/ + { $$ = new AstIfaceRefDType{$1, $4, "", *$1, *$4, $2}; } + ; + data_type_or_void: // ==IEEE: data_type_or_void data_type { $$ = $1; } //UNSUP yVOID { UNSUP } // No yTAGGED structures diff --git a/test_regress/t/t_class_unsup_bad.out b/test_regress/t/t_class_unsup_bad.out index eba10d5378..4e3bc999fd 100644 --- a/test_regress/t/t_class_unsup_bad.out +++ b/test_regress/t/t_class_unsup_bad.out @@ -1,10 +1,3 @@ -%Error-UNSUPPORTED: t/t_class_unsup_bad.v:7:1: Unsupported: virtual interface - 7 | virtual interface vi_t vi; - | ^~~~~~~ - ... For error description see https://verilator.org/warn/UNSUPPORTED?v=latest -%Error-UNSUPPORTED: t/t_class_unsup_bad.v:8:1: Unsupported: virtual data type - 8 | virtual vi_t vi2; - | ^~~~~~~ %Error: t/t_class_unsup_bad.v:29:24: Syntax error: 'const'/'rand'/'randc' not allowed before function/task declaration 29 | const function void func_const; endfunction | ^~~~~~~~~~ From 8bb8b27bf1aba387830675422324cc7c0b3f93ff Mon Sep 17 00:00:00 2001 From: Arkadiusz Kozdra Date: Wed, 12 Oct 2022 14:27:09 +0200 Subject: [PATCH 02/11] Use member selection for virtual interface vars --- src/V3AstNodeOther.h | 4 ++ src/V3AstNodes.cpp | 2 + src/V3Common.cpp | 2 +- src/V3Const.cpp | 1 + src/V3Dead.cpp | 2 +- src/V3EmitCFunc.cpp | 2 + src/V3EmitCFunc.h | 3 ++ src/V3Life.cpp | 4 +- src/V3LinkDot.cpp | 6 +-- src/V3Localize.cpp | 1 + src/V3Scope.cpp | 27 +++++++----- src/V3Width.cpp | 34 +++++++++++++++ test_regress/t/t_interface_virtual.out | 5 +++ test_regress/t/t_interface_virtual.pl | 22 ++++++++++ test_regress/t/t_interface_virtual.v | 58 ++++++++++++++++++++++++++ 15 files changed, 156 insertions(+), 17 deletions(-) create mode 100644 test_regress/t/t_interface_virtual.out create mode 100755 test_regress/t/t_interface_virtual.pl create mode 100644 test_regress/t/t_interface_virtual.v diff --git a/src/V3AstNodeOther.h b/src/V3AstNodeOther.h index 3ef73b94a9..d61105b58b 100644 --- a/src/V3AstNodeOther.h +++ b/src/V3AstNodeOther.h @@ -2000,6 +2000,7 @@ class AstVar final : public AstNode { bool m_sigUserRdPublic : 1; // User C code accesses this signal, read only bool m_sigUserRWPublic : 1; // User C code accesses this signal, read-write bool m_usedClock : 1; // Signal used as a clock + bool m_usedVirtIface : 1; // Signal used through a virtual interface bool m_usedParam : 1; // Parameter is referenced (on link; later signals not setup) bool m_usedLoopIdx : 1; // Variable subject of for unrolling bool m_funcLocal : 1; // Local variable for a function @@ -2038,6 +2039,7 @@ class AstVar final : public AstNode { m_scClocked = false; m_scSensitive = false; m_usedClock = false; + m_usedVirtIface = false; m_usedParam = false; m_usedLoopIdx = false; m_sigPublic = false; @@ -2184,6 +2186,7 @@ class AstVar final : public AstNode { void attrSFormat(bool flag) { m_attrSFormat = flag; } void attrSplitVar(bool flag) { m_attrSplitVar = flag; } void usedClock(bool flag) { m_usedClock = flag; } + void usedVirtIface(bool flag) { m_usedVirtIface = flag; } void usedParam(bool flag) { m_usedParam = flag; } void usedLoopIdx(bool flag) { m_usedLoopIdx = flag; } void sigPublic(bool flag) { m_sigPublic = flag; } @@ -2267,6 +2270,7 @@ class AstVar final : public AstNode { return bdtypep && bdtypep->isBitLogic(); } bool isUsedClock() const { return m_usedClock; } + bool isUsedVirtIface() const { return m_usedVirtIface; } bool isUsedParam() const { return m_usedParam; } bool isUsedLoopIdx() const { return m_usedLoopIdx; } bool isSc() const { return m_sc; } diff --git a/src/V3AstNodes.cpp b/src/V3AstNodes.cpp index 5ea8c44c6f..bacaf4fa03 100644 --- a/src/V3AstNodes.cpp +++ b/src/V3AstNodes.cpp @@ -724,6 +724,8 @@ AstNodeDType::CTypeRecursed AstNodeDType::cTypeRecurse(bool compound) const { info.m_type += ">"; } else if (const auto* const adtypep = VN_CAST(dtypep, ClassRefDType)) { info.m_type = "VlClassRef<" + EmitCBaseVisitor::prefixNameProtect(adtypep) + ">"; + } else if (const auto* const adtypep = VN_CAST(dtypep, IfaceRefDType)) { + info.m_type = EmitCBaseVisitor::prefixNameProtect(adtypep->ifaceViaCellp()) + "*"; } else if (const auto* const adtypep = VN_CAST(dtypep, UnpackArrayDType)) { if (adtypep->isCompound()) compound = true; const CTypeRecursed sub = adtypep->subDTypep()->cTypeRecurse(compound); diff --git a/src/V3Common.cpp b/src/V3Common.cpp index 9961585b9a..dd0cc52501 100644 --- a/src/V3Common.cpp +++ b/src/V3Common.cpp @@ -68,7 +68,7 @@ static void makeToStringMiddle(AstClass* nodep) { std::string comma; for (AstNode* itemp = nodep->membersp(); itemp; itemp = itemp->nextp()) { if (const auto* const varp = VN_CAST(itemp, Var)) { - if (!varp->isParam()) { + if (!varp->isParam() && !VN_IS(itemp->dtypep(), IfaceRefDType)) { string stmt = "out += \""; stmt += comma; comma = ", "; diff --git a/src/V3Const.cpp b/src/V3Const.cpp index cf90d21562..c45776628d 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -2607,6 +2607,7 @@ class ConstVisitor final : public VNVisitor { && v3Global.opt.fConst() // Default value, not a "known" constant for this usage && !nodep->varp()->isClassMember() + && !nodep->varp()->isUsedVirtIface() && !(nodep->varp()->isFuncLocal() && nodep->varp()->isNonOutput()) && !nodep->varp()->noSubst() && !nodep->varp()->isSigPublic()) || nodep->varp()->isParam())) { diff --git a/src/V3Dead.cpp b/src/V3Dead.cpp index 3c396f0802..843c2e6284 100644 --- a/src/V3Dead.cpp +++ b/src/V3Dead.cpp @@ -321,7 +321,7 @@ class DeadVisitor final : public VNVisitor { } bool mightElimVar(AstVar* nodep) const { if (nodep->isSigPublic()) return false; // Can't elim publics! - if (nodep->isIO() || nodep->isClassMember()) return false; + if (nodep->isIO() || nodep->isClassMember() || nodep->isUsedVirtIface()) return false; if (nodep->isTemp() && !nodep->isTrace()) return true; return m_elimUserVars; // Post-Trace can kill most anything } diff --git a/src/V3EmitCFunc.cpp b/src/V3EmitCFunc.cpp index 00c2baab2e..21dd9b9161 100644 --- a/src/V3EmitCFunc.cpp +++ b/src/V3EmitCFunc.cpp @@ -661,6 +661,8 @@ string EmitCFunc::emitVarResetRecurse(const AstVar* varp, const string& varNameP suffix + ".atDefault()" + cvtarray); } else if (VN_IS(dtypep, ClassRefDType)) { return ""; // Constructor does it + } else if (VN_IS(dtypep, IfaceRefDType)) { + return varNameProtected + suffix + " = nullptr;\n"; } else if (const AstDynArrayDType* const adtypep = VN_CAST(dtypep, DynArrayDType)) { // Access std::array as C array const string cvtarray = (adtypep->subDTypep()->isWide() ? ".data()" : ""); diff --git a/src/V3EmitCFunc.h b/src/V3EmitCFunc.h index 8369386bd0..e350dfb2ea 100644 --- a/src/V3EmitCFunc.h +++ b/src/V3EmitCFunc.h @@ -1145,6 +1145,9 @@ class EmitCFunc VL_NOT_FINAL : public EmitCConstInit { } else if (VN_IS(varModp, Class) && varModp != m_modp) { // Superclass member reference puts(prefixNameProtect(varModp) + "::"); + } else if (varp->isIfaceRef()) { + puts(nodep->selfPointerProtect(m_useSelfForThis)); + return; } else if (!nodep->selfPointer().empty()) { emitDereference(nodep->selfPointerProtect(m_useSelfForThis)); } diff --git a/src/V3Life.cpp b/src/V3Life.cpp index 41d3ab9827..1c4bea7dfc 100644 --- a/src/V3Life.cpp +++ b/src/V3Life.cpp @@ -141,7 +141,7 @@ class LifeBlock final { void checkRemoveAssign(const LifeMap::iterator& it) { const AstVar* const varp = it->first->varp(); LifeVarEntry* const entp = &(it->second); - if (!varp->isSigPublic()) { + if (!varp->isSigPublic() && !varp->isUsedVirtIface()) { // Rather than track what sigs AstUCFunc/AstUCStmt may change, // we just don't optimize any public sigs // Check the var entry, and remove if appropriate @@ -186,7 +186,7 @@ class LifeBlock final { const auto it = m_map.find(nodep); if (it != m_map.end()) { if (AstConst* const constp = it->second.constNodep()) { - if (!varrefp->varp()->isSigPublic()) { + if (!varrefp->varp()->isSigPublic() && !varrefp->varp()->isUsedVirtIface()) { // Aha, variable is constant; substitute in. // We'll later constant propagate UINFO(4, " replaceconst: " << varrefp << endl); diff --git a/src/V3LinkDot.cpp b/src/V3LinkDot.cpp index 0611e62977..1f74fb1e73 100644 --- a/src/V3LinkDot.cpp +++ b/src/V3LinkDot.cpp @@ -2388,7 +2388,7 @@ class LinkDotResolveVisitor final : public VNVisitor { m_ds.m_dotSymp = foundp; m_ds.m_dotPos = DP_SCOPE; // Upper AstDot visitor will handle it from here - } else if (VN_IS(foundp->nodep(), Cell) && allowVar && m_cellp) { + } else if (VN_IS(foundp->nodep(), Cell) && allowVar) { AstCell* const cellp = VN_AS(foundp->nodep(), Cell); if (VN_IS(cellp->modp(), Iface)) { // Interfaces can be referenced like a variable for interconnect @@ -2406,7 +2406,7 @@ class LinkDotResolveVisitor final : public VNVisitor { m_ds.m_dotPos = DP_SCOPE; UINFO(9, " cell -> iface varref " << foundp->nodep() << endl); AstNode* const newp - = new AstVarRef(ifaceRefVarp->fileline(), ifaceRefVarp, VAccess::READ); + = new AstVarRef{nodep->fileline(), ifaceRefVarp, VAccess::READ}; nodep->replaceWith(newp); VL_DO_DANGLING(pushDeletep(nodep), nodep); } else if (VN_IS(cellp->modp(), NotFoundModule)) { @@ -2417,7 +2417,7 @@ class LinkDotResolveVisitor final : public VNVisitor { } else if (AstVar* const varp = foundToVarp(foundp, nodep, VAccess::READ)) { AstIfaceRefDType* const ifacerefp = LinkDotState::ifaceRefFromArray(varp->subDTypep()); - if (ifacerefp) { + if (ifacerefp && varp->isIfaceRef()) { UASSERT_OBJ(ifacerefp->ifaceViaCellp(), ifacerefp, "Unlinked interface"); // Really this is a scope reference into an interface UINFO(9, "varref-ifaceref " << m_ds.m_dotText << " " << nodep << endl); diff --git a/src/V3Localize.cpp b/src/V3Localize.cpp index 0d5977dcc6..1de5d06e39 100644 --- a/src/V3Localize.cpp +++ b/src/V3Localize.cpp @@ -169,6 +169,7 @@ class LocalizeVisitor final : public VNVisitor { && !nodep->varp()->isFuncLocal() // Not already a function local (e.g.: argument) && !nodep->varp()->isStatic() // Not a static variable && !nodep->varp()->isClassMember() // Statically exists in design hierarchy + && !nodep->varp()->isUsedVirtIface() // Not used through a virtual interface && !nodep->varp()->valuep() // Does not have an initializer ) { UINFO(4, "Consider for localization: " << nodep << endl); diff --git a/src/V3Scope.cpp b/src/V3Scope.cpp index c0694e2ab6..45052ed871 100644 --- a/src/V3Scope.cpp +++ b/src/V3Scope.cpp @@ -43,6 +43,7 @@ class ScopeVisitor final : public VNVisitor { private: // NODE STATE // AstVar::user1p -> AstVarScope replacement for this variable + // AstCell::user2p -> AstScope*. The scope created inside the cell // AstTask::user2p -> AstTask*. Replacement task const VNUser1InUse m_inuser1; const VNUser2InUse m_inuser2; @@ -125,6 +126,10 @@ class ScopeVisitor final : public VNVisitor { AstNodeModule* const modp = cellp->modp(); UASSERT_OBJ(modp, cellp, "Unlinked mod"); iterate(modp); // Recursive call to visit(AstNodeModule) + if (VN_IS(modp, Iface)) { + // Remember newly created scope + cellp->user2p(m_scopep); + } } } } @@ -261,7 +266,13 @@ class ScopeVisitor final : public VNVisitor { void visit(AstVar* nodep) override { // Make new scope variable if (!nodep->user1p()) { - AstVarScope* const varscp = new AstVarScope(nodep->fileline(), m_scopep, nodep); + AstScope* scopep = m_scopep; + if (AstIfaceRefDType* const ifacerefp = VN_CAST(nodep->dtypep(), IfaceRefDType)) { + // Attach every non-virtual interface variable its inner scope + if (ifacerefp->cellp()) + scopep = VN_AS(ifacerefp->cellp()->user2p(), Scope); + } + AstVarScope* const varscp = new AstVarScope{nodep->fileline(), scopep, nodep}; UINFO(6, " New scope " << varscp << endl); if (m_aboveCellp && !m_aboveCellp->isTrace()) varscp->trace(false); nodep->user1p(varscp); @@ -280,15 +291,11 @@ class ScopeVisitor final : public VNVisitor { // VarRef needs to point to VarScope // Make sure variable has made user1p. UASSERT_OBJ(nodep->varp(), nodep, "Unlinked"); - if (nodep->varp()->isIfaceRef()) { - nodep->varScopep(nullptr); - } else { - // We may have not made the variable yet, and we can't make it now as - // the var's referenced package etc might not be created yet. - // So push to a list and post-correct. - // No check here for nodep->classOrPackagep(), will check when walk list. - m_varRefScopes.emplace(nodep, m_scopep); - } + // We may have not made the variable yet, and we can't make it now as + // the var's referenced package etc might not be created yet. + // So push to a list and post-correct. + // No check here for nodep->classOrPackagep(), will check when walk list. + m_varRefScopes.emplace(nodep, m_scopep); } void visit(AstScopeName* nodep) override { // If there's a %m in the display text, we add a special node that will contain the name() diff --git a/src/V3Width.cpp b/src/V3Width.cpp index e7901340a7..d98af06333 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -2523,6 +2523,20 @@ class WidthVisitor final : public VNVisitor { << foundp->warnOther() << "... Location of found object\n" << foundp->warnContextSecondary()); } + } else if (AstIfaceRefDType* const adtypep = VN_CAST(fromDtp, IfaceRefDType)) { + if (AstNode* const foundp = memberSelIface(nodep, adtypep)) { + if (AstVar* const varp = VN_CAST(foundp, Var)) { + nodep->dtypep(foundp->dtypep()); + nodep->varp(varp); + varp->usedVirtIface(true); + return; + } + UINFO(1, "found object " << foundp << endl); + nodep->v3fatalSrc("MemberSel of non-variable\n" + << nodep->warnContextPrimary() << '\n' + << foundp->warnOther() << "... Location of found object\n" + << foundp->warnContextSecondary()); + } } else if (VN_IS(fromDtp, EnumDType) // || VN_IS(fromDtp, AssocArrayDType) // || VN_IS(fromDtp, WildcardArrayDType) // @@ -2572,6 +2586,26 @@ class WidthVisitor final : public VNVisitor { << (suggest.empty() ? "" : nodep->fileline()->warnMore() + suggest)); return nullptr; // Caller handles error } + AstNode* memberSelIface(AstMemberSel* nodep, AstIfaceRefDType* adtypep) { + // Returns node if ok + // No need to width-resolve the interface, as it was done when we did the child + AstNodeModule* const ifacep = adtypep->ifacep(); + UASSERT_OBJ(ifacep, nodep, "Unlinked"); + // if (AstNode* const foundp = ifacep->findMember(nodep->name())) return foundp; + VSpellCheck speller; + for (AstNode* itemp = ifacep->stmtsp(); itemp; itemp = itemp->nextp()) { + if (itemp->name() == nodep->name()) return itemp; + if (VN_IS(itemp, Var) || VN_IS(itemp, Modport)) { + speller.pushCandidate(itemp->prettyName()); + } + } + const string suggest = speller.bestCandidateMsg(nodep->prettyName()); + nodep->v3error( + "Member " << nodep->prettyNameQ() << " not found in interface " + << ifacep->prettyNameQ() << "\n" + << (suggest.empty() ? "" : nodep->fileline()->warnMore() + suggest)); + return nullptr; // Caller handles error + } bool memberSelStruct(AstMemberSel* nodep, AstNodeUOrStructDType* adtypep) { // Returns true if ok if (AstMemberDType* const memberp = adtypep->findMember(nodep->name())) { diff --git a/test_regress/t/t_interface_virtual.out b/test_regress/t/t_interface_virtual.out new file mode 100644 index 0000000000..de89145b13 --- /dev/null +++ b/test_regress/t/t_interface_virtual.out @@ -0,0 +1,5 @@ +va.addr=aa va.data=11 ia.addr=aa ia.data=11 +vb.addr=bb vb.data=22 ib.addr=bb ib.data=22 +ca.fa.addr=aa ca.fa.data=11 ca.fa.addr=bb ca.fb.data=22 +cb.fa.addr=bb cb.fa.data=22 cb.fa.addr=aa cb.fb.data=11 +*-* All Finished *-* diff --git a/test_regress/t/t_interface_virtual.pl b/test_regress/t/t_interface_virtual.pl new file mode 100755 index 0000000000..6247bd1265 --- /dev/null +++ b/test_regress/t/t_interface_virtual.pl @@ -0,0 +1,22 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2020 by Wilson Snyder. This program is free software; you +# can redistribute it and/or modify it under the terms of either the GNU +# Lesser General Public License Version 3 or the Perl Artistic License +# Version 2.0. +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +scenarios(simulator => 1); + +compile( + ); + +execute( + check_finished => 1, + expect_filename => $Self->{golden_filename}, + ); + +ok(1); +1; diff --git a/test_regress/t/t_interface_virtual.v b/test_regress/t/t_interface_virtual.v new file mode 100644 index 0000000000..352ece4bc8 --- /dev/null +++ b/test_regress/t/t_interface_virtual.v @@ -0,0 +1,58 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2022 by Arkadiusz Kozdra. +// SPDX-License-Identifier: CC0-1.0 + +// See also t_interface_virtual_bad.v + +interface PBus; + logic req, grant; + logic [7:0] addr, data; + modport phy(input addr, ref data); +endinterface + +class Cls; + virtual PBus fa, fb; +endclass + +module t (/*AUTOARG*/); + + PBus ia, ib; + virtual PBus va, vb; + virtual PBus.phy pa, pb; + Cls ca, cb; + + initial begin + va = ia; + vb = ib; + + ca = new; + cb = new; + + va.addr = 8'haa; + ia.data = 8'h11; + + vb.addr = 8'hbb; + ib.data = 8'h22; + + $display("va.addr=%x", va.addr, " va.data=%x", va.data, " ia.addr=%x", ia.addr, " ia.data=%x", ia.data); + $display("vb.addr=%x", vb.addr, " vb.data=%x", vb.data, " ib.addr=%x", ib.addr, " ib.data=%x", ib.data); + + ca.fa = ia; + ca.fb = ib; + cb.fa = ib; + cb.fb = ia; +// pa = va; +// pb = vb; + +// pb.addr = 8'hb0; +// pa.addr = 8'ha0; + + $display("ca.fa.addr=%x", ca.fa.addr, " ca.fa.data=%x", ca.fa.data, " ca.fa.addr=%x", ca.fb.addr, " ca.fb.data=%x", ca.fb.data); + $display("cb.fa.addr=%x", cb.fa.addr, " cb.fa.data=%x", cb.fa.data, " cb.fa.addr=%x", cb.fb.addr, " cb.fb.data=%x", cb.fb.data); + + $write("*-* All Finished *-*\n"); + $finish; + end +endmodule From e9eb1e868949fdcd074b73ccad2df91023114061 Mon Sep 17 00:00:00 2001 From: Arkadiusz Kozdra Date: Mon, 3 Oct 2022 16:37:09 +0200 Subject: [PATCH 03/11] Add some negative tests --- src/V3LinkDot.cpp | 6 +-- src/V3Width.cpp | 25 ++++++++++++ test_regress/t/t_interface_virtual_bad.out | 29 +++++++++++++ test_regress/t/t_interface_virtual_bad.pl | 19 +++++++++ test_regress/t/t_interface_virtual_bad.v | 47 ++++++++++++++++++++++ 5 files changed, 123 insertions(+), 3 deletions(-) create mode 100644 test_regress/t/t_interface_virtual_bad.out create mode 100755 test_regress/t/t_interface_virtual_bad.pl create mode 100644 test_regress/t/t_interface_virtual_bad.v diff --git a/src/V3LinkDot.cpp b/src/V3LinkDot.cpp index 1f74fb1e73..698e98e0a5 100644 --- a/src/V3LinkDot.cpp +++ b/src/V3LinkDot.cpp @@ -2506,9 +2506,9 @@ class LinkDotResolveVisitor final : public VNVisitor { m_ds.m_dotText = VString::dot(m_ds.m_dotText, ".", nodep->name()); m_ds.m_dotSymp = foundp; m_ds.m_dotPos = DP_SCOPE; - UINFO(9, " cell -> iface varref " << foundp->nodep() << endl); - AstNode* newp - = new AstVarRef(ifaceRefVarp->fileline(), ifaceRefVarp, VAccess::READ); + UINFO(9, " modport -> iface varref " << foundp->nodep() << endl); + // We lose the modport name here, so we cannot detect mismatched modports. + AstNode* newp = new AstVarRef{nodep->fileline(), ifaceRefVarp, VAccess::READ}; auto* const cellarrayrefp = VN_CAST(m_ds.m_unlinkedScopep, CellArrayRef); if (cellarrayrefp) { // iface[vec].modport became CellArrayRef(iface, lsb) diff --git a/src/V3Width.cpp b/src/V3Width.cpp index d98af06333..3504575688 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -6046,6 +6046,31 @@ class WidthVisitor final : public VNVisitor { underp->v3error(ucfirst(nodep->prettyOperatorName()) << " expected non-interface on " << side << " but '" << underp->name() << "' is an interface."); + } else if (const AstIfaceRefDType* expIfaceRefp = VN_CAST(expDTypep, IfaceRefDType)) { + const AstIfaceRefDType* underIfaceRefp = VN_CAST(underp->dtypep(), IfaceRefDType); + if (!underIfaceRefp) { + underp->v3error(ucfirst(nodep->prettyOperatorName()) + << " expected " << expIfaceRefp->ifaceViaCellp()->prettyNameQ() + << " interface on " << side << " but " << underp->prettyNameQ() + << " is not an interface."); + } else if (expIfaceRefp->ifaceViaCellp() != underIfaceRefp->ifaceViaCellp()) { + underp->v3error(ucfirst(nodep->prettyOperatorName()) + << " expected " << expIfaceRefp->ifaceViaCellp()->prettyNameQ() + << " interface on " << side << " but '" << underp->name() + << "' is a different interface (" + << underIfaceRefp->ifaceViaCellp()->prettyNameQ() << ")."); + } else if (underIfaceRefp->modportp() + && expIfaceRefp->modportp() != underIfaceRefp->modportp()) { + underp->v3error(ucfirst(nodep->prettyOperatorName()) + << " expected " + << (expIfaceRefp->modportp() + ? expIfaceRefp->modportp()->prettyNameQ() + : "no") + << " interface modport on " << side << " but got " + << underIfaceRefp->modportp()->prettyNameQ() << " modport."); + } else { + underp = userIterateSubtreeReturnEdits(underp, WidthVP(expDTypep, FINAL).p()); + } } else { // Hope it just works out (perhaps a cast will deal with it) underp = userIterateSubtreeReturnEdits(underp, WidthVP(expDTypep, FINAL).p()); diff --git a/test_regress/t/t_interface_virtual_bad.out b/test_regress/t/t_interface_virtual_bad.out new file mode 100644 index 0000000000..c058a43ea1 --- /dev/null +++ b/test_regress/t/t_interface_virtual_bad.out @@ -0,0 +1,29 @@ +%Error: t/t_interface_virtual_bad.v:29:12: Operator ASSIGN expected 'PBus' interface on Assign RHS but 'q8__Viftop' is a different interface ('QBus'). + : ... In instance t + 29 | v8 = q8; + | ^~ +%Error: t/t_interface_virtual_bad.v:33:12: Operator ASSIGN expected no interface modport on Assign RHS but got 'phy' modport. + : ... In instance t + 33 | v8 = v8_phy; + | ^~~~~~ +%Error: t/t_interface_virtual_bad.v:35:17: Operator ASSIGN expected non-interface on Assign RHS but 'p8__Viftop' is an interface. + : ... In instance t + 35 | data = p8.phy; + | ^~~ +%Error: t/t_interface_virtual_bad.v:36:14: Operator ASSIGN expected non-interface on Assign RHS but 'v8_phy__Viftop' is an interface. + : ... In instance t + 36 | data = v8_phy; + | ^~~~~~ +%Error: t/t_interface_virtual_bad.v:37:14: Operator ASSIGN expected non-interface on Assign RHS but 'v8__Viftop' is an interface. + : ... In instance t + 37 | data = v8; + | ^~ +%Error: t/t_interface_virtual_bad.v:38:14: Operator ASSIGN expected non-interface on Assign RHS but 'p8__Viftop' is an interface. + : ... In instance t + 38 | data = p8; + | ^~ +%Error: t/t_interface_virtual_bad.v:39:12: Operator ASSIGN expected 'PBus' interface on Assign RHS but 'data' is not an interface. + : ... In instance t + 39 | v8 = data; + | ^~~~ +%Error: Exiting due to diff --git a/test_regress/t/t_interface_virtual_bad.pl b/test_regress/t/t_interface_virtual_bad.pl new file mode 100755 index 0000000000..7be596e0f4 --- /dev/null +++ b/test_regress/t/t_interface_virtual_bad.pl @@ -0,0 +1,19 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2020 by Wilson Snyder. This program is free software; you +# can redistribute it and/or modify it under the terms of either the GNU +# Lesser General Public License Version 3 or the Perl Artistic License +# Version 2.0. +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +scenarios(linter => 1); + +lint( + fails => 1, + expect_filename => $Self->{golden_filename}, + ); + +ok(1); +1; diff --git a/test_regress/t/t_interface_virtual_bad.v b/test_regress/t/t_interface_virtual_bad.v new file mode 100644 index 0000000000..3d03501ab4 --- /dev/null +++ b/test_regress/t/t_interface_virtual_bad.v @@ -0,0 +1,47 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2022 by Arkadiusz Kozdra. +// SPDX-License-Identifier: CC0-1.0 + +// See also t_interface_virtual.v + +interface PBus; + logic req, grant; + logic [7:0] addr, data; + modport phy(input addr, ref data); +endinterface + +interface QBus; +endinterface + +module t (/*AUTOARG*/); + + PBus p8; + QBus q8; + virtual PBus v8; + virtual PBus.phy v8_phy; + logic data; + + initial begin + v8 = p8; + p8 = v8; // error + v8 = q8; // error + v8_phy = p8; + v8_phy = v8; + v8_phy = p8.phy; + v8 = v8_phy; // error + v8 = p8.phy; // error + data = p8.phy; // error + data = v8_phy; // error + data = v8; // error + data = p8; // error + v8 = data; // error + v8.grant = 1'b1; + + $display("q8.grant=", p8.grant, " v8.grant=", v8.grant, v8_phy.addr); + + $write("*-* All Finished *-*\n"); + $finish; + end +endmodule From faa21cb391554ee9b40f6a23047388df6822f992 Mon Sep 17 00:00:00 2001 From: Arkadiusz Kozdra Date: Wed, 12 Oct 2022 15:33:21 +0200 Subject: [PATCH 04/11] make format --- src/V3Const.cpp | 3 +-- src/V3Scope.cpp | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/V3Const.cpp b/src/V3Const.cpp index c45776628d..b499a1ed4f 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -2606,8 +2606,7 @@ class ConstVisitor final : public VNVisitor { && m_doNConst && v3Global.opt.fConst() // Default value, not a "known" constant for this usage - && !nodep->varp()->isClassMember() - && !nodep->varp()->isUsedVirtIface() + && !nodep->varp()->isClassMember() && !nodep->varp()->isUsedVirtIface() && !(nodep->varp()->isFuncLocal() && nodep->varp()->isNonOutput()) && !nodep->varp()->noSubst() && !nodep->varp()->isSigPublic()) || nodep->varp()->isParam())) { diff --git a/src/V3Scope.cpp b/src/V3Scope.cpp index 45052ed871..ce60b19dec 100644 --- a/src/V3Scope.cpp +++ b/src/V3Scope.cpp @@ -269,8 +269,7 @@ class ScopeVisitor final : public VNVisitor { AstScope* scopep = m_scopep; if (AstIfaceRefDType* const ifacerefp = VN_CAST(nodep->dtypep(), IfaceRefDType)) { // Attach every non-virtual interface variable its inner scope - if (ifacerefp->cellp()) - scopep = VN_AS(ifacerefp->cellp()->user2p(), Scope); + if (ifacerefp->cellp()) scopep = VN_AS(ifacerefp->cellp()->user2p(), Scope); } AstVarScope* const varscp = new AstVarScope{nodep->fileline(), scopep, nodep}; UINFO(6, " New scope " << varscp << endl); From 562541d5510075e42d56cd033e5b26d00f73042a Mon Sep 17 00:00:00 2001 From: Arkadiusz Kozdra Date: Thu, 13 Oct 2022 08:15:52 +0200 Subject: [PATCH 05/11] Fix negative test --- test_regress/t/t_interface_virtual_bad.out | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test_regress/t/t_interface_virtual_bad.out b/test_regress/t/t_interface_virtual_bad.out index c058a43ea1..5fda0f7d82 100644 --- a/test_regress/t/t_interface_virtual_bad.out +++ b/test_regress/t/t_interface_virtual_bad.out @@ -10,11 +10,11 @@ : ... In instance t 35 | data = p8.phy; | ^~~ -%Error: t/t_interface_virtual_bad.v:36:14: Operator ASSIGN expected non-interface on Assign RHS but 'v8_phy__Viftop' is an interface. +%Error: t/t_interface_virtual_bad.v:36:14: Operator ASSIGN expected non-interface on Assign RHS but 'v8_phy' is an interface. : ... In instance t 36 | data = v8_phy; | ^~~~~~ -%Error: t/t_interface_virtual_bad.v:37:14: Operator ASSIGN expected non-interface on Assign RHS but 'v8__Viftop' is an interface. +%Error: t/t_interface_virtual_bad.v:37:14: Operator ASSIGN expected non-interface on Assign RHS but 'v8' is an interface. : ... In instance t 37 | data = v8; | ^~ From 5f4b0b7d159754ee1bd535ed1c98616f806bb98f Mon Sep 17 00:00:00 2001 From: Arkadiusz Kozdra Date: Thu, 13 Oct 2022 09:00:52 +0200 Subject: [PATCH 06/11] Fix broken modports and to_string handling --- src/V3Common.cpp | 17 ++++++++++++++++- src/V3Dead.cpp | 13 +++++++++++++ test_regress/t/t_interface_virtual.out | 6 ++++-- test_regress/t/t_interface_virtual.v | 19 +++++++++++++++---- 4 files changed, 48 insertions(+), 7 deletions(-) diff --git a/src/V3Common.cpp b/src/V3Common.cpp index dd0cc52501..907c5a33db 100644 --- a/src/V3Common.cpp +++ b/src/V3Common.cpp @@ -47,6 +47,19 @@ static void makeVlToString(AstClass* nodep) { funcp->addStmtsp(new AstCReturn{nodep->fileline(), exprp}); nodep->addStmtsp(funcp); } +static void makeVlToString(AstIface* nodep) { + AstCFunc* const funcp + = new AstCFunc{nodep->fileline(), "VL_TO_STRING", nullptr, "std::string"}; + funcp->argTypes("const " + EmitCBaseVisitor::prefixNameProtect(nodep) + "* obj"); + funcp->isMethod(false); + funcp->isConst(false); + funcp->isStatic(false); + funcp->protect(false); + AstNode* const exprp = new AstCMath{nodep->fileline(), "obj ? obj->name() : \"null\"", 0}; + exprp->dtypeSetString(); + funcp->addStmtsp(new AstCReturn{nodep->fileline(), exprp}); + nodep->addStmtsp(funcp); +} static void makeToString(AstClass* nodep) { AstCFunc* const funcp = new AstCFunc{nodep->fileline(), "to_string", nullptr, "std::string"}; funcp->isConst(true); @@ -68,7 +81,7 @@ static void makeToStringMiddle(AstClass* nodep) { std::string comma; for (AstNode* itemp = nodep->membersp(); itemp; itemp = itemp->nextp()) { if (const auto* const varp = VN_CAST(itemp, Var)) { - if (!varp->isParam() && !VN_IS(itemp->dtypep(), IfaceRefDType)) { + if (!varp->isParam()) { string stmt = "out += \""; stmt += comma; comma = ", "; @@ -117,6 +130,8 @@ void V3Common::commonAll() { makeVlToString(classp); makeToString(classp); makeToStringMiddle(classp); + } else if (AstIface* const ifacep = VN_CAST(nodep, Iface)) { + makeVlToString(ifacep); } } V3Global::dumpCheckGlobalTree("common", 0, dumpTree() >= 3); diff --git a/src/V3Dead.cpp b/src/V3Dead.cpp index 843c2e6284..d2ce5734c5 100644 --- a/src/V3Dead.cpp +++ b/src/V3Dead.cpp @@ -198,6 +198,19 @@ class DeadVisitor final : public VNVisitor { } if (nodep->classp()) nodep->classp()->user1Inc(); } + void visit(AstIfaceRefDType* nodep) override { + iterateChildren(nodep); + checkDType(nodep); + checkAll(nodep); + if (nodep->modportp()) { + if (m_elimCells) { + nodep->modportp(nullptr); + } else { + nodep->modportp()->user1Inc(); + } + } + if (nodep->ifaceViaCellp()) nodep->ifaceViaCellp()->user1Inc(); + } void visit(AstNodeDType* nodep) override { iterateChildren(nodep); checkDType(nodep); diff --git a/test_regress/t/t_interface_virtual.out b/test_regress/t/t_interface_virtual.out index de89145b13..2e63964b40 100644 --- a/test_regress/t/t_interface_virtual.out +++ b/test_regress/t/t_interface_virtual.out @@ -1,5 +1,7 @@ va.addr=aa va.data=11 ia.addr=aa ia.data=11 vb.addr=bb vb.data=22 ib.addr=bb ib.data=22 -ca.fa.addr=aa ca.fa.data=11 ca.fa.addr=bb ca.fb.data=22 -cb.fa.addr=bb cb.fa.data=22 cb.fa.addr=aa cb.fb.data=11 +ca.fa.addr=a0 ca.fa.data=11 ca.fa.addr=b0 ca.fb.data=22 +cb.fa.addr=b0 cb.fa.data=22 cb.fa.addr=a0 cb.fb.data=11 +gen.x[0].addr=a0 gen.x[1].addr=b0 +gen='{x:'{top.t.ia, top.t.ib, null, null} } *-* All Finished *-* diff --git a/test_regress/t/t_interface_virtual.v b/test_regress/t/t_interface_virtual.v index 352ece4bc8..ee2b0ddb39 100644 --- a/test_regress/t/t_interface_virtual.v +++ b/test_regress/t/t_interface_virtual.v @@ -16,12 +16,17 @@ class Cls; virtual PBus fa, fb; endclass +class Clsgen#(type T = logic); + T x[0:3]; +endclass + module t (/*AUTOARG*/); PBus ia, ib; virtual PBus va, vb; virtual PBus.phy pa, pb; Cls ca, cb; + Clsgen#(virtual PBus) gen; initial begin va = ia; @@ -29,6 +34,7 @@ module t (/*AUTOARG*/); ca = new; cb = new; + gen = new; va.addr = 8'haa; ia.data = 8'h11; @@ -43,14 +49,19 @@ module t (/*AUTOARG*/); ca.fb = ib; cb.fa = ib; cb.fb = ia; -// pa = va; -// pb = vb; + gen.x[0] = va; + gen.x[1] = vb; + + pa = va; + pb = vb; -// pb.addr = 8'hb0; -// pa.addr = 8'ha0; + pb.addr = 8'hb0; + pa.addr = 8'ha0; $display("ca.fa.addr=%x", ca.fa.addr, " ca.fa.data=%x", ca.fa.data, " ca.fa.addr=%x", ca.fb.addr, " ca.fb.data=%x", ca.fb.data); $display("cb.fa.addr=%x", cb.fa.addr, " cb.fa.data=%x", cb.fa.data, " cb.fa.addr=%x", cb.fb.addr, " cb.fb.data=%x", cb.fb.data); + $display("gen.x[0].addr=%x", gen.x[0].addr, " gen.x[1].addr=%x", gen.x[1].addr); + $display("gen=%p", gen); $write("*-* All Finished *-*\n"); $finish; From 50f4d786629895541443edaa7bf4c37385f6a481 Mon Sep 17 00:00:00 2001 From: Arkadiusz Kozdra Date: Thu, 13 Oct 2022 09:26:43 +0200 Subject: [PATCH 07/11] Add skipRefp on relevant type checks --- src/V3Width.cpp | 10 ++++---- src/verilog.y | 10 ++++---- test_regress/t/t_interface_virtual.v | 5 +++- test_regress/t/t_interface_virtual_bad.out | 28 +++++++++++----------- test_regress/t/t_interface_virtual_bad.v | 4 +++- 5 files changed, 32 insertions(+), 25 deletions(-) diff --git a/src/V3Width.cpp b/src/V3Width.cpp index 3504575688..a07508ae38 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -6041,13 +6041,15 @@ class WidthVisitor final : public VNVisitor { // Note the check uses the expected size, not the child's subDTypep as we want the // child node's width to end up correct for the assignment (etc) widthCheckSized(nodep, side, underp, expDTypep, extendRule, warnOn); - } else if (!VN_IS(expDTypep, IfaceRefDType) - && VN_IS(underp->dtypep(), IfaceRefDType)) { + } else if (!VN_IS(expDTypep->skipRefp(), IfaceRefDType) + && VN_IS(underp->dtypep()->skipRefp(), IfaceRefDType)) { underp->v3error(ucfirst(nodep->prettyOperatorName()) << " expected non-interface on " << side << " but '" << underp->name() << "' is an interface."); - } else if (const AstIfaceRefDType* expIfaceRefp = VN_CAST(expDTypep, IfaceRefDType)) { - const AstIfaceRefDType* underIfaceRefp = VN_CAST(underp->dtypep(), IfaceRefDType); + } else if (const AstIfaceRefDType* expIfaceRefp + = VN_CAST(expDTypep->skipRefp(), IfaceRefDType)) { + const AstIfaceRefDType* underIfaceRefp + = VN_CAST(underp->dtypep()->skipRefp(), IfaceRefDType); if (!underIfaceRefp) { underp->v3error(ucfirst(nodep->prettyOperatorName()) << " expected " << expIfaceRefp->ifaceViaCellp()->prettyNameQ() diff --git a/src/verilog.y b/src/verilog.y index eba4bde68c..df3c549d42 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -1955,11 +1955,6 @@ data_type: // ==IEEE: data_type | packageClassScopeE idType parameter_value_assignmentClass packed_dimensionListE { AstRefDType* const refp = new AstRefDType{$2, *$2, $1, $3}; $$ = GRAMMARP->createArray(refp, $4, true); } - // // Rules overlap virtual_interface_declaration - | yVIRTUAL__INTERFACE yINTERFACE data_typeVirtual - { $$ = $3; } - | yVIRTUAL__anyID data_typeVirtual - { $$ = $2; } ; data_typeBasic: // IEEE: part of data_type @@ -1989,6 +1984,11 @@ data_typeNoRef: // ==IEEE: data_type, excluding class_ty // // IEEE: class_scope: see data_type above // // IEEE: class_type: see data_type above // // IEEE: ps_covergroup: see data_type above + // // Rules overlap virtual_interface_declaration + | yVIRTUAL__INTERFACE yINTERFACE data_typeVirtual + { $$ = $3; } + | yVIRTUAL__anyID data_typeVirtual + { $$ = $2; } ; data_typeVirtual: // ==IEEE: data_type after yVIRTUAL [ yINTERFACE ] diff --git a/test_regress/t/t_interface_virtual.v b/test_regress/t/t_interface_virtual.v index ee2b0ddb39..878df0995d 100644 --- a/test_regress/t/t_interface_virtual.v +++ b/test_regress/t/t_interface_virtual.v @@ -12,8 +12,11 @@ interface PBus; modport phy(input addr, ref data); endinterface +typedef virtual PBus vpbus_t; +typedef vpbus_t vpbus2_t; + class Cls; - virtual PBus fa, fb; + vpbus2_t fa, fb; endclass class Clsgen#(type T = logic); diff --git a/test_regress/t/t_interface_virtual_bad.out b/test_regress/t/t_interface_virtual_bad.out index 5fda0f7d82..d0596b17d5 100644 --- a/test_regress/t/t_interface_virtual_bad.out +++ b/test_regress/t/t_interface_virtual_bad.out @@ -1,29 +1,29 @@ -%Error: t/t_interface_virtual_bad.v:29:12: Operator ASSIGN expected 'PBus' interface on Assign RHS but 'q8__Viftop' is a different interface ('QBus'). +%Error: t/t_interface_virtual_bad.v:31:12: Operator ASSIGN expected 'PBus' interface on Assign RHS but 'q8__Viftop' is a different interface ('QBus'). : ... In instance t - 29 | v8 = q8; + 31 | v8 = q8; | ^~ -%Error: t/t_interface_virtual_bad.v:33:12: Operator ASSIGN expected no interface modport on Assign RHS but got 'phy' modport. +%Error: t/t_interface_virtual_bad.v:35:12: Operator ASSIGN expected no interface modport on Assign RHS but got 'phy' modport. : ... In instance t - 33 | v8 = v8_phy; + 35 | v8 = v8_phy; | ^~~~~~ -%Error: t/t_interface_virtual_bad.v:35:17: Operator ASSIGN expected non-interface on Assign RHS but 'p8__Viftop' is an interface. +%Error: t/t_interface_virtual_bad.v:37:17: Operator ASSIGN expected non-interface on Assign RHS but 'p8__Viftop' is an interface. : ... In instance t - 35 | data = p8.phy; + 37 | data = p8.phy; | ^~~ -%Error: t/t_interface_virtual_bad.v:36:14: Operator ASSIGN expected non-interface on Assign RHS but 'v8_phy' is an interface. +%Error: t/t_interface_virtual_bad.v:38:14: Operator ASSIGN expected non-interface on Assign RHS but 'v8_phy' is an interface. : ... In instance t - 36 | data = v8_phy; + 38 | data = v8_phy; | ^~~~~~ -%Error: t/t_interface_virtual_bad.v:37:14: Operator ASSIGN expected non-interface on Assign RHS but 'v8' is an interface. +%Error: t/t_interface_virtual_bad.v:39:14: Operator ASSIGN expected non-interface on Assign RHS but 'v8' is an interface. : ... In instance t - 37 | data = v8; + 39 | data = v8; | ^~ -%Error: t/t_interface_virtual_bad.v:38:14: Operator ASSIGN expected non-interface on Assign RHS but 'p8__Viftop' is an interface. +%Error: t/t_interface_virtual_bad.v:40:14: Operator ASSIGN expected non-interface on Assign RHS but 'p8__Viftop' is an interface. : ... In instance t - 38 | data = p8; + 40 | data = p8; | ^~ -%Error: t/t_interface_virtual_bad.v:39:12: Operator ASSIGN expected 'PBus' interface on Assign RHS but 'data' is not an interface. +%Error: t/t_interface_virtual_bad.v:41:12: Operator ASSIGN expected 'PBus' interface on Assign RHS but 'data' is not an interface. : ... In instance t - 39 | v8 = data; + 41 | v8 = data; | ^~~~ %Error: Exiting due to diff --git a/test_regress/t/t_interface_virtual_bad.v b/test_regress/t/t_interface_virtual_bad.v index 3d03501ab4..bd4c31b029 100644 --- a/test_regress/t/t_interface_virtual_bad.v +++ b/test_regress/t/t_interface_virtual_bad.v @@ -15,11 +15,13 @@ endinterface interface QBus; endinterface +typedef virtual PBus vpbus_t; + module t (/*AUTOARG*/); PBus p8; QBus q8; - virtual PBus v8; + vpbus_t v8; virtual PBus.phy v8_phy; logic data; From 5888f564552158bab6a84955e8e772c366bed370 Mon Sep 17 00:00:00 2001 From: Arkadiusz Kozdra Date: Thu, 13 Oct 2022 09:36:26 +0200 Subject: [PATCH 08/11] Test with inliner turned off --- test_regress/t/t_interface_virtual_inl.pl | 27 +++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100755 test_regress/t/t_interface_virtual_inl.pl diff --git a/test_regress/t/t_interface_virtual_inl.pl b/test_regress/t/t_interface_virtual_inl.pl new file mode 100755 index 0000000000..b342b7b3a3 --- /dev/null +++ b/test_regress/t/t_interface_virtual_inl.pl @@ -0,0 +1,27 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2003-2009 by Wilson Snyder. This program is free software; you +# can redistribute it and/or modify it under the terms of either the GNU +# Lesser General Public License Version 3 or the Perl Artistic License +# Version 2.0. +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +scenarios(simulator => 1); + +top_filename("t/t_interface_virtual.v"); +golden_filename("t/t_interface_virtual.out"); + +compile( + # Avoid inlining so we find bugs in the non-inliner connection code + verilator_flags2 => ["-fno-inline"], + ); + +execute( + check_finished => 1, + expect_filename => $Self->{golden_filename}, + ); + +ok(1); +1; From b0306d35f05b4bd26292838561ad954a5d4e3665 Mon Sep 17 00:00:00 2001 From: Arkadiusz Kozdra Date: Thu, 13 Oct 2022 15:59:23 +0200 Subject: [PATCH 09/11] Partially sort flags in AstVar --- src/V3AstNodeOther.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/V3AstNodeOther.h b/src/V3AstNodeOther.h index d61105b58b..12fe0eef7f 100644 --- a/src/V3AstNodeOther.h +++ b/src/V3AstNodeOther.h @@ -2000,9 +2000,9 @@ class AstVar final : public AstNode { bool m_sigUserRdPublic : 1; // User C code accesses this signal, read only bool m_sigUserRWPublic : 1; // User C code accesses this signal, read-write bool m_usedClock : 1; // Signal used as a clock - bool m_usedVirtIface : 1; // Signal used through a virtual interface bool m_usedParam : 1; // Parameter is referenced (on link; later signals not setup) bool m_usedLoopIdx : 1; // Variable subject of for unrolling + bool m_usedVirtIface : 1; // Signal used through a virtual interface bool m_funcLocal : 1; // Local variable for a function bool m_funcReturn : 1; // Return variable for a function bool m_attrScBv : 1; // User force bit vector attribute @@ -2039,9 +2039,9 @@ class AstVar final : public AstNode { m_scClocked = false; m_scSensitive = false; m_usedClock = false; - m_usedVirtIface = false; m_usedParam = false; m_usedLoopIdx = false; + m_usedVirtIface = false; m_sigPublic = false; m_sigModPublic = false; m_sigUserRdPublic = false; @@ -2186,9 +2186,9 @@ class AstVar final : public AstNode { void attrSFormat(bool flag) { m_attrSFormat = flag; } void attrSplitVar(bool flag) { m_attrSplitVar = flag; } void usedClock(bool flag) { m_usedClock = flag; } - void usedVirtIface(bool flag) { m_usedVirtIface = flag; } void usedParam(bool flag) { m_usedParam = flag; } void usedLoopIdx(bool flag) { m_usedLoopIdx = flag; } + void usedVirtIface(bool flag) { m_usedVirtIface = flag; } void sigPublic(bool flag) { m_sigPublic = flag; } void sigModPublic(bool flag) { m_sigModPublic = flag; } void sigUserRdPublic(bool flag) { @@ -2270,9 +2270,9 @@ class AstVar final : public AstNode { return bdtypep && bdtypep->isBitLogic(); } bool isUsedClock() const { return m_usedClock; } - bool isUsedVirtIface() const { return m_usedVirtIface; } bool isUsedParam() const { return m_usedParam; } bool isUsedLoopIdx() const { return m_usedLoopIdx; } + bool isUsedVirtIface() const { return m_usedVirtIface; } bool isSc() const { return m_sc; } bool isScQuad() const; bool isScBv() const; From cfe76d57cfc2d81ace789ff63a2a37694a00c245 Mon Sep 17 00:00:00 2001 From: Arkadiusz Kozdra Date: Wed, 19 Oct 2022 12:50:04 +0200 Subject: [PATCH 10/11] Warn on unsupported unused virtual interfaces --- src/V3LinkDot.cpp | 14 ++++++++++--- .../t/t_interface_virtual_unused_bad.out | 5 +++++ .../t/t_interface_virtual_unused_bad.pl | 19 ++++++++++++++++++ .../t/t_interface_virtual_unused_bad.v | 20 +++++++++++++++++++ 4 files changed, 55 insertions(+), 3 deletions(-) create mode 100644 test_regress/t/t_interface_virtual_unused_bad.out create mode 100755 test_regress/t/t_interface_virtual_unused_bad.pl create mode 100644 test_regress/t/t_interface_virtual_unused_bad.v diff --git a/src/V3LinkDot.cpp b/src/V3LinkDot.cpp index d73b3c1be6..25f0293b9d 100644 --- a/src/V3LinkDot.cpp +++ b/src/V3LinkDot.cpp @@ -456,7 +456,7 @@ class LinkDotState final { } void computeIfaceVarSyms() { for (VSymEnt* varSymp : m_ifaceVarSyms) { - const AstVar* const varp = varSymp ? VN_AS(varSymp->nodep(), Var) : nullptr; + AstVar* const varp = varSymp ? VN_AS(varSymp->nodep(), Var) : nullptr; UINFO(9, " insAllIface se" << cvtToHex(varSymp) << " " << varp << endl); AstIfaceRefDType* const ifacerefp = ifaceRefFromArray(varp->subDTypep()); UASSERT_OBJ(ifacerefp, varp, "Non-ifacerefs on list!"); @@ -472,8 +472,16 @@ class LinkDotState final { ifacerefp->v3fatalSrc("Unlinked interface"); } } else if (ifacerefp->ifaceViaCellp()->dead()) { - ifacerefp->v3error("Parent instance's interface is not found: " - << AstNode::prettyNameQ(ifacerefp->ifaceName())); + if (varp->isIfaceRef()) { + ifacerefp->v3error("Parent instance's interface is not found: " + << AstNode::prettyNameQ(ifacerefp->ifaceName())); + } else { + ifacerefp->v3warn( + E_UNSUPPORTED, + "Unsupported: virtual interface never assigned any actual interface"); + varp->dtypep(ifacerefp->findCHandleDType()); + VL_DO_DANGLING(ifacerefp->unlinkFrBack()->deleteTree(), ifacerefp); + } continue; } VSymEnt* const ifaceSymp = getNodeSym(ifacerefp->ifaceViaCellp()); diff --git a/test_regress/t/t_interface_virtual_unused_bad.out b/test_regress/t/t_interface_virtual_unused_bad.out new file mode 100644 index 0000000000..5775cfc79b --- /dev/null +++ b/test_regress/t/t_interface_virtual_unused_bad.out @@ -0,0 +1,5 @@ +%Error-UNSUPPORTED: t/t_interface_virtual_unused_bad.v:14:12: Unsupported: virtual interface never assigned any actual interface + 14 | virtual QBus q8; + | ^~~~ + ... For error description see https://verilator.org/warn/UNSUPPORTED?v=latest +%Error: Exiting due to diff --git a/test_regress/t/t_interface_virtual_unused_bad.pl b/test_regress/t/t_interface_virtual_unused_bad.pl new file mode 100755 index 0000000000..7be596e0f4 --- /dev/null +++ b/test_regress/t/t_interface_virtual_unused_bad.pl @@ -0,0 +1,19 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2020 by Wilson Snyder. This program is free software; you +# can redistribute it and/or modify it under the terms of either the GNU +# Lesser General Public License Version 3 or the Perl Artistic License +# Version 2.0. +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +scenarios(linter => 1); + +lint( + fails => 1, + expect_filename => $Self->{golden_filename}, + ); + +ok(1); +1; diff --git a/test_regress/t/t_interface_virtual_unused_bad.v b/test_regress/t/t_interface_virtual_unused_bad.v new file mode 100644 index 0000000000..c0c7d185f8 --- /dev/null +++ b/test_regress/t/t_interface_virtual_unused_bad.v @@ -0,0 +1,20 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2022 by Arkadiusz Kozdra. +// SPDX-License-Identifier: CC0-1.0 + +// See also t_interface_virtual.v + +interface QBus; +endinterface + +module t (/*AUTOARG*/); + + virtual QBus q8; + + initial begin + $write("*-* All Finished *-*\n"); + $finish; + end +endmodule From 2a0d1af0a7326fe1d7719ba6ea32c96a91540f1b Mon Sep 17 00:00:00 2001 From: Arkadiusz Kozdra Date: Thu, 20 Oct 2022 11:17:50 +0200 Subject: [PATCH 11/11] Changes from code review --- src/V3Width.cpp | 2 +- test_regress/t/t_interface_virtual_bad.out | 5 +++++ test_regress/t/t_interface_virtual_bad.v | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/V3Width.cpp b/src/V3Width.cpp index b4dc24be4b..23319a0efa 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -6090,7 +6090,7 @@ class WidthVisitor final : public VNVisitor { << " interface modport on " << side << " but got " << underIfaceRefp->modportp()->prettyNameQ() << " modport."); } else { - underp = userIterateSubtreeReturnEdits(underp, WidthVP(expDTypep, FINAL).p()); + underp = userIterateSubtreeReturnEdits(underp, WidthVP{expDTypep, FINAL}.p()); } } else { // Hope it just works out (perhaps a cast will deal with it) diff --git a/test_regress/t/t_interface_virtual_bad.out b/test_regress/t/t_interface_virtual_bad.out index d0596b17d5..6afbdc1253 100644 --- a/test_regress/t/t_interface_virtual_bad.out +++ b/test_regress/t/t_interface_virtual_bad.out @@ -26,4 +26,9 @@ : ... In instance t 41 | v8 = data; | ^~~~ +%Error: t/t_interface_virtual_bad.v:44:79: Member 'gran' not found in interface 'PBus' + : ... In instance t + : ... Suggested alternative: 'grant' + 44 | $display("q8.grant=", p8.grant, " v8.grant=", v8.grant, v8_phy.addr, v8.gran); + | ^~~~ %Error: Exiting due to diff --git a/test_regress/t/t_interface_virtual_bad.v b/test_regress/t/t_interface_virtual_bad.v index bd4c31b029..3c4f690da5 100644 --- a/test_regress/t/t_interface_virtual_bad.v +++ b/test_regress/t/t_interface_virtual_bad.v @@ -41,7 +41,7 @@ module t (/*AUTOARG*/); v8 = data; // error v8.grant = 1'b1; - $display("q8.grant=", p8.grant, " v8.grant=", v8.grant, v8_phy.addr); + $display("q8.grant=", p8.grant, " v8.grant=", v8.grant, v8_phy.addr, v8.gran); $write("*-* All Finished *-*\n"); $finish;