From 7f9de6e48fcbbf359992423ad1c93dbe37d65584 Mon Sep 17 00:00:00 2001 From: Robert O'Callahan Date: Tue, 25 Nov 2025 02:19:51 +0000 Subject: [PATCH 1/5] Make coverage data thread-safe --- kernel/log.cc | 2 +- kernel/log.h | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/kernel/log.cc b/kernel/log.cc index 34e56f8ac..d712eda2c 100644 --- a/kernel/log.cc +++ b/kernel/log.cc @@ -720,7 +720,7 @@ dict> get_coverage_data() if (coverage_data.count(p->id)) log_warning("found duplicate coverage id \"%s\".\n", p->id); coverage_data[p->id].first = stringf("%s:%d:%s", p->file, p->line, p->func); - coverage_data[p->id].second += p->counter; + coverage_data[p->id].second += p->counter.load(std::memory_order_relaxed); } for (auto &it : coverage_data) diff --git a/kernel/log.h b/kernel/log.h index 144570026..197cfab8d 100644 --- a/kernel/log.h +++ b/kernel/log.h @@ -24,6 +24,7 @@ #include +#include #include #define YS_REGEX_COMPILE(param) std::regex(param, \ std::regex_constants::nosubs | \ @@ -298,15 +299,16 @@ void log_abort_internal(const char *file, int line); #define cover(_id) do { \ static CoverData __d __attribute__((section("yosys_cover_list"), aligned(1), used)) = { __FILE__, __FUNCTION__, _id, __LINE__, 0 }; \ - __d.counter++; \ + __d.counter.fetch_add(1, std::memory_order_relaxed); \ } while (0) struct CoverData { const char *file, *func, *id; - int line, counter; -} YS_ATTRIBUTE(packed); + int line; + std::atomic counter; +}; -// this two symbols are created by the linker for the "yosys_cover_list" ELF section +// this two symbols are created by the linker __start_yosys_cover_listfor the "yosys_cover_list" ELF section extern "C" struct CoverData __start_yosys_cover_list[]; extern "C" struct CoverData __stop_yosys_cover_list[]; From 4c8b537d7194bbb300c23162d07b9cf25b8d7447 Mon Sep 17 00:00:00 2001 From: Robert O'Callahan Date: Fri, 14 Nov 2025 01:52:22 +0000 Subject: [PATCH 2/5] Remove YOSYS_NO_IDS_REFCNT Refcounting is hardly used at all so this option is not that useful. We might want to have a different option that disables GC if that becomes a performance issue, but that should be a different option. --- kernel/rtlil.cc | 9 --------- kernel/rtlil.h | 6 ------ 2 files changed, 15 deletions(-) diff --git a/kernel/rtlil.cc b/kernel/rtlil.cc index d18a709c9..dc851c019 100644 --- a/kernel/rtlil.cc +++ b/kernel/rtlil.cc @@ -40,10 +40,8 @@ std::vector RTLIL::IdString::global_id_storage_; std::unordered_map RTLIL::IdString::global_id_index_; std::unordered_map RTLIL::IdString::global_autoidx_id_prefix_storage_; std::unordered_map RTLIL::IdString::global_autoidx_id_storage_; -#ifndef YOSYS_NO_IDS_REFCNT std::unordered_map RTLIL::IdString::global_refcount_storage_; std::vector RTLIL::IdString::global_free_idx_list_; -#endif static void populate(std::string_view name) { @@ -104,7 +102,6 @@ int RTLIL::IdString::really_insert(std::string_view p, std::unordered_map(malloc(p.size() + 1)); memcpy(buf, p.data(), p.size()); buf[p.size()] = 0; @@ -249,7 +242,6 @@ int RTLIL::OwningIdString::gc_count; void RTLIL::OwningIdString::collect_garbage() { int64_t start = PerformanceTimer::query(); -#ifndef YOSYS_NO_IDS_REFCNT IdStringCollector collector; for (auto &[idx, design] : *RTLIL::Design::get_all_designs()) { collector.trace(*design); @@ -291,7 +283,6 @@ void RTLIL::OwningIdString::collect_garbage() } it = global_autoidx_id_prefix_storage_.erase(it); } -#endif int64_t time_ns = PerformanceTimer::query() - start; Pass::subtract_from_current_runtime_ns(time_ns); gc_ns += time_ns; diff --git a/kernel/rtlil.h b/kernel/rtlil.h index 4da030e8d..a674d87e9 100644 --- a/kernel/rtlil.h +++ b/kernel/rtlil.h @@ -153,11 +153,9 @@ struct RTLIL::IdString static std::unordered_map global_autoidx_id_prefix_storage_; // Explicit string storage for autoidx IDs static std::unordered_map global_autoidx_id_storage_; -#ifndef YOSYS_NO_IDS_REFCNT // All (index, refcount) pairs in this map have refcount > 0. static std::unordered_map global_refcount_storage_; static std::vector global_free_idx_list_; -#endif static int refcount(int idx) { auto it = global_refcount_storage_.find(idx); @@ -597,7 +595,6 @@ private: } static void get_reference(int idx) { - #ifndef YOSYS_NO_IDS_REFCNT if (idx < static_cast(StaticId::STATIC_ID_END)) return; auto it = global_refcount_storage_.find(idx); @@ -605,7 +602,6 @@ private: global_refcount_storage_.insert(it, {idx, 1}); else ++it->second; - #endif #ifdef YOSYS_XTRACE_GET_PUT if (yosys_xtrace && idx >= static_cast(StaticId::STATIC_ID_END)) log("#X# GET-BY-INDEX '%s' (index %d, refcount %u)\n", from_index(idx), idx, refcount(idx)); @@ -614,7 +610,6 @@ private: void put_reference() { - #ifndef YOSYS_NO_IDS_REFCNT // put_reference() may be called from destructors after the destructor of // global_refcount_storage_ has been run. in this case we simply do nothing. if (index_ < static_cast(StaticId::STATIC_ID_END) || !destruct_guard_ok) @@ -628,7 +623,6 @@ private: if (--it->second == 0) { global_refcount_storage_.erase(it); } - #endif } }; From 8f0ecce53f4293e324dfe6cefbed26b6c86d6ade Mon Sep 17 00:00:00 2001 From: Robert O'Callahan Date: Mon, 24 Nov 2025 22:29:06 +0000 Subject: [PATCH 3/5] Forbid creating IdStrings and incrementing autoidx during multithreaded phases, and add dynamic checks for that We could make it safe to increment autoidx during multithreaded passes, but that's actually undesirable because it would lead to nondeterminism. If/when we need new IDs during parallel passes, we'll have to figure out how to allocate them in a deterministic way, and that will depend on the details of what the pass does. So don't try to tackle that now. --- frontends/blif/blifparse.cc | 2 +- kernel/rtlil.cc | 2 +- kernel/rtlil.h | 6 ++++++ kernel/yosys.cc | 24 +++++++++++++++++++++++- kernel/yosys_common.h | 25 ++++++++++++++++++++++++- 5 files changed, 55 insertions(+), 4 deletions(-) diff --git a/frontends/blif/blifparse.cc b/frontends/blif/blifparse.cc index 0aa3a173d..bff347ea2 100644 --- a/frontends/blif/blifparse.cc +++ b/frontends/blif/blifparse.cc @@ -245,7 +245,7 @@ void parse_blif(RTLIL::Design *design, std::istream &f, IdString dff_name, bool if (undef_wire != nullptr) module->rename(undef_wire, stringf("$undef$%d", ++blif_maxnum)); - autoidx = std::max(autoidx, blif_maxnum+1); + autoidx.ensure_at_least(blif_maxnum+1); blif_maxnum = 0; } diff --git a/kernel/rtlil.cc b/kernel/rtlil.cc index dc851c019..9477b04a8 100644 --- a/kernel/rtlil.cc +++ b/kernel/rtlil.cc @@ -98,7 +98,7 @@ int RTLIL::IdString::really_insert(std::string_view p, std::unordered_map(StaticId::STATIC_ID_END)) return; auto it = global_refcount_storage_.find(idx); @@ -610,6 +614,8 @@ private: void put_reference() { + log_assert(!Multithreading::active()); + // put_reference() may be called from destructors after the destructor of // global_refcount_storage_ has been run. in this case we simply do nothing. if (index_ < static_cast(StaticId::STATIC_ID_END) || !destruct_guard_ok) diff --git a/kernel/yosys.cc b/kernel/yosys.cc index 009da56b9..2c9b8304d 100644 --- a/kernel/yosys.cc +++ b/kernel/yosys.cc @@ -19,6 +19,7 @@ #include "kernel/yosys.h" #include "kernel/celltypes.h" +#include "kernel/log.h" #ifdef YOSYS_ENABLE_READLINE # include @@ -80,7 +81,7 @@ extern "C" PyObject* PyInit_pyosys(); YOSYS_NAMESPACE_BEGIN -int autoidx = 1; +Autoidx autoidx(1); int yosys_xtrace = 0; bool yosys_write_versions = true; const char* yosys_maybe_version() { @@ -108,9 +109,30 @@ uint32_t Hasher::fudge = 0; std::string yosys_share_dirname; std::string yosys_abc_executable; +bool Multithreading::active_ = false; + void init_share_dirname(); void init_abc_executable_name(); +Multithreading::Multithreading() { + log_assert(!active_); + active_ = true; +} + +Multithreading::~Multithreading() { + log_assert(active_); + active_ = false; +} + +void Autoidx::ensure_at_least(int v) { + value = std::max(value, v); +} + +int Autoidx::operator++(int) { + log_assert(!Multithreading::active()); + return value++; +} + void memhasher_on() { #if defined(__linux__) || defined(__FreeBSD__) diff --git a/kernel/yosys_common.h b/kernel/yosys_common.h index 2374182ee..55e7b71eb 100644 --- a/kernel/yosys_common.h +++ b/kernel/yosys_common.h @@ -267,7 +267,30 @@ int ceil_log2(int x) YS_ATTRIBUTE(const); template int GetSize(const T &obj) { return obj.size(); } inline int GetSize(RTLIL::Wire *wire); -extern int autoidx; +// When multiple threads are accessing RTLIL, one of these guard objects +// must exist. +struct Multithreading +{ + Multithreading(); + ~Multithreading(); + // Returns true when multiple threads are accessing RTLIL. + // autoidx cannot be used during such times. + // IdStrings cannot be created during such times. + static bool active() { return active_; } +private: + static bool active_; +}; + +struct Autoidx { + Autoidx(int value) : value(value) {} + operator int() const { return value; } + void ensure_at_least(int v); + int operator++(int); +private: + int value; +}; + +extern Autoidx autoidx; extern int yosys_xtrace; extern bool yosys_write_versions; From 948001f39f016de043e8cb59165015347b1a34bc Mon Sep 17 00:00:00 2001 From: Robert O'Callahan Date: Tue, 25 Nov 2025 00:47:11 +0000 Subject: [PATCH 4/5] Merge the two autoidx hashtables into one When something calls `IdString::c_str()` on an autoidx ID, we need to cache the full string in a thread-safe way. If we need to allocate an entry in some data structure to do that, it's difficult to do in a thread-safe no-performance-hazard way. So instead, store the cached string pointer in the same hashtable as the prefix pointer. --- kernel/rtlil.cc | 19 ++++++++----------- kernel/rtlil.h | 36 ++++++++++++++++++++---------------- 2 files changed, 28 insertions(+), 27 deletions(-) diff --git a/kernel/rtlil.cc b/kernel/rtlil.cc index 9477b04a8..6077237ac 100644 --- a/kernel/rtlil.cc +++ b/kernel/rtlil.cc @@ -38,8 +38,7 @@ bool RTLIL::IdString::destruct_guard_ok = false; RTLIL::IdString::destruct_guard_t RTLIL::IdString::destruct_guard; std::vector RTLIL::IdString::global_id_storage_; std::unordered_map RTLIL::IdString::global_id_index_; -std::unordered_map RTLIL::IdString::global_autoidx_id_prefix_storage_; -std::unordered_map RTLIL::IdString::global_autoidx_id_storage_; +std::unordered_map RTLIL::IdString::global_autoidx_id_storage_; std::unordered_map RTLIL::IdString::global_refcount_storage_; std::vector RTLIL::IdString::global_free_idx_list_; @@ -93,8 +92,9 @@ int RTLIL::IdString::really_insert(std::string_view p, std::unordered_map p_autoidx = parse_autoidx(p.substr(autoidx_pos)); if (p_autoidx.has_value()) { - auto prefix_it = global_autoidx_id_prefix_storage_.find(-*p_autoidx); - if (prefix_it != global_autoidx_id_prefix_storage_.end() && p.substr(0, autoidx_pos) == *prefix_it->second) + auto autoidx_it = global_autoidx_id_storage_.find(-*p_autoidx); + if (autoidx_it != global_autoidx_id_storage_.end() && + p.substr(0, autoidx_pos) == *autoidx_it->second.prefix) return -*p_autoidx; // Ensure NEW_ID/NEW_ID_SUFFIX will not create collisions with the ID // we're about to create. @@ -267,7 +267,7 @@ void RTLIL::OwningIdString::collect_garbage() global_free_idx_list_.push_back(i); } - for (auto it = global_autoidx_id_prefix_storage_.begin(); it != global_autoidx_id_prefix_storage_.end();) { + for (auto it = global_autoidx_id_storage_.begin(); it != global_autoidx_id_storage_.end();) { if (collector.live.find(it->first) != collector.live.end()) { ++it; continue; @@ -276,13 +276,10 @@ void RTLIL::OwningIdString::collect_garbage() ++it; continue; } - auto str_it = global_autoidx_id_storage_.find(it->first); - if (str_it != global_autoidx_id_storage_.end()) { - delete[] str_it->second; - global_autoidx_id_storage_.erase(str_it); - } - it = global_autoidx_id_prefix_storage_.erase(it); + delete[] it->second.full_str; + it = global_autoidx_id_storage_.erase(it); } + int64_t time_ns = PerformanceTimer::query() - start; Pass::subtract_from_current_runtime_ns(time_ns); gc_ns += time_ns; diff --git a/kernel/rtlil.h b/kernel/rtlil.h index 29eac294f..06c38c52e 100644 --- a/kernel/rtlil.h +++ b/kernel/rtlil.h @@ -134,6 +134,13 @@ struct RTLIL::IdString std::string_view str_view() const { return {buf, static_cast(size)}; } }; + struct AutoidxStorage { + // Append the negated (i.e. positive) ID to this string to get + // the real string. The prefix strings must live forever. + const std::string *prefix; + // Cache of the full string, or nullptr if not cached yet. + char *full_str; + }; // the global id string cache @@ -147,12 +154,9 @@ struct RTLIL::IdString static std::vector global_id_storage_; // Lookup table for non-autoidx IDs static std::unordered_map global_id_index_; - // Shared prefix string storage for autoidx IDs, which have negative - // indices. Append the negated (i.e. positive) ID to this string to get - // the real string. The prefix strings must live forever. - static std::unordered_map global_autoidx_id_prefix_storage_; - // Explicit string storage for autoidx IDs - static std::unordered_map global_autoidx_id_storage_; + // Storage for autoidx IDs, which have negative indices, i.e. all entries in this + // map have negative keys. + static std::unordered_map global_autoidx_id_storage_; // All (index, refcount) pairs in this map have refcount > 0. static std::unordered_map global_refcount_storage_; static std::vector global_free_idx_list_; @@ -205,7 +209,7 @@ struct RTLIL::IdString static IdString new_autoidx_with_prefix(const std::string *prefix) { log_assert(!Multithreading::active()); int index = -(autoidx++); - global_autoidx_id_prefix_storage_.insert({index, prefix}); + global_autoidx_id_storage_.insert({index, {prefix, nullptr}}); return from_index(index); } @@ -238,16 +242,16 @@ struct RTLIL::IdString inline const char *c_str() const { if (index_ >= 0) return global_id_storage_.at(index_).buf; - auto it = global_autoidx_id_storage_.find(index_); - if (it != global_autoidx_id_storage_.end()) - return it->second; - const std::string &prefix = *global_autoidx_id_prefix_storage_.at(index_); + AutoidxStorage &s = global_autoidx_id_storage_.at(index_); + if (s.full_str != nullptr) + return s.full_str; + const std::string &prefix = *s.prefix; std::string suffix = std::to_string(-index_); char *c = new char[prefix.size() + suffix.size() + 1]; memcpy(c, prefix.data(), prefix.size()); memcpy(c + prefix.size(), suffix.c_str(), suffix.size() + 1); - global_autoidx_id_storage_.insert(it, {index_, c}); + s.full_str = c; return c; } @@ -262,7 +266,7 @@ struct RTLIL::IdString *out += global_id_storage_.at(index_).str_view(); return; } - *out += *global_autoidx_id_prefix_storage_.at(index_); + *out += *global_autoidx_id_storage_.at(index_).prefix; *out += std::to_string(-index_); } @@ -348,7 +352,7 @@ struct RTLIL::IdString if (index_ >= 0) { return const_iterator(global_id_storage_.at(index_)); } - return const_iterator(global_autoidx_id_prefix_storage_.at(index_), -index_); + return const_iterator(global_autoidx_id_storage_.at(index_).prefix, -index_); } const_iterator end() const { return const_iterator(); @@ -358,7 +362,7 @@ struct RTLIL::IdString if (index_ >= 0) { return Substrings(global_id_storage_.at(index_)); } - return Substrings(global_autoidx_id_prefix_storage_.at(index_), -index_); + return Substrings(global_autoidx_id_storage_.at(index_).prefix, -index_); } inline bool lt_by_name(const IdString &rhs) const { @@ -411,7 +415,7 @@ struct RTLIL::IdString #endif return *(storage.buf + i); } - const std::string &id_start = *global_autoidx_id_prefix_storage_.at(index_); + const std::string &id_start = *global_autoidx_id_storage_.at(index_).prefix; if (i < id_start.size()) return id_start[i]; i -= id_start.size(); From b23dc345ae915f5ffad10fc8ebbdd3668dc6c849 Mon Sep 17 00:00:00 2001 From: Robert O'Callahan Date: Tue, 25 Nov 2025 01:02:01 +0000 Subject: [PATCH 5/5] Make it safe to access .c_str() for autoidx IDs in a multithreaded context --- kernel/rtlil.cc | 1 - kernel/rtlil.h | 19 +++++++++++++------ pyosys/generator.py | 3 +-- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/kernel/rtlil.cc b/kernel/rtlil.cc index 6077237ac..6960b7620 100644 --- a/kernel/rtlil.cc +++ b/kernel/rtlil.cc @@ -276,7 +276,6 @@ void RTLIL::OwningIdString::collect_garbage() ++it; continue; } - delete[] it->second.full_str; it = global_autoidx_id_storage_.erase(it); } diff --git a/kernel/rtlil.h b/kernel/rtlil.h index 06c38c52e..f841df1ed 100644 --- a/kernel/rtlil.h +++ b/kernel/rtlil.h @@ -139,7 +139,11 @@ struct RTLIL::IdString // the real string. The prefix strings must live forever. const std::string *prefix; // Cache of the full string, or nullptr if not cached yet. - char *full_str; + std::atomic full_str; + + AutoidxStorage(const std::string *prefix) : prefix(prefix), full_str(nullptr) {} + AutoidxStorage(AutoidxStorage&& other) : prefix(other.prefix), full_str(other.full_str.exchange(nullptr, std::memory_order_relaxed)) {} + ~AutoidxStorage() { delete[] full_str.load(std::memory_order_acquire); } }; // the global id string cache @@ -209,7 +213,7 @@ struct RTLIL::IdString static IdString new_autoidx_with_prefix(const std::string *prefix) { log_assert(!Multithreading::active()); int index = -(autoidx++); - global_autoidx_id_storage_.insert({index, {prefix, nullptr}}); + global_autoidx_id_storage_.insert({index, prefix}); return from_index(index); } @@ -244,15 +248,18 @@ struct RTLIL::IdString return global_id_storage_.at(index_).buf; AutoidxStorage &s = global_autoidx_id_storage_.at(index_); - if (s.full_str != nullptr) - return s.full_str; + char *full_str = s.full_str.load(std::memory_order_acquire); + if (full_str != nullptr) + return full_str; const std::string &prefix = *s.prefix; std::string suffix = std::to_string(-index_); char *c = new char[prefix.size() + suffix.size() + 1]; memcpy(c, prefix.data(), prefix.size()); memcpy(c + prefix.size(), suffix.c_str(), suffix.size() + 1); - s.full_str = c; - return c; + if (s.full_str.compare_exchange_strong(full_str, c, std::memory_order_acq_rel)) + return c; + delete[] c; + return full_str; } inline std::string str() const { diff --git a/pyosys/generator.py b/pyosys/generator.py index 25f87d570..7d4293abd 100644 --- a/pyosys/generator.py +++ b/pyosys/generator.py @@ -164,8 +164,7 @@ pyosys_headers = [ { "global_id_storage_", "global_id_index_", - "global_negative_id_storage_", - "global_negative_id_prefix_storage_", + "global_autoidx_id_storage_", "global_refcount_storage_", "global_free_idx_list_", "builtin_ff_cell_types",