zachjs / sv2v

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

sv2v 7e2450e regression: extra wire declaration results in Yosys error #119

Closed nmoroze closed 4 years ago

nmoroze commented 4 years ago

Hi! I've run into an error in a project I'm working on that appears starting with sv2v 7e2450e. Here's a minimal example that reproduces the error:

module example (
    input in,
    output out1,
        output out2
);
    parameter param = 1;

    generate
        if (param == 0) begin : gen_a
        end
        else begin : gen_b
            assign out1 = in & out2;
            assign out2 = 1'b0;
        end
    endgenerate
endmodule

If I convert this code through sv2v eb908b8d, Yosys (tested with current top-of-tree, 08eb082) is able to read the Verilog with no problems. For example, if the above code is in a file called minimal-broken.sv, then the following sequence of commands runs with no errors: sv2v minimal-broken.sv > minimal-broken.v && yosys -p 'read_verilog minimal-broken.v'

However, if I convert this code using sv2v 7e2450e and run the same commands, I get an error from Yosys:

-- Running command `read_verilog minimal-broken.v' --

1. Executing Verilog-2005 frontend: minimal-broken.v
Parsing Verilog input from `minimal-broken.v' to AST representation.
Generating RTLIL representation for module `\example'.
minimal-broken.v:14: ERROR: Failed to detect width of signal access `\gen_b.out2'!

The sole cause of the problem seems to be an extra wire declaration, wire out2;, that the newer version of sv2v inserts immediately above the assignment to out2. Deleting this declaration removes the error. I'm actually not sure what the Verilog semantics say about such an extra declaration, so I'm not 100% sure whether this is ultimately an sv2v bug or a Yosys bug, but I'd love to know if you have any insight on this.

For what it's worth, coming up with an example to reproduce this error seems to require the use of a generate block with named scopes, and the use of an output wire in the RHS of some combinational logic.

Thanks!

zachjs commented 4 years ago

Thanks for another detailed PR! I made an oversight in my support for implicit nets. This has now been fixed. Wires like input out1 in your example are technically implicitly typed, but they do not need these explicit declarations inserted to have their type resolved. True implicit nets do not have this issue as there are defined to be declared in the scope of their first usage.

I'm actually not sure what the Verilog semantics say about such an extra declaration

While including the declaration isn't illegal, it does shadow the top-level port declaration, thus breaking the intended behavior of the design.

nmoroze commented 4 years ago

Fix is working for me. Thanks for the prompt response!