From 55dc5a4e4f7335741d2155dc0183ed4e26e8ddf8 Mon Sep 17 00:00:00 2001 From: Eddie Hung Date: Mon, 29 Mar 2021 22:01:57 -0700 Subject: [PATCH] abc9: fix SCC issues (#2694) * xilinx: add SCC test for DSP48E1 * xilinx: Gate DSP48E1 being a whitebox behind ALLOW_WHITEBOX_DSP48E1 Have a test that checks it works through ABC9 when enabled * abc9 to break SCCs using $__ABC9_SCC_BREAKER module * Add test * abc9_ops: remove refs to (* abc9_keep *) on wires * abc9_ops: do not bypass cells in an SCC * Add myself to CODEOWNERS for abc9* * Fix compile * abc9_ops: run -prep_hier before scc * Fix tests * Remove bug reference pending fix * abc9: fix for -prep_hier -dff * xaiger: restore PI handling * abc9_ops: -prep_xaiger sigmap * abc9_ops: -mark_scc -> -break_scc * abc9: eliminate hard-coded abc9.box from tests Also tidy up * Address review --- CODEOWNERS | 2 ++ backends/aiger/xaiger.cc | 9 +++-- passes/techmap/abc9.cc | 17 ++++++---- passes/techmap/abc9_ops.cc | 63 +++++++++++++++++++++++------------ techlibs/common/abc9_model.v | 4 +++ techlibs/common/abc9_unmap.v | 5 +++ tests/simple_abc9/abc9.box | 3 -- tests/simple_abc9/abc9.v | 24 ++++++++++--- tests/simple_abc9/run-test.sh | 12 ++++--- 9 files changed, 94 insertions(+), 45 deletions(-) delete mode 100644 tests/simple_abc9/abc9.box diff --git a/CODEOWNERS b/CODEOWNERS index 0419e6e44..0c92691f4 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -16,6 +16,8 @@ backends/cxxrtl/ @whitequark passes/cmds/bugpoint.cc @whitequark passes/techmap/flowmap.cc @whitequark passes/opt/opt_lut.cc @whitequark +passes/techmap/abc9*.cc @eddiehung +backends/aiger/xaiger.cc @eddiehung ## External Contributors diff --git a/backends/aiger/xaiger.cc b/backends/aiger/xaiger.cc index 7ed8ff1cf..65ccc748f 100644 --- a/backends/aiger/xaiger.cc +++ b/backends/aiger/xaiger.cc @@ -156,7 +156,7 @@ struct XAigerWriter // promote keep wires for (auto wire : module->wires()) - if (wire->get_bool_attribute(ID::keep) || wire->get_bool_attribute(ID::abc9_keep)) + if (wire->get_bool_attribute(ID::keep)) sigmap.add(wire); for (auto wire : module->wires()) { @@ -177,11 +177,10 @@ struct XAigerWriter undriven_bits.insert(bit); unused_bits.insert(bit); - bool keep = wire->get_bool_attribute(ID::abc9_keep); - if (wire->port_input || keep) + if (wire->port_input) input_bits.insert(bit); - keep = keep || wire->get_bool_attribute(ID::keep); + bool keep = wire->get_bool_attribute(ID::keep); if (wire->port_output || keep) { if (bit != wirebit) alias_map[wirebit] = bit; @@ -433,7 +432,7 @@ struct XAigerWriter if (bit == State::Sx) continue; if (aig_map.count(bit)) - log_error("Visited AIG node more than once; this could be a combinatorial loop that has not been broken - see Yosys bug 2530\n"); + log_error("Visited AIG node more than once; this could be a combinatorial loop that has not been broken\n"); aig_map[bit] = 2*aig_m; } diff --git a/passes/techmap/abc9.cc b/passes/techmap/abc9.cc index 56bb15495..0fef4a9f2 100644 --- a/passes/techmap/abc9.cc +++ b/passes/techmap/abc9.cc @@ -283,9 +283,14 @@ struct Abc9Pass : public ScriptPass if (check_label("map")) { if (help_mode) - run("abc9_ops -prep_hier -prep_bypass [-prep_dff -dff]", "(option if -dff)"); + run("abc9_ops -prep_hier [-dff]", "(option if -dff)"); else - run(stringf("abc9_ops -prep_hier -prep_bypass %s", dff_mode ? "-prep_dff -dff" : "")); + run(stringf("abc9_ops -prep_hier %s", dff_mode ? "-dff" : "")); + run("scc -specify -set_attr abc9_scc_id {}"); + if (help_mode) + run("abc9_ops -prep_bypass [-prep_dff]", "(option if -dff)"); + else + run(stringf("abc9_ops -prep_bypass %s", dff_mode ? "-prep_dff" : "")); if (dff_mode) { run("design -copy-to $abc9_map @$abc9_flops", "(only if -dff)"); run("select -unset $abc9_flops", " (only if -dff)"); @@ -330,20 +335,20 @@ struct Abc9Pass : public ScriptPass run("design -stash $abc9_map"); run("design -load $abc9"); run("design -delete $abc9"); + // Insert bypass modules (and perform +/abc9_map.v transformations), except for those cells part of a SCC if (help_mode) run("techmap -wb -max_iter 1 -map %$abc9_map -map +/abc9_map.v [-D DFF]", "(option if -dff)"); else - run(stringf("techmap -wb -max_iter 1 -map %%$abc9_map -map +/abc9_map.v %s", dff_mode ? "-D DFF" : "")); + run(stringf("techmap -wb -max_iter 1 -map %%$abc9_map -map +/abc9_map.v %s a:abc9_scc_id %%n", dff_mode ? "-D DFF" : "")); run("design -delete $abc9_map"); } if (check_label("pre")) { run("read_verilog -icells -lib -specify +/abc9_model.v"); - run("scc -specify -set_attr abc9_scc_id {}"); if (help_mode) - run("abc9_ops -mark_scc -prep_delays -prep_xaiger [-dff]", "(option for -dff)"); + run("abc9_ops -break_scc -prep_delays -prep_xaiger [-dff]", "(option for -dff)"); else - run("abc9_ops -mark_scc -prep_delays -prep_xaiger" + std::string(dff_mode ? " -dff" : "")); + run("abc9_ops -break_scc -prep_delays -prep_xaiger" + std::string(dff_mode ? " -dff" : "")); if (help_mode) run("abc9_ops -prep_lut ", "(skip if -lut or -luts)"); else if (!lut_mode) diff --git a/passes/techmap/abc9_ops.cc b/passes/techmap/abc9_ops.cc index 3f3e667de..d3bb31cd9 100644 --- a/passes/techmap/abc9_ops.cc +++ b/passes/techmap/abc9_ops.cc @@ -544,18 +544,31 @@ void prep_dff_unmap(RTLIL::Design *design) } } -void mark_scc(RTLIL::Module *module) +void break_scc(RTLIL::Module *module) { // For every unique SCC found, (arbitrarily) find the first - // cell in the component, and replace its output connections - // with a new wire driven by the old connection but with a - // special (* abc9_keep *) attribute set (which is used by - // write_xaiger to break this wire into PI and POs) + // cell in the component, and interrupt all its output connections + // with the $__ABC9_SCC_BREAKER cell + + // Do not break SCCs which have a cell instantiating an abc9_bypass-able + // module (but which wouldn't have been bypassed) + auto design = module->design; + pool scc_cells; pool ids_seen; for (auto cell : module->cells()) { auto it = cell->attributes.find(ID::abc9_scc_id); if (it == cell->attributes.end()) continue; + scc_cells.insert(cell); + auto inst_module = design->module(cell->type); + if (inst_module && inst_module->has_attribute(ID::abc9_bypass)) + ids_seen.insert(it->second); + } + + SigSpec I, O; + for (auto cell : scc_cells) { + auto it = cell->attributes.find(ID::abc9_scc_id); + log_assert(it != cell->attributes.end()); auto id = it->second; auto r = ids_seen.insert(id); cell->attributes.erase(it); @@ -565,12 +578,21 @@ void mark_scc(RTLIL::Module *module) if (c.second.is_fully_const()) continue; if (cell->output(c.first)) { Wire *w = module->addWire(NEW_ID, GetSize(c.second)); - w->set_bool_attribute(ID::abc9_keep); - module->connect(w, c.second); + I.append(w); + O.append(c.second); c.second = w; } } } + + if (!I.empty()) + { + auto cell = module->addCell(NEW_ID, ID($__ABC9_SCC_BREAKER)); + log_assert(GetSize(I) == GetSize(O)); + cell->setParam(ID::WIDTH, GetSize(I)); + cell->setPort(ID::I, std::move(I)); + cell->setPort(ID::O, std::move(O)); + } } void prep_delays(RTLIL::Design *design, bool dff_mode) @@ -721,10 +743,8 @@ void prep_xaiger(RTLIL::Module *module, bool dff) bit_users[bit].insert(cell->name); if (cell->output(conn.first) && !abc9_flop) - for (const auto &chunk : conn.second.chunks()) - if (!chunk.wire->get_bool_attribute(ID::abc9_keep)) - for (auto b : sigmap(SigSpec(chunk))) - bit_drivers[b].insert(cell->name); + for (auto bit : sigmap(conn.second)) + bit_drivers[bit].insert(cell->name); } toposort.node(cell->name); } @@ -1424,7 +1444,6 @@ void reintegrate(RTLIL::Module *module, bool dff_mode) RTLIL::Wire *mapped_wire = mapped_mod->wire(port); RTLIL::Wire *wire = module->wire(port); log_assert(wire); - wire->attributes.erase(ID::abc9_keep); RTLIL::Wire *remap_wire = module->wire(remap_name(port)); RTLIL::SigSpec signal(wire, remap_wire->start_offset-wire->start_offset, GetSize(remap_wire)); @@ -1587,11 +1606,11 @@ struct Abc9OpsPass : public Pass { log(" insert `$__ABC9_DELAY' blackbox cells into the design to account for\n"); log(" certain required times.\n"); log("\n"); - log(" -mark_scc\n"); + log(" -break_scc\n"); log(" for an arbitrarily chosen cell in each unique SCC of each selected module\n"); - log(" (tagged with an (* abc9_scc_id = *) attribute), temporarily mark all\n"); - log(" wires driven by this cell's outputs with a (* keep *) attribute in order\n"); - log(" to break the SCC. this temporary attribute will be removed on -reintegrate.\n"); + log(" (tagged with an (* abc9_scc_id = *) attribute) interrupt all wires\n"); + log(" driven by this cell's outputs with a temporary $__ABC9_SCC_BREAKER cell\n"); + log(" to break the SCC.\n"); log("\n"); log(" -prep_xaiger\n"); log(" prepare the design for XAIGER output. this includes computing the\n"); @@ -1628,7 +1647,7 @@ struct Abc9OpsPass : public Pass { bool check_mode = false; bool prep_delays_mode = false; - bool mark_scc_mode = false; + bool break_scc_mode = false; bool prep_hier_mode = false; bool prep_bypass_mode = false; bool prep_dff_mode = false, prep_dff_submod_mode = false, prep_dff_unmap_mode = false; @@ -1650,8 +1669,8 @@ struct Abc9OpsPass : public Pass { valid = true; continue; } - if (arg == "-mark_scc") { - mark_scc_mode = true; + if (arg == "-break_scc") { + break_scc_mode = true; valid = true; continue; } @@ -1727,7 +1746,7 @@ struct Abc9OpsPass : public Pass { extra_args(args, argidx, design); if (!valid) - log_cmd_error("At least one of -check, -mark_scc, -prep_{delays,xaiger,dff[123],lut,box}, -write_{lut,box}, -reintegrate must be specified.\n"); + log_cmd_error("At least one of -check, -break_scc, -prep_{delays,xaiger,dff[123],lut,box}, -write_{lut,box}, -reintegrate must be specified.\n"); if (dff_mode && !check_mode && !prep_hier_mode && !prep_delays_mode && !prep_xaiger_mode && !reintegrate_mode) log_cmd_error("'-dff' option is only relevant for -prep_{hier,delay,xaiger} or -reintegrate.\n"); @@ -1764,8 +1783,8 @@ struct Abc9OpsPass : public Pass { write_lut(mod, write_lut_dst); if (!write_box_dst.empty()) write_box(mod, write_box_dst); - if (mark_scc_mode) - mark_scc(mod); + if (break_scc_mode) + break_scc(mod); if (prep_xaiger_mode) prep_xaiger(mod, dff_mode); if (reintegrate_mode) diff --git a/techlibs/common/abc9_model.v b/techlibs/common/abc9_model.v index 4fee60f75..570a1ec40 100644 --- a/techlibs/common/abc9_model.v +++ b/techlibs/common/abc9_model.v @@ -6,6 +6,10 @@ module $__ABC9_DELAY (input I, output O); endspecify endmodule +module $__ABC9_SCC_BREAKER (input [WIDTH-1:0] I, output [WIDTH-1:0] O); +parameter WIDTH = 0; +endmodule + (* abc9_flop, abc9_box, lib_whitebox *) module $__DFF_N__$abc9_flop (input C, D, Q, output n1); assign n1 = D; diff --git a/techlibs/common/abc9_unmap.v b/techlibs/common/abc9_unmap.v index c39648c62..b1bc4fb6e 100644 --- a/techlibs/common/abc9_unmap.v +++ b/techlibs/common/abc9_unmap.v @@ -9,3 +9,8 @@ module $__DFF_x__$abc9_flop (input C, D, (* init = 1'b0 *) input Q, output n1); $error("Unrecognised _TECHMAP_CELLTYPE_"); endgenerate endmodule + +module $__ABC9_SCC_BREAKER (input [WIDTH-1:0] I, output [WIDTH-1:0] O); +parameter WIDTH = 0; +assign O = I; +endmodule diff --git a/tests/simple_abc9/abc9.box b/tests/simple_abc9/abc9.box deleted file mode 100644 index b3c88437c..000000000 --- a/tests/simple_abc9/abc9.box +++ /dev/null @@ -1,3 +0,0 @@ -MUXF8 1 0 3 1 -#I0 I1 S -0 0 0 # O diff --git a/tests/simple_abc9/abc9.v b/tests/simple_abc9/abc9.v index 5e969c614..fba089b1f 100644 --- a/tests/simple_abc9/abc9.v +++ b/tests/simple_abc9/abc9.v @@ -213,7 +213,7 @@ module arbiter (clk, rst, request, acknowledge, grant, grant_valid, grant_encode input rst; endmodule -(* abc9_box_id=1, blackbox *) +(* abc9_box, blackbox *) module MUXF8(input I0, I1, S, output O); specify (I0 => O) = 0; @@ -300,15 +300,29 @@ endmodule module abc9_test036(input A, B, S, output [1:0] O); (* keep *) MUXF8 m ( - .I0(I0), - .I1(I1), + .I0(A), + .I1(B), .O(O[0]), .S(S) ); MUXF8 m2 ( - .I0(I0), - .I1(I1), + .I0(A), + .I1(B), .O(O[1]), .S(S) ); endmodule + +(* abc9_box, whitebox *) +module MUXF7(input I0, I1, S, output O); +assign O = S ? I1 : I0; +specify + (I0 => O) = 0; + (I1 => O) = 0; + (S => O) = 0; +endspecify +endmodule + +module abc9_test037(output o); +MUXF7 m(.I0(1'b1), .I1(1'b0), .S(o), .O(o)); +endmodule diff --git a/tests/simple_abc9/run-test.sh b/tests/simple_abc9/run-test.sh index b48505e29..4a5bf01a3 100755 --- a/tests/simple_abc9/run-test.sh +++ b/tests/simple_abc9/run-test.sh @@ -37,14 +37,18 @@ done cp ../simple/*.v . cp ../simple/*.sv . +rm specify.v # bug 2675 DOLLAR='?' -exec ${MAKE:-make} -f ../tools/autotest.mk $seed *.v *.sv EXTRA_FLAGS="-n 300 -p '\ +exec ${MAKE:-make} -f ../tools/autotest.mk $seed *.v *.sv EXTRA_FLAGS="-f \"verilog -noblackbox -specify\" -n 300 -p '\ + read_verilog -icells -lib +/abc9_model.v; \ hierarchy; \ synth -run coarse; \ opt -full; \ techmap; \ - abc9 -lut 4 -box ../abc9.box; \ + abc9 -lut 4; \ clean; \ - check -assert; \ + check -assert * abc9_test037 %d; \ select -assert-none t:${DOLLAR}_NOT_ t:${DOLLAR}_AND_ %%; \ - setattr -mod -unset blackbox'" + setattr -mod -unset blackbox -unset whitebox'" + +# NOTE: Skip 'check -assert' on abc9_test037 because it intentionally has a combinatorial loop -- 2.30.2