dbSta: move levelizedDrvrVertices() out of OpenSTA fork#10352
dbSta: move levelizedDrvrVertices() out of OpenSTA fork#10352openroad-ci wants to merge 4 commits intoThe-OpenROAD-Project:masterfrom
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
| 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". |
2 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)