diff --git a/frontends/ast/genrtlil.cc b/frontends/ast/genrtlil.cc index 65af26132..5e0356fd3 100644 --- a/frontends/ast/genrtlil.cc +++ b/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 current_case_assigned_bits; + ProcessGenerator(std::unique_ptr 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 &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 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 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 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; diff --git a/kernel/rtlil.cc b/kernel/rtlil.cc index 279c9b0e6..2badd20c3 100644 --- a/kernel/rtlil.cc +++ b/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 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 &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 &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(); } diff --git a/tests/unit/kernel/sigspecRemove2Test.cc b/tests/unit/kernel/sigspecRemove2Test.cc new file mode 100644 index 000000000..a6789c774 --- /dev/null +++ b/tests/unit/kernel/sigspecRemove2Test.cc @@ -0,0 +1,333 @@ +#include + +#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 createWires(int count, int width = 4) { + std::vector 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& 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 wiresToPool(const std::vector& wires) { + pool pool; + for (auto w : wires) + for (auto &bit : SigSpec(w)) + pool.insert(bit); + return pool; + } + + // Convert wires to set of SigBits + std::set wiresToSet(const std::vector& wires) { + std::set 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 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 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 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 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 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 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 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 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