Merge pull request #1645 from YosysHQ/eddie/fix1644

{ice40,xilinx}_dsp: improve robustess

9 files changed, 52 insertions, 20 deletions

diff --git a/passes/pmgen/ice40_dsp.cc b/passes/pmgen/ice40_dsp.cc
index f60e67158..c364cd91a 100644
--- a/passes/pmgen/ice40_dsp.cc
+++ b/passes/pmgen/ice40_dsp.cc
@@ -73,11 +73,11 @@ void create_ice40_dsp(ice40_dsp_pm &pm)
 
 	// SB_MAC16 Input Interface
 	SigSpec A = st.sigA;
-	A.extend_u0(16, st.mul->getParam(ID(A_SIGNED)).as_bool());
+	A.extend_u0(16, st.mul->parameters.at(ID(A_SIGNED), State::S0).as_bool());
 	log_assert(GetSize(A) == 16);
 
 	SigSpec B = st.sigB;
-	B.extend_u0(16, st.mul->getParam(ID(B_SIGNED)).as_bool());
+	B.extend_u0(16, st.mul->parameters.at(ID(B_SIGNED), State::S0).as_bool());
 	log_assert(GetSize(B) == 16);
 
 	SigSpec CD = st.sigCD;
@@ -248,8 +248,8 @@ void create_ice40_dsp(ice40_dsp_pm &pm)
 	cell->setParam(ID(BOTADDSUB_CARRYSELECT), Const(0, 2));
 
 	cell->setParam(ID(MODE_8x8), State::S0);
-	cell->setParam(ID(A_SIGNED), st.mul->getParam(ID(A_SIGNED)).as_bool());
-	cell->setParam(ID(B_SIGNED), st.mul->getParam(ID(B_SIGNED)).as_bool());
+	cell->setParam(ID(A_SIGNED), st.mul->parameters.at(ID(A_SIGNED), State::S0).as_bool());
+	cell->setParam(ID(B_SIGNED), st.mul->parameters.at(ID(B_SIGNED), State::S0).as_bool());
 
 	if (st.ffO) {
 		if (st.o_lo)
diff --git a/passes/pmgen/ice40_dsp.pmg b/passes/pmgen/ice40_dsp.pmg
index 6b6d2b56f..9d649cb98 100644
--- a/passes/pmgen/ice40_dsp.pmg
+++ b/passes/pmgen/ice40_dsp.pmg
@@ -56,11 +56,16 @@ code sigA sigB sigH
 			break;
 		sigH.append(O[i]);
 	}
+	// This sigM could have no users if downstream sinks (e.g. $add) is
+	//   narrower than $mul result, for example
+	if (i == 0)
+		reject;
+
 	log_assert(nusers(O.extract_end(i)) <= 1);
 endcode
 
 code argQ ffA ffAholdmux ffArstmux ffAholdpol ffArstpol sigA clock clock_pol
-	if (mul->type != \SB_MAC16 || !param(mul, \A_REG).as_bool()) {
+	if (mul->type != \SB_MAC16 || !param(mul, \A_REG, State::S0).as_bool()) {
 		argQ = sigA;
 		subpattern(in_dffe);
 		if (dff) {
@@ -81,7 +86,7 @@ code argQ ffA ffAholdmux ffArstmux ffAholdpol ffArstpol sigA clock clock_pol
 endcode
 
 code argQ ffB ffBholdmux ffBrstmux ffBholdpol ffBrstpol sigB clock clock_pol
-	if (mul->type != \SB_MAC16 || !param(mul, \B_REG).as_bool()) {
+	if (mul->type != \SB_MAC16 || !param(mul, \B_REG, State::S0).as_bool()) {
 		argQ = sigB;
 		subpattern(in_dffe);
 		if (dff) {
@@ -104,7 +109,7 @@ endcode
 code argD ffFJKG sigH clock clock_pol
 	if (nusers(sigH) == 2 &&
 			(mul->type != \SB_MAC16 ||
-			 (!param(mul, \TOP_8x8_MULT_REG).as_bool() && !param(mul, \BOT_8x8_MULT_REG).as_bool() && !param(mul, \PIPELINE_16x16_MULT_REG1).as_bool() && !param(mul, \PIPELINE_16x16_MULT_REG1).as_bool()))) {
+			 (!param(mul, \TOP_8x8_MULT_REG, State::S0).as_bool() && !param(mul, \BOT_8x8_MULT_REG, State::S0).as_bool() && !param(mul, \PIPELINE_16x16_MULT_REG1, State::S0).as_bool() && !param(mul, \PIPELINE_16x16_MULT_REG1, State::S0).as_bool()))) {
 		argD = sigH;
 		subpattern(out_dffe);
 		if (dff) {
@@ -143,7 +148,7 @@ endcode
 
 code argD ffH sigH sigO clock clock_pol
 	if (ffFJKG && nusers(sigH) == 2 &&
-			(mul->type != \SB_MAC16 || !param(mul, \PIPELINE_16x16_MULT_REG2).as_bool())) {
+			(mul->type != \SB_MAC16 || !param(mul, \PIPELINE_16x16_MULT_REG2, State::S0).as_bool())) {
 		argD = sigH;
 		subpattern(out_dffe);
 		if (dff) {
@@ -174,7 +179,7 @@ reject_ffH:		;
 endcode
 
 match add
-	if mul->type != \SB_MAC16 || (param(mul, \TOPOUTPUT_SELECT).as_int() == 3 && param(mul, \BOTOUTPUT_SELECT).as_int() == 3)
+	if mul->type != \SB_MAC16 || (param(mul, \TOPOUTPUT_SELECT, State::S0).as_int() == 3 && param(mul, \BOTOUTPUT_SELECT, State::S0).as_int() == 3)
 
 	select add->type.in($add)
 	choice <IdString> AB {\A, \B}
@@ -200,7 +205,7 @@ code sigCD sigO cd_signed
 		if ((actual_acc_width > actual_mul_width) && (natural_mul_width > actual_mul_width))
 			reject;
 		// If accumulator, check adder width and signedness
-		if (sigCD == sigH && (actual_acc_width != actual_mul_width) && (param(mul, \A_SIGNED).as_bool() != param(add, \A_SIGNED).as_bool()))
+		if (sigCD == sigH && (actual_acc_width != actual_mul_width) && (param(mul, \A_SIGNED, State::S0).as_bool() != param(add, \A_SIGNED).as_bool()))
 			reject;
 
 		sigO = port(add, \Y);
@@ -275,7 +280,7 @@ endcode
 
 code argQ ffCD ffCDholdmux ffCDholdpol ffCDrstpol sigCD clock clock_pol
 	if (!sigCD.empty() && sigCD != sigO &&
-			(mul->type != \SB_MAC16 || (!param(mul, \C_REG).as_bool() && !param(mul, \D_REG).as_bool()))) {
+			(mul->type != \SB_MAC16 || (!param(mul, \C_REG, State::S0).as_bool() && !param(mul, \D_REG, State::S0).as_bool()))) {
 		argQ = sigCD;
 		subpattern(in_dffe);
 		if (dff) {
@@ -328,6 +333,8 @@ arg argD argQ clock clock_pol
 
 code
 	dff = nullptr;
+	if (argQ.empty())
+		reject;
 	for (auto c : argQ.chunks()) {
 		if (!c.wire)
 			reject;
diff --git a/passes/pmgen/xilinx_dsp.pmg b/passes/pmgen/xilinx_dsp.pmg
index 5d3b9c2eb..af47ab111 100644
--- a/passes/pmgen/xilinx_dsp.pmg
+++ b/passes/pmgen/xilinx_dsp.pmg
@@ -120,7 +120,7 @@ endcode
 //      reset functionality, using a subpattern discussed above)
 //     If matched, treat 'A' input as input of ADREG
 code argQ ffAD ffADcemux ffADrstmux ffADcepol ffADrstpol sigA clock
-	if (param(dsp, \ADREG).as_int() == 0) {
+	if (param(dsp, \ADREG, 1).as_int() == 0) {
 		argQ = sigA;
 		subpattern(in_dffe);
 		if (dff) {
@@ -176,7 +176,7 @@ code argQ ffAD ffADcemux ffADrstmux ffADcepol ffADrstpol sigA clock ffA2 ffA2cem
 	// Only search for ffA2 if there was a pre-adder
 	//   (otherwise ffA2 would have been matched as ffAD)
 	if (preAdd) {
-		if (param(dsp, \AREG).as_int() == 0) {
+		if (param(dsp, \AREG, 1).as_int() == 0) {
 			argQ = sigA;
 			subpattern(in_dffe);
 			if (dff) {
@@ -237,7 +237,7 @@ endcode
 // (5) Match 'B' input for B2REG
 //     If B2REG, then match 'B' input for B1REG
 code argQ ffB2 ffB2cemux ffB2rstmux ffB2cepol ffBrstpol sigB clock ffB1 ffB1cemux ffB1rstmux ffB1cepol
-	if (param(dsp, \BREG).as_int() == 0) {
+	if (param(dsp, \BREG, 1).as_int() == 0) {
 		argQ = sigB;
 		subpattern(in_dffe);
 		if (dff) {
@@ -287,7 +287,7 @@ endcode
 
 // (6) Match 'D' input for DREG
 code argQ ffD ffDcemux ffDrstmux ffDcepol ffDrstpol sigD clock
-	if (param(dsp, \DREG).as_int() == 0) {
+	if (param(dsp, \DREG, 1).as_int() == 0) {
 		argQ = sigD;
 		subpattern(in_dffe);
 		if (dff) {
@@ -308,7 +308,7 @@ endcode
 
 // (7) Match 'P' output that exclusively drives an MREG
 code argD ffM ffMcemux ffMrstmux ffMcepol ffMrstpol sigM sigP clock
-	if (param(dsp, \MREG).as_int() == 0 && nusers(sigM) == 2) {
+	if (param(dsp, \MREG, 1).as_int() == 0 && nusers(sigM) == 2) {
 		argD = sigM;
 		subpattern(out_dffe);
 		if (dff) {
@@ -335,7 +335,7 @@ endcode
 //      recognised in xilinx_dsp.cc).
 match postAdd
 	// Ensure that Z mux is not already used
-	if port(dsp, \OPMODE, SigSpec()).extract(4,3).is_fully_zero()
+	if port(dsp, \OPMODE, SigSpec(0, 7)).extract(4,3).is_fully_zero()
 
 	select postAdd->type.in($add)
 	select GetSize(port(postAdd, \Y)) <= 48
@@ -363,7 +363,7 @@ endcode
 
 // (9) Match 'P' output that exclusively drives a PREG
 code argD ffP ffPcemux ffPrstmux ffPcepol ffPrstpol sigP clock
-	if (param(dsp, \PREG).as_int() == 0) {
+	if (param(dsp, \PREG, 1).as_int() == 0) {
 		int users = 2;
 		// If ffMcemux and no postAdd new-value net must have three users: ffMcemux, ffM and ffPcemux
 		if (ffMcemux && !postAdd) users++;
@@ -460,7 +460,7 @@ arg argD argQ clock
 
 code
 	dff = nullptr;
-	if (GetSize(argQ) == 0)
+	if (argQ.empty())
 		reject;
 	for (const auto &c : argQ.chunks()) {
 		// Abandon matches when 'Q' is a constant
diff --git a/techlibs/ice40/synth_ice40.cc b/techlibs/ice40/synth_ice40.cc
index 121bcff1f..d92e40726 100644
--- a/techlibs/ice40/synth_ice40.cc
+++ b/techlibs/ice40/synth_ice40.cc
@@ -273,7 +273,8 @@ struct SynthIce40Pass : public ScriptPass
 			run("opt_expr");
 			run("opt_clean");
 			if (help_mode || dsp) {
-				run("memory_dff");
+				run("memory_dff"); // ice40_dsp will merge registers, reserve memory port registers first
+				run("wreduce t:$mul");
 				run("techmap -map +/mul2dsp.v -map +/ice40/dsp_map.v -D DSP_A_MAXWIDTH=16 -D DSP_B_MAXWIDTH=16 "
 						"-D DSP_A_MINWIDTH=2 -D DSP_B_MINWIDTH=2 -D DSP_Y_MINWIDTH=11 "
 						"-D DSP_NAME=$__MUL16X16", "(if -dsp)");
diff --git a/tests/arch/ice40/bug1644.il.gz b/tests/arch/ice40/bug1644.il.gz
new file mode 100644
index 000000000..363c510ef
--- /dev/null
+++ b/tests/arch/ice40/bug1644.il.gz
diff --git a/tests/arch/ice40/bug1644.ys b/tests/arch/ice40/bug1644.ys
new file mode 100644
index 000000000..5950f0e3c
--- /dev/null
+++ b/tests/arch/ice40/bug1644.ys
@@ -0,0 +1,2 @@
+read_ilang bug1644.il.gz
+synth_ice40 -top top -dsp -json adc_dac_pass_through.json -run :map_bram
diff --git a/tests/arch/ice40/ice40_dsp.ys b/tests/arch/ice40/ice40_dsp.ys
new file mode 100644
index 000000000..250273859
--- /dev/null
+++ b/tests/arch/ice40/ice40_dsp.ys
@@ -0,0 +1,11 @@
+read_verilog <<EOT
+module top(input [15:0] a, b, output [31:0] o1, o2, o5);
+SB_MAC16 m1 (.A(a), .B(16'd1234), .O(o1));
+assign o2 = a * 16'd0;
+wire [31:0] o3, o4;
+SB_MAC16 m2 (.A(a), .B(b), .O(o3));
+assign o4 = a * b;
+SB_MAC16 m3 (.A(a), .B(b), .O(o5));
+endmodule
+EOT
+ice40_dsp
diff --git a/tests/various/bug1462.ys b/tests/arch/xilinx/bug1462.ys
index 15cab5121..15cab5121 100644
--- a/tests/various/bug1462.ys
+++ b/tests/arch/xilinx/bug1462.ys
diff --git a/tests/arch/xilinx/xilinx_dsp.ys b/tests/arch/xilinx/xilinx_dsp.ys
new file mode 100644
index 000000000..3b9f52930
--- /dev/null
+++ b/tests/arch/xilinx/xilinx_dsp.ys
@@ -0,0 +1,11 @@
+read_verilog <<EOT
+module top(input [24:0] a, input [17:0] b, output [42:0] o1, o2, o5);
+DSP48E1 m1 (.A(a), .B(16'd1234), .P(o1));
+assign o2 = a * 16'd0;
+wire [42:0] o3, o4;
+DSP48E1 m2 (.A(a), .B(b), .P(o3));
+assign o4 = a * b;
+DSP48E1 m3 (.A(a), .B(b), .P(o5));
+endmodule
+EOT
+xilinx_dsp