Merge pull request #4677 from YosysHQ/emil/opt_merge-hashing

opt_merge: hashing performance and correctness

passes/opt/opt_merge.cc

@@ -26,6 +26,8 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <set>
+#include <unordered_map>
+#include <array>
 
 
 USING_YOSYS_NAMESPACE
@@ -42,6 +44,22 @@ struct OptMergeWorker
 	CellTypes ct;
 	int total_count;
 
+	static vector<pair<SigBit, SigSpec>> sorted_pmux_in(const dict<RTLIL::IdString, RTLIL::SigSpec> &conn)
+	{
+		SigSpec sig_s = conn.at(ID::S);
+		SigSpec sig_b = conn.at(ID::B);
+
+		int s_width = GetSize(sig_s);
+		int width = GetSize(sig_b) / s_width;
+
+		vector<pair<SigBit, SigSpec>> sb_pairs;
+		for (int i = 0; i < s_width; i++)
+			sb_pairs.push_back(pair<SigBit, SigSpec>(sig_s[i], sig_b.extract(i*width, width)));
+
+		std::sort(sb_pairs.begin(), sb_pairs.end());
+		return sb_pairs;
+	}
+
 	static void sort_pmux_conn(dict<RTLIL::IdString, RTLIL::SigSpec> &conn)
 	{
 		SigSpec sig_s = conn.at(ID::S);
@@ -65,95 +83,78 @@ struct OptMergeWorker
 		}
 	}
 
-	std::string int_to_hash_string(unsigned int v)
+	Hasher hash_cell_inputs(const RTLIL::Cell *cell, Hasher h) const
 	{
-		if (v == 0)
+		// TODO: when implemented, use celltypes to match:
-			return "0";
+		// (builtin || stdcell) && (unary || binary) && symmetrical
-		std::string str = "";
-		while (v > 0) {
-			str += 'a' + (v & 15);
-			v = v >> 4;
-		}
-		return str;
-	}
-
-        uint64_t hash_cell_parameters_and_connections(const RTLIL::Cell *cell)
-	{
-		vector<string> hash_conn_strings;
-		std::string hash_string = cell->type.str() + "\n";
-
-		const dict<RTLIL::IdString, RTLIL::SigSpec> *conn = &cell->connections();
-		dict<RTLIL::IdString, RTLIL::SigSpec> alt_conn;
-
 		if (cell->type.in(ID($and), ID($or), ID($xor), ID($xnor), ID($add), ID($mul),
 				ID($logic_and), ID($logic_or), ID($_AND_), ID($_OR_), ID($_XOR_))) {
-			alt_conn = *conn;
+			std::array<RTLIL::SigSpec, 2> inputs = {
-			if (assign_map(alt_conn.at(ID::A)) < assign_map(alt_conn.at(ID::B))) {
+				assign_map(cell->getPort(ID::A)),
-				alt_conn[ID::A] = conn->at(ID::B);
+				assign_map(cell->getPort(ID::B))
-				alt_conn[ID::B] = conn->at(ID::A);
+			};
+			std::sort(inputs.begin(), inputs.end());
+			h = hash_ops<std::array<RTLIL::SigSpec, 2>>::hash_into(inputs, h);
+		} else if (cell->type.in(ID($reduce_xor), ID($reduce_xnor))) {
+			SigSpec a = assign_map(cell->getPort(ID::A));
+			a.sort();
+			h = a.hash_into(h);
+		} else if (cell->type.in(ID($reduce_and), ID($reduce_or), ID($reduce_bool))) {
+			SigSpec a = assign_map(cell->getPort(ID::A));
+			a.sort_and_unify();
+			h = a.hash_into(h);
+		} else if (cell->type == ID($pmux)) {
+			dict<RTLIL::IdString, RTLIL::SigSpec> conn = cell->connections();
+			assign_map.apply(conn.at(ID::A));
+			assign_map.apply(conn.at(ID::B));
+			assign_map.apply(conn.at(ID::S));
+			for (const auto& [s_bit, b_chunk] : sorted_pmux_in(conn)) {
+				h = s_bit.hash_into(h);
+				h = b_chunk.hash_into(h);
 			}
-			conn = &alt_conn;
+			h = assign_map(cell->getPort(ID::A)).hash_into(h);
-		} else
+		} else {
-		if (cell->type.in(ID($reduce_xor), ID($reduce_xnor))) {
+			std::vector<std::pair<IdString, SigSpec>> conns;
-			alt_conn = *conn;
+			for (const auto& conn : cell->connections()) {
-			assign_map.apply(alt_conn.at(ID::A));
+				conns.push_back(conn);
-			alt_conn.at(ID::A).sort();
+			}
-			conn = &alt_conn;
+			std::sort(conns.begin(), conns.end());
-		} else
+			for (const auto& [port, sig] : conns) {
-		if (cell->type.in(ID($reduce_and), ID($reduce_or), ID($reduce_bool))) {
+				if (!cell->output(port)) {
-			alt_conn = *conn;
+					h = port.hash_into(h);
-			assign_map.apply(alt_conn.at(ID::A));
+					h = assign_map(sig).hash_into(h);
-			alt_conn.at(ID::A).sort_and_unify();
-			conn = &alt_conn;
-		} else
-		if (cell->type == ID($pmux)) {
-			alt_conn = *conn;
-			assign_map.apply(alt_conn.at(ID::A));
-			assign_map.apply(alt_conn.at(ID::B));
-			assign_map.apply(alt_conn.at(ID::S));
-			sort_pmux_conn(alt_conn);
-			conn = &alt_conn;
-		}
-
-		for (auto &it : *conn) {
-			RTLIL::SigSpec sig;
-			if (cell->output(it.first)) {
-				if (it.first == ID::Q && RTLIL::builtin_ff_cell_types().count(cell->type)) {
-					// For the 'Q' output of state elements,
-					//   use its (* init *) attribute value
-					sig = initvals(it.second);
 				}
-				else
-					continue;
 			}
-			else
+
-				sig = assign_map(it.second);
+			if (RTLIL::builtin_ff_cell_types().count(cell->type))
-			string s = "C " + it.first.str() + "=";
+				h = initvals(cell->getPort(ID::Q)).hash_into(h);
-			for (auto &chunk : sig.chunks()) {
+
-				if (chunk.wire)
-					s += "{" + chunk.wire->name.str() + " " +
-							int_to_hash_string(chunk.offset) + " " +
-							int_to_hash_string(chunk.width) + "}";
-				else
-					s += RTLIL::Const(chunk.data).as_string();
-			}
-			hash_conn_strings.push_back(s + "\n");
 		}
-
+		return h;
-		for (auto &it : cell->parameters)
-			hash_conn_strings.push_back("P " + it.first.str() + "=" + it.second.as_string() + "\n");
-
-		std::sort(hash_conn_strings.begin(), hash_conn_strings.end());
-
-		for (auto it : hash_conn_strings)
-			hash_string += it;
-
-		return std::hash<std::string>{}(hash_string);
 	}
 
-	bool compare_cell_parameters_and_connections(const RTLIL::Cell *cell1, const RTLIL::Cell *cell2)
+	static Hasher hash_cell_parameters(const RTLIL::Cell *cell, Hasher h)
 	{
-		log_assert(cell1 != cell2);
+		using Paramvec = std::vector<std::pair<IdString, Const>>;
+		Paramvec params;
+		for (const auto& param : cell->parameters) {
+			params.push_back(param);
+		}
+		std::sort(params.begin(), params.end());
+		return hash_ops<Paramvec>::hash_into(params, h);
+	}
+
+	Hasher hash_cell_function(const RTLIL::Cell *cell, Hasher h) const
+	{
+		h.eat(cell->type);
+		h = hash_cell_inputs(cell, h);
+		h = hash_cell_parameters(cell, h);
+		return h;
+	}
+
+	bool compare_cell_parameters_and_connections(const RTLIL::Cell *cell1, const RTLIL::Cell *cell2) const
+	{
+		if (cell1 == cell2) return true;
 		if (cell1->type != cell2->type) return false;
 
 		if (cell1->parameters != cell2->parameters)
@@ -252,21 +253,51 @@ struct OptMergeWorker
 		initvals.set(&assign_map, module);
 
 		bool did_something = true;
+		// A cell may have to go through a lot of collisions if the hash
+		// function is performing poorly, but it's a symptom of something bad
+		// beyond the user's control.
 		while (did_something)
 		{
 			std::vector<RTLIL::Cell*> cells;
-			cells.reserve(module->cells_.size());
+			cells.reserve(module->cells().size());
-			for (auto &it : module->cells_) {
+			for (auto cell : module->cells()) {
-				if (!design->selected(module, it.second))
+				if (!design->selected(module, cell))
 					continue;
-				if (mode_keepdc && has_dont_care_initval(it.second))
+				if (cell->type.in(ID($meminit), ID($meminit_v2), ID($mem), ID($mem_v2))) {
+					// Ignore those for performance: meminit can have an excessively large port,
+					// mem can have an excessively large parameter holding the init data
 					continue;
-				if (ct.cell_known(it.second->type) || (mode_share_all && it.second->known()))
+				}
-					cells.push_back(it.second);
+				if (mode_keepdc && has_dont_care_initval(cell))
+					continue;
+				if (ct.cell_known(cell->type) || (mode_share_all && cell->known()))
+					cells.push_back(cell);
 			}
 
 			did_something = false;
-                        dict<uint64_t, RTLIL::Cell*> sharemap;
+
+			// We keep a set of known cells. They're hashed with our hash_cell_function
+			// and compared with our compare_cell_parameters_and_connections.
+			// Both need to capture OptMergeWorker to access initvals
+			struct CellPtrHash {
+				const OptMergeWorker& worker;
+				CellPtrHash(const OptMergeWorker& w) : worker(w) {}
+				std::size_t operator()(const Cell* c) const {
+					return (std::size_t)worker.hash_cell_function(c, Hasher()).yield();
+				}
+			};
+			struct CellPtrEqual {
+				const OptMergeWorker& worker;
+				CellPtrEqual(const OptMergeWorker& w) : worker(w) {}
+				bool operator()(const Cell* lhs, const Cell* rhs) const {
+					return worker.compare_cell_parameters_and_connections(lhs, rhs);
+				}
+			};
+			std::unordered_set<
+				RTLIL::Cell*,
+				CellPtrHash,
+				CellPtrEqual> known_cells (0, CellPtrHash(*this), CellPtrEqual(*this));
+
 			for (auto cell : cells)
 			{
 				if ((!mode_share_all && !ct.cell_known(cell->type)) || !cell->known())
@@ -275,36 +306,36 @@ struct OptMergeWorker
 				if (cell->type == ID($scopeinfo))
 					continue;
 
-				uint64_t hash = hash_cell_parameters_and_connections(cell);
+				auto [cell_in_map, inserted] = known_cells.insert(cell);
-				auto r = sharemap.insert(std::make_pair(hash, cell));
+				if (!inserted) {
-				if (!r.second) {
+					// We've failed to insert since we already have an equivalent cell
-					if (compare_cell_parameters_and_connections(cell, r.first->second)) {
+					Cell* other_cell = *cell_in_map;
-						if (cell->has_keep_attr()) {
+					if (cell->has_keep_attr()) {
-							if (r.first->second->has_keep_attr())
+						if (other_cell->has_keep_attr())
-								continue;
+							continue;
-							std::swap(r.first->second, cell);
+						known_cells.erase(other_cell);
-						}
+						known_cells.insert(cell);
-
+						std::swap(other_cell, cell);
-
-						did_something = true;
-						log_debug("  Cell `%s' is identical to cell `%s'.\n", cell->name.c_str(), r.first->second->name.c_str());
-						for (auto &it : cell->connections()) {
-							if (cell->output(it.first)) {
-								RTLIL::SigSpec other_sig = r.first->second->getPort(it.first);
-								log_debug("    Redirecting output %s: %s = %s\n", it.first.c_str(),
-										log_signal(it.second), log_signal(other_sig));
-								Const init = initvals(other_sig);
-								initvals.remove_init(it.second);
-								initvals.remove_init(other_sig);
-								module->connect(RTLIL::SigSig(it.second, other_sig));
-								assign_map.add(it.second, other_sig);
-								initvals.set_init(other_sig, init);
-							}
-						}
-						log_debug("    Removing %s cell `%s' from module `%s'.\n", cell->type.c_str(), cell->name.c_str(), module->name.c_str());
-						module->remove(cell);
-						total_count++;
 					}
+
+					did_something = true;
+					log_debug("  Cell `%s' is identical to cell `%s'.\n", cell->name.c_str(), other_cell->name.c_str());
+					for (auto &it : cell->connections()) {
+						if (cell->output(it.first)) {
+							RTLIL::SigSpec other_sig = other_cell->getPort(it.first);
+							log_debug("    Redirecting output %s: %s = %s\n", it.first.c_str(),
+									log_signal(it.second), log_signal(other_sig));
+							Const init = initvals(other_sig);
+							initvals.remove_init(it.second);
+							initvals.remove_init(other_sig);
+							module->connect(RTLIL::SigSig(it.second, other_sig));
+							assign_map.add(it.second, other_sig);
+							initvals.set_init(other_sig, init);
+						}
+					}
+					log_debug("    Removing %s cell `%s' from module `%s'.\n", cell->type.c_str(), cell->name.c_str(), module->name.c_str());
+					module->remove(cell);
+					total_count++;
 				}
 			}
 		}

tests/opt/opt_merge_basic.ys

@@ -0,0 +1,166 @@
+read_verilog <<EOT
+module top(A, B, X, Y);
+input [7:0] A, B;
+output [7:0] X, Y;
+assign X = A + B;
+assign Y = A + B;
+endmodule
+EOT
+# Most basic case
+# Binary
+select -assert-count 2 t:$add
+equiv_opt -assert opt_merge
+design -load postopt
+select -assert-count 1 t:$add
+
+design -reset
+read_verilog <<EOT
+module top(A, B, C, X, Y);
+input [7:0] A, B, C;
+output [7:0] X, Y;
+assign X = A + B;
+assign Y = A + C; // <- look here
+endmodule
+EOT
+# Reject on a different input
+select -assert-count 2 t:$add
+opt_merge
+select -assert-count 2 t:$add
+
+design -reset
+read_verilog <<EOT
+module top(A, X, Y);
+input [7:0] A;
+output X, Y;
+assign X = ^A;
+assign Y = ^A;
+endmodule
+EOT
+# Unary
+select -assert-count 2 t:$reduce_xor
+dump
+opt_merge
+select -assert-count 1 t:$reduce_xor
+
+design -reset
+read_verilog -icells <<EOT
+module top(A, B, X, Y);
+input [7:0] A, B;
+output X, Y;
+  \$reduce_xor  #(
+    .A_SIGNED(32'd0),
+    .A_WIDTH(32'd16),
+    .Y_WIDTH(32'd1),
+  ) one  (
+    .A({A, B}), // <- look here
+    .Y(X)
+  );
+  \$reduce_xor  #(
+    .A_SIGNED(32'd0),
+    .A_WIDTH(32'd16),
+    .Y_WIDTH(32'd1),
+  ) other  (
+    .A({B, A}), // <- look here
+    .Y(Y)
+  );
+endmodule
+EOT
+# Unary is sorted
+opt_expr
+select -assert-count 2 t:$reduce_xor
+equiv_opt -assert opt_merge
+design -load postopt
+select -assert-count 1 t:$reduce_xor
+
+design -reset
+read_verilog -icells <<EOT
+module top(A, B, X, Y);
+input [7:0] A, B;
+output X, Y;
+  \$reduce_or  #(
+    .A_SIGNED(32'd0),
+    .A_WIDTH(32'd24),
+    .Y_WIDTH(32'd1),
+  ) one  (
+    .A({A, B, B}), // <- look here
+    .Y(X)
+  );
+  \$reduce_or  #(
+    .A_SIGNED(32'd0),
+    .A_WIDTH(32'd24),
+    .Y_WIDTH(32'd1),
+  ) other  (
+    .A({A, A, B}), // <- look here
+    .Y(Y)
+  );
+endmodule
+EOT
+# Unary is unified when valid
+opt_expr
+select -assert-count 2 t:$reduce_or
+equiv_opt -assert opt_merge
+design -load postopt
+select -assert-count 1 t:$reduce_or
+
+design -reset
+read_verilog -icells <<EOT
+module top(A, B, X, Y);
+input [7:0] A, B;
+output X, Y;
+  \$reduce_xor  #(
+    .A_SIGNED(32'd0),
+    .A_WIDTH(32'd24),
+    .Y_WIDTH(32'd1),
+  ) one  (
+    .A({A, B, B}), // <- look here
+    .Y(X)
+  );
+  \$reduce_xor  #(
+    .A_SIGNED(32'd0),
+    .A_WIDTH(32'd24),
+    .Y_WIDTH(32'd1),
+  ) other  (
+    .A({A, A, B}), // <- look here
+    .Y(Y)
+  );
+endmodule
+EOT
+# Unary isn't unified when that would be invalid
+opt_expr
+select -assert-count 2 t:$reduce_xor
+equiv_opt -assert opt_merge
+design -load postopt
+select -assert-count 2 t:$reduce_xor
+
+# TODO pmux
+
+design -reset
+read_verilog <<EOT
+module top(A, B, X, Y);
+input [7:0] A, B;
+output X, Y;
+assign X = A > B;
+assign Y = A > B;
+endmodule
+EOT
+# Exercise the general case in hash_cell_inputs - accept
+opt_expr
+select -assert-count 2 t:$gt
+equiv_opt -assert opt_merge
+design -load postopt
+select -assert-count 1 t:$gt
+
+design -reset
+read_verilog <<EOT
+module top(A, B, C, X, Y);
+input [7:0] A, B, C;
+output X, Y;
+assign X = A > B;
+assign Y = A > C; // <- look here
+endmodule
+EOT
+# Exercise the general case in hash_cell_inputs - reject
+opt_expr
+select -assert-count 2 t:$gt
+opt_merge
+select -assert-count 2 t:$gt