MikePopoloski / slang

SystemVerilog compiler and language services
MIT License
558 stars 122 forks source link

Finish tagged keyword support. #1012

Closed mingzheTerapines closed 3 weeks ago

mingzheTerapines commented 4 weeks ago

Is your feature request related to a problem? Please describe. In slang version 6.0.89+895805e5 Slang removed the keyword "tagged" after translting without a type checking. sv code:

module top ();
int a;
typedef union tagged {
  int   INT_VAL;
  real REAL_VAL;
} int_or_real_t;
int_or_real_t t1;
  initial begin
    t1.REAL_VAL = a;
  end
endmodule

json code:

{
  "design": {
    "name": "$root",
    "kind": "Root",
    "addr": 2535475002528,
    "members": [
      {
        "name": "",
        "kind": "CompilationUnit",
        "addr": 2535474386144
      },
      {
        "name": "top",
        "kind": "Instance",
        "addr": 2535474387648,
        "body": {
          "name": "top",
          "kind": "InstanceBody",
          "addr": 2535474386440,
          "members": [
            {
              "name": "a",
              "kind": "Variable",
              "addr": 2535474386696,
              "type": "int",
              "lifetime": "Static"
            },
            {
              "name": "int_or_real_t",
              "kind": "TypeAlias",
              "addr": 2535474387040,
              "target": "union{int INT_VAL;real REAL_VAL;}top.u$1"
            },
            {
              "name": "t1",
              "kind": "Variable",
              "addr": 2535474387192,
              "type": "union{int INT_VAL;real REAL_VAL;}top.int_or_real_t",
              "lifetime": "Static"
            },
            {
              "name": "",
              "kind": "ProceduralBlock",
              "addr": 2535474387536,
              "procedureKind": "Initial",
              "body": {
                "kind": "Block",
                "blockKind": "Sequential",
                "body": {
                  "kind": "ExpressionStatement",
                  "expr": {
                    "kind": "Assignment",
                    "type": "real",
                    "left": {
                      "kind": "MemberAccess",
                      "type": "real",
                      "member": "2535474388304 REAL_VAL",
                      "value": {
                        "kind": "NamedValue",
                        "type": "union{int INT_VAL;real REAL_VAL;}top.int_or_real_t",
                        "symbol": "2535474387192 t1"
                      }
                    },
                    "right": {
                      "kind": "Conversion",
                      "type": "real",
                      "operand": {
                        "kind": "NamedValue",
                        "type": "int",
                        "symbol": "2535474386696 a"
                      }
                    },
                    "isNonBlocking": false
                  }
                }
              }
            }
          ],
          "definition": "2535474662400 top"
        },
        "connections": [
        ]
      }
    ]
  },
  "definitions": [
    {
      "name": "top",
      "kind": "Definition",
      "addr": 2535474662400,
      "defaultNetType": "2535475000288 wire",
      "definitionKind": "Module",
      "defaultLifetime": "Static",
      "unconnectedDrive": "None"
    }
  ]
}

Describe the solution you'd like It will be great if clang can do easy type checking while translating like packed-checking does, also keep tagged keywords in "type" value for complex type checking in IR.

Describe alternatives you've considered Sorry but , no alternatives considered.

Additional context No additional context.

MikePopoloski commented 4 weeks ago

I'm having a hard time understanding what you're asking here. The code example you posted seems fine to me and slang does support it. What additional type checking are you expecting to see?

cepheus69 commented 3 weeks ago

I think Slang has done type checking of tagged union during the procedure of generating AST.

mingzheTerapines commented 3 weeks ago
module top ();
int a;
typedef union tagged {
  int   INT_VAL;
  real REAL_VAL;
} int_or_real_t;
int_or_real_t t1;
  initial begin
    t1.REAL_VAL = a;
  end
endmodule

a is an integer but t1.REAL_VAL is a real. They are different types, but still allow assigning which is unexpected. Refer to IEEE Standard

The tag and value can only be updated together
consistently using a statically type-checked tagged union expression (see 11.9). The member value can only
be read with a type that is consistent with the current tag value (i.e., member name). Thus, it is impossible to
store a value of one type and (mis)interpret the bits as another type.

In sourcecode

static bool actuallyNeededCast(const Type& type, const Expression& operand) {
    // Check whether a cast was needed for the given operand to
    // reach the final type. This is true when the operand requires
    // an assignment-like context to determine its result.
    switch (operand.kind) {
        case ExpressionKind::NewArray:
        case ExpressionKind::NewClass:
        case ExpressionKind::NewCovergroup:
        case ExpressionKind::SimpleAssignmentPattern:
        case ExpressionKind::StructuredAssignmentPattern:
        case ExpressionKind::ReplicatedAssignmentPattern:
        case ExpressionKind::TaggedUnion:
            return true;
        case ExpressionKind::Concatenation:
            return operand.type->isUnpackedArray();
        case ExpressionKind::MinTypMax:
            return actuallyNeededCast(type, operand.as<MinTypMaxExpression>().selected());
        case ExpressionKind::ConditionalOp: {
            auto& cond = operand.as<ConditionalExpression>();
            return actuallyNeededCast(type, cond.left()) || actuallyNeededCast(type, cond.right());
        }
        default:
            return false;
    }
}

Personally I will return false if it is an tagged union.

mingzheTerapines commented 3 weeks ago
module top ();
int a;
typedef union tagged {
  int   INT_VAL;
  real REAL_VAL;
} int_or_real_t;
int_or_real_t t1;
  initial begin
    t1.REAL_VAL = a;
  end
endmodule

a is an integer but t1.REAL_VAL is a real. They are different types, but still allow assigning which is unexpected. Refer to IEEE Standard

The tag and value can only be updated together
consistently using a statically type-checked tagged union expression (see 11.9). The member value can only
be read with a type that is consistent with the current tag value (i.e., member name). Thus, it is impossible to
store a value of one type and (mis)interpret the bits as another type.

In sourcecode

static bool actuallyNeededCast(const Type& type, const Expression& operand) {
    // Check whether a cast was needed for the given operand to
    // reach the final type. This is true when the operand requires
    // an assignment-like context to determine its result.
    switch (operand.kind) {
        case ExpressionKind::NewArray:
        case ExpressionKind::NewClass:
        case ExpressionKind::NewCovergroup:
        case ExpressionKind::SimpleAssignmentPattern:
        case ExpressionKind::StructuredAssignmentPattern:
        case ExpressionKind::ReplicatedAssignmentPattern:
        case ExpressionKind::TaggedUnion:
            return true;
        case ExpressionKind::Concatenation:
            return operand.type->isUnpackedArray();
        case ExpressionKind::MinTypMax:
            return actuallyNeededCast(type, operand.as<MinTypMaxExpression>().selected());
        case ExpressionKind::ConditionalOp: {
            auto& cond = operand.as<ConditionalExpression>();
            return actuallyNeededCast(type, cond.left()) || actuallyNeededCast(type, cond.right());
        }
        default:
            return false;
    }
}

Personally I will return false if it is an tagged union.

mingzheTerapines commented 3 weeks ago
module top ();
int a;
typedef union tagged {
  int   INT_VAL;
  real REAL_VAL;
} int_or_real_t;
int_or_real_t t1;
  initial begin
    t1.REAL_VAL = a;
  end
endmodule

a is an integer but t1.REAL_VAL is a real. They are different types, but still allow assigning which is unexpected. Refer to IEEE Standard

The tag and value can only be updated together
consistently using a statically type-checked tagged union expression (see 11.9). The member value can only
be read with a type that is consistent with the current tag value (i.e., member name). Thus, it is impossible to
store a value of one type and (mis)interpret the bits as another type.

In sourcecode

static bool actuallyNeededCast(const Type& type, const Expression& operand) {
    // Check whether a cast was needed for the given operand to
    // reach the final type. This is true when the operand requires
    // an assignment-like context to determine its result.
    switch (operand.kind) {
        case ExpressionKind::NewArray:
        case ExpressionKind::NewClass:
        case ExpressionKind::NewCovergroup:
        case ExpressionKind::SimpleAssignmentPattern:
        case ExpressionKind::StructuredAssignmentPattern:
        case ExpressionKind::ReplicatedAssignmentPattern:
        case ExpressionKind::TaggedUnion:
            return true;
        case ExpressionKind::Concatenation:
            return operand.type->isUnpackedArray();
        case ExpressionKind::MinTypMax:
            return actuallyNeededCast(type, operand.as<MinTypMaxExpression>().selected());
        case ExpressionKind::ConditionalOp: {
            auto& cond = operand.as<ConditionalExpression>();
            return actuallyNeededCast(type, cond.left()) || actuallyNeededCast(type, cond.right());
        }
        default:
            return false;
    }
}

Personally I will return false if it is an tagged union.

mingzheTerapines commented 3 weeks ago
module top ();
int a;
typedef union tagged {
  int   INT_VAL;
  real REAL_VAL;
} int_or_real_t;
int_or_real_t t1;
  initial begin
    t1.REAL_VAL = a;
  end
endmodule

a is an integer but t1.REAL_VAL is a real. They are different types, but still allow assigning which is unexpected. Refer to IEEE Standard

The tag and value can only be updated together
consistently using a statically type-checked tagged union expression (see 11.9). The member value can only
be read with a type that is consistent with the current tag value (i.e., member name). Thus, it is impossible to
store a value of one type and (mis)interpret the bits as another type.
MikePopoloski commented 3 weeks ago

It's always allowed to assign an integer to a real because there are implicit conversion rules that will apply. You can see that conversion added in the AST. The quote from the LRM refers to assigning to the union as a whole, not its members. Assigning to a tagged union variable requires the use of a tagged expression, but assigning to its members does not.

The source snippet from slang that you posted is unrelated to anything in this thread; it's a heuristic for deciding whether to issue the Wuseless-cast warning.

mingzheTerapines commented 3 weeks ago

It's always allowed to assign an integer to a real because there are implicit conversion rules that will apply. You can see that conversion added in the AST. The quote from the LRM refers to assigning to the union as a whole, not its members. Assigning to a tagged union variable requires the use of a tagged expression, but assigning to its members does not.

The source snippet from slang that you posted is unrelated to anything in this thread; it's a heuristic for deciding whether to issue the Wuseless-cast warning.

Thanks for expalining and sorry for misunderstanding.