analysis: Fixes and improvements for ref port driver tracking

MikePopoloski · MikePopoloski · commit 078433e3c258 · 2025-11-02T08:42:42.000-05:00
diff --git a/include/slang/ast/LSPUtilities.h b/include/slang/ast/LSPUtilities.h
@@ -192,6 +192,9 @@ class SLANG_EXPORT LSPUtilities {
     ///
     /// Unlike @a visitLSPs, this method will expand indirect references such as
     /// modport port connections and ref port connections.
+    ///
+    /// @param alloc The allocator to use for any AST nodes that need to be created
+    ///              to represent expanded selections.
     static void expandIndirectLSPs(BumpAllocator& alloc, const Expression& expr,
                                    EvalContext& evalContext, LSPCallback callback,
                                    bool isLValue = true);
diff --git a/source/analysis/DriverTracker.cpp b/source/analysis/DriverTracker.cpp
@@ -144,6 +144,16 @@ void DriverTracker::propagateIndirectDrivers(AnalysisContext& context, DriverAll
                 LSPUtilities::expandIndirectLSPs(
                     context.alloc, *originalDriver->prefixExpression, evalCtx,
                     [&](const ValueSymbol& symbol, const Expression& lsp, bool isLValue) {
+                        // If the LSP maps to the same symbol as the original driver,
+                        // skip it to avoid infinite recursion. This can happen only if
+                        // this is a ref port and `expandIndirectLSPs` determined that
+                        // the driver doesn't actually apply to the port due to a
+                        // non-overlapping internal connection expression.
+                        if (&symbol == item.first) {
+                            SLANG_ASSERT(symbol.isConnectedToRefPort());
+                            return;
+                        }
+
                         addFromLSP(context, driverAlloc, originalDriver->kind,
                                    originalDriver->flags, *originalDriver->containingSymbol, symbol,
                                    lsp, isLValue, evalCtx, hierPortDrivers);
@@ -367,20 +377,28 @@ void DriverTracker::addDriver(AnalysisContext& context, DriverAlloc& driverAlloc
     }
 
     // Keep track of "indirect" drivers separately so we can revisit them at the end of analysis.
-    if (symbol.kind == SymbolKind::ModportPort || symbol.isConnectedToRefPort()) {
-        auto updater = [&](auto& item) { item.second.emplace_back(&driver, bounds); };
-        indirectDrivers.try_emplace_and_visit(&symbol, updater, updater);
+    auto indirectUpdater = [&](auto& item) { item.second.emplace_back(&driver, bounds); };
+    if (symbol.kind == SymbolKind::ModportPort) {
+        indirectDrivers.try_emplace_and_visit(&symbol, indirectUpdater, indirectUpdater);
+
+        // We don't do overlap detection for modports but we will still track them for downstream
+        // users to query later.
+        driverMap.insert(bounds, &driver, driverAlloc);
+        return;
+    }
+
+    if (symbol.isConnectedToRefPort()) {
+        indirectDrivers.try_emplace_and_visit(&symbol, indirectUpdater, indirectUpdater);
 
-        if (symbol.kind != SymbolKind::ModportPort && !driver.isFromSideEffect) {
+        if (!driver.isFromSideEffect) {
             // Ref port drivers are side effects that need to be applied to
             // non-canonical instances.
             hierPortDrivers.push_back({&driver, &symbol, nullptr});
         }
 
-        // We don't do overlap detection for modport / ref ports (the drivers apply to the
-        // underlying connection) but we will still track them for downstream users to query later.
-        driverMap.insert(bounds, &driver, driverAlloc);
-        return;
+        // For ref ports we will continue on and do normal overlap checking,
+        // since the ref port might not actually apply if it has an internal
+        // connection expression that points somewhere else.
     }
 
     if (driverMap.empty()) {
diff --git a/source/ast/LSPUtilities.cpp b/source/ast/LSPUtilities.cpp
@@ -8,6 +8,8 @@
 #include "slang/ast/LSPUtilities.h"
 
 #include "slang/ast/ASTVisitor.h"
+#include "slang/ast/EvalContext.h"
+#include "slang/ast/TypeProvider.h"
 
 namespace slang::ast {
 
@@ -62,17 +64,13 @@ void LSPUtilities::stringifyLSP(const Expression& expr, EvalContext& evalContext
     }
 }
 
-std::optional<DriverBitRange> LSPUtilities::getBounds(const Expression& prefixExpression,
-                                                      EvalContext& evalContext,
-                                                      const Type& rootType) {
+static std::optional<DriverBitRange> computeBounds(SmallVector<const Expression*>& path,
+                                                   size_t skip, EvalContext& evalContext,
+                                                   const Type& rootType) {
     auto type = &rootType.getCanonicalType();
     DriverBitRange result{0, type->getSelectableWidth() - 1};
 
-    SmallVector<const Expression*> path;
-    visitComponents(prefixExpression, /* includeRoot */ false,
-                    [&](const Expression& expr) { path.push_back(&expr); });
-
-    for (size_t i = path.size(); i > 0; i--) {
+    for (size_t i = path.size() - skip; i > 0; i--) {
         uint64_t start, width;
         auto& elem = *path[i - 1];
         if (elem.kind == ExpressionKind::MemberAccess) {
@@ -112,29 +110,14 @@ std::optional<DriverBitRange> LSPUtilities::getBounds(const Expression& prefixEx
     return result;
 }
 
-static Expression* translateSelector(BumpAllocator& alloc, Expression& value,
-                                     const Expression& sourceSelector) {
-    // TODO: need to translate selector indices because types may be only
-    // equivalent and not matching.
-    switch (sourceSelector.kind) {
-        case ExpressionKind::ElementSelect: {
-            auto& es = sourceSelector.as<ElementSelectExpression>();
-            return alloc.emplace<ElementSelectExpression>(*es.type, value, es.selector(),
-                                                          es.sourceRange);
-        }
-        case ExpressionKind::RangeSelect: {
-            auto& rs = sourceSelector.as<RangeSelectExpression>();
-            return alloc.emplace<RangeSelectExpression>(rs.getSelectionKind(), *rs.type, value,
-                                                        rs.left(), rs.right(), rs.sourceRange);
-        }
-        case ExpressionKind::MemberAccess: {
-            auto& ma = sourceSelector.as<MemberAccessExpression>();
-            return alloc.emplace<MemberAccessExpression>(*ma.type, value, ma.member,
-                                                         ma.sourceRange);
-        }
-        default:
-            SLANG_UNREACHABLE;
-    }
+std::optional<DriverBitRange> LSPUtilities::getBounds(const Expression& prefixExpression,
+                                                      EvalContext& evalContext,
+                                                      const Type& rootType) {
+    SmallVector<const Expression*> path;
+    visitComponents(prefixExpression, /* includeRoot */ false,
+                    [&](const Expression& expr) { path.push_back(&expr); });
+
+    return computeBounds(path, 0, evalContext, rootType);
 }
 
 static bool expandRefPortConn(BumpAllocator& alloc, EvalContext& evalContext, const Expression& lsp,
@@ -180,10 +163,61 @@ static bool expandRefPortConn(BumpAllocator& alloc, EvalContext& evalContext, co
         // glue the uncovered portion of the LSP onto the external connection.
         // The const_cast here is okay because we never mutate anything during analysis.
         auto newExpr = const_cast<Expression*>(&externalConn);
-        for (size_t i = elemsToRemove; i < lspPath.size(); i++) {
-            auto elem = lspPath[lspPath.size() - 1 - i];
-            newExpr = translateSelector(alloc, *newExpr, *elem);
+
+        // First, replicate all of the selects for unpacked types. The only way that
+        // types can mismatch here is for fixed size arrays, which can have differing
+        // ranges, so translate those along the way.
+        for (; elemsToRemove < lspPath.size(); elemsToRemove++) {
+            auto& ct = newExpr->type->getCanonicalType();
+            if (ct.isIntegral())
+                break;
+
+            auto elem = lspPath[lspPath.size() - 1 - elemsToRemove];
+            if (elem->kind == ExpressionKind::MemberAccess) {
+                auto& ma = elem->as<MemberAccessExpression>();
+                newExpr = alloc.emplace<MemberAccessExpression>(*ma.type, *newExpr, ma.member,
+                                                                ma.sourceRange);
+                continue;
+            }
+
+            auto targetDim = ct.getFixedRange();
+            auto translateIndex = [&](int32_t index) {
+                if (targetDim.isLittleEndian())
+                    return targetDim.upper() - index;
+                else
+                    return targetDim.lower() + index;
+            };
+
+            auto selection = elem->evalSelector(evalContext, /* enforceBounds */ true);
+            SLANG_ASSERT(selection);
+
+            selection->left = translateIndex(selection->left);
+            selection->right = translateIndex(selection->right);
+
+            if (elem->kind == ExpressionKind::ElementSelect) {
+                newExpr = &ElementSelectExpression::fromConstant(alloc, *newExpr,
+                                                                 selection->lower(),
+                                                                 evalContext.astCtx);
+            }
+            else {
+                newExpr = &RangeSelectExpression::fromConstant(alloc, *newExpr, *selection,
+                                                               evalContext.astCtx);
+            }
         }
+
+        // For remaining integral path components, compute the bounds and then
+        // recreate a corresponding select tree that achieves those same bounds.
+        if (elemsToRemove < lspPath.size()) {
+            auto bounds = computeBounds(lspPath, elemsToRemove, evalContext, *newExpr->type);
+            SLANG_ASSERT(bounds);
+
+            // Note that this can't overflow here because it's a packed type
+            // so the total width is bounded.
+            ConstantRange range{int32_t(bounds->second), int32_t(bounds->first)};
+            newExpr = &Expression::buildPackedSelectTree(alloc, *newExpr, range,
+                                                         evalContext.astCtx);
+        }
+
         LSPUtilities::visitLSPs(*newExpr, evalContext, callback, isLValue);
     }
     return true;
@@ -268,8 +302,7 @@ void LSPUtilities::expandIndirectLSP(BumpAllocator& alloc, EvalContext& evalCont
 
     if (!anyRefPorts) {
         // No ref ports or modports involved, so just invoke the callback directly.
-        // TODO:
-        // callback(symbol, lsp, isLValue);
+        callback(symbol, lsp, isLValue);
     }
 }
 
diff --git a/tests/unittests/analysis/MultiAssignTests.cpp b/tests/unittests/analysis/MultiAssignTests.cpp
@@ -1217,12 +1217,11 @@ endmodule
 
 TEST_CASE("Multi assign through ref ports 2") {
     auto& code = R"(
-// module r({a, b});
-//     ref logic a;
-//     output logic b;
-
-//     assign a = 1;
-// endmodule
+module r({a, b});
+    ref logic a;
+    output logic b;
+    assign a = 1;
+endmodule
 
 module v(ref .a(foo.b));
     struct { logic a; logic b; } foo;
@@ -1232,12 +1231,26 @@ endmodule
 module w(ref .a(foo[0][1]));
     logic [1:0] foo[2][3];
     assign foo[0][1][0] = 1;
+    assign foo[0][1][1] = 1;
+
+    assign foo[1][0] = 1;
+    assign foo[1][0][1] = 1;
+endmodule
+
+module x(ref .a(foo), .b(foo));
+    logic foo;
+    assign foo = 1;
+endmodule
+
+module y(ref .a(foo[0]));
+    logic [5:3] foo[2];
+    assign foo[0][5] = 1;
 endmodule
 
 module top;
-    // logic [1:0] q;
-    // r r1(q);
-    // assign q[1] = 1;
+    logic [1:0] q;
+    r r1(q);
+    assign q[1] = 1;
 
     logic s [1:0][3:1];
     v v1(s[1][1]);
@@ -1246,14 +1259,67 @@ module top;
     logic [1:0] t [1:0][3:1];
     w w1(t[1][1]);
     assign t[1][1][0] = 1;
+
+    logic u, v;
+    x x1(u, v);
+    assign {u, v} = 1;
+
+    logic [2:4] z;
+    y y1(z);
+    assign z[2:3] = 1;
 endmodule
 )";
 
     Compilation compilation;
     AnalysisManager analysisManager;
 
     auto [diags, design] = analyze(code, compilation, analysisManager);
-    REQUIRE(diags.size() == 2);
+    REQUIRE(diags.size() == 6);
+    CHECK(diags[0].code == diag::MultipleContAssigns);
+    CHECK(diags[1].code == diag::MultipleContAssigns);
+    CHECK(diags[2].code == diag::MultipleContAssigns);
+    CHECK(diags[3].code == diag::MultipleContAssigns);
+    CHECK(diags[4].code == diag::MultipleContAssigns);
+    CHECK(diags[5].code == diag::MultipleContAssigns);
+}
+
+TEST_CASE("Multi assign through ref ports 3") {
+    auto& code = R"(
+module y(ref .a(foo));
+    logic [5:3] foo;
+    assign foo[3] = 1;
+endmodule
+
+module b(ref logic[7:0] a [2:6]);
+    assign a[3][5:4] = 1;
+endmodule
+
+typedef struct { logic a[2:8]; } bar_t;
+module c(ref bar_t a);
+    assign a.a[4:5] = '{1, 0};
+endmodule
+
+module top;
+    logic [2:4] z;
+    y y1(z);
+    assign z[4] = 1;
+
+    struct packed { logic a; logic b; } [1:4] a [5:1];
+    b b1(a);
+    assign a[4][2].b = 1;
+
+    bar_t d;
+    c c1(d);
+    assign d.a[4:5] = '{0, 1};
+endmodule
+)";
+
+    Compilation compilation;
+    AnalysisManager analysisManager;
+
+    auto [diags, design] = analyze(code, compilation, analysisManager);
+    REQUIRE(diags.size() == 3);
     CHECK(diags[0].code == diag::MultipleContAssigns);
     CHECK(diags[1].code == diag::MultipleContAssigns);
+    CHECK(diags[2].code == diag::MultipleContAssigns);
 }
