diff --git a/scripts/diagnostics.txt b/scripts/diagnostics.txt index 40b7f02c8..43527f631 100644 --- a/scripts/diagnostics.txt +++ b/scripts/diagnostics.txt @@ -748,6 +748,7 @@ warning width-trunc WidthTruncate "implicit conversion truncates from {} to {} b warning port-width-expand PortWidthExpand "implicit conversion of port connection expands from {} to {} bits" warning port-width-trunc PortWidthTruncate "implicit conversion of port connection truncates from {} to {} bits" warning implicit-conv ImplicitConvert "implicit conversion from {} to {}" +warning packed-array-conv PackedArrayConv "implicit conversion from {} to {}" warning nonblocking-final NonblockingInFinal "nonblocking assignments in final blocks have no effect" warning port-coercion InputPortCoercion "input net port '{}' coerced to 'inout'" warning port-coercion OutputPortCoercion "output net port '{}' coerced to 'inout'" @@ -1140,7 +1141,8 @@ group default = { real-underflow real-overflow vector-overflow int-overflow unco raw-protect-eof protected-envelope specify-param dup-timing-path invalid-pulsestyle negative-timing-limit bad-procedural-force duplicate-defparam implicit-port-type-mismatch split-distweight-op dpi-pure-task multibit-edge unknown-sys-name unknown-library - dup-config-rule unused-config-cell unused-config-instance specify-condition-expr } + dup-config-rule unused-config-cell unused-config-instance specify-condition-expr + packed-array-conv } group extra = { empty-member empty-stmt dup-import pointless-void-cast case-gen-none case-gen-dup unused-result format-real ignored-slice task-ignored width-trunc dup-attr event-const @@ -1153,7 +1155,7 @@ group pedantic = { empty-pattern implicit-net-port nonstandard-escape-code nonst group conversion = { width-trunc width-expand port-width-trunc port-width-expand implicit-conv constant-conversion sign-conversion float-bool-conv int-bool-conv float-int-conv - int-float-conv float-widen float-narrow } + int-float-conv float-widen float-narrow packed-array-conv } group unused = { unused-def unused-net unused-implicit-net unused-variable undriven-net unassigned-variable unused-but-set-net unused-but-set-variable unused-port undriven-port unused-argument diff --git a/scripts/warning_docs.txt b/scripts/warning_docs.txt index 381b6fb27..a1c18e51a 100644 --- a/scripts/warning_docs.txt +++ b/scripts/warning_docs.txt @@ -1379,3 +1379,16 @@ primitive d_edge_ff (q, clock, data); endtable endprimitive ``` + +-Wpacked-array-conv +Indicates an implicit conversion between types with differing +packed array dimensions, even if their overall bit width is the same. +This will not warn for types with a single packed dimension; at least +one of the types must have multiple declared dimensions. +``` +module m; + logic [3:1][1:0] a; + logic [5:0] b; + initial b = a; +endmodule +``` diff --git a/source/ast/expressions/AssignmentExpressions.cpp b/source/ast/expressions/AssignmentExpressions.cpp index 3b242af67..b5acf0d6c 100644 --- a/source/ast/expressions/AssignmentExpressions.cpp +++ b/source/ast/expressions/AssignmentExpressions.cpp @@ -158,9 +158,13 @@ Expression* Expression::tryConnectPortArray(const ASTContext& context, const Typ // bit ranges from it -- the range select expression works on the declared // range of the packed array so a multidimensional wouldn't work correctly // without this conversion. - result = &ConversionExpression::makeImplicit( - context, comp.getType(*instPortWidth, result->type->getIntegralFlags()), - ConversionKind::Implicit, *result, nullptr, {}); + // + // We set the UnevaluatedBranch flag here so that we don't get any warnings + // related to implicit conversions. + auto& vecType = comp.getType(*instPortWidth, result->type->getIntegralFlags()); + result = &ConversionExpression::makeImplicit(context.resetFlags(ASTFlags::UnevaluatedBranch), + vecType, ConversionKind::Implicit, *result, + nullptr, {}); // We have enough bits to assign each port on each instance, so now we just need // to pick the right ones. The spec says we start with all right hand indices diff --git a/source/ast/expressions/ConversionExpression.cpp b/source/ast/expressions/ConversionExpression.cpp index 9950d3b8a..f306deb0c 100644 --- a/source/ast/expressions/ConversionExpression.cpp +++ b/source/ast/expressions/ConversionExpression.cpp @@ -11,6 +11,7 @@ #include "slang/ast/Bitstream.h" #include "slang/ast/Compilation.h" #include "slang/ast/EvalContext.h" +#include "slang/ast/expressions/LiteralExpressions.h" #include "slang/ast/expressions/MiscExpressions.h" #include "slang/ast/expressions/OperatorExpressions.h" #include "slang/ast/symbols/InstanceSymbols.h" @@ -88,6 +89,21 @@ bool isUnionMemberType(const Type& left, const Type& right) { return false; } +bool hasSameDims(const Type& left, const Type& right) { + if (left.getFixedRange().width() != right.getFixedRange().width()) + return false; + + auto le = left.getArrayElementType(); + auto re = right.getArrayElementType(); + if (bool(le) != bool(re)) + return false; + + if (!le) + return true; + + return hasSameDims(*le, *re); +} + void checkImplicitConversions(const ASTContext& context, const Type& sourceType, const Type& targetType, const Expression& op, const Expression* parentExpr, SourceRange operatorRange, @@ -97,6 +113,31 @@ void checkImplicitConversions(const ASTContext& context, const Type& sourceType, t.kind == SymbolKind::EnumType; }; + auto isMultiDimArray = [](const Type& t) { + return t.kind == SymbolKind::PackedArrayType && t.getArrayElementType()->getBitWidth() > 1; + }; + + auto parentIsComparison = [&] { + if (parentExpr && parentExpr->kind == ExpressionKind::BinaryOp) { + switch (parentExpr->as().op) { + case BinaryOperator::Equality: + case BinaryOperator::Inequality: + case BinaryOperator::CaseEquality: + case BinaryOperator::CaseInequality: + case BinaryOperator::GreaterThanEqual: + case BinaryOperator::GreaterThan: + case BinaryOperator::LessThanEqual: + case BinaryOperator::LessThan: + case BinaryOperator::WildcardEquality: + case BinaryOperator::WildcardInequality: + return true; + default: + break; + } + } + return false; + }; + auto addDiag = [&](DiagCode code) -> Diagnostic& { auto& diag = context.addDiag(code, op.sourceRange); if (operatorRange.start()) @@ -127,6 +168,30 @@ void checkImplicitConversions(const ASTContext& context, const Type& sourceType, return; } + // Warn for conversions between packed arrays of differing + // dimension counts or sizes. + if (isMultiDimArray(lt) || isMultiDimArray(rt) && !hasSameDims(lt, rt)) { + // Avoid warning for assignments or comparisons with 0 or '0, '1. + auto isZeroOrUnsized = [](const Expression& e) { + auto expr = &e.unwrapImplicitConversions(); + if (expr->kind == ExpressionKind::ConditionalOp) { + if (auto known = expr->as().knownSide()) + expr = known; + } + + return expr->kind == ExpressionKind::UnbasedUnsizedIntegerLiteral || + (expr->kind == ExpressionKind::IntegerLiteral && + bool(expr->as().getValue() == 0)); + }; + + if (!isZeroOrUnsized(op) && + (!parentIsComparison() || + !isZeroOrUnsized(parentExpr->as().right()))) { + addDiag(diag::PackedArrayConv) << sourceType << targetType; + return; + } + } + // Check to rule out false positives: try to eval as a constant. // We'll ignore any constants, because they will get their own more // fine grained warning during eval. @@ -136,27 +201,7 @@ void checkImplicitConversions(const ASTContext& context, const Type& sourceType, // Warn for sign conversions. if (lt.isSigned() != rt.isSigned()) { // Comparisons get their own warning elsewhere. - bool isComparison = false; - if (parentExpr && parentExpr->kind == ExpressionKind::BinaryOp) { - switch (parentExpr->as().op) { - case BinaryOperator::Equality: - case BinaryOperator::Inequality: - case BinaryOperator::CaseEquality: - case BinaryOperator::CaseInequality: - case BinaryOperator::GreaterThanEqual: - case BinaryOperator::GreaterThan: - case BinaryOperator::LessThanEqual: - case BinaryOperator::LessThan: - case BinaryOperator::WildcardEquality: - case BinaryOperator::WildcardInequality: - isComparison = true; - break; - default: - break; - } - } - - if (!isComparison) + if (!parentIsComparison()) addDiag(diag::SignConversion) << sourceType << targetType; } diff --git a/tests/unittests/ast/ClassTests.cpp b/tests/unittests/ast/ClassTests.cpp index a9198e447..c0ca86759 100644 --- a/tests/unittests/ast/ClassTests.cpp +++ b/tests/unittests/ast/ClassTests.cpp @@ -1918,7 +1918,7 @@ class D; bit [3:0][2:1] B [5:1][4]; constraint c1 { foreach (A[i, j, k]) A[i][j][k] inside {2,4,8,16}; - foreach (B[q, r, , s]) B[q][r] inside {1,2,3}; + foreach (B[q, r, , s]) 32'(B[q][r]) inside {1,2,3}; } randc int b; diff --git a/tests/unittests/ast/ExpressionTests.cpp b/tests/unittests/ast/ExpressionTests.cpp index 50765b148..c28fc2ab6 100644 --- a/tests/unittests/ast/ExpressionTests.cpp +++ b/tests/unittests/ast/ExpressionTests.cpp @@ -783,13 +783,17 @@ endmodule compilation.addSyntaxTree(tree); auto& diags = compilation.getAllDiagnostics(); - REQUIRE(diags.size() == 6); + REQUIRE(diags.size() == 10); CHECK(diags[0].code == diag::IndexOOB); CHECK(diags[1].code == diag::IndexOOB); - CHECK(diags[2].code == diag::RangeOOB); + CHECK(diags[2].code == diag::PackedArrayConv); CHECK(diags[3].code == diag::RangeOOB); - CHECK(diags[4].code == diag::RangeOOB); + CHECK(diags[4].code == diag::PackedArrayConv); CHECK(diags[5].code == diag::RangeOOB); + CHECK(diags[6].code == diag::PackedArrayConv); + CHECK(diags[7].code == diag::RangeOOB); + CHECK(diags[8].code == diag::PackedArrayConv); + CHECK(diags[9].code == diag::RangeOOB); } TEST_CASE("Empty concat error") { @@ -2827,7 +2831,7 @@ endmodule TEST_CASE("Index oob tryEval regress GH #602") { auto tree = SyntaxTree::fromText(R"( module top #( - parameter [2:0][4:0] IDX_MAP = {5'd1, 5'd3, 5'd4} + parameter [2:0][4:0] IDX_MAP = '{5'd1, 5'd3, 5'd4} ); logic [4:0] sig; diff --git a/tests/unittests/ast/TypeTests.cpp b/tests/unittests/ast/TypeTests.cpp index a57e4bab6..eb6a51356 100644 --- a/tests/unittests/ast/TypeTests.cpp +++ b/tests/unittests/ast/TypeTests.cpp @@ -947,7 +947,7 @@ module m; union packed { logic [3:0] a; bit [1:4] b; } [4:1] u; enum { A, B, C } [4:1] e; - initial e = A; + initial e[1] = A; endmodule )");