Yosys asserts on signed signal assigned to unsigned instance port

tomverbeure commented 3 years ago

Steps to reproduce the issue

Reduced example:

test.v:

module test;
    wire signed [31:0] a_ff;

    my_dff #(32) u_my_dff(
        .dout(a_ff[31:0])
    );
endmodule

Yosys:

read_verilog -sv test.v

Expected behavior

File gets read correctly. verilator --lint-only doesn't have an issue with it.

Actual behavior

yosys> read_verilog -sv test.v
1. Executing Verilog-2005 frontend: test.v
Parsing SystemVerilog input from `test.v' to AST representation.
Generating RTLIL representation for module `\test'.
ERROR: Assert `arg->is_signed == sig.as_wire()->is_signed' failed in frontends/ast/genrtlil.cc:1828.

I don't think SystemVerilog requires equal signedness between argument and instance port, but even if it does, it should only check that during elaboration.

Isolation

Adding submodule definition -> also fail

The following case fails too, with the same error:

module my_dff #( parameter WIDTH=1) (output logic signed [WIDTH-1:0] dout);

    assign dout = 0;

endmodule

module test;
        wire signed [31:0] a_ff;

        my_dff #(32) u_my_dff(
                .dout(a_ff[31:0])
        );
endmodule

yosys> read_verilog -sv test.v
1. Executing Verilog-2005 frontend: test.v
Parsing SystemVerilog input from `test.v' to AST representation.
Generating RTLIL representation for module `\my_dff'.
Generating RTLIL representation for module `\test'.
ERROR: Assert `arg->is_signed == sig.as_wire()->is_signed' failed in frontends/ast/genrtlil.cc:1828.

Remove `signed` qualifier -> pass

When I remove the signed qualifiers, Yosys processes both cases fine.

module test;
        wire [31:0]  a_ff;

        my_dff #(32) u_my_dff(
                .dout(a_ff[31:0])
        );
endmodule

Change argument from vector to 1-bit wire -> pass

Also, when I keep the argument signed, but change argument from vector to a one-bit wire, it passes too:

module test;
        wire signed  a_ff;

        my_dff #(1) u_my_dff(
                .dout(a_ff)
        );
endmodule

Change argument from multi-bit vector to 1-bit vector -> fail

module test;
        wire signed [0:0]  a_ff;

        my_dff #(1) u_my_dff(
                .dout(a_ff[0:0])
        );
endmodule

zachjs commented 3 years ago

The port connection logic has an assertion which notices when a generated signal's signedness doesn't match the signedness of the AST node. In this case, a_ff[31:0] was being treated as a_ff, but this is incorrect. The latter is signed, while the former is not. The above PR adds a level of indirection to ensure that an expression like a_ff[31:0] is appropriately treated as unsigned.

tomverbeure commented 3 years ago

Amazing! The time between reporting this and the fix must be close to record.

After applying this patch to my local Yosys tree, I can successfully do SweRV -> sv2v-> Yosys read_verilog; hierarchy -top swerv; proc.

synth_ecp5 is now running. I expect it to take a while. :-)

YosysHQ / yosys