zachjs / sv2v

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

using interface arrays generates broken code #251

Closed tinebp closed 10 months ago

tinebp commented 11 months ago

sv2v tool doesn't appear to support the translation of interface arrays. the generated code fails to compile due to syntax errors. example:

VX_gbar_bus_if per_socket_gbar_bus_if[4](); VX_gbar_bus_if gbar_bus_if();

VX_gbar_arb #( .NUM_REQS (`4) ) gbar_arb ( .clk (clk), .reset (gbar_reset), .bus_in_if (per_socket_gbar_bus_if), .bus_out_if (gbar_bus_if) );

module VX_gbar_arb #( parameter NUM_REQS = 1 ) ( input wire clk, input wire reset, VX_gbar_bus_if.slave bus_in_if[NUM_REQS], VX_gbar_bus_if.master bus_out_if ); // code endmodule

zachjs commented 11 months ago

I'm having trouble replicating your issue. sv2v does support interface arrays.

  1. What version of sv2v are you using? sv2v --version
  2. What is the exact command line you are using? Are you passing the interface definition and the modules that use that interface into a single invocation of sv2v?
  3. Can you share exactly the SV you're passing into sv2v?
tinebp commented 11 months ago

Thank you so much for the quick response. We are trying to use Yosys for our open-source GPU Vortex OpenGPU and sv2v is a critical component of our toolchain to convert system-verilog to verilog. Attached is the complete source directory we are struggling to compile. The project's top module is Vortex and sv2v generated output file is project.v sv2v generates a wrong code in the line project.v:4878

The version of sv2v we using is 

$  sv2v --version $ sv2v v0.0.11-5-g896b375  

|

Vortex OpenGPU

Vortex OpenGPU has 7 repositories available. Follow their code on GitHub. |

|

|

On Sunday, July 30, 2023 at 01:34:15 PM EDT, Zachary Snow ***@***.***> wrote:  

I'm having trouble replicating your issue. sv2v does support interface arrays.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

zachjs commented 11 months ago

Attached is the complete source directory we are struggling to compile.

I don't see any source attached to your post. Could you try posting it using the web interface rather than via email?

tinebp commented 11 months ago

build_Vortex.zip Here is the source code, sorry for the confusion.

zachjs commented 11 months ago

I don't think sv2v is doing anything incorrect here. I'm guessing you are using Yosys. Please try passing the -defer option to read_verilog in your flow. Per the Yosys docs:

    -defer
        only read the abstract syntax tree and defer actual compilation
        to a later 'hierarchy' command. Useful in cases where the default
        parameters of modules yield invalid or not synthesizable code.
tinebp commented 11 months ago

Below is the problematic generated code that Yosys has found and I also believe is incorrect. Let me try to explain, line 4878 in project.v below has an index i that is coming from genvar i for the loop earlier. but that index is for the loop and sv2v is using it on req_arb arguments. this is outside the scope of the loop. I think you need to move req_arb inside the for loop if you want to index it with i. Let me know if I'm getting something wrong here.

.data_out({Vortex.genblk1[i].cluster.gbar_bus_if.req_id, Vortex.genblk1[i].cluster.gbar_bus_if.req_size_m1, Vortex.genblk1[i].cluster.gbar_bus_if.req_core_id}),

zachjs commented 11 months ago

There are two issues:

  1. sv2v doesn't handle shadowed genvars when elaborating modports, which ends up only being relevant with interface arrays. Thank you very much for highlighting this issue!
  2. I think the design can only be read into Yosys with read_verilog -defer. I was thrown off because fixing this issue masks the other one.

I will work on fixing sv2v, but I think it will take some effort to get this right. Thank you for you patience.

tinebp commented 11 months ago

Thank you so much! Can you explain a bit more 1) and propose one solution I can use to implement interface arrays that work with sv2v, I will change our code to make it compatible.

zachjs commented 11 months ago

The issue is more obvious in a line like the below. Clearly the same i isn't the intended generate loop index at every level of the hierarchy. While sv2v takes care to disambiguate things that might be "shadowed" in this way, genvars are a bit special and aren't being handled accordingly.

.decr(Vortex.genblk1[i].cluster.genblk2[i].socket.genblk2[i].core.fetch_if.ibuf_pop[i]),

If you're eager for a workaround until I fix the behavior, renaming the conflicting genvars to have unique names would work.

zachjs commented 10 months ago

I just pushed ba94920ee0174826550d81690545811dd8822506 to master, which should fix the issue you filed. With this change, sv2v assigns a unique name to every genvar. I'm very interested to hear if the fix works on your end. Thank you again for providing this report!

It also now seems that -defer really isn't necessary when reading your design into Yosys. The nature of the shadowing here was fairly complex and threw off my initial analysis.

zachjs commented 10 months ago

@tinebp Were you able to get sv2v working for your use case with the fix above?

tinebp commented 10 months ago

Hi,

We rebuilt sv2v with the fixes and it now works perfectly well. We also noticed that all previous warnings were not firing anymore, we allowed us to turn off those detections. Thank you so much for this update, your utility is currently an essential part of our regression suite, Regards,Blaise On Sunday, August 20, 2023 at 02:04:11 PM EDT, Zachary Snow @.***> wrote:

@tinebp Were you able to get sv2v working for your use case with the fix above?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

zachjs commented 10 months ago

I'm happy to hear sv2v is working well for your use case. Thank you again for explaining the issue to me! I'm closing this as it seems there is nothing outstanding. If you run into problems, please reopen or file a new issue.