zachjs / sv2v

SystemVerilog to Verilog conversion
BSD 3-Clause "New" or "Revised" License
540 stars 52 forks source link

Unable to parse sturct constant #129

Closed t123yh closed 3 years ago

t123yh commented 3 years ago

For example, the code below is synthesizable by Vivado:

typedef struct packed {
    bit [4:0] regRead1;
    bit [4:0] regRead2;
    bit enable;
} ControlSignals;

const ControlSignals kControlNop = '{
    regRead1: 0,
    regRead2: 0,
    enable: 0
};

But sv2v reports an error while trying to parse this.

test.sv:7:7: Parse error: unexpected token 'ControlSignals' (Id_simple).

I wonder if this is expected? Thanks.

zachjs commented 3 years ago

sv2v does not currently support the const keyword, but I am interested in addressing this.

A const isn't really a constant in the traditional sense (a localparam is), but rather a variable restricted to either static or automatic initialization semantics. The use case for const given in the LRM is that it allows for the use of hierarchical identifiers, which are not allowed in constant expressions.

In this example, kControlNop cannot itself be used as a constant_primary. I believe it would be more appropriate to use localparam rather than const for kControlNop. Am I missing something here? However, sv2v cannot necessarily swap any const for localparam as the initialization could contain hierarchical references which sv2v cannot resolve. It seems like the appropriate conversion would be to just drop the const altogether. What do you think? I would not likely enforce the semantics that a const variable doesn't appear on the LHS of an assignment.

t123yh commented 3 years ago

Yes, I agree that localparam is a more appropriate option. I wonder if it makes sense to add some kind of warning with const?

zachjs commented 3 years ago

Do other tools generate warnings for this usage? If not, I'm inclined to simply add basic support for const by dropping the modifier and not generating a warning.

zachjs commented 3 years ago

I realized sv2v actually does what I described for const declarations, but just didn't handle const declarations using typenames like ControlSignals. I've resolved this issue. I'm curious if this works correctly for you with the localparam reverted back to const in your source.

zachjs commented 3 years ago

Thanks for reporting this issue! I believe it has been appropriately resolved. If you run into any other problems, please feel free to file another issue!