Refactoring and clean up of assignment conversion logic

bindings/python/ASTBindings.cpp

@@ -79,7 +79,6 @@ void registerAST(py::module_& m) {
         .value("InsideConcatenation", ASTFlags::InsideConcatenation)
         .value("UnevaluatedBranch", ASTFlags::UnevaluatedBranch)
         .value("AllowDataType", ASTFlags::AllowDataType)
-        .value("EnumInitializer", ASTFlags::EnumInitializer)
         .value("NoAttributes", ASTFlags::NoAttributes)
         .value("AssignmentAllowed", ASTFlags::AssignmentAllowed)
         .value("AssignmentDisallowed", ASTFlags::AssignmentDisallowed)

include/slang/ast/ASTContext.h

@@ -46,148 +46,145 @@ enum class SLANG_EXPORT ASTFlags : uint64_t {
     /// the first argument to system methods like $bits
     AllowDataType = 1ull << 2,
 
-    /// The expression being created is an enum value initializer.
-    EnumInitializer = 1ull << 3,
-
     /// Attributes are disallowed on expressions in this context.
-    NoAttributes = 1ull << 4,
+    NoAttributes = 1ull << 3,
 
     /// Assignment is allowed in this context. This flag is cleared
     /// for nested subexpressions, unless they are directly parenthesized.
-    AssignmentAllowed = 1ull << 5,
+    AssignmentAllowed = 1ull << 4,
 
     /// Assignments are disallowed in this context. As opposed to the AssignmentAllowed
     /// flag, this is not cleared and overrides that fact even if we are in a
     /// procedural context and would otherwise be allowed to modify variables.
-    AssignmentDisallowed = 1ull << 6,
+    AssignmentDisallowed = 1ull << 5,
 
     /// Expression is not inside a procedural context.
-    NonProcedural = 1ull << 7,
+    NonProcedural = 1ull << 6,
 
     /// Expression is for a static variable's initializer. References to automatic
     /// variables will be disallowed.
-    StaticInitializer = 1ull << 8,
+    StaticInitializer = 1ull << 7,
 
     /// Streaming operator is allowed in assignment target, assignment source, bit-stream casting
     /// argument, or stream expressions of another streaming concatenation. This flag is cleared for
     /// nested subexpressions, unless they are directly parenthesized.
-    StreamingAllowed = 1ull << 9,
+    StreamingAllowed = 1ull << 8,
 
     /// This is the first expression appearing as an expression statement; potentially this
     /// indicates whether a subroutine invocation is as a task (if set) or as a function (unset).
     /// Cleared for nested subexpressions.
-    TopLevelStatement = 1ull << 10,
+    TopLevelStatement = 1ull << 9,
 
     /// Expression is allowed to be the unbounded literal '$' such as inside a queue select.
-    AllowUnboundedLiteral = 1ull << 11,
+    AllowUnboundedLiteral = 1ull << 10,
 
     /// Expression is allowed to do arithmetic with an unbounded literal.
-    AllowUnboundedLiteralArithmetic = 1ull << 12,
+    AllowUnboundedLiteralArithmetic = 1ull << 11,
 
     /// AST creation is happening within a function body
-    Function = 1ull << 13,
+    Function = 1ull << 12,
 
     /// AST creation is happening within a final block.
-    Final = 1ull << 14,
+    Final = 1ull << 13,
 
     /// AST creation is happening within the intra-assignment timing control of
     /// a non-blocking assignment expression.
-    NonBlockingTimingControl = 1ull << 15,
+    NonBlockingTimingControl = 1ull << 14,
 
     /// AST creation is happening within an event expression.
-    EventExpression = 1ull << 16,
+    EventExpression = 1ull << 15,
 
     /// AST creation is in a context where type reference expressions are allowed.
-    AllowTypeReferences = 1ull << 17,
+    AllowTypeReferences = 1ull << 16,
 
     /// AST creation is happening within an assertion expression (sequence or property).
-    AssertionExpr = 1ull << 18,
+    AssertionExpr = 1ull << 17,
 
     /// Allow binding a clocking block as part of a top-level event expression.
-    AllowClockingBlock = 1ull << 19,
+    AllowClockingBlock = 1ull << 18,
 
     /// AST creation is for checking an assertion argument, prior to it being expanded as
     /// part of an actual instance.
-    AssertionInstanceArgCheck = 1ull << 20,
+    AssertionInstanceArgCheck = 1ull << 19,
 
     /// AST creation is for a cycle delay or sequence repetition, where references to
     /// assertion formal ports have specific type requirements.
-    AssertionDelayOrRepetition = 1ull << 21,
+    AssertionDelayOrRepetition = 1ull << 20,
 
     /// AST creation is for the left hand side of an assignment operation.
-    LValue = 1ull << 22,
+    LValue = 1ull << 21,
 
     /// AST creation is for the negation of a property, which disallows recursive
     /// instantiations.
-    PropertyNegation = 1ull << 23,
+    PropertyNegation = 1ull << 22,
 
     /// AST creation is for a property that has come after a positive advancement
     /// of time within the parent property definition.
-    PropertyTimeAdvance = 1ull << 24,
+    PropertyTimeAdvance = 1ull << 23,
 
     /// AST creation is for an argument passed to a recursive property instance.
-    RecursivePropertyArg = 1ull << 25,
+    RecursivePropertyArg = 1ull << 24,
 
     /// AST creation is inside a concurrent assertion's action block.
-    ConcurrentAssertActionBlock = 1ull << 26,
+    ConcurrentAssertActionBlock = 1ull << 25,
 
     /// AST creation is for a covergroup expression that permits referencing a
     /// formal argument of an overridden sample method.
-    AllowCoverageSampleFormal = 1ull << 27,
+    AllowCoverageSampleFormal = 1ull << 26,
 
     /// Expressions are allowed to reference coverpoint objects directly.
-    AllowCoverpoint = 1ull << 28,
+    AllowCoverpoint = 1ull << 27,
 
     /// User-defined nettypes are allowed to be looked up in this context.
-    AllowNetType = 1ull << 29,
+    AllowNetType = 1ull << 28,
 
     /// AST creation is for an output (or inout) port or function argument.
-    OutputArg = 1ull << 30,
+    OutputArg = 1ull << 29,
 
     /// AST creation is for a procedural assign statement.
-    ProceduralAssign = 1ull << 31,
+    ProceduralAssign = 1ull << 30,
 
     /// AST creation is for a procedural force / release / deassign statement.
-    ProceduralForceRelease = 1ull << 32,
+    ProceduralForceRelease = 1ull << 31,
 
     /// AST creation is in a context that allows interconnect nets.
-    AllowInterconnect = 1ull << 33,
+    AllowInterconnect = 1ull << 32,
 
     /// AST creation is in a context where drivers should not be registered for
     /// lvalues, even if they otherwise would normally be. This is used, for example,
     /// in potentially unrollable for loops to let the loop unroller handle the drivers.
-    NotADriver = 1ull << 34,
+    NotADriver = 1ull << 33,
 
     /// AST creation is for a range expression inside a streaming concatenation operator.
-    StreamingWithRange = 1ull << 35,
+    StreamingWithRange = 1ull << 34,
 
     /// AST creation is happening inside a specify block.
-    SpecifyBlock = 1ull << 36,
+    SpecifyBlock = 1ull << 35,
 
     /// AST creation is for a DPI argument type.
-    DPIArg = 1ull << 37,
+    DPIArg = 1ull << 36,
 
     /// AST creation is for an assertion instance's default argument.
-    AssertionDefaultArg = 1ull << 38,
+    AssertionDefaultArg = 1ull << 37,
 
     /// AST creation is for an lvalue that also counts as an rvalue. Only valid
     /// when combined with the LValue flag -- used for things like the pre & post
     /// increment and decrement operators.
-    LAndRValue = 1ull << 39,
+    LAndRValue = 1ull << 38,
 
     /// AST binding should not count symbol references towards that symbol being "used".
     /// If this flag is not set, accessing a variable or net in an expression will count
     /// that symbol as being "used".
-    NoReference = 1ull << 40,
+    NoReference = 1ull << 39,
 
     /// AST binding is for a parameter inside a SystemVerilog configuration.
-    ConfigParam = 1ull << 41,
+    ConfigParam = 1ull << 40,
 
     /// AST binding is for the contents of the type() operator.
-    TypeOperator = 1ull << 42,
+    TypeOperator = 1ull << 41,
 
     /// AST binding is inside a fork-join_any or fork-join_none block.
-    ForkJoinAnyNone = 1ull << 43
+    ForkJoinAnyNone = 1ull << 42
 };
 SLANG_BITMASK(ASTFlags, ForkJoinAnyNone)
 

include/slang/ast/Expression.h

@@ -340,6 +340,10 @@ class SLANG_EXPORT Expression {
     /// Returns true if any subexpression of this expression is a hierarchical reference.
     bool hasHierarchicalReference() const;
 
+    /// If this expression is an implicit conversion, recursively unwraps to the
+    /// target operand. Otherwise returns `*this`.
+    const Expression& unwrapImplicitConversions() const;
+
     /// @brief Casts this expression to the given concrete derived type.
     ///
     /// Asserts that the type is appropriate given this expression's kind.

include/slang/ast/expressions/AssignmentExpressions.h

@@ -109,7 +109,7 @@ class SLANG_EXPORT ConversionExpression : public Expression {
         operand_(&operand) {}
 
     /// @returns true if this is an implicit conversion
-    bool isImplicit() const { return conversionKind < ConversionKind::Explicit; }
+    bool isImplicit() const { return conversionKind < ConversionKind::StreamingConcat; }
 
     /// @returns the operand of the conversion
     const Expression& operand() const { return *operand_; }

source/ast/Expression.cpp

@@ -717,6 +717,18 @@ bool Expression::hasHierarchicalReference() const {
     return visitor.any;
 }
 
+const Expression& Expression::unwrapImplicitConversions() const {
+    auto expr = this;
+    while (expr->kind == ExpressionKind::Conversion) {
+        auto& conv = expr->as<ConversionExpression>();
+        if (!conv.isImplicit())
+            break;
+
+        expr = &conv.operand();
+    }
+    return *expr;
+}
+
 Expression& Expression::create(Compilation& compilation, const ExpressionSyntax& syntax,
                                const ASTContext& ctx, bitmask<ASTFlags> extraFlags,
                                const Type* assignmentTarget) {

source/ast/expressions/AssignmentExpressions.cpp

@@ -36,26 +36,6 @@ using namespace slang;
 using namespace slang::ast;
 using namespace slang::syntax;
 
-bool checkEnumInitializer(const ASTContext& context, const Type& lt, const Expression& rhs) {
-    // [6.19] says that the initializer for an enum value must be an integer expression that
-    // does not truncate any bits. Furthermore, if a sized literal constant is used, it must
-    // be sized exactly to the size of the enum base type.
-    const Type& rt = *rhs.type;
-    if (!rt.isIntegral()) {
-        context.addDiag(diag::ValueMustBeIntegral, rhs.sourceRange);
-        return false;
-    }
-
-    if (lt.getBitWidth() != rt.getBitWidth() && rhs.kind == ExpressionKind::IntegerLiteral &&
-        !rhs.as<IntegerLiteral>().isDeclaredUnsized) {
-        auto& diag = context.addDiag(diag::EnumValueSizeMismatch, rhs.sourceRange);
-        diag << rt.getBitWidth() << lt.getBitWidth();
-        return false;
-    }
-
-    return true;
-}
-
 // This function exists to handle a case like:
 //      integer i;
 //      enum { A, B } foo;
@@ -133,27 +113,18 @@ void checkImplicitConversions(const ASTContext& context, const Type& sourceType,
         return diag;
     };
 
-    auto expr = &op;
-    while (expr->kind == ExpressionKind::Conversion) {
-        auto& conv = expr->as<ConversionExpression>();
-        if (!conv.isImplicit())
-            break;
-
-        expr = &conv.operand();
-    }
-
-    if (expr->kind == ExpressionKind::LValueReference)
-        return;
-
     // Don't warn about conversions in compound assignment operators.
-    if (expr->kind == ExpressionKind::BinaryOp) {
-        auto left = &expr->as<BinaryExpression>().left();
-        while (left->kind == ExpressionKind::Conversion)
-            left = &left->as<ConversionExpression>().operand();
+    auto isCompoundAssign = [&] {
+        auto& expr = op.unwrapImplicitConversions();
+        if (expr.kind == ExpressionKind::LValueReference)
+            return true;
 
-        if (left->kind == ExpressionKind::LValueReference)
-            return;
-    }
+        return expr.kind == ExpressionKind::BinaryOp &&
+               expr.as<BinaryExpression>().left().unwrapImplicitConversions().kind ==
+                   ExpressionKind::LValueReference;
+    };
+    if (isCompoundAssign())
+        return;
 
     const Type& lt = targetType.getCanonicalType();
     const Type& rt = sourceType.getCanonicalType();
@@ -175,10 +146,8 @@ void checkImplicitConversions(const ASTContext& context, const Type& sourceType,
         if (lt.isSigned() != rt.isSigned()) {
             // Comparisons get their own warning elsewhere.
             bool isComparison = false;
-            const BinaryExpression* binExpr = nullptr;
             if (parentExpr && parentExpr->kind == ExpressionKind::BinaryOp) {
-                binExpr = &parentExpr->as<BinaryExpression>();
-                switch (binExpr->op) {
+                switch (parentExpr->as<BinaryExpression>().op) {
                     case BinaryOperator::Equality:
                     case BinaryOperator::Inequality:
                     case BinaryOperator::CaseEquality:
@@ -430,8 +399,13 @@ Expression& Expression::convertAssignment(const ASTContext& context, const Type&
 
     Expression* result = &expr;
     const Type* rt = expr.type;
+
+    auto finalizeType = [&](const Type& t) {
+        contextDetermined(context, result, nullptr, t, assignmentRange, /* isAssignment */ true);
+    };
+
     if (type.isEquivalent(*rt)) {
-        contextDetermined(context, result, nullptr, *rt, assignmentRange, /* isAssignment */ true);
+        finalizeType(*rt);
 
         if (type.isVoid())
             context.addDiag(diag::VoidAssignment, expr.sourceRange);
@@ -532,28 +506,28 @@ Expression& Expression::convertAssignment(const ASTContext& context, const Type&
     }
 
     if (type.isNumeric() && rt->isNumeric()) {
-        if (context.flags.has(ASTFlags::EnumInitializer) &&
-            !checkEnumInitializer(context, type, *result)) {
-            return badExpr(comp, &expr);
-        }
-
         // The "signednessFromRt" flag is important here; only the width of the lhs is
         // propagated down to operands, not the sign flag. Once the expression is appropriately
         // sized, the makeImplicit call down below will convert the sign for us.
         rt = binaryOperatorType(comp, &type, rt, false, /* signednessFromRt */ true);
-        bool expanding = type.isEquivalent(*rt);
-        if (expanding)
-            rt = &type;
 
-        contextDetermined(context, result, nullptr, *rt, assignmentRange, /* isAssignment */ true);
-        if (expanding)
-            return *result;
-
-        // Do not convert (truncate) enum initializer so out of range value can be checked.
-        if (context.flags.has(ASTFlags::EnumInitializer)) {
-            selfDetermined(context, result);
+        // If the final type is the same type (or equivalent) of the lhs, we know we're
+        // performing an expansion of the rhs. The propagation performed by contextDetermined
+        // will ensure that the expression has the correct resulting type, so we don't need an
+        // additional implicit conversion. What's not obvious here is that simple assignments like:
+        //      logic [3:0] a;
+        //      logic [8:0] b;
+        //      initial b = a;
+        // will still result in an appropriate conversion warning because the type propagation
+        // visitor will see that we're in an assignment and insert an implicit conversion for us.
+        if (type.isEquivalent(*rt)) {
+            finalizeType(type);
             return *result;
         }
+
+        // Otherwise use the common type and fall through to get an implicit conversion
+        // created for us.
+        finalizeType(*rt);
     }
 
     return ConversionExpression::makeImplicit(context, type, ConversionKind::Implicit, *result,
@@ -954,6 +928,9 @@ ConstantValue ConversionExpression::convert(EvalContext& context, const Type& fr
                                        conversionKind == ConversionKind::StreamingConcat);
     }
 
+    const bool checkImplicit = conversionKind == ConversionKind::Implicit &&
+                               !context.astCtx.flags.has(ASTFlags::UnevaluatedBranch);
+
     if (to.isIntegral()) {
         // [11.8.2] last bullet says: the operand shall be sign-extended only if the propagated type
         // is signed. It is different from [11.8.3] ConstantValue::convertToInt uses.
@@ -962,7 +939,7 @@ ConstantValue ConversionExpression::convert(EvalContext& context, const Type& fr
             value.integer().setSigned(to.isSigned());
 
         auto result = value.convertToInt(to.getBitWidth(), to.isSigned(), to.isFourState());
-        if (conversionKind == ConversionKind::Implicit) {
+        if (checkImplicit) {
             if (value.isInteger()) {
                 auto& oldInt = value.integer();
                 auto& newInt = result.integer();
@@ -995,7 +972,7 @@ ConstantValue ConversionExpression::convert(EvalContext& context, const Type& fr
 
     if (to.isFloating()) {
         auto result = to.getBitWidth() == 32 ? value.convertToShortReal() : value.convertToReal();
-        if (conversionKind == ConversionKind::Implicit && value.isInteger()) {
+        if (checkImplicit && value.isInteger()) {
             const bool differs = result.isReal()
                                      ? (int64_t)result.real() != value.integer().as<int64_t>()
                                      : (int32_t)result.shortReal() != value.integer().as<int32_t>();