From bbceaa6b5e50aecc5bf8e4ab0cc03ad4abccd966 Mon Sep 17 00:00:00 2001 From: Krystine Sherwin <93062060+KrystalDelusion@users.noreply.github.com> Date: Tue, 14 Oct 2025 14:59:32 +1300 Subject: [PATCH 1/6] docs: Note partial support of modports --- docs/source/using_yosys/verilog.rst | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/source/using_yosys/verilog.rst b/docs/source/using_yosys/verilog.rst index 92f223e49..95c0168ba 100644 --- a/docs/source/using_yosys/verilog.rst +++ b/docs/source/using_yosys/verilog.rst @@ -370,7 +370,10 @@ from SystemVerilog: - array literals are currently not supported - SystemVerilog interfaces (SVIs) are supported. Modports for specifying whether - ports are inputs or outputs are supported. + ports are inputs or outputs are supported when used with named arguments, but + not positional arguments. i.e. ``driver_mod d0(.intf(intf0), + .in(inputs[0]));`` is supported but ``driver_mod d0(intf0, inputs[0]);`` is + not. - Assignments within expressions are supported. From 7bb0a1913e46cc205e89e1fda88eb31c85da4d86 Mon Sep 17 00:00:00 2001 From: Krystine Sherwin <93062060+KrystalDelusion@users.noreply.github.com> Date: Wed, 15 Oct 2025 09:10:33 +1300 Subject: [PATCH 2/6] hierarchy.cc: Raise error on positional interface Add test to check that it does error. --- passes/hierarchy/hierarchy.cc | 69 +++++++++++++++++++-------- tests/svinterfaces/positional_args.ys | 33 +++++++++++++ tests/svinterfaces/run-test.sh | 1 + 3 files changed, 82 insertions(+), 21 deletions(-) create mode 100644 tests/svinterfaces/positional_args.ys mode change 100755 => 100644 tests/svinterfaces/run-test.sh diff --git a/passes/hierarchy/hierarchy.cc b/passes/hierarchy/hierarchy.cc index ea68add18..c0e6a9104 100644 --- a/passes/hierarchy/hierarchy.cc +++ b/passes/hierarchy/hierarchy.cc @@ -156,6 +156,21 @@ std::string basic_cell_type(const std::string celltype, int pos[3] = nullptr) { return basicType; } +// Try to read an IdString as a numbered connection name ("$123" or similar), +// writing the result to dst. If the string isn't of the right format, ignore +// dst and return false. +bool read_id_num(RTLIL::IdString str, int *dst) +{ + log_assert(dst); + + const char *c_str = str.c_str(); + if (c_str[0] != '$' || !('0' <= c_str[1] && c_str[1] <= '9')) + return false; + + *dst = atoi(c_str + 1); + return true; +} + // A helper struct for expanding a module's interface connections in expand_module struct IFExpander { @@ -283,15 +298,42 @@ struct IFExpander RTLIL::IdString conn_name, const RTLIL::SigSpec &conn_signals) { - // Check if the connection is present as an interface in the sub-module's port list - const RTLIL::Wire *wire = submodule.wire(conn_name); - if (!wire || !wire->get_bool_attribute(ID::is_interface)) + // Does the connection look like an interface + const auto &bits = conn_signals; + if ( + bits.size() != 1 || + !bits[0].wire->get_bool_attribute(ID::is_interface) || + conn_signals[0].wire->name.str().find("$dummywireforinterface") != 0 + ) return; + // Check if the connection is present as an interface in the sub-module's port list + int id; + if (read_id_num(conn_name, &id)) { + /* Interface expansion is incompatible with positional arguments + * during expansion, the port list gets each interface signal + * inserted after the interface itself which means that the argument + * positions in the parent module no longer match. + * + * Supporting this would require expanding the interfaces in the + * parent module, renumbering the arguments to match, and then + * iterating over the ports list to find the matching interface + * (refactoring on_interface to accept different conn_names on the + * parent and child). + */ + log_error("Unable to connect `%s' to submodule `%s' with positional interface argument `%s'!\n", + module.name, + submodule.name, + conn_signals[0].wire->name.str().substr(23) + ); + } else { + // Lookup connection by name + const RTLIL::Wire *wire = submodule.wire(conn_name); + if (!wire || !wire->get_bool_attribute(ID::is_interface)) + return; + } // If the connection looks like an interface, handle it. - const auto &bits = conn_signals; - if (bits.size() == 1 && bits[0].wire->get_bool_attribute(ID::is_interface)) - on_interface(submodule, conn_name, conn_signals); + on_interface(submodule, conn_name, conn_signals); } // Iterate over the connections in a cell, tracking any interface @@ -376,21 +418,6 @@ RTLIL::Module *get_module(RTLIL::Design &design, return nullptr; } -// Try to read an IdString as a numbered connection name ("$123" or similar), -// writing the result to dst. If the string isn't of the right format, ignore -// dst and return false. -bool read_id_num(RTLIL::IdString str, int *dst) -{ - log_assert(dst); - - const char *c_str = str.c_str(); - if (c_str[0] != '$' || !('0' <= c_str[1] && c_str[1] <= '9')) - return false; - - *dst = atoi(c_str + 1); - return true; -} - // Check that the connections on the cell match those that are defined // on the type: each named connection should match the name of a port // and each positional connection should have an index smaller than diff --git a/tests/svinterfaces/positional_args.ys b/tests/svinterfaces/positional_args.ys new file mode 100644 index 000000000..6017a1c25 --- /dev/null +++ b/tests/svinterfaces/positional_args.ys @@ -0,0 +1,33 @@ +read_verilog -sv << EOF +interface simple_if; + logic receiver; + logic driver; +endinterface + +module driver_mod(simple_if intf, input in); + assign intf.driver = in; +endmodule + +module receiver_mod(simple_if intf); + assign intf.receiver = intf.driver; +endmodule + +module top( + input logic [1:0] inputs, + output logic [1:0] outputs +); + simple_if intf0(); + simple_if intf1(); + + driver_mod d0(intf0, inputs[0]); + driver_mod d1(intf1, inputs[1]); + + receiver_mod r0(intf0); + receiver_mod r1(intf1); + + assign outputs = {intf0.receiver, intf1.receiver}; +endmodule +EOF + +logger -expect error "Unable to connect.* with positional interface" 1 +hierarchy -top top diff --git a/tests/svinterfaces/run-test.sh b/tests/svinterfaces/run-test.sh old mode 100755 new mode 100644 index afa222766..71bdcd67a --- a/tests/svinterfaces/run-test.sh +++ b/tests/svinterfaces/run-test.sh @@ -5,3 +5,4 @@ ./run_simple.sh load_and_derive ./run_simple.sh resolve_types +./run_simple.sh positional_args From 37ba29482ef98c11ffea81185d5b869a6986f280 Mon Sep 17 00:00:00 2001 From: Krystine Sherwin <93062060+KrystalDelusion@users.noreply.github.com> Date: Wed, 15 Oct 2025 09:17:52 +1300 Subject: [PATCH 3/6] docs: Amend modports support to all SVI Also some formatting fixes. --- docs/source/using_yosys/verilog.rst | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/docs/source/using_yosys/verilog.rst b/docs/source/using_yosys/verilog.rst index 95c0168ba..a557360b7 100644 --- a/docs/source/using_yosys/verilog.rst +++ b/docs/source/using_yosys/verilog.rst @@ -9,7 +9,7 @@ Yosys and there are currently no plans to add support for them: - Non-synthesizable language features as defined in - IEC 62142(E):2005 / IEEE Std. 1364.1(E):2002 + IEC 62142(E):2005 / IEEE Std. 1364.1(E):2002 - The ``tri``, ``triand`` and ``trior`` net types @@ -356,24 +356,29 @@ from SystemVerilog: files being read into the same design afterwards. - typedefs are supported (including inside packages) - - type casts are currently not supported + + - type casts are currently not supported - enums are supported (including inside packages) - - but are currently not strongly typed + + - but are currently not strongly typed - packed structs and unions are supported - - arrays of packed structs/unions are currently not supported - - structure literals are currently not supported + + - arrays of packed structs/unions are currently not supported + - structure literals are currently not supported - multidimensional arrays are supported - - array assignment of unpacked arrays is currently not supported - - array literals are currently not supported -- SystemVerilog interfaces (SVIs) are supported. Modports for specifying whether - ports are inputs or outputs are supported when used with named arguments, but - not positional arguments. i.e. ``driver_mod d0(.intf(intf0), - .in(inputs[0]));`` is supported but ``driver_mod d0(intf0, inputs[0]);`` is - not. + - array assignment of unpacked arrays is currently not supported + - array literals are currently not supported + +- SystemVerilog interfaces (SVIs), including modports for specifying whether + ports are inputs or outputs, are partially supported. + + - interfaces must be provided as *named* arguments, not positional arguments. + i.e. ``foo bar(.intf(intf0), .x(x));`` is supported but ``foo bar(intf0, + x);`` is not. - Assignments within expressions are supported. From 5d2d5441090c0621a7288735027af93f8e616899 Mon Sep 17 00:00:00 2001 From: Krystine Sherwin <93062060+KrystalDelusion@users.noreply.github.com> Date: Wed, 15 Oct 2025 09:38:43 +1300 Subject: [PATCH 4/6] hierarchy.cc: Don't segfault --- passes/hierarchy/hierarchy.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/passes/hierarchy/hierarchy.cc b/passes/hierarchy/hierarchy.cc index c0e6a9104..bfdb80459 100644 --- a/passes/hierarchy/hierarchy.cc +++ b/passes/hierarchy/hierarchy.cc @@ -302,7 +302,9 @@ struct IFExpander const auto &bits = conn_signals; if ( bits.size() != 1 || + bits[0].wire == nullptr || !bits[0].wire->get_bool_attribute(ID::is_interface) || + conn_signals[0].wire == nullptr || conn_signals[0].wire->name.str().find("$dummywireforinterface") != 0 ) return; From 10a55119a9b1008b52f18c499a406dc9d3e25ec7 Mon Sep 17 00:00:00 2001 From: Krystine Sherwin <93062060+KrystalDelusion@users.noreply.github.com> Date: Wed, 15 Oct 2025 09:42:47 +1300 Subject: [PATCH 5/6] hierarchy.cc: Tidying --- passes/hierarchy/hierarchy.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/passes/hierarchy/hierarchy.cc b/passes/hierarchy/hierarchy.cc index bfdb80459..416997bee 100644 --- a/passes/hierarchy/hierarchy.cc +++ b/passes/hierarchy/hierarchy.cc @@ -299,12 +299,10 @@ struct IFExpander const RTLIL::SigSpec &conn_signals) { // Does the connection look like an interface - const auto &bits = conn_signals; if ( - bits.size() != 1 || - bits[0].wire == nullptr || - !bits[0].wire->get_bool_attribute(ID::is_interface) || + conn_signals.size() != 1 || conn_signals[0].wire == nullptr || + conn_signals[0].wire->get_bool_attribute(ID::is_interface) == false || conn_signals[0].wire->name.str().find("$dummywireforinterface") != 0 ) return; From c599d6a67e1ddd54afd503cdc245cb0234d1c54c Mon Sep 17 00:00:00 2001 From: Krystine Sherwin <93062060+KrystalDelusion@users.noreply.github.com> Date: Wed, 15 Oct 2025 09:49:53 +1300 Subject: [PATCH 6/6] tests/svinterfaces: re-chmod test script --- tests/svinterfaces/run-test.sh | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 tests/svinterfaces/run-test.sh diff --git a/tests/svinterfaces/run-test.sh b/tests/svinterfaces/run-test.sh old mode 100644 new mode 100755