From 6e91f402dfd25e7c021eca2b8315c32c259d8d4c Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Thu, 24 Apr 2025 11:36:55 +0200 Subject: [PATCH 1/6] rtlil: less sketchy IdString interner lifetime --- kernel/rtlil.cc | 13 ----- kernel/rtlil.h | 137 +++++++++++++++++++++++------------------------- kernel/yosys.cc | 1 + 3 files changed, 67 insertions(+), 84 deletions(-) diff --git a/kernel/rtlil.cc b/kernel/rtlil.cc index dd78b202d..311e4d435 100644 --- a/kernel/rtlil.cc +++ b/kernel/rtlil.cc @@ -32,19 +32,6 @@ YOSYS_NAMESPACE_BEGIN -bool RTLIL::IdString::destruct_guard_ok = false; -RTLIL::IdString::destruct_guard_t RTLIL::IdString::destruct_guard; -std::vector RTLIL::IdString::global_id_storage_; -dict RTLIL::IdString::global_id_index_; -#ifndef YOSYS_NO_IDS_REFCNT -std::vector RTLIL::IdString::global_refcount_storage_; -std::vector RTLIL::IdString::global_free_idx_list_; -#endif -#ifdef YOSYS_USE_STICKY_IDS -int RTLIL::IdString::last_created_idx_[8]; -int RTLIL::IdString::last_created_idx_ptr_; -#endif - #define X(_id) IdString RTLIL::ID::_id; #include "kernel/constids.inc" #undef X diff --git a/kernel/rtlil.h b/kernel/rtlil.h index 96c8c523b..533de1be5 100644 --- a/kernel/rtlil.h +++ b/kernel/rtlil.h @@ -106,42 +106,41 @@ namespace RTLIL typedef std::pair SigSig; }; +#define YOSYS_XTRACE_GET_PUT +#undef YOSYS_SORT_ID_FREE_LIST +#undef YOSYS_USE_STICKY_IDS +#undef YOSYS_NO_IDS_REFCNT + +class Interner { + public: + std::vector storage; + dict map; + #ifndef YOSYS_NO_IDS_REFCNT + std::vector refcounts; + std::vector free_list; + #endif + #ifdef YOSYS_USE_STICKY_IDS + int RTLIL::IdString::last_created_idx_[8]; + int RTLIL::IdString::last_created_idx_ptr_; + #endif + static Interner& get() { + static Interner instance; + return instance; + } +}; +inline const auto& internerInitializer = Interner::get(); + struct RTLIL::IdString { - #undef YOSYS_XTRACE_GET_PUT - #undef YOSYS_SORT_ID_FREE_LIST - #undef YOSYS_USE_STICKY_IDS - #undef YOSYS_NO_IDS_REFCNT - - // the global id string cache - - static bool destruct_guard_ok; // POD, will be initialized to zero - static struct destruct_guard_t { - destruct_guard_t() { destruct_guard_ok = true; } - ~destruct_guard_t() { destruct_guard_ok = false; } - } destruct_guard; - - static std::vector global_id_storage_; - static dict global_id_index_; -#ifndef YOSYS_NO_IDS_REFCNT - static std::vector global_refcount_storage_; - static std::vector global_free_idx_list_; -#endif - -#ifdef YOSYS_USE_STICKY_IDS - static int last_created_idx_ptr_; - static int last_created_idx_[8]; -#endif - static inline void xtrace_db_dump() { #ifdef YOSYS_XTRACE_GET_PUT - for (int idx = 0; idx < GetSize(global_id_storage_); idx++) + for (int idx = 0; idx < GetSize(Interner::get().storage); idx++) { - if (global_id_storage_.at(idx) == nullptr) + if (Interner::get().storage.at(idx) == nullptr) log("#X# DB-DUMP index %d: FREE\n", idx); else - log("#X# DB-DUMP index %d: '%s' (ref %d)\n", idx, global_id_storage_.at(idx), global_refcount_storage_.at(idx)); + log("#X# DB-DUMP index %d: '%s' (ref %d)\n", idx, Interner::get().storage.at(idx), Interner::get().refcounts.at(idx)); } #endif } @@ -157,7 +156,7 @@ struct RTLIL::IdString } #endif #ifdef YOSYS_SORT_ID_FREE_LIST - std::sort(global_free_idx_list_.begin(), global_free_idx_list_.end(), std::greater()); + std::sort(Interner::get().free_list.begin(), Interner::get().free_list.end(), std::greater()); #endif } @@ -165,11 +164,11 @@ struct RTLIL::IdString { if (idx) { #ifndef YOSYS_NO_IDS_REFCNT - global_refcount_storage_[idx]++; + Interner::get().refcounts[idx]++; #endif #ifdef YOSYS_XTRACE_GET_PUT if (yosys_xtrace) - log("#X# GET-BY-INDEX '%s' (index %d, refcount %d)\n", global_id_storage_.at(idx), idx, global_refcount_storage_.at(idx)); + log("#X# GET-BY-INDEX '%s' (index %d, refcount %d)\n", Interner::get().storage.at(idx), idx, Interner::get().refcounts.at(idx)); #endif } return idx; @@ -177,19 +176,17 @@ struct RTLIL::IdString static int get_reference(const char *p) { - log_assert(destruct_guard_ok); - if (!p[0]) return 0; - auto it = global_id_index_.find((char*)p); - if (it != global_id_index_.end()) { + auto it = Interner::get().map.find((char*)p); + if (it != Interner::get().map.end()) { #ifndef YOSYS_NO_IDS_REFCNT - global_refcount_storage_.at(it->second)++; + Interner::get().refcounts.at(it->second)++; #endif #ifdef YOSYS_XTRACE_GET_PUT if (yosys_xtrace) - log("#X# GET-BY-NAME '%s' (index %d, refcount %d)\n", global_id_storage_.at(it->second), it->second, global_refcount_storage_.at(it->second)); + log("#X# GET-BY-NAME '%s' (index %d, refcount %d)\n", Interner::get().storage.at(it->second), it->second, Interner::get().refcounts.at(it->second)); #endif return it->second; } @@ -201,31 +198,31 @@ struct RTLIL::IdString log_error("Found control character or space (0x%02x) in string '%s' which is not allowed in RTLIL identifiers\n", *c, p); #ifndef YOSYS_NO_IDS_REFCNT - if (global_free_idx_list_.empty()) { - if (global_id_storage_.empty()) { - global_refcount_storage_.push_back(0); - global_id_storage_.push_back((char*)""); - global_id_index_[global_id_storage_.back()] = 0; + if (Interner::get().free_list.empty()) { + if (Interner::get().storage.empty()) { + Interner::get().refcounts.push_back(0); + Interner::get().storage.push_back((char*)""); + Interner::get().map[Interner::get().storage.back()] = 0; } - log_assert(global_id_storage_.size() < 0x40000000); - global_free_idx_list_.push_back(global_id_storage_.size()); - global_id_storage_.push_back(nullptr); - global_refcount_storage_.push_back(0); + log_assert(Interner::get().storage.size() < 0x40000000); + Interner::get().free_list.push_back(Interner::get().storage.size()); + Interner::get().storage.push_back(nullptr); + Interner::get().refcounts.push_back(0); } - int idx = global_free_idx_list_.back(); - global_free_idx_list_.pop_back(); - global_id_storage_.at(idx) = strdup(p); - global_id_index_[global_id_storage_.at(idx)] = idx; - global_refcount_storage_.at(idx)++; + int idx = Interner::get().free_list.back(); + Interner::get().free_list.pop_back(); + Interner::get().storage.at(idx) = strdup(p); + Interner::get().map[Interner::get().storage.at(idx)] = idx; + Interner::get().refcounts.at(idx)++; #else - if (global_id_storage_.empty()) { - global_id_storage_.push_back((char*)""); - global_id_index_[global_id_storage_.back()] = 0; + if (Interner::get().storage.empty()) { + Interner::get().storage.push_back((char*)""); + Interner::get().map[Interner::get().storage.back()] = 0; } - int idx = global_id_storage_.size(); - global_id_storage_.push_back(strdup(p)); - global_id_index_[global_id_storage_.back()] = idx; + int idx = Interner::get().storage.size(); + Interner::get().storage.push_back(strdup(p)); + Interner::get().map[Interner::get().storage.back()] = idx; #endif if (yosys_xtrace) { @@ -235,7 +232,7 @@ struct RTLIL::IdString #ifdef YOSYS_XTRACE_GET_PUT if (yosys_xtrace) - log("#X# GET-BY-NAME '%s' (index %d, refcount %d)\n", global_id_storage_.at(idx), idx, global_refcount_storage_.at(idx)); + log("#X# GET-BY-NAME '%s' (index %d, refcount %d)\n", Interner::get().storage.at(idx), idx, Interner::get().refcounts.at(idx)); #endif #ifdef YOSYS_USE_STICKY_IDS @@ -253,18 +250,15 @@ struct RTLIL::IdString #ifndef YOSYS_NO_IDS_REFCNT static inline void put_reference(int idx) { - // 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 (!destruct_guard_ok || !idx) + if (!idx) return; - #ifdef YOSYS_XTRACE_GET_PUT if (yosys_xtrace) { - log("#X# PUT '%s' (index %d, refcount %d)\n", global_id_storage_.at(idx), idx, global_refcount_storage_.at(idx)); + log("#X# PUT '%s' (index %d, refcount %d)\n", Interner::get().storage.at(idx), idx, Interner::get().refcounts.at(idx)); } #endif - int &refcount = global_refcount_storage_[idx]; + int &refcount = Interner::get().refcounts[idx]; if (--refcount > 0) return; @@ -275,14 +269,14 @@ struct RTLIL::IdString static inline void free_reference(int idx) { if (yosys_xtrace) { - log("#X# Removed IdString '%s' with index %d.\n", global_id_storage_.at(idx), idx); + log("#X# Removed IdString '%s' with index %d.\n", Interner::get().storage.at(idx), idx); log_backtrace("-X- ", yosys_xtrace-1); } - global_id_index_.erase(global_id_storage_.at(idx)); - free(global_id_storage_.at(idx)); - global_id_storage_.at(idx) = nullptr; - global_free_idx_list_.push_back(idx); + Interner::get().map.erase(Interner::get().storage.at(idx)); + free(Interner::get().storage.at(idx)); + Interner::get().storage.at(idx) = nullptr; + Interner::get().free_list.push_back(idx); } #else static inline void put_reference(int) { } @@ -315,11 +309,11 @@ struct RTLIL::IdString } inline const char *c_str() const { - return global_id_storage_.at(index_); + return Interner::get().storage.at(index_); } inline std::string str() const { - return std::string(global_id_storage_.at(index_)); + return std::string(Interner::get().storage.at(index_)); } inline bool operator<(const IdString &rhs) const { @@ -447,6 +441,7 @@ inline bool RTLIL::IdString::in(const pool &rhs) const { return rhs.co inline bool RTLIL::IdString::in(const pool &&rhs) const { return rhs.count(*this) != 0; } namespace RTLIL { + inline const auto& ensure_init = internerInitializer; namespace ID { #define X(_id) extern IdString _id; #include "kernel/constids.inc" diff --git a/kernel/yosys.cc b/kernel/yosys.cc index 9b0bc92ce..9d6181400 100644 --- a/kernel/yosys.cc +++ b/kernel/yosys.cc @@ -185,6 +185,7 @@ void yosys_setup() if(already_setup) return; already_setup = true; + (void)Interner::instance(); #ifdef WITH_PYTHON // With Python 3.12, calling PyImport_AppendInittab on an already From 272b7fa6978d1a24b1a3bdf1db0daa468b57ef6d Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Thu, 24 Apr 2025 11:42:17 +0200 Subject: [PATCH 2/6] fixup! rtlil: less sketchy IdString interner lifetime --- kernel/rtlil.h | 2 +- kernel/yosys.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/kernel/rtlil.h b/kernel/rtlil.h index 533de1be5..fb19257d9 100644 --- a/kernel/rtlil.h +++ b/kernel/rtlil.h @@ -106,7 +106,7 @@ namespace RTLIL typedef std::pair SigSig; }; -#define YOSYS_XTRACE_GET_PUT +#undef YOSYS_XTRACE_GET_PUT #undef YOSYS_SORT_ID_FREE_LIST #undef YOSYS_USE_STICKY_IDS #undef YOSYS_NO_IDS_REFCNT diff --git a/kernel/yosys.cc b/kernel/yosys.cc index 9d6181400..0ac5c4f99 100644 --- a/kernel/yosys.cc +++ b/kernel/yosys.cc @@ -185,7 +185,7 @@ void yosys_setup() if(already_setup) return; already_setup = true; - (void)Interner::instance(); + (void)Interner::get(); #ifdef WITH_PYTHON // With Python 3.12, calling PyImport_AppendInittab on an already From d8cae1d90443fb8c4b30384f088c10ebac9e7d9e Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Fri, 25 Apr 2025 00:49:37 +0200 Subject: [PATCH 3/6] ast: add GC for dev debugging --- frontends/ast/ast.cc | 26 +++++++++++ frontends/ast/ast.h | 66 +++++++++++++++++++++++++++ frontends/verilog/verilog_frontend.cc | 12 +++++ 3 files changed, 104 insertions(+) diff --git a/frontends/ast/ast.cc b/frontends/ast/ast.cc index 431f7b4f8..fbfa77a18 100644 --- a/frontends/ast/ast.cc +++ b/frontends/ast/ast.cc @@ -248,6 +248,9 @@ AstNode::AstNode(AstNodeType type, AstNode *child1, AstNode *child2, AstNode *ch children.push_back(child4); fixup_hierarchy_flags(); +#ifdef ASTNODE_GC + Tagger::get().reg(this); +#endif } // create a (deep recursive) copy of a node @@ -283,12 +286,28 @@ void AstNode::cloneInto(AstNode *other) const // delete all children in this node void AstNode::delete_children() { +#ifdef ASTNODE_GC + for (auto &it : children) + if (Tagger::get().nodes.count(it)) + delete it; + else + log("skip child %p\n", it); +#else for (auto &it : children) delete it; +#endif children.clear(); +#ifdef ASTNODE_GC + for (auto &it : attributes) + if (Tagger::get().nodes.count(it.second)) + delete it.second; + else + log("skip attr %p\n", it.second); +#else for (auto &it : attributes) delete it.second; +#endif attributes.clear(); } @@ -296,7 +315,14 @@ void AstNode::delete_children() AstNode::~AstNode() { astnodes--; +#ifdef ASTNODE_GC + Tagger::get().unreg(this); +#endif delete_children(); + if (yosys_xtrace) { + log("DESTR %X %p\n", hashidx_, this); + log_backtrace("", yosys_xtrace-1); + } } // create a nice text representation of the node diff --git a/frontends/ast/ast.h b/frontends/ast/ast.h index 2c2d408ce..923594a3c 100644 --- a/frontends/ast/ast.h +++ b/frontends/ast/ast.h @@ -467,6 +467,72 @@ namespace AST_INTERNAL AST::AstNode *original_ast = nullptr); } +#undef ASTNODE_GC +#ifdef ASTNODE_GC +struct Tagger { + std::set nodes; + std::set tagged; + static Tagger& get() { + static Tagger instance; + return instance; + } + void reg(AST::AstNode* p) { + if (!p) + return; + log_assert(!nodes.count(p)); + nodes.insert(p); + } + void unreg(AST::AstNode* p) { + if (!p || !nodes.count(p)) + return; + nodes.erase(p); + } + void tag(AST::AstNode* p) { + if (!p) + return; + tagged.insert(p); + for (AST::AstNode* c : p->children) + tag(c); + for (auto x : p->attributes) + tag(x.second); + tag(p->id2ast); + } + void clear() { + tagged.clear(); + } + void dump_untagged() { + Yosys::log("Dumping untagged:\n"); + for (auto p : nodes) { + if (!tagged.count(p)) { + Yosys::log(">> %p\n", p); + p->dumpAst(stdout, ""); + } + } + } + void shadow_kill(AST::AstNode* p) { + if (!p || !nodes.count(p)) + return; + for (AST::AstNode* c : p->children) + shadow_kill(c); + for (auto x : p->attributes) + shadow_kill(x.second); + shadow_kill(p->id2ast); + delete p; + } + void kill_untagged() { + auto copy = nodes; + while (!copy.empty()) { + AST::AstNode* p = *(copy.begin()); + copy.erase(p); + fflush(stdout); + if (!tagged.count(p)) { + shadow_kill(p); + } + } + } +}; +#endif // ASTNODE_GC + YOSYS_NAMESPACE_END #endif diff --git a/frontends/verilog/verilog_frontend.cc b/frontends/verilog/verilog_frontend.cc index 14a0f16c0..2e4ea9859 100644 --- a/frontends/verilog/verilog_frontend.cc +++ b/frontends/verilog/verilog_frontend.cc @@ -39,6 +39,7 @@ YOSYS_NAMESPACE_BEGIN using namespace VERILOG_FRONTEND; + // use the Verilog bison/flex parser to generate an AST and use AST::process() to convert it to RTLIL static std::vector verilog_defaults; @@ -528,7 +529,15 @@ struct VerilogFrontend : public Frontend { AST::process(design, current_ast, flag_nodisplay, flag_dump_ast1, flag_dump_ast2, flag_no_dump_ptr, flag_dump_vlog1, flag_dump_vlog2, flag_dump_rtlil, flag_nolatches, flag_nomeminit, flag_nomem2reg, flag_mem2reg, flag_noblackbox, lib_mode, flag_nowb, flag_noopt, flag_icells, flag_pwires, flag_nooverwrite, flag_overwrite, flag_defer, default_nettype_wire); +#ifdef ASTNODE_GC + Tagger::get().clear(); + Tagger::get().tag(current_ast); + for (RTLIL::Module* m : design->modules()) + if (AST::AstModule* am = dynamic_cast(m)) + Tagger::get().tag(am->ast); + Tagger::get().dump_untagged(); +#endif if (!flag_nopp) delete lexin; @@ -539,6 +548,9 @@ struct VerilogFrontend : public Frontend { delete current_ast; current_ast = NULL; +#ifdef ASTNODE_GC + Tagger::get().kill_untagged(); +#endif log("Successfully finished Verilog frontend.\n"); } From 0d8ea6f8c38d860b135a4b84bc6db5f18fd24b1a Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Fri, 25 Apr 2025 11:00:19 +0200 Subject: [PATCH 4/6] fixup! ast: add GC for dev debugging --- frontends/ast/ast.h | 3 +-- frontends/verilog/verilog_frontend.cc | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/frontends/ast/ast.h b/frontends/ast/ast.h index 923594a3c..b60cae247 100644 --- a/frontends/ast/ast.h +++ b/frontends/ast/ast.h @@ -467,7 +467,7 @@ namespace AST_INTERNAL AST::AstNode *original_ast = nullptr); } -#undef ASTNODE_GC +#define ASTNODE_GC #ifdef ASTNODE_GC struct Tagger { std::set nodes; @@ -524,7 +524,6 @@ struct Tagger { while (!copy.empty()) { AST::AstNode* p = *(copy.begin()); copy.erase(p); - fflush(stdout); if (!tagged.count(p)) { shadow_kill(p); } diff --git a/frontends/verilog/verilog_frontend.cc b/frontends/verilog/verilog_frontend.cc index 2e4ea9859..c798beff7 100644 --- a/frontends/verilog/verilog_frontend.cc +++ b/frontends/verilog/verilog_frontend.cc @@ -536,7 +536,7 @@ struct VerilogFrontend : public Frontend { if (AST::AstModule* am = dynamic_cast(m)) Tagger::get().tag(am->ast); - Tagger::get().dump_untagged(); + // Tagger::get().dump_untagged(); #endif if (!flag_nopp) From 6d8cb335124e1b9b1cc0d4ffc5412b4b63a49862 Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Fri, 25 Apr 2025 11:00:35 +0200 Subject: [PATCH 5/6] fixup! fixup! ast: add GC for dev debugging --- frontends/ast/ast.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontends/ast/ast.h b/frontends/ast/ast.h index b60cae247..f83e02964 100644 --- a/frontends/ast/ast.h +++ b/frontends/ast/ast.h @@ -467,7 +467,7 @@ namespace AST_INTERNAL AST::AstNode *original_ast = nullptr); } -#define ASTNODE_GC +#undef ASTNODE_GC #ifdef ASTNODE_GC struct Tagger { std::set nodes; From a5458d21ef8166128d43a4497d7400cdf09377f6 Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Fri, 25 Apr 2025 11:10:16 +0200 Subject: [PATCH 6/6] fixup! fixup! fixup! ast: add GC for dev debugging --- frontends/ast/ast.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frontends/ast/ast.cc b/frontends/ast/ast.cc index fbfa77a18..be0c81078 100644 --- a/frontends/ast/ast.cc +++ b/frontends/ast/ast.cc @@ -290,8 +290,8 @@ void AstNode::delete_children() for (auto &it : children) if (Tagger::get().nodes.count(it)) delete it; - else - log("skip child %p\n", it); + // else + // log("skip child %p\n", it); #else for (auto &it : children) delete it; @@ -302,8 +302,8 @@ void AstNode::delete_children() for (auto &it : attributes) if (Tagger::get().nodes.count(it.second)) delete it.second; - else - log("skip attr %p\n", it.second); + // else + // log("skip attr %p\n", it.second); #else for (auto &it : attributes) delete it.second;