Skip to content

dbSta: move levelizedDrvrVertices() out of OpenSTA fork #10352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
maliberty merged 7 commits into from
May 12, 2026
Merged

dbSta: move levelizedDrvrVertices() out of OpenSTA fork #10352

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4003e28
moved sta stuff from OSTA PR 327 to dbSta maintaining correctness
dsengupta0628 May 7, 2026
44ceff3
remove testing file
dsengupta0628 May 7, 2026
5828eb8
address clang rev
dsengupta0628 May 7, 2026
a6428e1
enhance the test and fix netlist mutation crash issue
dsengupta0628 May 7, 2026
a112df7
Merge branch 'master' into or_move_drvr_level_to_dbsta
dsengupta0628 May 8, 2026
d6ec358
Merge branch 'master' into or_move_drvr_level_to_dbsta
dsengupta0628 May 11, 2026
dbe5123
add todo item post upstream change
dsengupta0628 May 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/dbSta/include/db_sta/dbSta.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "odb/dbObject.h"
#include "sta/Clock.hh"
#include "sta/Delay.hh"
#include "sta/GraphClass.hh"
#include "sta/Liberty.hh"
#include "sta/MinMax.hh"
#include "sta/Sta.hh"
Expand Down Expand Up @@ -83,6 +84,7 @@ class dbSta;
class dbNetwork;
class dbStaReport;
class dbStaCbk;
class DbStaLevelizeObserver;
class PatternMatch;
class TestCell;

Expand Down Expand Up @@ -222,10 +224,18 @@ class dbSta : public Sta, public odb::dbDatabaseObserver
using Sta::replaceCell;
using Sta::slack;

// Drivers sorted by (level, name) for determinism.
const VertexSeq& levelizedDrvrVertices();
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Discard the cached driver-vertex list. Callers that mutate the timing
// graph outside the LevelizeObserver path (e.g. dbStaCbk on dbInst
// create/destroy) must invalidate so the next query rebuilds.
void invalidateLevelizedDrvrVertices();

private:
void makeReport() override;
void makeNetwork() override;
void makeSdcNetwork() override;
void makeObservers() override;

void replaceCell(Instance* inst,
Cell* to_cell,
Expand All @@ -238,6 +248,9 @@ class dbSta : public Sta, public odb::dbDatabaseObserver
dbStaReport* db_report_ = nullptr;
std::unique_ptr<dbStaCbk> db_cbk_;
std::set<dbStaState*> sta_states_;

VertexSeq levelized_drvr_vertices_;
bool drvr_vertices_level_valid_ = false;
};

// Utilities for TestCell
Expand Down
6 changes: 6 additions & 0 deletions src/dbSta/src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,12 @@ target_include_directories(dbSta_lib
PUBLIC
../include
${PROJECT_SOURCE_DIR}/include
PRIVATE
# Needed for search/Levelize.hh and the unprefixed transitive
# OpenSTA headers it includes (e.g. Graph.hh) which are not part of
# OpenSTA's public include.
${OPENSTA_HOME}
${OPENSTA_HOME}/include/sta
)


Expand Down
77 changes: 77 additions & 0 deletions src/dbSta/src/dbSta.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,14 @@
#include "odb/dbBlockCallBackObj.h"
#include "odb/dbObject.h"
#include "odb/dbTypes.h"
#include "search/Levelize.hh"
#include "sta/ArcDelayCalc.hh"
#include "sta/Clock.hh"
#include "sta/Delay.hh"
#include "sta/EquivCells.hh"
#include "sta/Graph.hh"
#include "sta/GraphCmp.hh"
#include "sta/GraphDelayCalc.hh"
#include "sta/Liberty.hh"
#include "sta/MinMax.hh"
#include "sta/Mode.hh"
Expand All @@ -53,6 +56,7 @@
#include "sta/PortDirection.hh"
#include "sta/ReportTcl.hh"
#include "sta/Sdc.hh"
#include "sta/Search.hh"
#include "sta/Sta.hh"
#include "sta/StaMain.hh"
#include "sta/Transition.hh"
Expand Down Expand Up @@ -288,6 +292,73 @@ void dbSta::makeSdcNetwork()
sdc_network_ = new dbSdcNetwork(network_);
}

// Levelize::setObserver takes ownership and deletes the prior observer,
// so this composite must replicate the StaLevelizeObserver behavior that
// Sta::makeObservers installs (forwarding to Search and GraphDelayCalc)
// in addition to invalidating dbSta's cache.
class DbStaLevelizeObserver : public LevelizeObserver
{
public:
explicit DbStaLevelizeObserver(dbSta* sta) : sta_(sta) {}
void levelsChangedBefore() override
{
sta_->search()->levelsChangedBefore();
sta_->graphDelayCalc()->levelsChangedBefore();
sta_->invalidateLevelizedDrvrVertices();
}
void levelChangedBefore(Vertex* vertex) override
{
sta_->search()->levelChangedBefore(vertex);
sta_->graphDelayCalc()->levelChangedBefore(vertex);
sta_->invalidateLevelizedDrvrVertices();
}

private:
dbSta* sta_;
};

void dbSta::makeObservers()
{
Sta::makeObservers();
levelize_->setObserver(new DbStaLevelizeObserver(this));
}
Comment on lines +320 to +324
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Copy link
Copy Markdown
Contributor

@dsengupta0628 dsengupta0628 May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard to address without change in OpenSTA

  1. 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.
  2. 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

Copy link
Copy Markdown
Contributor

@dsengupta0628 dsengupta0628 May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@povik FYI

Copy link
Copy Markdown
Contributor

@jjcherry56 jjcherry56 May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@dsengupta0628 dsengupta0628 May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@jjcherry56 jjcherry56 May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@dsengupta0628 dsengupta0628 May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


void dbSta::invalidateLevelizedDrvrVertices()
{
if (drvr_vertices_level_valid_) {
drvr_vertices_level_valid_ = false;
levelized_drvr_vertices_.clear();
Copy link
Copy Markdown
Contributor

@jjcherry56 jjcherry56 May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the the clear is pointless.

Copy link
Copy Markdown
Contributor

@dsengupta0628 dsengupta0628 May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

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_;
}

Comment on lines +334 to +361
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. Iteration or sorting over pointer keys is deterministic if a custom comparator is defined for the pointer type.
  2. 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.
  3. A single-pass approach can be more efficient than a two-pass approach by allowing for early exit and avoiding redundant processing.

Copy link
Copy Markdown
Contributor

@dsengupta0628 dsengupta0628 May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no actionable item here

void dbSta::postReadLef(odb::dbTech* tech, odb::dbLib* library)
{
if (library) {
Expand Down Expand Up @@ -1100,13 +1171,19 @@ void dbStaCbk::setNetwork(dbNetwork* network)
void dbStaCbk::inDbInstCreate(odb::dbInst* inst)
{
sta_->makeInstanceAfter(network_->dbToSta(inst));
// New driver vertices may exist; invalidate cached driver-vertex list.
sta_->invalidateLevelizedDrvrVertices();
}

void dbStaCbk::inDbInstDestroy(odb::dbInst* inst)
{
// This is called after the iterms have been destroyed
// so it side-steps Sta::deleteInstanceAfter.
sta_->deleteLeafInstanceBefore(network_->dbToSta(inst));
// Sta::deleteLeafInstanceBefore calls Levelize::deleteVertexBefore
// directly (bypassing the LevelizeObserver), so the dbSta cache must be
// invalidated explicitly here to avoid a dangling Vertex* on next query.
sta_->invalidateLevelizedDrvrVertices();
}

void dbStaCbk::inDbModuleCreate(odb::dbModule* module)
Expand Down
32 changes: 32 additions & 0 deletions src/dbSta/src/dbSta.i
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@
#include "db_sta/IpChecker.hh"
#include "db_sta/MakeDbSta.hh"
#include "ord/OpenRoad.hh"
#include "sta/Graph.hh"
#include "sta/Network.hh"
#include "sta/Property.hh"
#include "sta/Report.hh"
#include "sta/VerilogWriter.hh"

namespace ord {
Expand Down Expand Up @@ -221,6 +224,35 @@ replace_hier_module_cmd(odb::dbModInst* mod_inst, odb::dbModule* module)

namespace sta {

void
report_levelized_drvr_vertices(int max_count)
{
ord::OpenRoad *openroad = ord::getOpenRoad();
sta::dbSta *sta = openroad->getSta();
sta->ensureGraph();
const sta::VertexSeq &drvrs = sta->levelizedDrvrVertices();
sta::Report *report = sta->report();
const sta::Network *network = sta->network();
report->report("driver vertex count {}", drvrs.size());
sta::Level prev_level = -1;
int n = 0;
for (sta::Vertex *vertex : drvrs) {
sta::Level level = vertex->level();
if (level < prev_level) {
report->report("level order violated {} {} < {}",
network->pathName(vertex->pin()),
level, prev_level);
}
if (n < max_count) {
report->report("{} level={}",
network->pathName(vertex->pin()),
level);
}
prev_level = level;
n++;
}
}

void check_axioms_cmd()
{
ord::OpenRoad* openroad = ord::getOpenRoad();
Expand Down
11 changes: 11 additions & 0 deletions src/dbSta/test/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ ALL_TESTS = [
"hierwrite",
"hier_deep",
"hier_weird_port",
"levelize_drvr_vertices1",
"make_port",
"network_edit1",
"power1",
Expand Down Expand Up @@ -133,6 +134,16 @@ filegroup(
"hierclock_gate.v",
"hierclock_out.vok",
],
"levelize_drvr_vertices1": [
"gcd_asap7.v",
"asap7/asap7sc7p5t_AO_RVT_FF_nldm_211120.lib.gz",
"asap7/asap7sc7p5t_INVBUF_RVT_FF_nldm_220122.lib.gz",
"asap7/asap7sc7p5t_OA_RVT_FF_nldm_211120.lib.gz",
"asap7/asap7sc7p5t_SIMPLE_RVT_FF_nldm_211120.lib.gz",
"asap7/asap7sc7p5t_SEQ_RVT_FF_nldm_220123.lib",
"asap7/asap7_tech_1x_201209.lef",
"asap7/asap7sc7p5t_28_R_1x_220121a.lef",
],
"network_edit1": [
"reg3.def",
],
Expand Down
1 change: 1 addition & 0 deletions src/dbSta/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ or_integration_tests(
hierwrite
hier_deep
hier_weird_port
levelize_drvr_vertices1
make_port
network_edit1
power1
Expand Down
Loading
Loading