Merge ad7a776d73 into dc77140275

frontends/ast/genrtlil.cc

@@ -342,6 +342,9 @@ struct AST_INTERNAL::ProcessGenerator
 	// The most recently assigned $print or $check cell \PRIORITY.
 	int last_effect_priority;
 
+	// Track which signals have been assigned in current_case to avoid unnecessary removeSignalFromCaseTree calls
+	pool<RTLIL::SigBit> current_case_assigned_bits;
+
 	ProcessGenerator(std::unique_ptr<AstNode> a, RTLIL::SigSpec initSyncSignalsArg = RTLIL::SigSpec()) : always(std::move(a)), initSyncSignals(initSyncSignalsArg), last_effect_priority(0)
 	{
 		// rewrite lookahead references
@@ -430,6 +433,10 @@ struct AST_INTERNAL::ProcessGenerator
 			subst_rvalue_map = subst_lvalue_from.to_sigbit_dict(RTLIL::SigSpec(RTLIL::State::Sx, GetSize(subst_lvalue_from)));
 		} else {
 			addChunkActions(current_case->actions, subst_lvalue_to, subst_lvalue_from);
+			// Track initial assignments
+			for (auto &bit : subst_lvalue_to)
+				if (bit.wire != NULL)
+					current_case_assigned_bits.insert(bit);
 		}
 
 		// process the AST
@@ -557,14 +564,42 @@ struct AST_INTERNAL::ProcessGenerator
 	// e.g. when the last statement in the code "a = 23; if (b) a = 42; a = 0;" is processed this
 	// function is called to clean up the first two assignments as they are overwritten by
 	// the third assignment.
-	void removeSignalFromCaseTree(const RTLIL::SigSpec &pattern, RTLIL::CaseRule *cs)
+	void removeSignalFromCaseTree(const pool<RTLIL::SigBit> &pattern_bits, RTLIL::CaseRule *cs)
 	{
-		for (auto it = cs->actions.begin(); it != cs->actions.end(); it++)
-			it->first.remove2(pattern, &it->second);
+		// Optimization: check actions in reverse order and stop early if we've found all pattern bits
+		pool<RTLIL::SigBit> remaining_bits = pattern_bits;
+
+		for (auto it = cs->actions.rbegin(); it != cs->actions.rend(); ++it) {
+			bool has_pattern = false;
+			for (auto &bit : it->first) {
+				if (bit.wire != NULL && remaining_bits.count(bit)) {
+					has_pattern = true;
+					remaining_bits.erase(bit);
+				}
+			}
+
+			if (has_pattern) {
+				it->first.remove2(pattern_bits, &it->second);
+			}
+
+			// Early exit if we've processed all bits in pattern
+			if (remaining_bits.empty())
+				break;
+		}
 
 		for (auto it = cs->switches.begin(); it != cs->switches.end(); it++)
 			for (auto it2 = (*it)->cases.begin(); it2 != (*it)->cases.end(); it2++)
-				removeSignalFromCaseTree(pattern, *it2);
+				removeSignalFromCaseTree(pattern_bits, *it2);
+	}
+
+	void removeSignalFromCaseTree(const RTLIL::SigSpec &pattern, RTLIL::CaseRule *cs)
+	{
+		pool<RTLIL::SigBit> pattern_bits;
+		pattern_bits.reserve(pattern.size());
+		for (auto &bit : pattern)
+			if (bit.wire != NULL)
+				pattern_bits.insert(bit);
+		removeSignalFromCaseTree(pattern_bits, cs);
 	}
 
 	// add an assignment (aka "action") but split it up in chunks. this way huge assignments
@@ -623,7 +658,23 @@ struct AST_INTERNAL::ProcessGenerator
 						subst_rvalue_map.set(unmapped_lvalue[i], rvalue[i]);
 				}
 
-				removeSignalFromCaseTree(lvalue, current_case);
+				// Check if any bits in lvalue have been assigned before in current_case
+				bool has_overlap = false;
+				for (auto &bit : lvalue) {
+					if (bit.wire != NULL && current_case_assigned_bits.count(bit)) {
+						has_overlap = true;
+						break;
+					}
+				}
+
+				if (has_overlap)
+					removeSignalFromCaseTree(lvalue, current_case);
+
+				// Track newly assigned bits
+				for (auto &bit : lvalue)
+					if (bit.wire != NULL)
+						current_case_assigned_bits.insert(bit);
+
 				remove_unwanted_lvalue_bits(lvalue, rvalue);
 				current_case->actions.push_back(RTLIL::SigSig(lvalue, rvalue));
 			}
@@ -670,9 +721,15 @@ struct AST_INTERNAL::ProcessGenerator
 
 					RTLIL::CaseRule *backup_case = current_case;
 					current_case = new RTLIL::CaseRule;
+					pool<RTLIL::SigBit> backup_assigned_bits = std::move(current_case_assigned_bits);
+					current_case_assigned_bits.clear();
 					set_src_attr(current_case, child.get());
 					last_generated_case = current_case;
 					addChunkActions(current_case->actions, this_case_eq_ltemp, this_case_eq_rvalue);
+					// Track temp assignments
+					for (auto &bit : this_case_eq_ltemp)
+						if (bit.wire != NULL)
+							current_case_assigned_bits.insert(bit);
 					for (auto& node : child->children) {
 						if (node->type == AST_DEFAULT)
 							default_case = current_case;
@@ -686,6 +743,7 @@ struct AST_INTERNAL::ProcessGenerator
 					else
 						log_assert(current_case->compare.size() == 0);
 					current_case = backup_case;
+					current_case_assigned_bits = std::move(backup_assigned_bits);
 
 					subst_lvalue_map.restore();
 					subst_rvalue_map.restore();
@@ -714,8 +772,24 @@ struct AST_INTERNAL::ProcessGenerator
 					subst_rvalue_map.set(this_case_eq_lvalue[i], this_case_eq_ltemp[i]);
 
 				this_case_eq_lvalue.replace(subst_lvalue_map.stdmap());
-				removeSignalFromCaseTree(this_case_eq_lvalue, current_case);
+
+				// Check if any bits in lvalue have been assigned before in current_case
+				bool has_overlap = false;
+				for (auto &bit : this_case_eq_lvalue) {
+					if (bit.wire != NULL && current_case_assigned_bits.count(bit)) {
+						has_overlap = true;
+						break;
+					}
+				}
+
+				if (has_overlap)
+					removeSignalFromCaseTree(this_case_eq_lvalue, current_case);
+
 				addChunkActions(current_case->actions, this_case_eq_lvalue, this_case_eq_ltemp);
+				// Track newly assigned bits
+				for (auto &bit : this_case_eq_lvalue)
+					if (bit.wire != NULL)
+						current_case_assigned_bits.insert(bit);
 			}
 			break;
 

kernel/rtlil.cc

@@ -4990,31 +4990,35 @@ void RTLIL::SigSpec::remove2(const RTLIL::SigSpec &pattern, RTLIL::SigSpec *othe
 		other->unpack();
 	}
 
-	bool modified = false;
-	bool other_modified = false;
-	for (int i = GetSize(bits_) - 1; i >= 0; i--)
-	{
-		if (bits_[i].wire == NULL) continue;
+	//  Convert pattern to pool for O(1) lookup, avoiding O(n*m) chunk iteration
+	pool<SigBit> pattern_bits;
+	pattern_bits.reserve(pattern.size());
+	for (auto &bit : pattern)
+		if (bit.wire != NULL)
+			pattern_bits.insert(bit);
 
-		for (auto &pattern_chunk : pattern.chunks())
-			if (bits_[i].wire == pattern_chunk.wire &&
-				bits_[i].offset >= pattern_chunk.offset &&
-				bits_[i].offset < pattern_chunk.offset + pattern_chunk.width) {
-				modified = true;
-				bits_.erase(bits_.begin() + i);
-				if (other != NULL) {
-					other_modified = true;
-					other->bits_.erase(other->bits_.begin() + i);
-				}
-				break;
+	// Compact in-place to avoid O(n^2) erase operations
+	size_t write_idx = 0;
+	for (size_t read_idx = 0; read_idx < bits_.size(); read_idx++)
+	{
+		if (!(bits_[read_idx].wire != NULL && pattern_bits.count(bits_[read_idx]))) {
+			if (write_idx != read_idx) {
+				bits_[write_idx] = bits_[read_idx];
+				if (other != NULL)
+					other->bits_[write_idx] = other->bits_[read_idx];
 			}
+			write_idx++;
+		}
 	}
 
+	bool modified = (write_idx < bits_.size());
 	if (modified) {
+		bits_.resize(write_idx);
 		hash_.clear();
 		try_repack();
 	}
-	if (other_modified) {
+	if (other != NULL && modified) {
+		other->bits_.resize(write_idx);
 		other->hash_.clear();
 		other->try_repack();
 	}
@@ -5041,24 +5045,27 @@ void RTLIL::SigSpec::remove2(const pool<RTLIL::SigBit> &pattern, RTLIL::SigSpec
 		other->unpack();
 	}
 
-	bool modified = false;
-	bool other_modified = false;
-	for (int i = GetSize(bits_) - 1; i >= 0; i--) {
-		if (bits_[i].wire != NULL && pattern.count(bits_[i])) {
-			modified = true;
-			bits_.erase(bits_.begin() + i);
-			if (other != NULL) {
-				other_modified = true;
-				other->bits_.erase(other->bits_.begin() + i);
+	// Avoid O(n^2) complexity by compacting in-place
+	size_t write_idx = 0;
+	for (size_t read_idx = 0; read_idx < bits_.size(); read_idx++) {
+		if (!(bits_[read_idx].wire != NULL && pattern.count(bits_[read_idx]))) {
+			if (write_idx != read_idx) {
+				bits_[write_idx] = bits_[read_idx];
+				if (other != NULL)
+					other->bits_[write_idx] = other->bits_[read_idx];
 			}
+			write_idx++;
 		}
 	}
 
+	bool modified = (write_idx < bits_.size());
 	if (modified) {
+		bits_.resize(write_idx);
 		hash_.clear();
 		try_repack();
 	}
-	if (other_modified) {
+	if (other != NULL && modified) {
+		other->bits_.resize(write_idx);
 		other->hash_.clear();
 		other->try_repack();
 	}
@@ -5074,24 +5081,27 @@ void RTLIL::SigSpec::remove2(const std::set<RTLIL::SigBit> &pattern, RTLIL::SigS
 		other->unpack();
 	}
 
-	bool modified = false;
-	bool other_modified = false;
-	for (int i = GetSize(bits_) - 1; i >= 0; i--) {
-		if (bits_[i].wire != NULL && pattern.count(bits_[i])) {
-			modified = true;
-			bits_.erase(bits_.begin() + i);
-			if (other != NULL) {
-				other_modified = true;
-				other->bits_.erase(other->bits_.begin() + i);
+	// Avoid O(n^2) complexity by compacting in-place
+	size_t write_idx = 0;
+	for (size_t read_idx = 0; read_idx < bits_.size(); read_idx++) {
+		if (!(bits_[read_idx].wire != NULL && pattern.count(bits_[read_idx]))) {
+			if (write_idx != read_idx) {
+				bits_[write_idx] = bits_[read_idx];
+				if (other != NULL)
+					other->bits_[write_idx] = other->bits_[read_idx];
 			}
+			write_idx++;
 		}
 	}
 
+	bool modified = (write_idx < bits_.size());
 	if (modified) {
+		bits_.resize(write_idx);
 		hash_.clear();
 		try_repack();
 	}
-	if (other_modified) {
+	if (other != NULL && modified) {
+		other->bits_.resize(write_idx);
 		other->hash_.clear();
 		other->try_repack();
 	}

tests/unit/kernel/sigspecRemove2Test.cc

@@ -0,0 +1,333 @@
+#include <gtest/gtest.h>
+
+#include "kernel/rtlil.h"
+
+YOSYS_NAMESPACE_BEGIN
+
+// Test fixture with helper functions
+class SigSpecRemove2Test : public ::testing::Test {
+protected:
+	Design* d;
+	Module* m;
+
+	void SetUp() override {
+		d = new Design;
+		m = d->addModule("$test");
+	}
+
+	void TearDown() override {
+		delete d;
+	}
+
+	// Create n wires with given width
+	std::vector<Wire*> createWires(int count, int width = 4) {
+		std::vector<Wire*> wires;
+		for (int i = 0; i < count; i++) {
+			Wire* w = m->addWire(stringf("$w%d", i), width);
+			wires.push_back(w);
+		}
+		return wires;
+	}
+
+	// Append all wires to a SigSpec
+	SigSpec wiresAsSigSpec(const std::vector<Wire*>& wires) {
+		SigSpec sig;
+		for (auto w : wires)
+			sig.append(w);
+		return sig;
+	}
+
+	// Create a SigSpec of constants
+	SigSpec constsAsSigSpec(int count, int width = 4) {
+		SigSpec sig;
+		for (int i = 0; i < count; i++)
+			sig.append(Const(i, width));
+		return sig;
+	}
+
+	// Convert wires to pool of SigBits
+	pool<SigBit> wiresToPool(const std::vector<Wire*>& wires) {
+		pool<SigBit> pool;
+		for (auto w : wires)
+			for (auto &bit : SigSpec(w))
+				pool.insert(bit);
+		return pool;
+	}
+
+	// Convert wires to set of SigBits
+	std::set<SigBit> wiresToSet(const std::vector<Wire*>& wires) {
+		std::set<SigBit> set;
+		for (auto w : wires)
+			for (auto &bit : SigSpec(w))
+				set.insert(bit);
+		return set;
+	}
+};
+
+TEST_F(SigSpecRemove2Test, WithSigSpecPattern)
+{
+	auto wires = createWires(4);
+	SigSpec sig = wiresAsSigSpec(wires);
+	SigSpec pattern(wires[1]);  // Remove w2
+	SigSpec other = constsAsSigSpec(4);
+
+	EXPECT_EQ(sig.size(), 16);
+	sig.remove2(pattern, &other);
+	EXPECT_EQ(sig.size(), 12);
+	EXPECT_EQ(other.size(), 12);
+
+	// Verify correct wires remain (w1, w3, w4)
+	SigSpec expected;
+	expected.append(wires[0]);
+	expected.append(wires[2]);
+	expected.append(wires[3]);
+	EXPECT_EQ(sig, expected);
+}
+
+TEST_F(SigSpecRemove2Test, WithPoolPattern)
+{
+	auto wires = createWires(3);
+	SigSpec sig = wiresAsSigSpec(wires);
+	auto pattern = wiresToPool({wires[1]});
+	SigSpec other = constsAsSigSpec(3);
+
+	EXPECT_EQ(sig.size(), 12);
+	sig.remove2(pattern, &other);
+	EXPECT_EQ(sig.size(), 8);
+	EXPECT_EQ(other.size(), 8);
+
+	// Verify correct wires remain (w0, w2)
+	SigSpec expected;
+	expected.append(wires[0]);
+	expected.append(wires[2]);
+	EXPECT_EQ(sig, expected);
+}
+
+TEST_F(SigSpecRemove2Test, WithSetPattern)
+{
+	auto wires = createWires(3);
+	SigSpec sig = wiresAsSigSpec(wires);
+	auto pattern = wiresToSet({wires[1]});
+	SigSpec other = constsAsSigSpec(3);
+
+	EXPECT_EQ(sig.size(), 12);
+	sig.remove2(pattern, &other);
+	EXPECT_EQ(sig.size(), 8);
+	EXPECT_EQ(other.size(), 8);
+
+	// Verify correct wires remain (w0, w2)
+	SigSpec expected;
+	expected.append(wires[0]);
+	expected.append(wires[2]);
+	EXPECT_EQ(sig, expected);
+}
+
+TEST_F(SigSpecRemove2Test, ManyElements)
+{
+	const int num_wires = 100;
+	auto wires = createWires(num_wires);
+	SigSpec sig = wiresAsSigSpec(wires);
+
+	// Remove every other wire
+	std::vector<Wire*> to_remove;
+	for (int i = 0; i < num_wires; i += 2)
+		to_remove.push_back(wires[i]);
+	auto pattern = wiresToPool(to_remove);
+
+	EXPECT_EQ(sig.size(), num_wires * 4);
+	sig.remove2(pattern, nullptr);
+	EXPECT_EQ(sig.size(), (num_wires / 2) * 4);
+
+	// Verify odd-indexed wires remain (w1, w3, w5, ..., w99)
+	SigSpec expected;
+	for (int i = 1; i < num_wires; i += 2)
+		expected.append(wires[i]);
+	EXPECT_EQ(sig, expected);
+}
+
+// Test remove2 with very large dataset to check scaling
+TEST_F(SigSpecRemove2Test, VeryLargeScalingTest)
+{
+	const int num_wires = 50000;
+	auto wires = createWires(num_wires);
+	SigSpec sig = wiresAsSigSpec(wires);
+	SigSpec other = constsAsSigSpec(num_wires);
+
+	// Create pattern with many chunks (one per wire)
+	SigSpec pattern;
+	for (int i = 0; i < num_wires; i += 2)
+		pattern.append(wires[i]);
+
+	EXPECT_EQ(sig.size(), num_wires * 4);
+	sig.remove2(pattern, &other);
+	EXPECT_EQ(sig.size(), (num_wires / 2) * 4);
+	EXPECT_EQ(other.size(), (num_wires / 2) * 4);
+
+	// Spot-check: odd-indexed wires should remain at expected positions
+	for (int i = 0; i < num_wires / 2; i++) {
+		EXPECT_EQ(sig[i * 4 + 0].wire, wires[i * 2 + 1]);
+		EXPECT_EQ(sig[i * 4 + 1].wire, wires[i * 2 + 1]);
+		EXPECT_EQ(sig[i * 4 + 2].wire, wires[i * 2 + 1]);
+		EXPECT_EQ(sig[i * 4 + 3].wire, wires[i * 2 + 1]);
+	}
+}
+
+// Test multiple sequential removals (simulates removeSignalFromCaseTree)
+TEST_F(SigSpecRemove2Test, MultipleSequentialRemovals)
+{
+	const int num_wires = 512;
+	auto wires = createWires(num_wires);
+
+	// Create many actions, each with one wire
+	std::vector<SigSpec> actions;
+	for (auto w : wires)
+		actions.push_back(SigSpec(w));
+
+	// Remove half the wires from all actions
+	for (int i = 0; i < num_wires / 2; i++) {
+		SigSpec pattern(wires[i]);
+		for (auto &action : actions)
+			if (action.size() > 0)
+				action.remove2(pattern, nullptr);
+	}
+
+	// Verify correct actions were cleared
+	for (int i = 0; i < num_wires; i++) {
+		EXPECT_EQ(actions[i].size(), i < num_wires / 2 ? 0 : 4);
+	}
+
+	// Verify remaining actions contain the correct wires
+	for (int i = num_wires / 2; i < num_wires; i++) {
+		SigSpec expected(wires[i]);
+		EXPECT_EQ(actions[i], expected);
+	}
+}
+
+// Test remove2 with very large dataset to check scaling
+TEST_F(SigSpecRemove2Test, PoolOverloadLargeDataset)
+{
+	const int num_wires = 50000;
+	auto wires = createWires(num_wires, 1);
+	SigSpec sig = wiresAsSigSpec(wires);
+	SigSpec other = constsAsSigSpec(num_wires, 1);
+
+	// Remove half
+	pool<SigBit> pattern;
+	for (int i = 0; i < num_wires; i += 2)
+		pattern.insert(SigBit(wires[i], 0));
+
+	EXPECT_EQ(sig.size(), num_wires);
+	sig.remove2(pattern, &other);
+	EXPECT_EQ(sig.size(), num_wires / 2);
+	EXPECT_EQ(other.size(), num_wires / 2);
+
+	// Spot-check: odd-indexed wires should remain
+	for (int i = 0; i < num_wires / 2; i++) {
+		EXPECT_EQ(sig[i].wire, wires[i * 2 + 1]);
+	}
+}
+
+// Test set overload (same perf characteristics as pool)
+TEST_F(SigSpecRemove2Test, SetOverloadLargeDataset)
+{
+	const int num_wires = 50000;
+	auto wires = createWires(num_wires, 1);
+	SigSpec sig = wiresAsSigSpec(wires);
+	SigSpec other = constsAsSigSpec(num_wires, 1);
+
+	// Remove half
+	std::set<SigBit> pattern;
+	for (int i = 0; i < num_wires; i += 2)
+		pattern.insert(SigBit(wires[i], 0));
+
+	EXPECT_EQ(sig.size(), num_wires);
+	sig.remove2(pattern, &other);
+	EXPECT_EQ(sig.size(), num_wires / 2);
+	EXPECT_EQ(other.size(), num_wires / 2);
+
+	// Spot-check: odd-indexed wires should remain
+	for (int i = 0; i < num_wires / 2; i++) {
+		EXPECT_EQ(sig[i].wire, wires[i * 2 + 1]);
+	}
+}
+
+// Worst case: remove almost all elements
+TEST_F(SigSpecRemove2Test, RemoveAlmostAllElements)
+{
+	const int num_wires = 10000;
+	auto wires = createWires(num_wires, 1);
+	SigSpec sig = wiresAsSigSpec(wires);
+
+	// Remove all but last
+	pool<SigBit> pattern;
+	for (int i = 0; i < num_wires - 1; i++)
+		pattern.insert(SigBit(wires[i], 0));
+
+	EXPECT_EQ(sig.size(), num_wires);
+	sig.remove2(pattern, nullptr);
+	EXPECT_EQ(sig.size(), 1);
+	EXPECT_EQ(sig[0].wire, wires[num_wires - 1]);
+}
+
+TEST_F(SigSpecRemove2Test, EmptyPattern)
+{
+	auto wires = createWires(1);
+	SigSpec sig(wires[0]);
+	pool<SigBit> empty_pattern;
+
+	EXPECT_EQ(sig.size(), 4);
+	sig.remove2(empty_pattern, nullptr);
+	EXPECT_EQ(sig.size(), 4);
+
+	// Verify the wire is unchanged
+	SigSpec expected(wires[0]);
+	EXPECT_EQ(sig, expected);
+}
+
+// Test that NULL wire bits (constants) are not removed
+TEST_F(SigSpecRemove2Test, NullWireBitsStay)
+{
+	auto wires = createWires(1);
+	SigSpec sig;
+	sig.append(wires[0]);
+	sig.append(Const(15, 4));
+
+	// Try to remove the constants
+	pool<SigBit> pattern;
+	SigSpec const_spec(Const(15, 4));
+	for (auto &bit : const_spec)
+		pattern.insert(bit);
+
+	EXPECT_EQ(sig.size(), 8);
+	sig.remove2(pattern, nullptr);
+	EXPECT_EQ(sig.size(), 8);  // Constants stay
+
+	// Verify original content is preserved
+	SigSpec expected;
+	expected.append(wires[0]);
+	expected.append(Const(15, 4));
+	EXPECT_EQ(sig, expected);
+}
+
+TEST_F(SigSpecRemove2Test, PartialBitRemoval)
+{
+	Wire* w = m->addWire("$w1", 8);
+	SigSpec sig(w);
+
+	// Remove bits 2-5
+	pool<SigBit> pattern;
+	for (int i = 2; i < 6; i++)
+		pattern.insert(SigBit(w, i));
+
+	EXPECT_EQ(sig.size(), 8);
+	sig.remove2(pattern, nullptr);
+	EXPECT_EQ(sig.size(), 4);
+
+	// Verify only bits 0,1,6,7 remain
+	EXPECT_EQ(sig[0], SigBit(w, 0));
+	EXPECT_EQ(sig[1], SigBit(w, 1));
+	EXPECT_EQ(sig[2], SigBit(w, 6));
+	EXPECT_EQ(sig[3], SigBit(w, 7));
+}
+
+YOSYS_NAMESPACE_END