MikePopoloski / slang

SystemVerilog compiler and language services
MIT License
555 stars 121 forks source link

Constant evaluation during AST serialization? #299

Closed Kuree closed 4 years ago

Kuree commented 4 years ago

Although the changes made by https://github.com/MikePopoloski/slang/commit/f97eec20d59f1a5afa27c42f1ac91a1c98162856 makes the code more efficient, it also removes the constant value output during the AST serialization. Based on the git diff, "constant" filed has been removed from the serialization.

For a third-party tool that digests the AST JSON, the "constant" field saves a lot of efforts of looking up symbols and parsing types, for instance, enums. Do you think it is worthwhile to evaluate the constant values during AST serialization?

MikePopoloski commented 4 years ago

Yeah, that sounds fine to me. I have more generic plans for doing the constant evaluation when it comes to generating sim code, but I don't think it hurts to have the ASTSerializer always try to eval.

Kuree commented 4 years ago

I haven't had any luck implementing that feature without overhauling the serialization. Do you have any recommendation on how to do this?

MikePopoloski commented 4 years ago

ASTSerializer::visit has a case for expressions. Can you just call eval() there and output the constant if it returns a valid value?

MikePopoloski commented 4 years ago

Made this change myself; let me know if it doesn't work properly for you.

Kuree commented 4 years ago

Sorry I got caught by other stuff.

I was not sure about whether to add compilation to the AST serializer. It seems that it should be the write way to do based on your commit.

I tested it out on my test cases and it doesn't work for named values with enum. Here is an example to reproduce:

module test_enum;

typedef enum logic {
    STATE_0 = 0,
    STATE_1 = 1
} STATE;

STATE a = STATE_0;

endmodule

Here is what's generated from AST serialization for the assignment:

        {
          "name": "a",
          "kind": "Variable",
          "addr": 94164640199696,
          "type": {
            "name": "STATE",
            "kind": "TypeAlias",
            "addr": 94164640075960,
            "target": "enum{STATE_0=1'd0,STATE_1=1'd1}test_enum.e$2"
          },
          "initializer": {
            "kind": "NamedValue",
            "type": "enum{STATE_0=1'd0,STATE_1=1'd1}test_enum.e$2",
            "symbol": "94164640076384 STATE_0",
            "isHierarchical": false
          },
          "lifetime": "Static",
          "isConstant": false,
          "isCompilerGenerated": false
        }
MikePopoloski commented 4 years ago

Hmm, I'll take a look.

MikePopoloski commented 4 years ago

When I run this locally I get the "constant" field as part of a's initializer:

            "name": "a",
            "kind": "Variable",
            "type": {
              "name": "STATE",
              "kind": "TypeAlias",
              "target": "enum{STATE_0=1'd0,STATE_1=1'd1}test_enum.e$1"
            },
            "initializer": {
              "kind": "NamedValue",
              "type": "enum{STATE_0=1'd0,STATE_1=1'd1}test_enum.e$1",
              "symbol": "STATE_0",
              "isHierarchical": false,
              "constant": "1'b0"
            },
            "lifetime": "Static",
            "isConstant": false,
            "isCompilerGenerated": false
Kuree commented 4 years ago

I did a clean clone and build, and got your result. It could be some branching issue on my end. Sorry about that.