[slang] Introduce net aliases bits duplication checks #1045
Conversation
|
I think with it LRM section 10.11 is done |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1045 +/- ##
==========================================
+ Coverage 93.76% 94.71% +0.94%
==========================================
Files 189 191 +2
Lines 47139 47695 +556
==========================================
+ Hits 44201 45172 +971
+ Misses 2938 2523 -415
... and 172 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
likeamahoney
force-pushed
the
feature/net-alias-duplication-check
branch
from
49d4723
to
16b72d5
Compare
| // Store any net alias bits to check it's possible intersections. | ||
| // Since alias statements can appear anywhere module instance statements can appears | ||
| // good option is to store aliased bits info in the module instance scope. | ||
| mutable AliasMap* aliasedBits; |
There was a problem hiding this comment.
It seems unnecessary to have an alias map allocated for every instance. The rules about net alias overlap are global, so it seems fine to have just one alias map stored in the Compilation object that all aliases can access. Then you don't need a custom allocator for it either.
There was a problem hiding this comment.
reimplemented all in order to last commentary
source/ast/symbols/MemberSymbols.cpp
Outdated
| @@ -2196,7 +2196,6 @@ NetAliasSymbol& NetAliasSymbol::fromSyntax(const ASTContext& parentContext, | |||
| if (!netType.isError()) { | |||
| SmallVector<const IdentifierNameSyntax*> implicitNetNames; | |||
| Expression::findPotentiallyImplicitNets(*expr, context, implicitNetNames); | |||
|
|
|||
There was a problem hiding this comment.
Random newline deletion seems unrelated to the rest of the diff
source/ast/symbols/MemberSymbols.cpp
Outdated
| if (const auto* sym = processExpr(expr)) { | ||
| const auto& eSelExpr = expr.template as<ElementSelectExpression>(); | ||
| // If value can't be evaluated that means it is unbounded | ||
| const auto evalVal = toInt(context.tryEval(eSelExpr.selector())); |
There was a problem hiding this comment.
I don't think this is correct in all cases. An example is when there is multiple selections of a multi-dimensional array. It's actually pretty tricky to figure out the statically assigned bits of an arbitrary expression -- that's essentially what the ValueDriver code in ValueSymbol.cpp is doing to be able to issue multi-driven errors. It occurs to me that it may make more sense to use that code to implement the net alias check. You could add a new flag to AssignFlags that says it's a net alias and do the check for overlap there.
There was a problem hiding this comment.
Great Idea! The solution was reimplemented according to it!
likeamahoney
force-pushed
the
feature/net-alias-duplication-check
branch
from
16b72d5
to
4dd5370
Compare
| @@ -7,6 +7,8 @@ | |||
| //------------------------------------------------------------------------------ | |||
| #include "slang/ast/symbols/ValueSymbol.h" | |||
|
|
|||
| #include <iostream> | |||
There was a problem hiding this comment.
The iostream include isn't needed right?
| @@ -136,7 +136,8 @@ void ASTContext::setAttributes(const Expression& expr, | |||
|
|
|||
| void ASTContext::addDriver(const ValueSymbol& symbol, const Expression& longestStaticPrefix, | |||
| bitmask<AssignFlags> assignFlags) const { | |||
| if (flags.has(ASTFlags::NotADriver) || scope->isUninstantiated()) | |||
| if ((flags.has(ASTFlags::NotADriver) && !assignFlags.has(AssignFlags::NetAlias)) || | |||
There was a problem hiding this comment.
I think you can get rid of this change if you simply don't use the NotADriver flag when binding the net alias expressions
| @@ -2119,16 +2124,18 @@ 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); | |||
There was a problem hiding this comment.
Down here, you can remove the NotADriver
|
|
||
| for (auto exprSyntax : syntax->as<NetAliasSyntax>().nets) { | ||
| for (auto exprSyntax : nets) { | ||
| auto& netRef = Expression::bind(*exprSyntax, context); | ||
| if (!netRef.requireLValue(context)) |
There was a problem hiding this comment.
And then here pass your new AssignFlag and then you won't need the additional addDriver call below I think.
Introduce 2 new net alias bit checks from SystemVerilog LRM section 10.11: