From e49f9765a2c0eb86a4977e4ceb6ca70c31dca153 Mon Sep 17 00:00:00 2001 From: Robert O'Callahan Date: Mon, 1 Sep 2025 04:49:29 +0000 Subject: [PATCH] Make `SigSpecConstIterator` iterate over SigSpec without unpacking To make this efficient, the iterator keeps `chunk_index_hint` pointing to the current chunk. This is only a hint, since it is not possible to maintain its validity if the `SigSpec` representation is temporarily changed to unpacked. In that case we have to resync using binary search. --- kernel/rtlil.cc | 17 +++++++++ kernel/rtlil.h | 66 ++++++++++++++++++++++++++++------ tests/unit/kernel/rtlilTest.cc | 32 +++++++++++++++++ 3 files changed, 105 insertions(+), 10 deletions(-) diff --git a/kernel/rtlil.cc b/kernel/rtlil.cc index 2777e5891..3212bf2e0 100644 --- a/kernel/rtlil.cc +++ b/kernel/rtlil.cc @@ -4447,6 +4447,23 @@ bool RTLIL::SigChunk::operator !=(const RTLIL::SigChunk &other) const return true; } +const RTLIL::SigChunk &RTLIL::SigSpecConstIterator::find_chunk() +{ + int low = 0; + int high = GetSize(sig_p->chunks_); + while (high - low >= 2) { + int mid = (low + high) / 2; + if (sig_p->chunks_[mid].offset_in_sigspec <= bit_index) + low = mid; + else + high = mid; + } + chunk_index_hint = low; + const RTLIL::SigChunk &chunk = sig_p->chunks_[chunk_index_hint]; + log_assert(chunk.offset_in_sigspec <= bit_index && bit_index < chunk.offset_in_sigspec + chunk.width); + return chunk; +} + RTLIL::SigSpec::SigSpec(std::initializer_list parts) { cover("kernel.rtlil.sigspec.init.list"); diff --git a/kernel/rtlil.h b/kernel/rtlil.h index 351e55ff9..0dc5e07bc 100644 --- a/kernel/rtlil.h +++ b/kernel/rtlil.h @@ -1217,18 +1217,29 @@ struct RTLIL::SigSpecIterator struct RTLIL::SigSpecConstIterator { typedef std::input_iterator_tag iterator_category; - typedef RTLIL::SigBit value_type; + typedef const RTLIL::SigBit& value_type; typedef ptrdiff_t difference_type; typedef RTLIL::SigBit* pointer; typedef RTLIL::SigBit& reference; const RTLIL::SigSpec *sig_p; - int index; + SigBit bit; + int bit_index; + // Index of chunk containing bit_index, or GetSize(chunks_) if bit_index == width_. + // or 0 if *sig_p is not packed. This is a hint and may be incorrect in unusual + // circumstances, e.g. when the SigSpec changes from unpacked to packed + // during iteration. + int chunk_index_hint; - inline const RTLIL::SigBit &operator*() const; - inline bool operator!=(const RTLIL::SigSpecConstIterator &other) const { return index != other.index; } - inline bool operator==(const RTLIL::SigSpecIterator &other) const { return index == other.index; } - inline void operator++() { index++; } + inline const RTLIL::SigBit &operator*(); + inline bool operator!=(const RTLIL::SigSpecConstIterator &other) const { return !(*this == other); } + inline bool operator==(const RTLIL::SigSpecConstIterator &other) const { return bit_index == other.bit_index; } + inline void operator++(); + +private: + // Must be called when sig_p is packed and `bit_index` is in range. Finds the chunk containing `bit_index` + // and sets chunk_index_hint. + const RTLIL::SigChunk &find_chunk(); }; struct RTLIL::SigSpec @@ -1255,6 +1266,7 @@ private: // Only used by Module::remove(const pool &wires) // but cannot be more specific as it isn't yet declared friend struct RTLIL::Module; + friend struct RTLIL::SigSpecConstIterator; public: SigSpec() : width_(0), hash_(0) {} @@ -1292,8 +1304,20 @@ public: inline RTLIL::SigSpecIterator begin() { RTLIL::SigSpecIterator it; it.sig_p = this; it.index = 0; return it; } inline RTLIL::SigSpecIterator end() { RTLIL::SigSpecIterator it; it.sig_p = this; it.index = width_; return it; } - inline RTLIL::SigSpecConstIterator begin() const { RTLIL::SigSpecConstIterator it; it.sig_p = this; it.index = 0; return it; } - inline RTLIL::SigSpecConstIterator end() const { RTLIL::SigSpecConstIterator it; it.sig_p = this; it.index = width_; return it; } + inline RTLIL::SigSpecConstIterator begin() const { + RTLIL::SigSpecConstIterator it; + it.sig_p = this; + it.bit_index = 0; + it.chunk_index_hint = 0; + return it; + } + inline RTLIL::SigSpecConstIterator end() const { + RTLIL::SigSpecConstIterator it; + it.sig_p = this; + it.bit_index = width_; + it.chunk_index_hint = GetSize(chunks_); + return it; + } void sort(); void sort_and_unify(); @@ -2313,8 +2337,30 @@ inline RTLIL::SigBit &RTLIL::SigSpecIterator::operator*() const { return (*sig_p)[index]; } -inline const RTLIL::SigBit &RTLIL::SigSpecConstIterator::operator*() const { - return (*sig_p)[index]; +inline const RTLIL::SigBit &RTLIL::SigSpecConstIterator::operator*() { + if (!sig_p->packed()) + return sig_p->bits_.at(bit_index); + if (chunk_index_hint < GetSize(sig_p->chunks_)) { + const SigChunk &chunk = sig_p->chunks_[chunk_index_hint]; + if (chunk.offset_in_sigspec <= bit_index && bit_index < chunk.offset_in_sigspec + chunk.width) { + bit = chunk[bit_index - chunk.offset_in_sigspec]; + return bit; + } + } + const SigChunk &chunk = find_chunk(); + bit = chunk[bit_index - chunk.offset_in_sigspec]; + return bit; +} + +inline void RTLIL::SigSpecConstIterator::operator++() { + ++bit_index; + if (!sig_p->packed()) + return; + if (chunk_index_hint < GetSize(sig_p->chunks_)) { + const SigChunk &chunk = sig_p->chunks_[chunk_index_hint]; + if (chunk.offset_in_sigspec + chunk.width == bit_index) + ++chunk_index_hint; + } } inline RTLIL::SigBit::SigBit(const RTLIL::SigSpec &sig) { diff --git a/tests/unit/kernel/rtlilTest.cc b/tests/unit/kernel/rtlilTest.cc index 557355ed9..83f6aa061 100644 --- a/tests/unit/kernel/rtlilTest.cc +++ b/tests/unit/kernel/rtlilTest.cc @@ -361,6 +361,38 @@ namespace RTLIL { EXPECT_FALSE(Const().is_onehot(&pos)); } + TEST_F(KernelRtlilTest, SigSpecConstIteratorRepresentationChange) { + std::unique_ptr mod = std::make_unique(); + std::vector wires; + SigSpec spec; + for (int i = 0; i < 10; i++) { + wires.push_back(mod->addWire(IdString(stringf("\\test%d", i)), 10)); + spec.append(wires.back()); + } + + const SigSpec &const_spec = spec; + SigSpecConstIterator it = const_spec.begin(); + for (int i = 0; i < 55; ++i) + ++it; + EXPECT_EQ(*it, SigBit(wires[5], 5)); + + // Force unpacking of the spec. + EXPECT_EQ(spec[55], SigBit(wires[5], 5)); + // Make sure the iterator is still OK + EXPECT_EQ(*it, SigBit(wires[5], 5)); + + // Advance iterator while unpacked + for (int i = 0; i < 20; ++i) + ++it; + EXPECT_EQ(*it, SigBit(wires[7], 5)); + + // Force packing of the spec. + SigSpec other = spec; + EXPECT_FALSE(spec < other); + // Make sure iterator is still OK + EXPECT_EQ(*it, SigBit(wires[7], 5)); + } + class WireRtlVsHdlIndexConversionTest : public KernelRtlilTest, public testing::WithParamInterface>