Merge pull request #5388 from jix/bufnorm-followup

Refactor and fixes to incremental bufNormalize + related changes

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));

kernel/rtlil.cc

@@ -2829,6 +2829,13 @@ void RTLIL::Module::remove(const pool<RTLIL::Wire*> &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);

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 *> 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
+		// Wires with their unique driving cell port. We pick the first driver
-		// driven by multiple (potential) drivers, this is indicated by a
+		// we encounter, with the exception that we ensure that a module input
-		// nullptr as cell.
+		// 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<Wire *, std::pair<Cell *, IdString>> direct_driven_wires;
 
-		// Set of non-unique or driving cell ports for each processed wire.
+		// Set of cell ports that need a fresh intermediate wire. These are all
-		dict<Wire *, pool<std::pair<Cell *, IdString>>> direct_driven_wires_conflicts;
+		// cell ports that drive non-full-wire sigspecs, cell ports driving
-
+		// module input ports, and cell ports driving wires that are already
-		// Set of cell ports that need a fresh intermediate wire.
+		// driven.
 		pool<std::pair<Cell *, IdString>> 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();
+			// Make sure all wires of the cell port are enqueued, ensuring we
-				if (!w->port_input || w->port_output) {
+			// detect other connected drivers (output and inout).
-					buf_norm_cell_queue.insert(cell);
+			for (auto chunk : sig.chunks())
-					remove(cell);
+				if (chunk.is_wire())
-					return;
+					wire_queue_entries(chunk.wire);
-				}
+
-				w->driverCell_ = cell;
+			if (cell->type == ID($buf) && cell->attributes.empty() && !cell->name.isPublic()) {
-				w->driverPort_ = ID::Y;
-			} else 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();
-
+				if (!w->port_input || w->port_output) {
-				// We try to store the current port as unique driver, if this
+					// If the full cell port is connected to a full non-input-port wire, pick it as driver
-				// succeeds we're done with the port.
+					auto [found, inserted] = direct_driven_wires.emplace(w, {cell, port});
-				auto [found, inserted] = direct_driven_wires.emplace(w, {cell, port});
+					if (inserted || (found->second.first == cell && found->second.second == port))
-				if (inserted || (found->second.first == cell && found->second.second == port))
+						return;
-					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);
 				}
 			}
 
@@ -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);
+			direct_driven_wires.emplace(w, {cell, port});
-			if (port_dir == RTLIL::PD_INOUT || port_dir == RTLIL::PD_UNKNOWN) {
+			cell->setPort(port, w); // Hits the fast path that doesn't enqueue w
-				direct_driven_wires.emplace(w, {nullptr, {}});
+			wire_queue_entries(w); // Needed so we pick up the sig <-> w connection
-				direct_driven_wires_conflicts[w].emplace(cell, port);
-			} else {
-				direct_driven_wires.emplace(w, {cell, port});
-			}
-
-			cell->setPort(port, w);
-			wire_queue_entries(w);
 		}
 
 		// 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<SigBit> conflicted;
+		// We pick SigMap representatives by prioritizing input ports over
-		pool<SigBit> driven;
+		// driven wires over other/unknown wires.
-
+		for (bool input_ports : {false, true}) {
-		// 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}) {
 			for (auto const &[wire, cellport] : direct_driven_wires) {
-				if (cellport.first == nullptr)
+				if ((wire->port_input && !wire->port_output) == input_ports) {
-					continue;
+					for (int i = 0; i != GetSize(wire); ++i) {
-				auto const &[cell, port] = cellport;
+						SigBit driver = SigBit(wire, i);
-
-				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 {
 						sigmap.database.promote(driver);
 					}
 				}
 			}
 		}
 
-		// Ensure that module level inout ports are directly driven or
+		// All three pool<SigBit> below are in terms of sigmapped bits
-		// connected using `$connect` cells and never `$buf`fered.
+		// Bits that are known to have a unique driver that is an unconditional driver or one or more inout drivers
-		for (auto wire : wire_queue_entries) {
+		pool<SigBit> driven;
-			if (!wire->port_input || !wire->port_output)
+		// Bits that have multiple unconditional drivers, this forces the use of `$connect`
-				continue;
+		pool<SigBit> conflicted;
+		// Bits that are driven by an inout driver
+		pool<SigBit> 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 driver = sigmap(SigBit(wire, i));
-				SigBit repr = sigmap(driver);
+				if (cell->type == ID($tribuf) || cell->port_dir(port) == RTLIL::PD_INOUT) {
-				if (driver != repr)
+					// We add inout drivers to `driven` in a separate loop below
-					driven.erase(repr);
+					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)
+		// If a wire has one or more inout drivers and an unconditional driver, that's still a conflict
-			driven.erase(bit);
+		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<pair<SigBit, SigBit>> 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;
 

passes/cmds/portarcs.cc

@@ -124,7 +124,8 @@ struct PortarcsPass : Pass {
 				TopoSort<SigBit> 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));

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)
+		if (verbose) {
-			log_debug("  removing buffer cell `%s': %s = %s\n", cell->name,
+			if (cell->type == ID($connect))
-					log_signal(cell->getPort(ID::Y)), log_signal(cell->getPort(ID::A)));
+				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())

tests/various/bufnorm_opt_clean.ys

@@ -0,0 +1,154 @@
+
+read_aiger <<EOF
+aag 124 11 9 2 104
+2
+4
+6
+8
+10
+12
+14
+16
+18
+20
+22
+24 42
+26 57
+28 58
+30 68
+32 86
+34 113
+36 114
+38 105
+40 133
+228
+248
+42 26 5
+44 35 33
+46 36 3
+48 46 44
+50 36 31
+52 28 25
+54 53 50
+56 55 49
+58 38 25
+60 40 28
+62 60 25
+64 37 30
+66 64 24
+68 66 62
+70 29 7
+72 40 37
+74 7 2
+76 74 73
+78 76 12
+80 78 71
+82 76 13
+84 83 14
+86 84 80
+88 34 31
+90 89 58
+92 40 37
+94 39 26
+96 94 93
+98 96 91
+100 98 14
+102 39 6
+104 40 27
+106 104 103
+108 107 55
+110 109 98
+112 110 100
+114 24 10
+116 26 8
+118 7 5
+120 119 117
+122 5 3
+124 27 25
+126 125 123
+128 126 120
+130 129 83
+132 131 111
+134 48 12
+136 121 48
+138 137 135
+140 139 130
+142 138 81
+144 114 91
+146 145 57
+148 38 29
+150 148 25
+152 151 48
+154 153 69
+156 155 147
+158 157 143
+160 159 140
+162 147 81
+164 163 86
+166 165 21
+168 167 161
+170 169 22
+172 113 19
+174 133 113
+176 174 172
+178 108 83
+180 179 155
+182 181 19
+184 182 140
+186 185 177
+188 112 21
+190 189 176
+192 146 140
+194 192 164
+196 112 86
+198 196 165
+200 198 194
+202 181 156
+204 145 83
+206 205 17
+208 178 138
+210 209 206
+212 140 132
+214 212 210
+216 214 202
+218 194 161
+220 218 216
+222 220 201
+224 222 190
+226 224 186
+228 226 170
+230 167 157
+232 182 172
+234 193 21
+236 235 232
+238 202 197
+240 238 185
+242 238 188
+244 242 240
+246 244 237
+248 246 231
+c
+aigfuzz -s -1 1145242652
+seed 1145242652
+fuzzer layers
+depth 9
+width 19
+input fraction 13%
+latch fraction 84%
+lower fraction 20%
+monotonicity -1
+and closure
+EOF
+
+bufnorm -update
+
+delete -output o:* %R1
+opt_clean
+bufnorm -update
+
+delete -output o:* %R1
+tee -q debug opt_clean
+bufnorm -update
+
+bufnorm -reset
+opt_clean