diff --git a/bindings/python/ASTBindings.cpp b/bindings/python/ASTBindings.cpp index f8d9c5092..7e30d0042 100644 --- a/bindings/python/ASTBindings.cpp +++ b/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) diff --git a/include/slang/ast/ASTContext.h b/include/slang/ast/ASTContext.h index 0dc4938ae..2337f95b3 100644 --- a/include/slang/ast/ASTContext.h +++ b/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) diff --git a/include/slang/ast/Expression.h b/include/slang/ast/Expression.h index 44cc602dd..65d95b78a 100644 --- a/include/slang/ast/Expression.h +++ b/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. diff --git a/include/slang/ast/expressions/AssignmentExpressions.h b/include/slang/ast/expressions/AssignmentExpressions.h index 695e86c21..55d45b972 100644 --- a/include/slang/ast/expressions/AssignmentExpressions.h +++ b/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_; } diff --git a/source/ast/Expression.cpp b/source/ast/Expression.cpp index eb2435b2c..2272fed21 100644 --- a/source/ast/Expression.cpp +++ b/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(); + if (!conv.isImplicit()) + break; + + expr = &conv.operand(); + } + return *expr; +} + Expression& Expression::create(Compilation& compilation, const ExpressionSyntax& syntax, const ASTContext& ctx, bitmask extraFlags, const Type* assignmentTarget) { diff --git a/source/ast/expressions/AssignmentExpressions.cpp b/source/ast/expressions/AssignmentExpressions.cpp index ee313c2ee..f4a98e5f1 100644 --- a/source/ast/expressions/AssignmentExpressions.cpp +++ b/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().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(); - 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().left(); - while (left->kind == ExpressionKind::Conversion) - left = &left->as().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().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(); - switch (binExpr->op) { + switch (parentExpr->as().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() : (int32_t)result.shortReal() != value.integer().as(); diff --git a/source/ast/types/AllTypes.cpp b/source/ast/types/AllTypes.cpp index 4a976e27c..15443d09e 100644 --- a/source/ast/types/AllTypes.cpp +++ b/source/ast/types/AllTypes.cpp @@ -11,13 +11,16 @@ #include "slang/ast/ASTSerializer.h" #include "slang/ast/Compilation.h" #include "slang/ast/Expression.h" +#include "slang/ast/expressions/LiteralExpressions.h" #include "slang/ast/symbols/ClassSymbols.h" #include "slang/ast/symbols/InstanceSymbols.h" #include "slang/ast/symbols/MemberSymbols.h" #include "slang/ast/symbols/VariableSymbols.h" #include "slang/diagnostics/ConstEvalDiags.h" #include "slang/diagnostics/DeclarationsDiags.h" +#include "slang/diagnostics/ExpressionsDiags.h" #include "slang/diagnostics/LookupDiags.h" +#include "slang/diagnostics/NumericDiags.h" #include "slang/diagnostics/TypesDiags.h" #include "slang/numeric/MathUtils.h" #include "slang/syntax/AllSyntax.h" @@ -335,24 +338,43 @@ const Type& EnumType::fromSyntax(Compilation& compilation, const EnumTypeSyntax& // For enumerands that have an initializer, set it up appropriately. auto setInitializer = [&](EnumValueSymbol& ev, const EqualsValueClauseSyntax& initializer) { - ev.setInitializerSyntax(*initializer.expr, initializer.equals.location()); - + // Clear our previous state in case there's an error and we need to exit early. first = false; - previous = ev.getValue(); - previousRange = ev.getInitializer()->sourceRange; + previous = nullptr; - if (!previous) + // Create the initializer expression. + ev.setInitializerSyntax(*initializer.expr, initializer.equals.location()); + auto initExpr = ev.getInitializer(); + SLANG_ASSERT(initExpr); + + // Drill down to the original initializer before any conversions are applied. + initExpr = &initExpr->unwrapImplicitConversions(); + previousRange = initExpr->sourceRange; + + // [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. + auto& rt = *initExpr->type; + if (!rt.isIntegral()) { + context.addDiag(diag::ValueMustBeIntegral, previousRange); return; + } + + if (bitWidth != rt.getBitWidth() && initExpr->kind == ExpressionKind::IntegerLiteral && + !initExpr->as().isDeclaredUnsized) { + auto& diag = context.addDiag(diag::EnumValueSizeMismatch, previousRange); + diag << rt.getBitWidth() << bitWidth; + } - auto loc = previousRange.start(); - auto& value = previous.integer(); // checkEnumInitializer ensures previous is integral + ev.getValue(); + if (!initExpr->constant) + return; // An enumerated name with x or z assignments assigned to an enum with no explicit data type // or an explicit 2-state declaration shall be a syntax error. + auto& value = initExpr->constant->integer(); if (!base->isFourState() && value.hasUnknown()) { - context.addDiag(diag::EnumValueUnknownBits, loc) << value << *base; - ev.setValue(nullptr); - previous = nullptr; + context.addDiag(diag::EnumValueUnknownBits, previousRange) << value << *base; return; } @@ -372,26 +394,13 @@ const Type& EnumType::fromSyntax(Compilation& compilation, const EnumTypeSyntax& } if (!good) { - context.addDiag(diag::EnumValueOutOfRange, loc) << value << *base; - ev.setValue(nullptr); - previous = nullptr; + context.addDiag(diag::EnumValueOutOfRange, previousRange) << value << *base; return; } } - // Implicit casting to base type to ensure value matches the underlying type. - if (value.getBitWidth() != bitWidth) { - auto cv = previous.convertToInt(bitWidth, base->isSigned(), base->isFourState()); - ev.setValue(cv); - previous = std::move(cv); - } - else { - if (!base->isFourState()) - value.flattenUnknowns(); - value.setSigned(base->isSigned()); - } - - checkValue(previous, loc); + previous = ev.getValue(); + checkValue(previous, previousRange.start()); }; // For enumerands that have no initializer, infer the value via this function. @@ -573,7 +582,10 @@ const ConstantValue& EnumValueSymbol::getValue(SourceRange referencingRange) con auto scope = getParentScope(); SLANG_ASSERT(scope); - ASTContext ctx(*scope, LookupLocation::max); + // We set UnevaluatedBranch here so that we don't get any implicit + // conversion warnings, since we check those manually in the enum + // value creation path. + ASTContext ctx(*scope, LookupLocation::max, ASTFlags::UnevaluatedBranch); if (evaluating) { SLANG_ASSERT(referencingRange.start()); diff --git a/source/ast/types/DeclaredType.cpp b/source/ast/types/DeclaredType.cpp index 4a75da7de..323817880 100644 --- a/source/ast/types/DeclaredType.cpp +++ b/source/ast/types/DeclaredType.cpp @@ -419,7 +419,7 @@ void DeclaredType::resolveAt(const ASTContext& context) const { const Type* targetType = type; if (targetType->isEnum() && scope.asSymbol().kind == SymbolKind::EnumType) { targetType = &targetType->as().baseType; - extraFlags = ASTFlags::EnumInitializer; + extraFlags = ASTFlags::UnevaluatedBranch; } else if (flags.has(DeclaredTypeFlags::AllowUnboundedLiteral)) { extraFlags = ASTFlags::AllowUnboundedLiteral;