From f03e2c30aa3ad92bffb7ecd7179fe859d1b993b0 Mon Sep 17 00:00:00 2001
From: Eddie Hung <eddie@fpgeh.com>
Date: Thu, 9 Sep 2021 10:05:55 -0700
Subject: [PATCH] abc9: replace cell type/parameters if derived type already
 processed (#2991)

* Add close bracket

* Add testcase

* Replace cell type/param if in unmap_design

* Improve abc9_box error message too

* Update comment as per review
---
 passes/techmap/abc9_ops.cc  | 28 ++++++++++++++++++++++------
 techlibs/ecp5/synth_ecp5.cc |  2 +-
 tests/arch/ecp5/bug2731.ys  |  7 +++++++
 3 files changed, 30 insertions(+), 7 deletions(-)
 create mode 100644 tests/arch/ecp5/bug2731.ys

diff --git a/passes/techmap/abc9_ops.cc b/passes/techmap/abc9_ops.cc
index 4b5def5eb..c3eaa70d1 100644
--- a/passes/techmap/abc9_ops.cc
+++ b/passes/techmap/abc9_ops.cc
@@ -180,8 +180,16 @@ void prep_hier(RTLIL::Design *design, bool dff_mode)
 					continue;
 			}
 			else {
-				if (!derived_module->get_bool_attribute(ID::abc9_box) && !derived_module->get_bool_attribute(ID::abc9_bypass))
+				if (!derived_module->get_bool_attribute(ID::abc9_box) && !derived_module->get_bool_attribute(ID::abc9_bypass)) {
+					if (unmap_design->module(derived_type)) {
+						// If derived_type is present in unmap_design, it means that it was processed previously, but found to be incompatible -- e.g. if
+						// it contained a non-zero initial state. In this case, continue to replace the cell type/parameters so that it has the same properties
+						// as a compatible type, yet will be safely unmapped later
+						cell->type = derived_type;
+						cell->parameters.clear();
+					}
 					continue;
+				}
 			}
 
 			if (!unmap_design->module(derived_type)) {
@@ -442,7 +450,14 @@ void prep_dff(RTLIL::Design *design)
 			if (!inst_module->get_bool_attribute(ID::abc9_flop))
 				continue;
 			log_assert(!inst_module->get_blackbox_attribute(true /* ignore_wb */));
-			log_assert(cell->parameters.empty());
+			if (!cell->parameters.empty())
+			{
+				// At this stage of the ABC9 flow, cells instantiating (* abc9_flop *) modules must not contain any parameters -- instead it should
+				// be instantiating the derived module which will have had any parameters constant-propagated.
+				// This task is expected to be performed by `abc9_ops -prep_hier`, but it looks like it failed to do so for this design.
+				// Please file a bug report!
+				log_error("Not expecting parameters on cell '%s' instantiating module '%s' marked (* abc9_flop *)\n", log_id(cell->name), log_id(cell->type));
+			}
 			modules_sel.select(inst_module);
 		}
 }
@@ -783,10 +798,11 @@ void prep_xaiger(RTLIL::Module *module, bool dff)
 			continue;
 		if (!cell->parameters.empty())
 		{
-		    // At this stage of the ABC9 flow, all modules must be nonparametric, because ABC itself requires concrete netlists, and the presence of
-		    // parameters implies a non-concrete netlist. This means an (* abc9_box *) parametric module but due to a bug somewhere this hasn't been
-		    // uniquified into a concrete parameter-free module. This is a bug, and a bug report would be welcomed.
-		    log_error("Not expecting parameters on module '%s'  marked (* abc9_box *)\n", log_id(cell_name));
+			// At this stage of the ABC9 flow, cells instantiating (* abc9_box *) modules must not contain any parameters -- instead it should
+			// be instantiating the derived module which will have had any parameters constant-propagated.
+			// This task is expected to be performed by `abc9_ops -prep_hier`, but it looks like it failed to do so for this design.
+			// Please file a bug report!
+			log_error("Not expecting parameters on cell '%s' instantiating module '%s' marked (* abc9_box *)\n", log_id(cell_name), log_id(cell->type));
 		}
 		log_assert(box_module->get_blackbox_attribute());
 
diff --git a/techlibs/ecp5/synth_ecp5.cc b/techlibs/ecp5/synth_ecp5.cc
index 2df9a1f87..dc67fc71b 100644
--- a/techlibs/ecp5/synth_ecp5.cc
+++ b/techlibs/ecp5/synth_ecp5.cc
@@ -326,7 +326,7 @@ struct SynthEcp5Pass : public ScriptPass
 			}
 			run("dfflegalize" + dfflegalize_args, "($_DFFSR_*_ only if -asyncprld, $_*DFFE_* only if not -nodffe)");
 			if ((abc9 && dff) || help_mode)
-				run("zinit -all w:* t:$_DFF_?_ t:$_DFFE_??_ t:$_SDFF*", "(only if -abc9 and -dff");
+				run("zinit -all w:* t:$_DFF_?_ t:$_DFFE_??_ t:$_SDFF*", "(only if -abc9 and -dff)");
 			run(stringf("techmap -D NO_LUT %s -map +/ecp5/cells_map.v", help_mode ? "[-D ASYNC_PRLD]" : (asyncprld ? "-D ASYNC_PRLD" : "")));
 			run("opt_expr -undriven -mux_undef");
 			run("simplemap");
diff --git a/tests/arch/ecp5/bug2731.ys b/tests/arch/ecp5/bug2731.ys
new file mode 100644
index 000000000..c609cea3b
--- /dev/null
+++ b/tests/arch/ecp5/bug2731.ys
@@ -0,0 +1,7 @@
+read_verilog -icells <<EOF
+module top(input c, r, input [1:0] d, output reg [1:0] q);
+TRELLIS_FF #(.REGSET("SET")) ff1(.CLK(c), .LSR(r), .DI(d[0]), .Q(q[0]));
+TRELLIS_FF #(.REGSET("SET")) ff2(.CLK(c), .LSR(r), .DI(d[1]), .Q(q[1]));
+endmodule
+EOF
+synth_ecp5 -abc9 -dff
-- 
2.30.2