From 987d3560d1a1081fb92682e57573cfe4a3110e94 Mon Sep 17 00:00:00 2001 From: James Cherry Date: Tue, 28 Jan 2025 09:23:38 -0700 Subject: [PATCH 1/4] CodingGuidelines Signed-off-by: James Cherry --- doc/CodingGuidelines.txt | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/doc/CodingGuidelines.txt b/doc/CodingGuidelines.txt index bd838801..1053ffd2 100644 --- a/doc/CodingGuidelines.txt +++ b/doc/CodingGuidelines.txt @@ -61,12 +61,20 @@ instead, write this: fatal ("virtual memory exhausted"); -Use braces around if/for bodies that are more than one line. -IE, +Do not use braces around if/for that are one line. + if (pred) - for (int i = 0; i < len; i++) { // this body should be in {}'s + bar = 1; + else + bar = 3; + +Use braces around if/for bodies that are more than one line. + + if (pred) { + for (int i = 0; i < len; i++) { ... } + } Add a default clause to all switches calling switchCaseNotHandled: From 9c610f63451bc508f2d150a02ff8b20be74d7f1f Mon Sep 17 00:00:00 2001 From: James Cherry Date: Tue, 28 Jan 2025 10:17:02 -0700 Subject: [PATCH 2/4] deleteFilterTags() Signed-off-by: James Cherry --- search/Search.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/search/Search.cc b/search/Search.cc index bb217733..ee60884a 100644 --- a/search/Search.cc +++ b/search/Search.cc @@ -589,13 +589,14 @@ Search::deleteFilterTags() void Search::deleteFilterClkInfos() { - ClkInfoSet::Iterator clk_info_iter(clk_info_set_); - while (clk_info_iter.hasNext()) { - ClkInfo *clk_info = clk_info_iter.next(); + for (auto itr = clk_info_set_->cbegin(); itr != clk_info_set_->cend(); ) { + ClkInfo *clk_info = *itr; if (clk_info->refsFilter(this)) { - clk_info_set_->erase(clk_info); + itr = clk_info_set_->erase(itr); delete clk_info; } + else + itr++; } } From 8797ac5add14881daec45efc281f089873cd8f6a Mon Sep 17 00:00:00 2001 From: James Cherry Date: Thu, 30 Jan 2025 08:44:04 -0700 Subject: [PATCH 3/4] thread safety commit 9e5184529d2d4221aa858038c444bbe12f786a11 Author: James Cherry Date: Thu Jan 30 08:10:30 2025 -0700 cmake ENABLE_TSAN Signed-off-by: James Cherry commit 9fc69ab7ea1fd943b2c27f0abfe78f978b8e8df5 Author: James Cherry Date: Wed Jan 29 17:43:02 2025 -0700 GraphDelayCalc::findDriverDelays eval tristates together Signed-off-by: James Cherry commit 51a7497ddc3bee114d9b56285a7db435bb66d54b Author: James Cherry Date: Wed Jan 29 16:37:25 2025 -0700 LibertyCell::ensureVoltageWaveforms atomic Signed-off-by: James Cherry commit e180de9a10534d780273ea77154a5bf6378fb5ee Author: James Cherry Date: Wed Jan 29 16:05:42 2025 -0700 bfs_in_queue_ atomic Signed-off-by: James Cherry commit 61477e5ba6842096d9a2de9a5ca9f72bc38d699e Author: James Cherry Date: Wed Jan 29 16:05:10 2025 -0700 Search::tags_ atomic Signed-off-by: James Cherry commit df9a29d10ceffae6e181d702f15ab0c73a97a64c Author: James Cherry Date: Wed Jan 29 15:09:02 2025 -0700 ArrayTable Signed-off-by: James Cherry commit e5d094184ff46c34d20209766b86b9a7bf18dc3b Author: James Cherry Date: Wed Jan 29 11:39:57 2025 -0700 ArrayTable atomic blocks_ Signed-off-by: James Cherry commit 072f028433057dc30ee3baae292c7a387d2e54cb Author: James Cherry Date: Wed Jan 29 11:39:22 2025 -0700 use_tsan option Signed-off-by: James Cherry Signed-off-by: James Cherry --- CMakeLists.txt | 6 +++--- dcalc/GraphDelayCalc.cc | 21 +++++++-------------- include/sta/ArrayTable.hh | 12 +++--------- include/sta/Graph.hh | 14 ++++++++------ include/sta/Liberty.hh | 3 ++- include/sta/Search.hh | 7 +++---- search/Search.cc | 16 ++-------------- 7 files changed, 28 insertions(+), 51 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 3bf9dafb..c704ddc6 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -38,7 +38,7 @@ project(STA VERSION 2.6.0 option(CUDD_DIR "CUDD BDD package directory") option(USE_TCL_READLINE "Use TCL readline package" ON) -option(USE_SANITIZE "Compile with santize address enabled") +option(ENABLE_TSAN "Compile with thread santizer enabled" OFF) # Turn on to debug compiler args. set(CMAKE_VERBOSE_MAKEFILE OFF) @@ -550,8 +550,8 @@ endif() # common to gcc/clang set(CXX_FLAGS -Wall -Wextra -pedantic -Wcast-qual -Wredundant-decls -Wformat-security) -if(USE_SANITIZE) - message(STATUS "Sanitize: ${USE_SANITIZE}") +if(ENABLE_TSAN) + message(STATUS "Thread sanitizer: ${ENABLE_TSAN}") set(CXX_FLAGS "${CXX_FLAGS};-fsanitize=thread") set(CMAKE_EXE_LINKER_FLAGS "-fsanitize=thread") endif() diff --git a/dcalc/GraphDelayCalc.cc b/dcalc/GraphDelayCalc.cc index ca6ffd56..d2bf002a 100644 --- a/dcalc/GraphDelayCalc.cc +++ b/dcalc/GraphDelayCalc.cc @@ -685,13 +685,15 @@ GraphDelayCalc::findDriverDelays(Vertex *drvr_vertex, LoadPinIndexMap &load_pin_index_map) { MultiDrvrNet *multi_drvr = findMultiDrvrNet(drvr_vertex); - if (multi_drvr == nullptr - || (multi_drvr - && (!multi_drvr->parallelGates(network_) - || drvr_vertex == multi_drvr->dcalcDrvr()))) { + if (multi_drvr == nullptr) { initLoadSlews(drvr_vertex); findDriverDelays1(drvr_vertex, multi_drvr, arc_delay_calc, load_pin_index_map); } + else if (drvr_vertex == multi_drvr->dcalcDrvr()) { + initLoadSlews(drvr_vertex); + for (Vertex *drvr : multi_drvr->drvrs()) + findDriverDelays1(drvr, multi_drvr, arc_delay_calc, load_pin_index_map); + } arc_delay_calc_->finishDrvrPin(); } @@ -825,16 +827,7 @@ GraphDelayCalc::findDriverDelays1(Vertex *drvr_vertex, LoadPinIndexMap &load_pin_index_map) { initSlew(drvr_vertex); - if (multi_drvr - && multi_drvr->parallelGates(network_)) { - // Only init on the trigger driver. - if (drvr_vertex == multi_drvr->dcalcDrvr()) { - for (auto vertex : multi_drvr->drvrs()) - initWireDelays(vertex); - } - } - else - initWireDelays(drvr_vertex); + initWireDelays(drvr_vertex); bool delay_changed = false; array delay_exists = {false, false}; VertexInEdgeIterator edge_iter(drvr_vertex, graph_); diff --git a/include/sta/ArrayTable.hh b/include/sta/ArrayTable.hh index 1f11e394..33a6c3da 100644 --- a/include/sta/ArrayTable.hh +++ b/include/sta/ArrayTable.hh @@ -26,6 +26,7 @@ #include // memcpy #include +#include #include "ObjectId.hh" #include "Error.hh" @@ -77,8 +78,7 @@ private: // Don't use std::vector so growing blocks_ can be thread safe. size_t blocks_size_; size_t blocks_capacity_; - ArrayBlock* *blocks_; - ArrayBlock* *prev_blocks_; + std::atomic**> blocks_; // Linked list of free arrays indexed by array size. std::vector free_list_; static constexpr ObjectId idx_mask_ = block_size - 1; @@ -91,8 +91,7 @@ ArrayTable::ArrayTable() : free_idx_(object_idx_null), blocks_size_(0), blocks_capacity_(1024), - blocks_(new ArrayBlock*[blocks_capacity_]), - prev_blocks_(nullptr) + blocks_(new ArrayBlock*[blocks_capacity_]) { } @@ -101,7 +100,6 @@ ArrayTable::~ArrayTable() { deleteBlocks(); delete [] blocks_; - delete [] prev_blocks_; } template @@ -173,10 +171,6 @@ ArrayTable::pushBlock(ArrayBlock *block) size_t new_capacity = blocks_capacity_ * 1.5; ArrayBlock** new_blocks = new ArrayBlock*[new_capacity]; memcpy(new_blocks, blocks_, blocks_capacity_ * sizeof(ArrayBlock*)); - if (prev_blocks_) - delete [] prev_blocks_; - // Preserve block array for other threads to reference. - prev_blocks_ = blocks_; blocks_ = new_blocks; blocks_capacity_ = new_capacity; } diff --git a/include/sta/Graph.hh b/include/sta/Graph.hh index 3197a378..f627ee6f 100644 --- a/include/sta/Graph.hh +++ b/include/sta/Graph.hh @@ -25,6 +25,7 @@ #pragma once #include +#include #include "Iterator.hh" #include "Map.hh" @@ -365,14 +366,12 @@ protected: EdgeId in_edges_; // Edges to this vertex. EdgeId out_edges_; // Edges from this vertex. - // 32 bits + // 28 bits unsigned int tag_group_index_:tag_group_index_bits; // 24 - // Each bit corresponds to a different BFS queue. - unsigned int bfs_in_queue_:int(BfsIndex::bits); // 4 - unsigned int slew_annotated_:slew_annotated_bits; + unsigned int slew_annotated_:slew_annotated_bits; // 4 // 32 bits - unsigned int level_:Graph::vertex_level_bits; + unsigned int level_:Graph::vertex_level_bits; // 24 // Levelization search state. // LevelColor gcc barfs if this is dcl'd. unsigned color_:2; @@ -383,6 +382,9 @@ protected: bool is_bidirect_drvr_:1; bool is_reg_clk_:1; + // Each bit corresponds to a different BFS queue. + std::atomic bfs_in_queue_; // 4 + // 15 bits bool is_disabled_constraint_:1; bool is_gated_clk_enable_:1; @@ -394,7 +396,7 @@ protected: bool has_downstream_clk_pin_:1; bool crpr_path_pruning_disabled_:1; bool requireds_pruned_:1; - unsigned object_idx_:VertexTable::idx_bits; + unsigned object_idx_:VertexTable::idx_bits; // 7 private: friend class Graph; diff --git a/include/sta/Liberty.hh b/include/sta/Liberty.hh index 84ade180..1ac7921f 100644 --- a/include/sta/Liberty.hh +++ b/include/sta/Liberty.hh @@ -25,6 +25,7 @@ #pragma once #include +#include #include "MinMax.hh" #include "RiseFallMinMax.hh" @@ -644,7 +645,7 @@ protected: bool leakage_power_exists_; LibertyPgPortMap pg_port_map_; bool has_internal_ports_; - bool have_voltage_waveforms_; + std::atomic have_voltage_waveforms_; std::mutex waveform_lock_; const char *footprint_; const char *user_function_class_; diff --git a/include/sta/Search.hh b/include/sta/Search.hh index e1dfeb4f..bf0194dc 100644 --- a/include/sta/Search.hh +++ b/include/sta/Search.hh @@ -25,6 +25,7 @@ #pragma once #include +#include #include "MinMax.hh" #include "UnorderedSet.hh" @@ -591,15 +592,13 @@ protected: TagSet *tag_set_; // Entries in tags_ may be missing where previous filter tags were deleted. TagIndex tag_capacity_; - Tag **tags_; - Tag **tags_prev_; + std::atomic tags_; TagIndex tag_next_; // Holes in tags_ left by deleting filter tags. std::vector tag_free_indices_; std::mutex tag_lock_; TagGroupSet *tag_group_set_; - TagGroup **tag_groups_; - TagGroup **tag_groups_prev_; + std::atomic tag_groups_; TagGroupIndex tag_group_next_; // Holes in tag_groups_ left by deleting filter tag groups. std::vector tag_group_free_indices_; diff --git a/search/Search.cc b/search/Search.cc index ee60884a..5639d4f0 100644 --- a/search/Search.cc +++ b/search/Search.cc @@ -240,15 +240,13 @@ Search::init(StaState *sta) worst_slacks_ = nullptr; arrival_iter_ = new BfsFwdIterator(BfsIndex::arrival, nullptr, sta); required_iter_ = new BfsBkwdIterator(BfsIndex::required, search_adj_, sta); - tag_capacity_ = 127; + tag_capacity_ = 128; tag_set_ = new TagSet(tag_capacity_); clk_info_set_ = new ClkInfoSet(ClkInfoLess(sta)); tag_next_ = 0; tags_ = new Tag*[tag_capacity_]; - tags_prev_ = nullptr; - tag_group_capacity_ = 127; + tag_group_capacity_ = tag_capacity_; tag_groups_ = new TagGroup*[tag_group_capacity_]; - tag_groups_prev_ = nullptr; tag_group_next_ = 0; tag_group_set_ = new TagGroupSet(tag_group_capacity_); pending_latch_outputs_ = new VertexSet(graph_); @@ -280,9 +278,7 @@ Search::~Search() delete tag_set_; delete clk_info_set_; delete [] tags_; - delete [] tags_prev_; delete [] tag_groups_; - delete [] tag_groups_prev_; delete tag_group_set_; delete search_adj_; delete eval_pred_; @@ -2670,15 +2666,11 @@ Search::findTagGroup(TagGroupBldr *tag_bldr) // If tag_groups_ needs to grow make the new array and copy the // contents into it before updating tags_groups_ so that other threads // can use Search::tagGroup(TagGroupIndex) without returning gubbish. - // std::vector doesn't seem to follow this protocol so multi-thread - // search fails occasionally if a vector is used for tag_groups_. if (tag_group_next_ == tag_group_capacity_) { TagGroupIndex tag_capacity = tag_group_capacity_ * 2; TagGroup **tag_groups = new TagGroup*[tag_capacity]; memcpy(tag_groups, tag_groups_, tag_group_capacity_ * sizeof(TagGroup*)); - delete [] tag_groups_prev_; - tag_groups_prev_ = tag_groups_; tag_groups_ = tag_groups; tag_group_capacity_ = tag_capacity; tag_group_set_->reserve(tag_capacity); @@ -2911,14 +2903,10 @@ Search::findTag(const RiseFall *rf, // If tags_ needs to grow make the new array and copy the // contents into it before updating tags_ so that other threads // can use Search::tag(TagIndex) without returning gubbish. - // std::vector doesn't seem to follow this protocol so multi-thread - // search fails occasionally if a vector is used for tags_. if (tag_next_ == tag_capacity_) { TagIndex tag_capacity = tag_capacity_ * 2; Tag **tags = new Tag*[tag_capacity]; memcpy(tags, tags_, tag_capacity_ * sizeof(Tag*)); - delete [] tags_prev_; - tags_prev_ = tags_; tags_ = tags; tag_capacity_ = tag_capacity; tag_set_->reserve(tag_capacity); From 41d1d1bb7b045118705a1cad64d68a0d78343f52 Mon Sep 17 00:00:00 2001 From: Matt Liberty Date: Fri, 31 Jan 2025 03:08:36 +0000 Subject: [PATCH 4/4] Avoid parallel build conflicts (#198) The Liberty, Sdf, and Verilog parsers were writing to the same files causing random failures in parallel builds. position.hh and stack.hh are removed by switching to 3.0 to 3.2 in the require statement. location.hh is renamed per-parser with api.location.file. Signed-off-by: Matt Liberty --- liberty/LibertyParse.yy | 3 ++- liberty/LibertyScanner.hh | 2 +- sdf/SdfParse.yy | 3 ++- sdf/SdfScanner.hh | 2 +- verilog/VerilogParse.yy | 3 ++- verilog/VerilogScanner.hh | 2 +- 6 files changed, 9 insertions(+), 6 deletions(-) diff --git a/liberty/LibertyParse.yy b/liberty/LibertyParse.yy index df362a2e..9d75f238 100644 --- a/liberty/LibertyParse.yy +++ b/liberty/LibertyParse.yy @@ -38,11 +38,12 @@ #define loc_line(loc) loc.begin.line %} -%require "3.0" +%require "3.2" %skeleton "lalr1.cc" %debug %define api.namespace {sta} %locations +%define api.location.file "LibertyLocation.hh" %define parse.assert %parse-param { LibertyScanner *scanner } %parse-param { LibertyParser *reader } diff --git a/liberty/LibertyScanner.hh b/liberty/LibertyScanner.hh index 4e35c0cd..54eaf12a 100644 --- a/liberty/LibertyScanner.hh +++ b/liberty/LibertyScanner.hh @@ -30,7 +30,7 @@ #include #endif -#include "location.hh" +#include "LibertyLocation.hh" #include "LibertyParse.hh" namespace sta { diff --git a/sdf/SdfParse.yy b/sdf/SdfParse.yy index 1730daf7..023433b7 100644 --- a/sdf/SdfParse.yy +++ b/sdf/SdfParse.yy @@ -35,11 +35,12 @@ #pragma GCC diagnostic ignored "-Wunused-but-set-variable" %} -%require "3.0" +%require "3.2" %skeleton "lalr1.cc" %debug %define api.namespace {sta} %locations +%define api.location.file "SdfLocation.hh" %define parse.assert %parse-param { SdfScanner *scanner } %parse-param { SdfReader *reader } diff --git a/sdf/SdfScanner.hh b/sdf/SdfScanner.hh index ef74ccd3..4e4bd9ec 100644 --- a/sdf/SdfScanner.hh +++ b/sdf/SdfScanner.hh @@ -30,7 +30,7 @@ #include #endif -#include "location.hh" +#include "SdfLocation.hh" #include "SdfParse.hh" namespace sta { diff --git a/verilog/VerilogParse.yy b/verilog/VerilogParse.yy index c020c601..c06bb7a7 100644 --- a/verilog/VerilogParse.yy +++ b/verilog/VerilogParse.yy @@ -48,11 +48,12 @@ sta::VerilogParse::error(const location_type &loc, } %} -%require "3.0" +%require "3.2" %skeleton "lalr1.cc" %debug %define api.namespace {sta} %locations +%define api.location.file "VerilogLocation.hh" %define parse.assert %parse-param { VerilogScanner *scanner } %parse-param { VerilogReader *reader } diff --git a/verilog/VerilogScanner.hh b/verilog/VerilogScanner.hh index 2a080ac9..94838685 100644 --- a/verilog/VerilogScanner.hh +++ b/verilog/VerilogScanner.hh @@ -30,7 +30,7 @@ #include #endif -#include "location.hh" +#include "VerilogLocation.hh" #include "VerilogParse.hh" namespace sta {