zachjs / sv2v

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

Hierarchical naming for interfaces #185

Closed tinebp closed 2 years ago

tinebp commented 3 years ago

First of all great work with this tool, we have been using it extensively in our build process.

There is a functionality recommendation that will add more impact to this work.

At the moment, Xilinx Vivado doesn't support hierarchical naming in their Verilog HDL compiler.

The following clever conversion of interface that sv2v uses doesn't work on Vivado:

   generate
    if (1) begin : req_if
        wire valid;
        wire ready;
    end
endgenerate

My recommendation is to instead simply use a flat naming convention like the following:

wire req_if_valid; wire req_if_ready;

When doing the conversion from systemverilog to verilog, my recommendation is to target the most basic functionality in Verilog to guarantee maximum compatibility!

Again, great work!

tinebp commented 3 years ago

I found a simple fix to this issue.

The bug that Vivado generates is actually the following: "[Synth 8-6038] cannot resolve hierarchical name for the item" After investigating it further it looks like the issue is in how the interface variables are named:

sv2v generates names like: ..

Vivado recommend removing from the variable name.

instead, it should be: .

This should be a simple bug fix in sv2v

zachjs commented 3 years ago

If sv2v is generating hierarchical identifiers with two dots, this is indeed a bug. However, without an example for reproducing this scenario, it is difficult for me to know what is going wrong. Can you share a (perhaps simplified) reproduction example so I can debug the issue?

tinebp commented 3 years ago

sv2v_test.zip Attached is our project makefile and source for sv2v that we created so that we could use yosys synthesis. To reproduce all bugs, unpack the zip file and run: $make build_no_fpu $make build

zachjs commented 3 years ago

The only error I'm running into using the provided test case is a failure on an illegal struct access. I don't see the ".." issue you referred to above.

The struct access issue originates in id_queue. That module (apparently intentionally) has invalid default parameters. Relevant here is the type parameter data_t defaulting to logic, which is illegal because the module goes on to access individual bits of a data_t embedded within a struct. Of course, accessing a sub-range of a logic is not possible. I will look to improve sv2v's error messaging here.

It is not inherently problematic to have a module with invalid default type parameters. However, Verilog requires that modules which are not instantiated by other modules are considered top-level modules. Hence, sv2v instantiates id_queue with its (invalid) default parameters because it is not used by any other module, and so appears to be a top-level module.

zachjs commented 3 years ago

I just added the error checking I described above. With the latest, running make build on the provided test case now yields a proper error: illegal access to bit j of linked_data_q[i].data, which has type logic.

zachjs commented 2 years ago

@tinebp Do you have any further issues here? I'm eager to help if so!

zachjs commented 2 years ago

Given the prolonged inactivity, I am closing this issue. Please update here if you are still facing an issue.