dbSta: move levelizedDrvrVertices() out of OpenSTA fork#10352
Conversation
Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| using Sta::slack; | ||
|
|
||
| // Drivers sorted by (level, name) for determinism. | ||
| const VertexSeq& levelizedDrvrVertices(); |
There was a problem hiding this comment.
warning: no header providing "sta::VertexSeq" is directly included [misc-include-cleaner]
src/dbSta/include/db_sta/dbSta.hh:12:
- #include "db_sta/DelayFmt.hh" // IWYU pragma: keep
+ #include "GraphClass.hh"
+ #include "db_sta/DelayFmt.hh" // IWYU pragma: keep| } | ||
| VertexNameLess name_less(net); | ||
| std::sort(levelized_drvr_vertices_.begin(), | ||
| levelized_drvr_vertices_.end(), |
There was a problem hiding this comment.
warning: use a ranges version of this algorithm [modernize-use-ranges]
| levelized_drvr_vertices_.end(), | |
| std::ranges::sort(levelized_drvr_vertices_, | |
| , |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a caching mechanism for levelized driver vertices within dbSta, including a new observer (DbStaLevelizeObserver) to handle cache invalidation when graph levels change. It also adds a Tcl command report_levelized_drvr_vertices and a test case to verify the functionality. Feedback was provided regarding the reliance on internal knowledge of the base class's observer setup and the necessity of ensuring deterministic sorting during vertex iteration.
| void dbSta::makeObservers() | ||
| { | ||
| Sta::makeObservers(); | ||
| levelize_->setObserver(new DbStaLevelizeObserver(this)); | ||
| } |
There was a problem hiding this comment.
The dbSta::makeObservers override replaces the StaLevelizeObserver created by the base class. While the DbStaLevelizeObserver correctly replicates the behavior of the base observer, this pattern relies on internal knowledge of Sta::makeObservers. If the base class is updated to install additional observers or change the behavior of StaLevelizeObserver, this override might become out of sync. However, given the current OpenSTA API where setObserver takes ownership and deletes the previous one, this is the only way to extend the behavior. It would be safer to verify if OpenSTA could be updated to support observer chaining or multiple observers.
There was a problem hiding this comment.
Hard to address without change in OpenSTA
- Behavior duplication: DbStaLevelizeObserver::levels{,Changed}Before literally repeats what file-local StaLevelizeObserver (sta/search/Sta.cc) does. If upstream changes that observer (adds work, adds another observer in Sta::makeObservers), our override silently drops the new behavior.
- API limitation: Levelize::setObserver (sta/search/Levelize.cc) deletes prior + replaces. Single-slot. No chaining. Forces composite-by-replacement pattern.
Proper fix would be to add chaining/multi-observer API to Levelize:
void addObserver(LevelizeObserver*); // append, takes ownership
void removeObserver(LevelizeObserver*);
Then dbSta installs only the cache-invalidation observer; upstream's StaLevelizeObserver stays untouched. No replication, no drift risk.
I don't know if I should add this in OpenSTA and push upstream @maliberty @precisionmoon
I am trying to see if I can create a regression to catch any such drift
There was a problem hiding this comment.
It would make more sense for this observer to inherit from the existing observer and then call the parent class function instead of duplicating the contents.
There was a problem hiding this comment.
Thanks, and agreed in principle but blocked with current implementation: StaLevelizeObserver is file-local in sta/search/Sta.cc, no header, so dbSta can't include or subclass it as-is.
Full fix needs OpenSTA changes (move StaLevelizeObserver + LevelizeObserver to public headers, add Sta::setLevelizeObserver). If you agree, I can file a PR in parallaxsw/OpenSTA, or if you add this, I can merge it too.
| const VertexSeq& dbSta::levelizedDrvrVertices() | ||
| { | ||
| ensureLevelized(); | ||
| if (!drvr_vertices_level_valid_) { | ||
| Graph* g = graph(); | ||
| // Approx half of vertices are drivers. | ||
| levelized_drvr_vertices_.reserve(g->vertexCount() / 2); | ||
| Network* net = network(); | ||
| VertexIterator vertex_iter(g); | ||
| while (vertex_iter.hasNext()) { | ||
| Vertex* vertex = vertex_iter.next(); | ||
| if (vertex->isDriver(net)) { | ||
| levelized_drvr_vertices_.push_back(vertex); | ||
| } | ||
| } | ||
| VertexNameLess name_less(net); | ||
| std::ranges::sort(levelized_drvr_vertices_, | ||
| [&name_less](const Vertex* a, const Vertex* b) { | ||
| if (a->level() != b->level()) { | ||
| return a->level() < b->level(); | ||
| } | ||
| return name_less(a, b); | ||
| }); | ||
| drvr_vertices_level_valid_ = true; | ||
| } | ||
| return levelized_drvr_vertices_; | ||
| } | ||
|
|
There was a problem hiding this comment.
The implementation of levelizedDrvrVertices iterates over all vertices in the graph (O(V)) and then sorts the drivers (O(D log D)). This matches the original implementation's complexity and maintains consistency. Ensure that the sorting of drivers is deterministic if they are represented by pointers, using a custom comparator if necessary. While a single-pass approach or iterating over nets might be more efficient for large designs, the current cached approach is acceptable and avoids replacing a specialized implementation without verification.
References
- Iteration or sorting over pointer keys is deterministic if a custom comparator is defined for the pointer type.
- In performance-critical code, do not replace a specialized implementation for small inputs with a more generic one without verifying that performance is not negatively impacted.
- A single-pass approach can be more efficient than a two-pass approach by allowing for early exit and avoiding redundant processing.
There was a problem hiding this comment.
There is no actionable item here
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
clang-tidy review says "All clean, LGTM! 👍" |
| { | ||
| if (drvr_vertices_level_valid_) { | ||
| drvr_vertices_level_valid_ = false; | ||
| levelized_drvr_vertices_.clear(); |
There was a problem hiding this comment.
the the clear is pointless.
There was a problem hiding this comment.
The clear is required for correctness- without it, the next rebuild's push_back appends to the stale contents, returning a corrupted concatenated list. After dbStaCbk::inDbInstDestroy it would also be dangling Vertex*. Did you mean to move the clear to the rebuild path? That would definitely be cheaper invalidate (flag flip only), but at the cost of holding stale pointers in the private member until the next query. Please clarify. Thanks.
| { | ||
| Sta::makeObservers(); | ||
| levelize_->setObserver(new DbStaLevelizeObserver(this)); | ||
| } |
There was a problem hiding this comment.
I think it makes more sense to add a set function for the levelize observer on the Sta class than to change the include search path to get access to Levelize.hh
There was a problem hiding this comment.
Absolutely agreed! The setter on Sta is much cleaner. Worth noting though LevelizeObserver itself lives in sta/search/Levelize.hh (private include path), and DbStaLevelizeObserver must subclass it. So even with sta::setLevelizeObserver, dbSta still needs OPENSTA_HOME on the include path unless we also promote LevelizeObserver to a public header (sta/include/sta/LevelizeObserver.hh). Would you be okay to add these two changes in upstream? Either I can create a PR or if you add it, I will merge it here. Thanks!
|
parallaxsw/OpenSTA#433 to address @jjcherry56's comments. If upstream is accepted and merged, I will update the code in dbSta. For now we need this change to update the sta pointer in OpenROAD otherwise OpenROAD will not compile @maliberty |
Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
4 participants
Summary
Re-implements
levelizedDrvrVertices()indbStausing only public OpenSTA APIs, so the OpenSTA submodule can drop its fork-only PR #327 additions and stay compatible with upstreamparallaxsw/OpenSTA.All existing
rsz/call sites (Resizer.cc,RepairDesign.cc,Rebuffer.cc) are unchanged — they callsta_->levelizedDrvrVertices()wheresta_issta::dbSta*, which inheritsSta, so the newdbSta::levelizedDrvrVertices()resolves via overload.Cache invalidation uses the existing
LevelizeObserverhook. BecauseLevelize::setObserver()replaces the prior observer (deletes it, no chain),dbSta::makeObservers()installs a composite(
DbStaLevelizeObserver) that replicatesSta'sStaLevelizeObserverwork (forwarding toSearch+GraphDelayCalc) and additionally invalidates the dbSta cache. Algorithm and sort key (levelthenVertexNameLess) match the original Levelize implementation byte-for-byte.Companion OpenSTA PR: <PR 58>
Changes
dbSta/include/db_sta/dbSta.hh— publiclevelizedDrvrVertices(); privatemakeObservers()override +invalidateLevelizedDrvrVertices()(friend of observer); cache membersdbSta/src/dbSta.cc—DbStaLevelizeObserver(composite, file-local);makeObservers()override; cachedlevelizedDrvrVertices()rebuilddbSta/src/dbSta.i—sta::report_levelized_drvr_vertices(int)Tcl helper (was insta/graph/Graph.i)dbSta/src/CMakeLists.txt— add${OPENSTA_HOME}+${OPENSTA_HOME}/include/staPRIVATE ondbSta_lib(needed forsearch/Levelize.hhand the unprefixed transitiveGraph.hhetc.)dbSta/test/levelize_drvr_vertices1.{tcl,ok}+gcd_asap7.v— relocated regression testdbSta/test/CMakeLists.txt+dbSta/test/BUILD— register test in CMake + Bazel with required asap7 LEFs/libssrc/sta→ companion fork PR headNotes
.okwas regenerated against current OpenSTA semantics (full-liberty link in OpenROAD context). 375 driver-vertex count vs the old 106 because:PortDirection::unknown⇒ not driver perVertex::isDriver).link_designflow which needs LEF and resolves all cells.Type of Change
Impact
Hopefully no impact!
Verification
./etc/Build.sh).Related Issues
Reverts OSTA PR The-OpenROAD-Project/OpenSTA#327 (commit https://github.com/The-OpenROAD-Project-private/OpenSTA/commit/94bb37bb4a7b34e6839918fb5f7a37e9b801d85a)