From 9396e5e5fe40f90e44498597ae4df4cc391561e1 Mon Sep 17 00:00:00 2001 From: Jannis Harder Date: Tue, 23 Sep 2025 14:40:22 +0200 Subject: [PATCH 1/4] portarcs: Ignore all bufnorm helper cells The `portarcs` pass was already ignoring `$buf` cells when loading timing data, but now bufnorm will also emit `$input_port` and `$connect` helper cells, which need to be ignored as well. --- passes/cmds/portarcs.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/passes/cmds/portarcs.cc b/passes/cmds/portarcs.cc index 36870489a..8cc5d1236 100644 --- a/passes/cmds/portarcs.cc +++ b/passes/cmds/portarcs.cc @@ -124,7 +124,8 @@ struct PortarcsPass : Pass { TopoSort sort; for (auto cell : m->cells()) - if (cell->type != ID($buf)) { + // Ignore all bufnorm helper cells + if (!cell->type.in(ID($buf), ID($input_port), ID($connect))) { auto tdata = tinfo.find(cell->type); if (tdata == tinfo.end()) log_cmd_error("Missing timing data for module '%s'.\n", log_id(cell->type)); From 90669ab4eba25878ee592b80bc4ba5f79386b81c Mon Sep 17 00:00:00 2001 From: Jannis Harder Date: Tue, 23 Sep 2025 14:44:13 +0200 Subject: [PATCH 2/4] aiger2: Only fail for reachable undirected bufnorm helper cells The aiger2 backend checks for unsupported cells during indexing. This causes it to fail when `$connect` or `$tribuf` (as workaround for missing 'z-$buf support) cells are present in the module. Since bufnorm adds these cells automatically, it is very easy to end up with them due to unconnected wires or e.g. `$specify` cells, which do not pose an actual problem for the backend, since it will never encounter those during a traversal. With this, we ignore them during indexing and only produce an actual error message if we reach such a cell during the traversal. --- backends/aiger2/aiger.cc | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/backends/aiger2/aiger.cc b/backends/aiger2/aiger.cc index b63e51bde..41e1b91c1 100644 --- a/backends/aiger2/aiger.cc +++ b/backends/aiger2/aiger.cc @@ -105,6 +105,13 @@ struct Index { if (allow_blackboxes) { info.found_blackboxes.insert(cell); } else { + // Even if we don't allow blackboxes these might still be + // present outside of any traversed input cones, so we + // can't bail at this point. If they are hit by a traversal + // (which can only really happen with $tribuf not + // $connect), we can still detect this as an error later. + if (cell->type == ID($connect) || (cell->type == ID($tribuf) && cell->has_attribute(ID(aiger2_zbuf)))) + continue; if (!submodule || submodule->get_blackbox_attribute()) log_error("Unsupported cell type: %s (%s in %s)\n", log_id(cell->type), log_id(cell), log_id(m)); @@ -483,7 +490,8 @@ struct Index { { Design *design = index.design; auto &minfo = leaf_minfo(index); - log_assert(minfo.suboffsets.count(cell)); + if (!minfo.suboffsets.count(cell)) + log_error("Reached unsupport cell %s (%s in %s)\n", log_id(cell->type), log_id(cell), log_id(cell->module)); Module *def = design->module(cell->type); log_assert(def); levels.push_back(Level(index.modules.at(def), cell)); From cbc1055517b6936de34332c480da9e7a9aae658f Mon Sep 17 00:00:00 2001 From: Jannis Harder Date: Tue, 23 Sep 2025 21:54:55 +0200 Subject: [PATCH 3/4] opt_clean: Fix debug output when cleaning up bufnorm cells --- passes/opt/opt_clean.cc | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/passes/opt/opt_clean.cc b/passes/opt/opt_clean.cc index dc3f015cd..996a9b3c9 100644 --- a/passes/opt/opt_clean.cc +++ b/passes/opt/opt_clean.cc @@ -635,9 +635,17 @@ void rmunused_module(RTLIL::Module *module, bool purge_mode, bool verbose, bool } } for (auto cell : delcells) { - if (verbose) - log_debug(" removing buffer cell `%s': %s = %s\n", cell->name, - log_signal(cell->getPort(ID::Y)), log_signal(cell->getPort(ID::A))); + if (verbose) { + if (cell->type == ID($connect)) + log_debug(" removing connect cell `%s': %s <-> %s\n", cell->name, + log_signal(cell->getPort(ID::A)), log_signal(cell->getPort(ID::B))); + else if (cell->type == ID($input_port)) + log_debug(" removing input port marker cell `%s': %s\n", cell->name, + log_signal(cell->getPort(ID::Y))); + else + log_debug(" removing buffer cell `%s': %s = %s\n", cell->name, + log_signal(cell->getPort(ID::Y)), log_signal(cell->getPort(ID::A))); + } module->remove(cell); } if (!delcells.empty()) From 86fb2f16f7863634e1df9e692520656fa55a36a7 Mon Sep 17 00:00:00 2001 From: Jannis Harder Date: Tue, 23 Sep 2025 14:28:10 +0200 Subject: [PATCH 4/4] bufnorm: Refactor and fix incremental bufNormalize This fixes some edge cases the previous version didn't handle properly by simplifying the logic of determining directly driven wires and representatives to use as buffer inputs. --- kernel/rtlil.cc | 7 + kernel/rtlil_bufnorm.cc | 224 ++++++++++++++--------------- tests/various/bufnorm_opt_clean.ys | 154 ++++++++++++++++++++ 3 files changed, 270 insertions(+), 115 deletions(-) create mode 100644 tests/various/bufnorm_opt_clean.ys diff --git a/kernel/rtlil.cc b/kernel/rtlil.cc index e9d051de8..8daf2c821 100644 --- a/kernel/rtlil.cc +++ b/kernel/rtlil.cc @@ -2829,6 +2829,13 @@ void RTLIL::Module::remove(const pool &wires) delete_wire_worker.wires_p = &wires; rewrite_sigspecs2(delete_wire_worker); + if (design->flagBufferedNormalized) { + for (auto wire : wires) { + buf_norm_wire_queue.erase(wire); + buf_norm_connect_index.erase(wire); + } + } + for (auto &it : wires) { log_assert(wires_.count(it->name) != 0); wires_.erase(it->name); diff --git a/kernel/rtlil_bufnorm.cc b/kernel/rtlil_bufnorm.cc index 6d619d9e6..d0561f880 100644 --- a/kernel/rtlil_bufnorm.cc +++ b/kernel/rtlil_bufnorm.cc @@ -43,6 +43,7 @@ void RTLIL::Design::bufNormalize(bool enable) wire->driverCell_ = nullptr; wire->driverPort_ = IdString(); } + module->buf_norm_connect_index.clear(); } flagBufferedNormalized = false; @@ -126,15 +127,19 @@ void RTLIL::Module::bufNormalize() idict wire_queue_entries; // Ordered queue of wires to process int wire_queue_pos = 0; // Index up to which we processed the wires - // Wires with their unique driving cell port. If we know a wire is - // driven by multiple (potential) drivers, this is indicated by a - // nullptr as cell. + // Wires with their unique driving cell port. We pick the first driver + // we encounter, with the exception that we ensure that a module input + // port can only get $input_port drivers and that $input_port drivers + // cannot drive any other modules. If we reject an $input_port driver + // because it's not driving an input port or because there already is + // another $input_port driver for the same port, we also delete that + // $input_port cell. dict> direct_driven_wires; - // Set of non-unique or driving cell ports for each processed wire. - dict>> direct_driven_wires_conflicts; - - // Set of cell ports that need a fresh intermediate wire. + // Set of cell ports that need a fresh intermediate wire. These are all + // cell ports that drive non-full-wire sigspecs, cell ports driving + // module input ports, and cell ports driving wires that are already + // driven. pool> pending_ports; // This helper will be called for every output/inout cell port that is @@ -149,27 +154,14 @@ void RTLIL::Module::bufNormalize() SigSpec const &sig = cell->getPort(port); - if (cell->type == ID($input_port)) { - // If an `$input_port` cell isn't fully connected to a full - // input port wire, we remove it since the wires are still the - // canonical source of module ports and the `$input_port` cells - // are just helpers to simplfiy the bufnorm invariant. - log_assert(port == ID::Y); - if (!sig.is_wire()) { - buf_norm_cell_queue.insert(cell); - remove(cell); - return; - } - Wire *w = sig.as_wire(); - if (!w->port_input || w->port_output) { - buf_norm_cell_queue.insert(cell); - remove(cell); - return; - } - w->driverCell_ = cell; - w->driverPort_ = ID::Y; - } else if (cell->type == ID($buf) && cell->attributes.empty() && !cell->name.isPublic()) { + // Make sure all wires of the cell port are enqueued, ensuring we + // detect other connected drivers (output and inout). + for (auto chunk : sig.chunks()) + if (chunk.is_wire()) + wire_queue_entries(chunk.wire); + + if (cell->type == ID($buf) && cell->attributes.empty() && !cell->name.isPublic()) { // For a plain `$buf` cell, we enqueue all wires on its input // side, bypass it using module level connections (skipping 'z // bits) and then remove the cell. Eventually the module level @@ -206,47 +198,40 @@ void RTLIL::Module::bufNormalize() if (!sig_y.empty()) connect(sig_y, sig_a); + remove(cell); + log_assert(GetSize(buf_norm_wire_queue) <= 1); + buf_norm_wire_queue.clear(); + return; + } else if (cell->type == ID($input_port)) { + log_assert(port == ID::Y); + if (sig.is_wire()) { + Wire *w = sig.as_wire(); + if (w->port_input && !w->port_output) { + // An $input_port cell can only drive a full wire module input port + auto [found, inserted] = direct_driven_wires.emplace(w, {cell, port}); + if (!inserted || (found->second.first == cell && found->second.second == port)) + return; + } + } + + // If an `$input_port` cell isn't driving a full + // input port wire, we remove it since the wires are still the + // canonical source of module ports + buf_norm_cell_queue.insert(cell); remove(cell); + log_assert(GetSize(buf_norm_wire_queue) <= 1); + buf_norm_wire_queue.clear(); return; } - // Make sure all wires of the cell port are enqueued, ensuring we - // detect other connected drivers (output and inout). - for (auto const &chunk : sig.chunks()) - if (chunk.wire) - wire_queue_entries(chunk.wire); - if (sig.is_wire()) { - // If the full cell port is connected to a full wire, we might be - // able to keep that connection if this is a unique output port driving that wire Wire *w = sig.as_wire(); - - // We try to store the current port as unique driver, if this - // succeeds we're done with the port. - auto [found, inserted] = direct_driven_wires.emplace(w, {cell, port}); - if (inserted || (found->second.first == cell && found->second.second == port)) - return; - - // When this failed, we store this port as a conflict. If we - // had already stored a candidate for a unique driver, we also - // move it to the conflicts, leaving a nullptr marker. - - auto &conflicts = direct_driven_wires_conflicts[w]; - if (Cell *other_cell = found->second.first) { - if (other_cell->type == ID($input_port)) { - // Multiple input port cells - log_assert(cell->type != ID($input_port)); - } else { - pending_ports.insert(found->second); - conflicts.emplace(found->second); - found->second = {nullptr, {}}; - } - } - if (cell->type == ID($input_port)) { - found->second = {cell, port}; - } else { - conflicts.emplace(cell, port); + if (!w->port_input || w->port_output) { + // If the full cell port is connected to a full non-input-port wire, pick it as driver + auto [found, inserted] = direct_driven_wires.emplace(w, {cell, port}); + if (inserted || (found->second.first == cell && found->second.second == port)) + return; } } @@ -256,16 +241,22 @@ void RTLIL::Module::bufNormalize() pending_ports.emplace(cell, port); }; + // We enqueue all enqueued wires for `$buf`/`$connect` processing (clearing the module level queue). + for (auto wire : buf_norm_wire_queue) + wire_queue_entries(wire); + buf_norm_wire_queue.clear(); + + // Only after clearing the `buf_norm_wire_queue` are we allowed to call + // enqueue_cell_port, since we're using assertions to check against + // unintended wires being enqueued into `buf_norm_wire_queue` that + // would prevent us from restoring the bufnorm invariants in a single + // pass. + // We process all explicitly enqueued cell ports (clearing the module level queue). for (auto const &[cell, port_name] : buf_norm_cell_port_queue) enqueue_cell_port(cell, port_name); buf_norm_cell_port_queue.clear(); - // And enqueue all wires for `$buf`/`$connect` processing (clearing the module level queue). - for (auto wire : buf_norm_wire_queue) - wire_queue_entries(wire); - buf_norm_wire_queue.clear(); - // We also enqueue all wires that saw newly added module level connections. for (auto &[a, b] : connections_) for (auto &sig : {a, b}) @@ -302,8 +293,11 @@ void RTLIL::Module::bufNormalize() if (chunk.wire) wire_queue_entries(chunk.wire); connect(sig_a, sig_b); + buf_norm_cell_queue.insert(connect_cell); remove(connect_cell); + log_assert(GetSize(buf_norm_wire_queue) <= 2); + buf_norm_wire_queue.clear(); } } } @@ -315,6 +309,9 @@ void RTLIL::Module::bufNormalize() // As a first step for re-normalization we add all require intermediate // wires for cell output and inout ports. for (auto &[cell, port] : pending_ports) { + log_assert(cell->type != ID($input_port)); + log_assert(!cell->type.empty()); + log_assert(!pending_deleted_cells.count(cell)); SigSpec const &sig = cell->getPort(port); Wire *w = addWire(NEW_ID, GetSize(sig)); @@ -323,16 +320,9 @@ void RTLIL::Module::bufNormalize() // correspond to what you would get if the intermediate wires had // been in place from the beginning. connect(sig, w); - auto port_dir = cell->port_dir(port); - if (port_dir == RTLIL::PD_INOUT || port_dir == RTLIL::PD_UNKNOWN) { - direct_driven_wires.emplace(w, {nullptr, {}}); - direct_driven_wires_conflicts[w].emplace(cell, port); - } else { - direct_driven_wires.emplace(w, {cell, port}); - } - - cell->setPort(port, w); - wire_queue_entries(w); + direct_driven_wires.emplace(w, {cell, port}); + cell->setPort(port, w); // Hits the fast path that doesn't enqueue w + wire_queue_entries(w); // Needed so we pick up the sig <-> w connection } // At this point we're done with creating wires and know which ones are @@ -346,7 +336,7 @@ void RTLIL::Module::bufNormalize() wire->driverPort_.clear(); } - // For the unique output cell ports fully connected to a full wire, we + // For the unique driving cell ports fully connected to a full wire, we // can update the bufnorm data right away. For all other wires we will // have to create new `$buf` cells. for (auto const &[wire, cellport] : direct_driven_wires) { @@ -373,54 +363,55 @@ void RTLIL::Module::bufNormalize() SigMap sigmap(this); new_connections({}); - pool conflicted; - pool driven; - - // We iterate over all direct driven wires and try to make that wire's - // sigbits the representative sigbit for the net. We do a second pass - // to detect conflicts to then remove the conflicts from `driven`. - for (bool check : {false, true}) { + // We pick SigMap representatives by prioritizing input ports over + // driven wires over other/unknown wires. + for (bool input_ports : {false, true}) { for (auto const &[wire, cellport] : direct_driven_wires) { - if (cellport.first == nullptr) - continue; - auto const &[cell, port] = cellport; - - SigSpec z_mask; - if (cell->type == ID($buf)) - z_mask = cell->getPort(ID::A); - - for (int i = 0; i != GetSize(wire); ++i) { - SigBit driver = SigBit(wire, i); - if (!z_mask.empty() && z_mask[i] == State::Sz) - continue; - if (check) { - SigBit repr = sigmap(driver); - if (repr != driver) - conflicted.insert(repr); - else - driven.insert(repr); - } else { + if ((wire->port_input && !wire->port_output) == input_ports) { + for (int i = 0; i != GetSize(wire); ++i) { + SigBit driver = SigBit(wire, i); sigmap.database.promote(driver); } } } } - // Ensure that module level inout ports are directly driven or - // connected using `$connect` cells and never `$buf`fered. - for (auto wire : wire_queue_entries) { - if (!wire->port_input || !wire->port_output) - continue; + // All three pool below are in terms of sigmapped bits + // Bits that are known to have a unique driver that is an unconditional driver or one or more inout drivers + pool driven; + // Bits that have multiple unconditional drivers, this forces the use of `$connect` + pool conflicted; + // Bits that are driven by an inout driver + pool weakly_driven; + + for (auto const &[wire, cellport] : direct_driven_wires) { + auto const &[cell, port] = cellport; for (int i = 0; i != GetSize(wire); ++i) { - SigBit driver = SigBit(wire, i); - SigBit repr = sigmap(driver); - if (driver != repr) - driven.erase(repr); + SigBit driver = sigmap(SigBit(wire, i)); + if (cell->type == ID($tribuf) || cell->port_dir(port) == RTLIL::PD_INOUT) { + // We add inout drivers to `driven` in a separate loop below + weakly_driven.insert(driver); + } else { + // We remove driver conflicts from `driven` in a separate loop below + bool inserted = driven.insert(driver).second; + if (!inserted) + conflicted.insert(driver); + } } } - for (auto &bit : conflicted) - driven.erase(bit); + // If a wire has one or more inout drivers and an unconditional driver, that's still a conflict + for (auto driver : weakly_driven) + if (!driven.insert(driver).second) + conflicted.insert(driver); + + // This only leaves the drivers matching `driven`'s definition above + for (auto driver : conflicted) + driven.erase(driver); + + // Having picked representatives and checked whether they are unique + // drivers, we can turn the connecitivty of our sigmap back into $buf + // and $connect cells. // Module level bitwise connections not representable by `$buf` cells pool> undirected_connections; @@ -561,6 +552,8 @@ void RTLIL::Cell::unsetPort(const RTLIL::IdString& portname) w->driverCell_ = nullptr; w->driverPort_ = IdString(); module->buf_norm_wire_queue.insert(w); + } else if (w->driverCell_) { + log_assert(w->driverCell_->getPort(w->driverPort_) == w); } } @@ -630,7 +623,8 @@ void RTLIL::Cell::setPort(const RTLIL::IdString& portname, RTLIL::SigSpec signal // bufNormalize call if ((dir == RTLIL::PD_OUTPUT || dir == RTLIL::PD_INOUT) && signal.is_wire()) { Wire *w = signal.as_wire(); - if (w->driverCell_ == nullptr) { + if (w->driverCell_ == nullptr && ( + (w->port_input && !w->port_output) == (type == ID($input_port)))) { w->driverCell_ = this; w->driverPort_ = portname; diff --git a/tests/various/bufnorm_opt_clean.ys b/tests/various/bufnorm_opt_clean.ys new file mode 100644 index 000000000..e1036f920 --- /dev/null +++ b/tests/various/bufnorm_opt_clean.ys @@ -0,0 +1,154 @@ + +read_aiger <