zachjs / sv2v

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

[feature request] support for field name preservation when struct conversion #253

Open Dragon-Git opened 1 year ago

Dragon-Git commented 1 year ago

Sv2v version 0.11.0 is represented by vectors when converting the structure, and the name of the field cannot be seen in the converted Verilog code, resulting in poor readability, can the name information of the structure be retained in the converted Verilog code? example:

typedef struct {
  logic [31:0] a;
  logic [31:0] b;
  logic op;
} data_struct_t;

module src (
  output data_struct_t my_data 
  );
  assign my_data.a = 32'h12345678;
  assign my_data.b = 32'h87654321;
  assign my_data.op = 1'b1;
endmodule

module dest (
  input data_struct_t my_data,
  output logic result
  );
  assign result = my_data.op? my_data.a - my_data.b : my_data.b - my_data.a;
endmodule

module top;
  data_struct_t my_data;

  src src_inst (
    .my_data(my_data)
  );

  dest dest_inst (
    .my_data(my_data)
  );

endmodule

Now convert to:

module src (my_data);
        output wire [64:0] my_data;
        assign my_data[64-:32] = 32'h12345678;
        assign my_data[32-:32] = 32'h87654321;
        assign my_data[0] = 1'b1;
endmodule
module dest (
        my_data,
        result
);
        input wire [64:0] my_data;
        output wire result;
        assign result = (my_data[0] ? my_data[64-:32] - my_data[32-:32] : my_data[32-:32] - my_data[64-:32]);
endmodule
module top;
        wire [64:0] my_data;
        src src_inst(.my_data(my_data));
        dest dest_inst(.my_data(my_data));
endmodule

Expected:

module src (my_data);
        output wire [64:0] my_data;
        assign my_data_a = 32'h12345678;
        assign my_data_b = 32'h87654321;
        assign my_data_op = 1'b1;
endmodule
module dest (
        my_data,
        result
);
        input wire [64:0] my_data;
        output wire result;
        assign result = (my_data_op ? my_data_a - my_data_b : my_data_b - my_data_a);
endmodule
module top;
        wire [64:0] my_data;
        src src_inst(.my_data(my_data));
        dest dest_inst(.my_data(my_data));
endmodule
zachjs commented 1 year ago

Thanks for filing this request! I agree that sv2v's struct conversion doesn't produce highly readable output. Many of sv2v's conversions exhibit this limitation to some extent. sv2v was envisioned as part of synthesis flows, producing generated output that didn't necessarily need to be readable. Can you help me understand your use case here? What do you intend to do with the converted output? Is there a pain point in your existing workflow, e.g., #194?

Dragon-Git commented 1 year ago

For debugging purposes, it is more convenient to have the field names preserved, especially when dealing with very large structs.

I use peakrdl-regblock to convert systemrdl to register systemverilog files, currently it can be synthesized using Design Compiler, but the readability of the synthesized netlist is lower than that of the netlist synthesized based on verilog files, which affects debugging efficiency during gate-level simulation, so I wanted to use sv2v as a pre-synthesis tool to enhance the readability of the netlist.

sv2v was envisioned as part of synthesis flows, producing generated output that didn't necessarily need to be readable.

While I agree with the fact that sv2v was primarily designed for synthesis flows and readability might not be a priority, supporting the preservation of field names would greatly enhance the functionality of sv2v in this context.

KatCe commented 1 year ago

Hello. We could also need this feature for debugging purposes. We use sv2v in order to get a Verilog version of a design. Then we feed it into our Yosys-based flow, where we perform further analysis passes. Manually debugging the transformed design (in a wave form viewer) is quite time consuming when the names are not preserved. Would appreciate it. Thank you!

sifferman commented 1 year ago

I wanted to recommend that if this feature is added, I think it should be added only as an option. Arrays of structs are currently synthesized as a BRAM. But if an array of structs is separated into multiple different arrays, then extra BRAMs may be inferred.

Example:

struct {
  logic a,
  logic b
} s[8];
reg [1:0] s [0:7];
// or
reg s_a [0:7];
reg s_b [0:7];
KatCe commented 1 year ago

Implementing it as an option sounds good. I also have another use case now, where I would need this feature for generating verification code.

gggmmm commented 11 months ago

I wanted to recommend that if this feature is added, I think it should be added only as an option. Arrays of structs are currently synthesized as a BRAM. But if an array of structs is separated into multiple different arrays, then extra BRAMs may be inferred.

Example:

struct {
  logic a,
  logic b
} s[8];
reg [1:0] s [0:7];
// or
reg s_a [0:7];
reg s_b [0:7];

Then perhaps a pragma before the code you want not to be touched would be best. That way you get other struct nicely maintained name-wise, and the one that you want as BRAM are synthesized as such. The suggestion of a general flag to disable it completely would still be valid with the pragma.