From 73cf658996c22cd08e207df73dce4fcf267c5d7b Mon Sep 17 00:00:00 2001 From: Zachary Snow Date: Sun, 11 Feb 2024 13:28:14 -0500 Subject: [PATCH 1/2] verilog: indirect AST_CONCAT and AST_TO_UNSIGNED port connections - AST_CONCAT and AST_TO_UNSIGNED are always unsigned, but may generate RTLIL that exclusively reference a signed wire. - AST_CONCAT may also contain a memory write. --- CHANGELOG | 5 +++ frontends/ast/genrtlil.cc | 8 ++--- frontends/ast/simplify.cc | 6 ++++ tests/simple/memwr_port_connection.sv | 10 +++--- tests/verilog/signed_concat.v | 45 +++++++++++++++++++++++++++ tests/verilog/signed_concat.ys | 7 +++++ 6 files changed, 72 insertions(+), 9 deletions(-) create mode 100644 tests/verilog/signed_concat.v create mode 100644 tests/verilog/signed_concat.ys diff --git a/CHANGELOG b/CHANGELOG index 6fb7a92e5..1558f36ce 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -4,6 +4,11 @@ List of major changes and improvements between releases Yosys 0.50 .. Yosys 0.51-dev -------------------------- + * Verilog + - Fixed an issue that prevented using `{}` or `$unsigned()` for + certain signed expressions in port connections + - Fixed an issue that prevented writing to a memory word via a concatenation + in an output port connection Yosys 0.49 .. Yosys 0.50 -------------------------- diff --git a/frontends/ast/genrtlil.cc b/frontends/ast/genrtlil.cc index d3982b92b..2da804953 100644 --- a/frontends/ast/genrtlil.cc +++ b/frontends/ast/genrtlil.cc @@ -2124,11 +2124,9 @@ RTLIL::SigSpec AstNode::genRTLIL(int width_hint, bool sign_hint) if (sig.is_wire()) { // if the resulting SigSpec is a wire, its // signedness should match that of the AstNode - if (arg->type == AST_IDENTIFIER && arg->id2ast && arg->id2ast->is_signed && !arg->is_signed) - // fully-sliced signed wire will be resolved - // once the module becomes available - log_assert(attributes.count(ID::reprocess_after)); - else + // unless this instantiation depends on module + // information that isn't available yet + if (!attributes.count(ID::reprocess_after)) log_assert(arg->is_signed == sig.as_wire()->is_signed); } else if (arg->is_signed) { // non-trivial signed nodes are indirected through diff --git a/frontends/ast/simplify.cc b/frontends/ast/simplify.cc index d35756d4e..1ac149f10 100644 --- a/frontends/ast/simplify.cc +++ b/frontends/ast/simplify.cc @@ -1292,6 +1292,12 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin else if (contains_unbased_unsized(value)) // unbased unsized literals extend to width of the context lookup_suggested = true; + else if (value->type == AST_TO_UNSIGNED) + // inner expression may be signed by default + lookup_suggested = true; + else if (value->type == AST_CONCAT && value->children.size() == 1) + // concat of a single expression is equivalent to $unsigned + lookup_suggested = true; } } diff --git a/tests/simple/memwr_port_connection.sv b/tests/simple/memwr_port_connection.sv index 5bf414e08..bc60d7e52 100644 --- a/tests/simple/memwr_port_connection.sv +++ b/tests/simple/memwr_port_connection.sv @@ -5,9 +5,11 @@ module producer( endmodule module top( - output logic [3:0] out + output logic [3:0] out0, out1 ); - logic [3:0] v[0:0]; - producer p(v[0]); - assign out = v[0]; + logic [3:0] v[1:0]; + producer p0(v[0]); + producer p1({v[1]}); + assign out0 = v[0]; + assign out1 = v[1]; endmodule diff --git a/tests/verilog/signed_concat.v b/tests/verilog/signed_concat.v new file mode 100644 index 000000000..ca5a5e02f --- /dev/null +++ b/tests/verilog/signed_concat.v @@ -0,0 +1,45 @@ +`define OUTPUTS(mode) \ + o``mode``0, \ + o``mode``1, \ + o``mode``2, \ + o``mode``3, \ + o``mode``4 + +module gate( + input [1:0] iu, + input signed [1:0] is, + output [2:0] `OUTPUTS(u), + output signed [2:0] `OUTPUTS(s) +); +`define INSTANCES(mode) \ + mod m``mode``0({i``mode}, {o``mode``0}); \ + mod m``mode``1($unsigned(i``mode), o``mode``1); \ + mod m``mode``2({i``mode[1:0]}, o``mode``2); \ + mod m``mode``3({$signed(i``mode)}, o``mode``3); \ + mod m``mode``4($unsigned({i``mode}), o``mode``4); +`INSTANCES(u) +`INSTANCES(s) +`undef INSTANCES +endmodule + +module gold( + input [1:0] iu, is, + output [2:0] `OUTPUTS(u), `OUTPUTS(s) +); +`define INSTANCES(mode) \ + assign o``mode``0 = i``mode; \ + assign o``mode``1 = i``mode; \ + assign o``mode``2 = i``mode; \ + assign o``mode``3 = i``mode; \ + assign o``mode``4 = i``mode; +`INSTANCES(u) +`INSTANCES(s) +`undef INSTANCES +endmodule + +module mod( + input [2:0] inp, + output [2:0] out +); + assign out = inp; +endmodule diff --git a/tests/verilog/signed_concat.ys b/tests/verilog/signed_concat.ys new file mode 100644 index 000000000..44ede82d6 --- /dev/null +++ b/tests/verilog/signed_concat.ys @@ -0,0 +1,7 @@ +read_verilog signed_concat.v +hierarchy +proc +flatten gate +equiv_make gold gate equiv +equiv_simple +equiv_status -assert From ef9772bd8d4186071cf1d500541a74fecb51ec1d Mon Sep 17 00:00:00 2001 From: Zachary Snow Date: Sun, 14 Apr 2024 11:55:57 -0400 Subject: [PATCH 2/2] address review comments --- frontends/ast/simplify.cc | 11 ++++++++--- tests/simple/memwr_port_connection.sv | 8 +++++++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/frontends/ast/simplify.cc b/frontends/ast/simplify.cc index 1ac149f10..2128d4cd6 100644 --- a/frontends/ast/simplify.cc +++ b/frontends/ast/simplify.cc @@ -1260,6 +1260,8 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin } if (type == AST_CELL) { + // when a module lookup is suggested, any port connection that is not a + // plain identifier will be indirected through a new wire bool lookup_suggested = false; for (AstNode *child : children) { @@ -1282,7 +1284,7 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin continue; } if (elem->type == AST_MEMORY) - // need to determine is the is a read or wire + // need to determine is the is a read or write lookup_suggested = true; else if (elem->type == AST_WIRE && elem->is_signed && !value->children.empty()) // this may be a fully sliced signed wire which needs @@ -1295,9 +1297,12 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin else if (value->type == AST_TO_UNSIGNED) // inner expression may be signed by default lookup_suggested = true; - else if (value->type == AST_CONCAT && value->children.size() == 1) - // concat of a single expression is equivalent to $unsigned + else if (value->type == AST_CONCAT) { + // concat of a single expression is equivalent to $unsigned; + // concats could also contain one or references to memories, + // which may ambiguously be reads or writes lookup_suggested = true; + } } } diff --git a/tests/simple/memwr_port_connection.sv b/tests/simple/memwr_port_connection.sv index bc60d7e52..3ee26c016 100644 --- a/tests/simple/memwr_port_connection.sv +++ b/tests/simple/memwr_port_connection.sv @@ -5,11 +5,17 @@ module producer( endmodule module top( - output logic [3:0] out0, out1 + output logic [3:0] out0, out1, out2, out3 ); logic [3:0] v[1:0]; + logic [1:0] u[1:0]; + logic [1:0] t[1:0]; producer p0(v[0]); producer p1({v[1]}); + producer p2({u[1], u[0]}); + producer p3({{t[1]}, {t[0]}}); assign out0 = v[0]; assign out1 = v[1]; + assign out2 = {u[1], u[0]}; + assign out3 = {t[1], t[0]}; endmodule