From 528e209bea076da44e942d71f7a0b519651358d4 Mon Sep 17 00:00:00 2001 From: Yan <62220457+likeamahoney@users.noreply.github.com> Date: Thu, 24 Oct 2024 01:28:08 +0300 Subject: [PATCH] [slang] Introduce net aliases bits duplication checks (#1045) --- include/slang/ast/Compilation.h | 5 ++ include/slang/ast/SemanticFacts.h | 9 +- include/slang/ast/symbols/ValueSymbol.h | 15 +++- scripts/diagnostics.txt | 3 + source/ast/Compilation.cpp | 14 +++ source/ast/symbols/MemberSymbols.cpp | 97 ++++++++++++++++++-- source/ast/symbols/ValueSymbol.cpp | 34 ++++++- tests/unittests/ast/MemberTests.cpp | 115 +++++++++++++++++++++++- 8 files changed, 280 insertions(+), 12 deletions(-) diff --git a/include/slang/ast/Compilation.h b/include/slang/ast/Compilation.h index 9afb5ed7f..6f217046b 100644 --- a/include/slang/ast/Compilation.h +++ b/include/slang/ast/Compilation.h @@ -711,6 +711,11 @@ class SLANG_EXPORT Compilation : public BumpAllocator { /// Queries if any errors have been issued on any scope within this compilation. bool hasIssuedErrors() const { return numErrors > 0; }; + /// Check that provided diagnotic code and location with it's note is exists in the compilation + /// diagnostic scope + bool checkDiagAndNote(DiagCode diagCode, SourceLocation diagLoc, DiagCode noteCode, + SourceLocation noteLoc); + /// @{ private: diff --git a/include/slang/ast/SemanticFacts.h b/include/slang/ast/SemanticFacts.h index 21db38848..86632d62d 100644 --- a/include/slang/ast/SemanticFacts.h +++ b/include/slang/ast/SemanticFacts.h @@ -108,7 +108,7 @@ SLANG_ENUM(ForwardTypeRestriction, FTR); #undef FTR /// A set of flags that control how assignments are checked. -enum class SLANG_EXPORT AssignFlags : uint8_t { +enum class SLANG_EXPORT AssignFlags : uint16_t { /// No special assignment behavior specified. None = 0, @@ -137,9 +137,12 @@ enum class SLANG_EXPORT AssignFlags : uint8_t { /// The assignment is for an output port that was sliced due to an array of instances /// being connected to an array argument. - SlicedPort = 1 << 7 + SlicedPort = 1 << 7, + + /// The assignment declares net alias + NetAlias = 1 << 8 }; -SLANG_BITMASK(AssignFlags, SlicedPort) +SLANG_BITMASK(AssignFlags, NetAlias) /// A helper class that can extract semantic AST information from /// tokens and syntax nodes. diff --git a/include/slang/ast/symbols/ValueSymbol.h b/include/slang/ast/symbols/ValueSymbol.h index ac3c66df5..5849b401c 100644 --- a/include/slang/ast/symbols/ValueSymbol.h +++ b/include/slang/ast/symbols/ValueSymbol.h @@ -64,6 +64,10 @@ class SLANG_EXPORT ValueSymbol : public Symbol { void addDriver(DriverKind kind, const Expression& longestStaticPrefix, const Symbol& containingSymbol, bitmask flags) const; + void addDriver(DriverKind kind, const Expression& longestStaticPrefix, + const Symbol& containingSymbol, bitmask flags, + const SourceRange& symExprSR, DriverBitRange exprBounds) const; + void addDriver(DriverKind kind, DriverBitRange bounds, const Expression& longestStaticPrefix, const Symbol& containingSymbol, const Expression& procCallExpression) const; @@ -118,11 +122,15 @@ class SLANG_EXPORT ValueDriver { /// The kind of driver (procedural or continuous). DriverKind kind; + /// Stored driven value expression source range for diagnostic + const SourceRange* symExprSR = nullptr; + /// Constructs a new ValueDriver instance. ValueDriver(DriverKind kind, const Expression& longestStaticPrefix, - const Symbol& containingSymbol, bitmask flags) : + const Symbol& containingSymbol, bitmask flags, + const SourceRange* symExprSR = nullptr) : prefixExpression(&longestStaticPrefix), containingSymbol(&containingSymbol), flags(flags), - kind(kind) {} + kind(kind), symExprSR(symExprSR) {} /// Indicates whether the driver is for an input port. bool isInputPort() const { return flags.has(AssignFlags::InputPort); } @@ -138,6 +146,9 @@ class SLANG_EXPORT ValueDriver { /// Indicates whether the driver is for an assertion local variable formal argument. bool isLocalVarFormalArg() const { return flags.has(AssignFlags::AssertionLocalVarFormalArg); } + /// Indicates whether the driver is for a net alias. + bool isNetAlias() const { return flags.has(AssignFlags::NetAlias); } + /// Indicates whether the driver is inside a single-driver procedure (such as always_comb). bool isInSingleDriverProcedure() const; diff --git a/scripts/diagnostics.txt b/scripts/diagnostics.txt index 43527f631..e2c6ba75a 100644 --- a/scripts/diagnostics.txt +++ b/scripts/diagnostics.txt @@ -9,6 +9,8 @@ note NoteDeclarationHere "declared here" note NotePreviousMatch "previous match here" note NoteReferencedHere "referenced here" note NoteAssignedHere "also assigned here" +note NoteAliasedTo "which is aliased to" +note NoteAliasDeclaration "previous alias declaration here" note NoteDrivenHere "driven here" note NoteOriginalAssign "original assignment here" note NoteConfigRule "from configuration here" @@ -715,6 +717,7 @@ error MultipleContAssigns "cannot have multiple continuous assignments to variab error MultipleAlwaysAssigns "variable '{}' driven by {} procedure cannot be written to by any other process" error MultipleUWireDrivers "'uwire' net '{}' cannot have multiple drivers" error MultipleUDNTDrivers "net '{}' with user-defined nettype '{}' cannot have multiple drivers because it does not specify a resolution function" +error MultipleNetAlias "it is not allowed to specify an alias from an individual signal to itself or to specify a given alias more than once" error InputPortAssign "cannot assign to input port '{}'" error ClockVarTargetAssign "cannot write to '{}' with a continuous assignment since it's associated with an output clocking signal" error LocalFormalVarMultiAssign "cannot bind local variable '{}' to more than one output or inout local variable formal argument" diff --git a/source/ast/Compilation.cpp b/source/ast/Compilation.cpp index 6a7214e1e..4293f014a 100644 --- a/source/ast/Compilation.cpp +++ b/source/ast/Compilation.cpp @@ -1633,6 +1633,20 @@ Diagnostic& Compilation::addDiag(Diagnostic diag) { return it->second.back(); } +bool Compilation::checkDiagAndNote(DiagCode diagCode, SourceLocation diagLoc, DiagCode noteCode, + SourceLocation noteLoc) { + if (auto diagIt = diagMap.find({diagCode, diagLoc}); diagIt != diagMap.end()) { + for (auto& diag : diagIt->second) { + for (auto& note : diag.notes) { + if (note.code == noteCode && note.location == noteLoc) + return true; + } + } + } + + return false; +} + AssertionInstanceDetails* Compilation::allocAssertionDetails() { return assertionDetailsAllocator.emplace(); } diff --git a/source/ast/symbols/MemberSymbols.cpp b/source/ast/symbols/MemberSymbols.cpp index a3b5d86d1..c25ce7a02 100644 --- a/source/ast/symbols/MemberSymbols.cpp +++ b/source/ast/symbols/MemberSymbols.cpp @@ -2392,12 +2392,21 @@ NetAliasSymbol& NetAliasSymbol::fromSyntax(const ASTContext& parentContext, return *result; } +struct NetAliasPOD { + const ValueSymbol* sym; + const Expression* expr; + std::optional bounds; +}; + struct NetAliasVisitor { const ASTContext& context; const NetType* commonNetType = nullptr; bool issuedError = false; + SmallVector netAliases; + EvalContext& evalCtx; - NetAliasVisitor(const ASTContext& context) : context(context) {} + NetAliasVisitor(const ASTContext& context, EvalContext& evalCtx) : + context(context), evalCtx(evalCtx) {} template void visit(const T& expr) { @@ -2412,7 +2421,11 @@ struct NetAliasVisitor { context.addDiag(diag::NetAliasNotANet, expr.sourceRange) << sym->name; } else { - auto& nt = sym->template as().netType; + auto& netSym = sym->template as(); + netAliases.push_back( + {&netSym, &expr, + ValueDriver::getBounds(expr, evalCtx, netSym.getType())}); + auto& nt = netSym.netType; if (!commonNetType) { commonNetType = &nt; } @@ -2447,14 +2460,14 @@ std::span NetAliasSymbol::getNetReferences() const { auto syntax = getSyntax(); SLANG_ASSERT(scope && syntax); - // TODO: there should be a global check somewhere that any given bit - // of a net isn't aliased to the same target signal bit multiple times. bitwidth_t bitWidth = 0; bool issuedError = false; SmallVector buffer; ASTContext context(*scope, LookupLocation::after(*this), ASTFlags::NonProcedural | ASTFlags::NotADriver); - NetAliasVisitor visitor(context); + EvalContext evalCtx(ASTContext(*scope, LookupLocation::max)); + NetAliasVisitor visitor(context, evalCtx); + SmallVector>, 4> netAliases; for (auto exprSyntax : syntax->as().nets) { auto& netRef = Expression::bind(*exprSyntax, context); @@ -2472,9 +2485,83 @@ std::span NetAliasSymbol::getNetReferences() const { netRef.visit(visitor); buffer.push_back(&netRef); + if (!visitor.netAliases.empty()) { + netAliases.push_back( + std::make_pair(&netRef, visitor.netAliases.copy(scope->getCompilation()))); + visitor.netAliases.clear(); + } } netRefs = buffer.copy(scope->getCompilation()); + + // To process an each pair a single time. + SmallSet processed; + // Go through the list of net aliases with a sliding window adding each alias in pairs to each + // as a driver. Along the way, calculating the subexpessions bounds (there is no duplication of + // calculations, since the bounds are calculated only once for each subexpression.) and bit + // remainders (which will be processed on the next iterations), since the subexpressions can be + // of different bit lengths. + for (auto first : netAliases) { + processed.insert(first.first); + for (auto second : netAliases) { + if (processed.contains(second.first)) + continue; + + auto currFirst = first.second.begin(); + auto endFirst = first.second.end(); + auto currSecond = second.second.begin(); + auto endSecond = second.second.end(); + // boolean member means bit range remainder of left (if it is true) or right otherwise + std::optional> remainder = std::nullopt; + while (currFirst != endFirst && currSecond != endSecond) { + auto rangeFirst = currFirst->bounds.value_or( + DriverBitRange(0, currFirst->expr->type->getBitWidth())); + auto rangeSecond = currSecond->bounds.value_or( + DriverBitRange(0, currSecond->expr->type->getBitWidth())); + + if (remainder.has_value()) { + if (remainder.value().second) + rangeFirst = remainder.value().first; + else + rangeSecond = remainder.value().first; + + remainder = std::nullopt; + } + + auto rangeDiff = std::abs(int64_t(rangeFirst.second - rangeFirst.first)) - + std::abs(int64_t(rangeSecond.second - rangeSecond.first)); + auto currFirstSaved = currFirst; + auto currSecondSaved = currSecond; + if (rangeDiff > 0) { + int64_t newBound = (int64_t)rangeFirst.second - rangeDiff + 1; + SLANG_ASSERT(newBound >= 0); + remainder = + std::make_pair(DriverBitRange((uint64_t)newBound, rangeFirst.second), true); + rangeFirst = DriverBitRange(rangeFirst.first, (uint64_t)newBound - 1); + ++currSecond; + } + else if (rangeDiff < 0) { + int64_t newBound = (int64_t)rangeSecond.second + rangeDiff + 1; + SLANG_ASSERT(newBound >= 0); + remainder = std::make_pair( + DriverBitRange((uint64_t)newBound, rangeSecond.second), false); + rangeSecond = DriverBitRange(rangeSecond.first, (uint64_t)newBound - 1); + ++currFirst; + } + else { + ++currFirst; + ++currSecond; + } + + currFirstSaved->sym->addDriver(DriverKind::Continuous, *currSecondSaved->expr, + *currSecondSaved->sym, AssignFlags::NetAlias, + currFirstSaved->expr->sourceRange, rangeSecond); + currSecondSaved->sym->addDriver(DriverKind::Continuous, *currFirstSaved->expr, + *currFirstSaved->sym, AssignFlags::NetAlias, + currSecondSaved->expr->sourceRange, rangeFirst); + } + } + } return *netRefs; } diff --git a/source/ast/symbols/ValueSymbol.cpp b/source/ast/symbols/ValueSymbol.cpp index 5694cbca6..fa8278a21 100644 --- a/source/ast/symbols/ValueSymbol.cpp +++ b/source/ast/symbols/ValueSymbol.cpp @@ -171,11 +171,19 @@ static bool handleOverlap(const Scope& scope, std::string_view name, const Value code = diag::MultipleUWireDrivers; else if (isSingleDriverUDNT) code = diag::MultipleUDNTDrivers; + else if (driver.isNetAlias()) + code = diag::MultipleNetAlias; else if (driver.kind == DriverKind::Continuous && curr.kind == DriverKind::Continuous) code = diag::MultipleContAssigns; else code = diag::MixedVarAssigns; + // Filter out duplicate diagnostics with accuracy up to swap diagnostic and note. + if (driver.isNetAlias() && + scope.getCompilation().checkDiagAndNote(diag::MultipleNetAlias, currRange.start(), + diag::NoteAliasDeclaration, driverRange.start())) + return false; + auto& diag = scope.addDiag(code, driverRange); diag << name; if (isSingleDriverUDNT) { @@ -183,7 +191,14 @@ static bool handleOverlap(const Scope& scope, std::string_view name, const Value diag << netType->name; } - addAssignedHereNote(diag); + if (!driver.isNetAlias()) { + addAssignedHereNote(diag); + } + else { + diag.addNote(diag::NoteAliasedTo, *driver.symExprSR); + diag.addNote(diag::NoteAliasDeclaration, currRange); + diag.addNote(diag::NoteAliasedTo, *curr.symExprSR); + } return false; } @@ -204,6 +219,15 @@ void ValueSymbol::addDriver(DriverKind driverKind, const Expression& longestStat addDriver(*bounds, *driver); } +void ValueSymbol::addDriver(DriverKind driverKind, const Expression& longestStaticPrefix, + const Symbol& containingSymbol, bitmask flags, + const SourceRange& symExprSR, DriverBitRange exprBounds) const { + auto& comp = getParentScope()->getCompilation(); + auto driver = comp.emplace(driverKind, longestStaticPrefix, containingSymbol, + flags, &symExprSR); + addDriver(exprBounds, *driver); +} + void ValueSymbol::addDriver(DriverKind driverKind, DriverBitRange bounds, const Expression& longestStaticPrefix, const Symbol& containingSymbol, const Expression& procCallExpression) const { @@ -299,6 +323,7 @@ void ValueSymbol::addDriver(DriverBitRange bounds, const ValueDriver& driver) co // block to overlap even if the other block is an always_comb/ff. // - Assertion local variable formal arguments can't drive more than // one output to the same local variable. + // - Net bits are not aliased more than once bool isProblem = false; auto curr = *it; @@ -326,6 +351,13 @@ void ValueSymbol::addDriver(DriverBitRange bounds, const ValueDriver& driver) co } } + // If one of the drivers is an alias, then perform a check if the second one is an alias + if (curr->isNetAlias() || driver.isNetAlias()) { + isProblem = curr->isNetAlias() && driver.isNetAlias(); + // Check that all net alias drivers are have the same net alias symbol scope + isProblem = isProblem && (curr->containingSymbol == driver.containingSymbol); + } + if (isProblem) { if (!handleOverlap(*scope, name, *curr, driver, isNet, isUWire, isSingleDriverUDNT, netType)) { diff --git a/tests/unittests/ast/MemberTests.cpp b/tests/unittests/ast/MemberTests.cpp index 03d537fd6..6349fae42 100644 --- a/tests/unittests/ast/MemberTests.cpp +++ b/tests/unittests/ast/MemberTests.cpp @@ -1974,6 +1974,16 @@ module overlap2(inout wire [15:0] bus16, inout wire [11:0] low12, high12); alias high12[7:0] = low12[11:4]; endmodule +module overlap3(inout wire [15:0] bus16, inout wire [11:0] low12, high12); + alias low12 = bus16[11:0]; + alias high12 = bus16[15:4]; +endmodule + +module overlap4(inout wire [15:0] bus16, inout wire [11:0] low12, high12); + alias {high12, low12[3:0]} = bus16; + alias low12[11:4] = high12[7:0]; +endmodule + module lib1_dff(input logic Reset, Clk, Data, Q, Q_Bar); endmodule @@ -1987,6 +1997,58 @@ module my_dff(rst, clk, d, q, q_bar); alias Q_ = q_bar = Q_Bar = qbar; lib1_dff my_dff (.*); endmodule + +module m; + wire [1:0] a, b, c; + alias a = b[1:0]; + alias c = b[1:0]; +endmodule + +module m1(input wire [15:0] a, input wire [1:0] b); + alias {a[0], a[1]} = b[1:0]; +endmodule + +module m2(input wire [15:0] a, input wire [1:0] b); + alias {a[0], a[1]} = b; +endmodule + +module m3(input wire [15:0] a, input wire [1:0] b); + alias a[1] = b[0]; + alias a[0] = b[1]; +endmodule + +module m4(input wire [15:0] a, input wire [2:0] b, input wire [2:0] c, input wire [2:0] d, input wire [2:0] e); + alias {a[1], c[0]} = {d[0], b[1]}; +endmodule + +module m5(input wire [15:0] a, input wire [2:0] b, input wire [2:0] c, input wire [2:0] d, input wire [2:0] e); + alias {b, b} = {c, d}; +endmodule + +module m6(input wire [15:0] a, input wire [2:0] b, input wire [2:0] c, input wire [2:0] d, input wire [2:0] e); + alias {b, e} = {c, d}; +endmodule + +module m7(input wire [15:0] a, input wire [2:0] b, input wire [2:0] c, input wire [2:0] d, input wire [2:0] e); + alias b = d; + alias {b, e[0]} = {c, d[0]}; + alias e[1] = d[1]; +endmodule + +module m8(input wire [15:0] a, input wire [2:0] b, input wire [2:0] c, input wire [2:0] d, input wire [2:0] e); + alias {b, e[0]} = {c, d[0]}; + alias e[1] = d[2]; +endmodule + +module m9(input wire [15:0] a, input wire [15:0] b, input wire [14:0] c, input wire [15:0] d, input wire [2:0] e); + alias b[13:0] = d[13:0]; + alias b = {c, d[14]}; +endmodule + +module m10(input wire [15:0] a, input wire [1:0] b); + alias a[1] = b[0]; + alias a[0] = a[1]; +endmodule )"); Compilation compilation; @@ -2006,18 +2068,69 @@ module m; alias a = 1; alias a = c = m.c = d; endmodule + +module overlap(inout wire [15:0] bus16, inout wire [11:0] low12, high12, inout wire [32:0] c, inout wire [16:0] b, inout wire [32:0] b2); + alias bus16 = {{high12[4:0], high12[6:0]}, bus16[3:0]} = {bus16[15:12], high12}; + alias low12[0:0] = a; + alias low12 = {low12} = {low12}; + alias a = a; + alias b[2:0] = {c[2:0]}; + alias b1 = {c[1:1]}; + alias b2 = c; + alias bus16 = {high12[11:8], low12}; + alias bus16 = {high12, low12[3:0]}; + alias bus16 = {high12, bus16[3:0]} = {bus16[15:12], low12}; + module overlapnested(inout wire [15:0] bus15); + alias bus15 = bus16; + alias bus15 = bus16; + endmodule + overlapnested ov(bus16); +endmodule + +module m1(input wire [15:0] a, input wire [1:0] b); + alias {a[0], a[1]} = b[1:0]; + alias {a[0], a[1]} = b; + alias a[1] = b[0]; + alias a[0] = b[1]; +endmodule + +module m2(input wire [15:0] a, input wire [2:0] b, input wire [2:0] c, input wire [2:0] d, input wire [2:0] e); + alias {b, b} = {c, d}; + alias {b, e} = {c, d}; +endmodule + +module m3(input wire [15:0] a, input wire [2:0] b, input wire [2:0] c, input wire [2:0] d, input wire [2:0] e); + alias b = d; + alias {b, e[0]} = {c, d[0]}; + alias e[1] = d[1]; + alias {b, e[0]} = {c, d[0]}; + alias e[1] = d[2]; +endmodule + +module m10(input wire [15:0] a, input wire [15:0] b, input wire [14:0] c, input wire [15:0] d, input wire [2:0] e); + alias b[14:0] = d[14:0]; + alias b = {c, d[14]}; +endmodule + +module m11(input wire [15:0] a, input wire [1:0] b); + alias a[1] = b[0]; + alias a[0] = b[0]; +endmodule )"); Compilation compilation; compilation.addSyntaxTree(tree); auto& diags = compilation.getAllDiagnostics(); - REQUIRE(diags.size() == 5); + REQUIRE(diags.size() == 48); CHECK(diags[0].code == diag::NetAliasWidthMismatch); CHECK(diags[1].code == diag::ExpressionNotAssignable); CHECK(diags[2].code == diag::NetAliasNotANet); CHECK(diags[3].code == diag::NetAliasHierarchical); CHECK(diags[4].code == diag::NetAliasCommonNetType); + CHECK(diags[5].code == diag::MultipleNetAlias); + for (size_t i = 6; i < 48; ++i) + CHECK(diags[i].code == diag::MultipleNetAlias); } TEST_CASE("Action block parsing regress GH #911") {