zachjs / sv2v

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

Modport binding fails for interface instances with same name as a type #213

Closed dsdve closed 2 years ago

dsdve commented 2 years ago
typedef logic sometype;

interface foo;
endinterface

module bar(foo f);
endmodule

module test();
    foo sometype();
    bar b(.f(sometype));
endmodule

This fails with

sv2v: could not resolve modport binding sometype for port f of type foo, within scope test, near iftest.sv:11:2

Changing the name of the foo instance in test resolves this. Interestingly enough, if an instance of the type exists in the same scope, regardless of its name, the instance name can stay the same:

module test();
    sometype s;
    foo sometype();
    bar b(.f(sometype));
endmodule

A similar thing happens for types in packages (Note the pkg::sometype in the interface):

package pkg;
    typedef logic sometype;
endpackage

interface foo;
    pkg::sometype x; /* no errors without this */
endinterface

module bar(foo f);
endmodule

module test();
    foo pkg_sometype();
    bar b(.f(pkg_sometype));
endmodule

This raises the same error, which can also be worked around by introducing a pkg::sometype in the test module.

I think both cases are allowed by the LRM (judging by section 18.13 never mind, wrong document), though I can see how the former might be problematic. I don't see how the latter case can be illegal.

I should add that the package case is the one I actually care about, though of course I can simply rename the instances, so for me personally, this issue is not too important.

dsdve commented 2 years ago

Maybe related problem with packages:

package foo;
    typedef logic bar;
endpackage

module test(output foo::bar foo_bar);
endmodule 

emits

module test (foo_bar);
    output foo_bar foo_bar;
endmodule

instead of

module test (foo_bar);
    output wire foo_bar;
endmodule
zachjs commented 2 years ago

Thank you for sharing these cases! I pushed fixes for the two underlying issues. Please let me know if it works for you!

dsdve commented 2 years ago

Thanks! The initial issue does seem to be fixed. However the code from my comment still converts the same. But maybe that's simply an unrelated issue?

zachjs commented 2 years ago

The issue demonstrated by your second case had additional complicating factor: the troublesome declaration was not referenced elsewhere in the module. I've just pushed a fix to cover this case too.

dsdve commented 2 years ago

Works for me. Thanks again!