[slang] Introduce net aliases bits duplication checks #1045

likeamahoney · 2024-07-04T14:55:27Z

Introduce 2 new net alias bit checks from SystemVerilog LRM section 10.11:

check that net aliased to itself
check that aliased bits specified more than once at module instance scope

likeamahoney · 2024-07-04T14:57:35Z

I think with it LRM section 10.11 is done

codecov · 2024-07-04T15:12:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.77%. Comparing base (f6e3510) to head (7b7474f).
Report is 42 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1045   +/-   ##
=======================================
  Coverage   94.76%   94.77%           
=======================================
  Files         191      191           
  Lines       47743    47825   +82     
=======================================
+ Hits        45243    45325   +82     
  Misses       2500     2500

Files with missing lines	Coverage Δ
include/slang/ast/Compilation.h	`100.00% <ø> (ø)`
include/slang/ast/SemanticFacts.h	`84.21% <100.00%> (ø)`
include/slang/ast/symbols/ValueSymbol.h	`100.00% <100.00%> (ø)`
source/ast/Compilation.cpp	`96.74% <100.00%> (+0.01%)`	⬆️
source/ast/symbols/MemberSymbols.cpp	`97.57% <100.00%> (+0.10%)`	⬆️
source/ast/symbols/ValueSymbol.cpp	`98.03% <100.00%> (+0.13%)`	⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6e3510...7b7474f. Read the comment docs.

MikePopoloski · 2024-07-14T20:13:17Z

include/slang/ast/symbols/InstanceSymbols.h

+    // 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;


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.

reimplemented all in order to last commentary

MikePopoloski · 2024-07-14T20:13:56Z

source/ast/symbols/MemberSymbols.cpp

@@ -2196,7 +2196,6 @@ NetAliasSymbol& NetAliasSymbol::fromSyntax(const ASTContext& parentContext,
        if (!netType.isError()) {
            SmallVector<const IdentifierNameSyntax*> implicitNetNames;
            Expression::findPotentiallyImplicitNets(*expr, context, implicitNetNames);
-


Random newline deletion seems unrelated to the rest of the diff

MikePopoloski · 2024-07-14T20:32:26Z

source/ast/symbols/MemberSymbols.cpp

+                    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()));


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.

Great Idea! The solution was reimplemented according to it!

MikePopoloski

Ok yeah, this is way better. I left some additional comments to help simplify this a bit more.

MikePopoloski · 2024-07-21T18:39:44Z

source/ast/symbols/ValueSymbol.cpp

@@ -7,6 +7,8 @@
 //------------------------------------------------------------------------------
 #include "slang/ast/symbols/ValueSymbol.h"

+#include <iostream>


The iostream include isn't needed right?

MikePopoloski · 2024-07-22T18:49:02Z

source/ast/ASTContext.cpp

@@ -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)) ||


I think you can get rid of this change if you simply don't use the NotADriver flag when binding the net alias expressions

MikePopoloski · 2024-07-22T18:49:25Z

source/ast/symbols/MemberSymbols.cpp

@@ -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);


Down here, you can remove the NotADriver

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.

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.

MikePopoloski · 2024-07-22T18:50:20Z

source/ast/symbols/MemberSymbols.cpp


-    for (auto exprSyntax : syntax->as<NetAliasSyntax>().nets) {
+    for (auto exprSyntax : nets) {
        auto& netRef = Expression::bind(*exprSyntax, context);
        if (!netRef.requireLValue(context))


And then here pass your new AssignFlag and then you won't need the additional addDriver call below I think.

MikePopoloski · 2024-07-24T14:18:56Z

source/ast/symbols/MemberSymbols.cpp

@@ -2144,6 +2159,21 @@ std::span<const Expression* const> NetAliasSymbol::getNetReferences() const {

        netRef.visit(visitor);
        buffer.push_back(&netRef);
+
+        if (exprSyntax != *nets.begin()) {


This logic seems confusing to me; you really just need to check whether the same symbol appears in more than one of the net alias expressions right? The aliasedSyms map in the NetAliasVisitor is already tracking that; instead of insert_or_assign up above you can just check whether the insertion fails because the symbol already exists, in which case you can issue the error right there. Then you don't need any of this special casing for it here and the separate map tracking the same thing.

solution was reimplemented by focusing on merge net alias expressions

aliasedSyms map logic was reduced from NetAliasVisitor

My solution no longer ignores the leftoutermost net alias expression

To relate net alias expressions to existing alias sybmols, I used a comparison of their syntaxes

it's not a best solution but in this case it is possible, since in aliases all symbols are global nets from the point of view of the module, in the future we need to come up with a better comparator

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.

Provided example works correctly regardless of the alias permutations.

MikePopoloski · 2024-07-24T14:29:19Z

source/ast/symbols/MemberSymbols.cpp

@@ -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);


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.

likeamahoney · 2024-08-12T10:51:48Z

closes #947

MikePopoloski · 2024-08-15T20:15:25Z

include/slang/ast/SemanticFacts.h

-    SlicedPort = 1 << 7
+    SlicedPort = 1 << 7,
+
+    // The assignment declares net alias


Should use triple comments to make it show up in documentation, like other entries above.

MikePopoloski · 2024-08-16T09:53:19Z

include/slang/ast/ASTContext.h

@@ -298,6 +299,9 @@ class SLANG_EXPORT ASTContext {
    /// Various flags that control how AST creation is performed.
    bitmask<ASTFlags> flags;

+    /// Placeholder for storing currently processed net alias
+    const NetAliasSymbol* netAlias = nullptr;


It would be good to not add another member to ASTContext if we can help it, especially since net aliases are very rare. I think you should be able to reuse instanceOrProc for this. We can rename it to something more generic like "relevantSymbol" maybe and then you can set it when you create the specialized ASTContext for checking net aliases.

MikePopoloski · 2024-08-16T10:24:39Z

source/ast/symbols/MemberSymbols.cpp

+
+    // Preprocessing net alias expressions to relate them to existing net aliases.
+    const NetAliasSymbol* current = this;
+    for (auto exprSyntax : nets) {


The logic here still seems wrong. Consider this simple example:

module m; wire [1:0] a, b, c; alias a = b[1:0]; alias c = b[1:0]; endmodule

which results in the following error (when it should not):

source:5:15: error: it is not allowed to specify an alias from an individual signal to itself or to specify a given alias more than once alias c = b[1:0]; ^~~~~~ source:4:15: note: alias declared here alias a = b[1:0]; ^~~~~~

I don't think this mergeOrAddNetAlias idea works or is even necessary really. We don't need a compilation-wide map. Each net can already track which bits have been aliased using the driver mechanism. I think what you need to do is to do an addDriver() call across the cartesian product of the aliases in a given net alias statement. Instead of the "containing symbol" being the net alias, instead you make it the net it's being aliased to.

As an example:

alias a = b = c;

you want the following sequence of addDriver calls to be made, where the argument here is what gets set as the "containing symbol" inside ValueDriver:
a.addDriver(b);
a.addDriver(c);
b.addDriver(a);
b.addDriver(c);
c.addDriver(a);
c.addDriver(b);

And then I think the existing logic will just do the right thing without the need for a global map or comparing syntax nodes to each other.

sorry for delay

solution was reimplemented according to your suggestion

but i need to add more complex tests

CI was fixed and new tests were added

likeamahoney force-pushed the feature/net-alias-duplication-check branch from 49d4723 to 16b72d5 Compare July 12, 2024 15:05

MikePopoloski reviewed Jul 14, 2024

View reviewed changes

likeamahoney force-pushed the feature/net-alias-duplication-check branch from 16b72d5 to 4dd5370 Compare July 16, 2024 12:18

MikePopoloski reviewed Jul 22, 2024

View reviewed changes

likeamahoney force-pushed the feature/net-alias-duplication-check branch from 54382d4 to 1ea3267 Compare July 23, 2024 19:49

MikePopoloski reviewed Jul 24, 2024

View reviewed changes

likeamahoney force-pushed the feature/net-alias-duplication-check branch from 848e6c6 to 3a128e4 Compare August 1, 2024 15:29

MikePopoloski reviewed Aug 16, 2024

View reviewed changes

likeamahoney force-pushed the feature/net-alias-duplication-check branch 3 times, most recently from 622f998 to cd0a5d0 Compare August 23, 2024 00:25

[slang] Introduce net aliases bits duplication checks

677fa45

likeamahoney force-pushed the feature/net-alias-duplication-check branch from 44d93ee to 677fa45 Compare August 23, 2024 11:22

Add new tests

ef47ec7

likeamahoney force-pushed the feature/net-alias-duplication-check branch 2 times, most recently from 8fed29e to 6cff3b3 Compare August 23, 2024 20:03

Fix implicit conversion

bc6e58e

likeamahoney force-pushed the feature/net-alias-duplication-check branch from 6cff3b3 to bc6e58e Compare August 23, 2024 20:21

Add more tests

7b7474f

MikePopoloski merged commit 528e209 into MikePopoloski:master Oct 23, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[slang] Introduce net aliases bits duplication checks #1045

[slang] Introduce net aliases bits duplication checks #1045

likeamahoney commented Jul 4, 2024

likeamahoney commented Jul 4, 2024

codecov bot commented Jul 4, 2024 •

edited

Loading

MikePopoloski Jul 14, 2024

likeamahoney Jul 16, 2024

MikePopoloski Jul 14, 2024

likeamahoney Jul 16, 2024

MikePopoloski Jul 14, 2024

likeamahoney Jul 16, 2024

MikePopoloski left a comment

MikePopoloski Jul 21, 2024

likeamahoney Jul 23, 2024

MikePopoloski Jul 22, 2024

likeamahoney Jul 23, 2024

MikePopoloski Jul 22, 2024

likeamahoney Jul 23, 2024

MikePopoloski Jul 24, 2024

MikePopoloski Jul 22, 2024

likeamahoney Jul 23, 2024

MikePopoloski Jul 24, 2024

likeamahoney Aug 1, 2024

likeamahoney Aug 1, 2024

likeamahoney Aug 1, 2024

likeamahoney Aug 12, 2024

MikePopoloski Jul 24, 2024

likeamahoney commented Aug 12, 2024

MikePopoloski Aug 15, 2024

likeamahoney Aug 23, 2024

MikePopoloski Aug 16, 2024

likeamahoney Aug 23, 2024

MikePopoloski Aug 16, 2024

likeamahoney Aug 23, 2024

likeamahoney Aug 23, 2024

likeamahoney Aug 23, 2024

likeamahoney Aug 23, 2024

[slang] Introduce net aliases bits duplication checks #1045

[slang] Introduce net aliases bits duplication checks #1045

Conversation

likeamahoney commented Jul 4, 2024

likeamahoney commented Jul 4, 2024

codecov bot commented Jul 4, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikePopoloski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

likeamahoney commented Aug 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 4, 2024 •

edited

Loading