Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[slang] Introduce net aliases bits duplication checks #1045

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions include/slang/ast/Compilation.h
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
9 changes: 6 additions & 3 deletions include/slang/ast/SemanticFacts.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,

Expand Down Expand Up @@ -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.
Expand Down
15 changes: 13 additions & 2 deletions include/slang/ast/symbols/ValueSymbol.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ class SLANG_EXPORT ValueSymbol : public Symbol {
void addDriver(DriverKind kind, const Expression& longestStaticPrefix,
const Symbol& containingSymbol, bitmask<AssignFlags> flags) const;

void addDriver(DriverKind kind, const Expression& longestStaticPrefix,
const Symbol& containingSymbol, bitmask<AssignFlags> flags,
const SourceRange& symExprSR, DriverBitRange exprBounds) const;

void addDriver(DriverKind kind, DriverBitRange bounds, const Expression& longestStaticPrefix,
const Symbol& containingSymbol, const Expression& procCallExpression) const;

Expand Down Expand Up @@ -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<AssignFlags> flags) :
const Symbol& containingSymbol, bitmask<AssignFlags> 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); }
Expand All @@ -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;

Expand Down
3 changes: 3 additions & 0 deletions scripts/diagnostics.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -713,6 +715,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"
Expand Down
14 changes: 14 additions & 0 deletions source/ast/Compilation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1631,6 +1631,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();
}
Expand Down
97 changes: 92 additions & 5 deletions source/ast/symbols/MemberSymbols.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2210,12 +2210,21 @@ NetAliasSymbol& NetAliasSymbol::fromSyntax(const ASTContext& parentContext,
return *result;
}

struct NetAliasPOD {
const ValueSymbol* sym;
const Expression* expr;
std::optional<DriverBitRange> bounds;
};

struct NetAliasVisitor {
const ASTContext& context;
const NetType* commonNetType = nullptr;
bool issuedError = false;
SmallVector<NetAliasPOD, 4> netAliases;
EvalContext& evalCtx;

NetAliasVisitor(const ASTContext& context) : context(context) {}
NetAliasVisitor(const ASTContext& context, EvalContext& evalCtx) :
context(context), evalCtx(evalCtx) {}

template<typename T>
void visit(const T& expr) {
Expand All @@ -2230,7 +2239,11 @@ struct NetAliasVisitor {
context.addDiag(diag::NetAliasNotANet, expr.sourceRange) << sym->name;
}
else {
auto& nt = sym->template as<NetSymbol>().netType;
auto& netSym = sym->template as<NetSymbol>();
netAliases.push_back(
{&netSym, &expr,
ValueDriver::getBounds(expr, evalCtx, netSym.getType())});
auto& nt = netSym.netType;
if (!commonNetType) {
commonNetType = &nt;
}
Expand Down Expand Up @@ -2265,14 +2278,14 @@ std::span<const Expression* const> 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<const Expression*> buffer;
ASTContext context(*scope, LookupLocation::after(*this),
ASTFlags::NonProcedural | ASTFlags::NotADriver);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Down here, you can remove the NotADriver

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flag is needed to skip the first alias (the leftmost one) in order to prevent FP. I left it, but remove it after the first iteration of the loop.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not following why the first alias expression should be exempt from the duplicate checking. There isn't anything special about the first alias vs any others; you can reverse the order without changing the behavior.

I do think we need more info included in the addDriver call though, since it's only an error if the overlap occurs with the same target net? Like you can alias the same range of bits from one net to multiple different nets, it's only error if it's the same nets each time. This example from the LRM I think won't pass with the current code?

module overlap(inout wire [15:0] bus16, inout wire [11:0] low12, high12);
alias bus16[11:0] = low12;
alias bus16[15:4] = high12;
endmodule

module overlap(inout wire [15:0] bus16, inout wire [11:0] low12, high12);
alias bus16 = {high12, low12[3:0]};
alias high12[7:0] = low12[11:4];
endmodule

Or maybe it will, but not if you reverse the ordering in the alias statements.

NetAliasVisitor visitor(context);
EvalContext evalCtx(ASTContext(*scope, LookupLocation::max));
NetAliasVisitor visitor(context, evalCtx);
SmallVector<std::pair<const Expression*, std::span<NetAliasPOD>>, 4> netAliases;

for (auto exprSyntax : syntax->as<NetAliasSyntax>().nets) {
auto& netRef = Expression::bind(*exprSyntax, context);
Expand All @@ -2290,9 +2303,83 @@ std::span<const Expression* const> 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<const Expression*, 4> 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<std::pair<DriverBitRange, bool>> 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;
}

Expand Down
34 changes: 33 additions & 1 deletion source/ast/symbols/ValueSymbol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -171,19 +171,34 @@ 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) {
SLANG_ASSERT(netType);
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;
}

Expand All @@ -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<AssignFlags> flags,
const SourceRange& symExprSR, DriverBitRange exprBounds) const {
auto& comp = getParentScope()->getCompilation();
auto driver = comp.emplace<ValueDriver>(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 {
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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)) {
Expand Down
Loading