Merge pull request #10352 from The-OpenROAD-Project-staging/or_move_drvr_level_to_dbsta

 dbSta: move levelizedDrvrVertices() out of OpenSTA fork

Author: maliberty

docs/user/LevelizeObserver-upstream-plan.md

@@ -0,0 +1,223 @@
+# Proposed OpenSTA upstream changes — `LevelizeObserver` + `Sta::setLevelizeObserver`
+
+## Status
+
+Proposed. Not yet submitted upstream.
+
+- OpenSTA branch staged at: `parallaxsw/OpenSTA` fork
+- OpenROAD dependency: blocked until upstream lands.
+
+## Motivation
+
+OpenROAD's `dbSta` needs to attach its own `LevelizeObserver` so the
+driver-vertex cache (`dbSta::levelizedDrvrVertices()`) is invalidated when
+the timing graph levels change. Today this requires three workarounds:
+
+1. **`OPENSTA_HOME` private include path.** `LevelizeObserver` is declared in
+   `sta/search/Levelize.hh`, which is not part of OpenSTA's public include.
+   `src/dbSta/src/CMakeLists.txt` adds `${OPENSTA_HOME}` and
+   `${OPENSTA_HOME}/include/sta` as `PRIVATE` includes on `dbSta_lib` to
+   reach it.
+2. **Reaching into `Sta::levelize_`.** `dbSta::makeObservers` calls
+   `levelize_->setObserver(...)` directly. `levelize_` is `protected` on
+   `StaState`, so it works, but it is a private contract.
+3. **Replicating `StaLevelizeObserver` behavior.**
+   `Levelize::setObserver` overwrites and deletes any prior observer (single
+   slot, no chain). The observer that `Sta::makeObservers` installs —
+   `StaLevelizeObserver` — forwards level-change events to `Search` and
+   `GraphDelayCalc` so their incremental-update iterators stay consistent.
+   Because `dbSta` must replace that observer, it has to duplicate the
+   forwarding by hand inside `DbStaLevelizeObserver`. If upstream ever adds
+   work to `StaLevelizeObserver`, the dbSta copy silently drifts.
+
+Reviewer feedback (gemini) on the OpenROAD PR called out both points:
+
+> The `dbSta::makeObservers` override replaces the `StaLevelizeObserver`
+> created by the base class. ... it would be safer to verify if OpenSTA
+> could be updated to support observer chaining or multiple observers.
+
+> 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.
+
+> 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`.
+
+## Proposed OpenSTA changes
+
+Five small changes, ~12 LOC of behavior. No functional change to existing
+flows.
+
+### 1. New public header `include/sta/LevelizeObserver.hh`
+
+Holds both observer classes that previously lived in
+`search/Levelize.hh` and `search/Sta.cc`.
+
+```cpp
+namespace sta {
+
+class Vertex;
+class Search;
+class GraphDelayCalc;
+
+class LevelizeObserver {
+public:
+  virtual ~LevelizeObserver() = default;
+  virtual void levelsChangedBefore() = 0;
+  virtual void levelChangedBefore(Vertex *vertex) = 0;
+};
+
+class StaLevelizeObserver : public LevelizeObserver {
+public:
+  StaLevelizeObserver(Search *search, GraphDelayCalc *graph_delay_calc);
+  void levelsChangedBefore() override;
+  void levelChangedBefore(Vertex *vertex) override;
+private:
+  Search *search_;
+  GraphDelayCalc *graph_delay_calc_;
+};
+
+} // namespace sta
+```
+
+### 2. `search/Levelize.hh`
+
+Drop the inline `LevelizeObserver` class body. Include the new public
+header.
+
+```diff
++#include "sta/LevelizeObserver.hh"
+ ...
+-class LevelizeObserver
+-{ ... };
+```
+
+### 3. `search/Sta.cc`
+
+Drop the file-local `StaLevelizeObserver` class body. Keep the function
+definitions (they now define the public class).
+
+### 4. `include/sta/Sta.hh`
+
+Add public setter.
+
+```diff
++class LevelizeObserver;
+ ...
++  // Replace the Levelize observer. Takes ownership; deletes any prior
++  // observer. Subclass StaLevelizeObserver to extend the default behavior
++  // (Search + GraphDelayCalc forwarding) without re-implementing it.
++  void setLevelizeObserver(LevelizeObserver *observer);
+```
+
+### 5. `search/Sta.cc`
+
+One-line implementation.
+
+```cpp
+void Sta::setLevelizeObserver(LevelizeObserver *observer) {
+  levelize_->setObserver(observer);
+}
+```
+
+CMake / Bazel pick up the new header automatically:
+- CMake `install(DIRECTORY include/sta DESTINATION include)`.
+- Bazel `opensta_lib` already globs `include/sta/*.hh`.
+
+## OpenROAD follow-up after upstream lands
+
+### `src/dbSta/src/dbSta.cc`
+
+Replace the current observer + override with the cleaner version that
+inherits from `StaLevelizeObserver` and uses `Sta::setLevelizeObserver`:
+
+```cpp
+#include "sta/LevelizeObserver.hh"   // public — no OPENSTA_HOME hack
+
+class DbStaLevelizeObserver : public StaLevelizeObserver
+{
+ public:
+  DbStaLevelizeObserver(dbSta* sta, Search* s, GraphDelayCalc* gdc)
+      : StaLevelizeObserver(s, gdc), sta_(sta) {}
+
+  void levelsChangedBefore() override {
+    StaLevelizeObserver::levelsChangedBefore();   // parent: Search + GDC
+    sta_->invalidateLevelizedDrvrVertices();
+  }
+  void levelChangedBefore(Vertex* v) override {
+    StaLevelizeObserver::levelChangedBefore(v);
+    sta_->invalidateLevelizedDrvrVertices();
+  }
+
+ private:
+  dbSta* sta_;
+};
+
+void dbSta::makeObservers() {
+  Sta::makeObservers();
+  setLevelizeObserver(
+      new DbStaLevelizeObserver(this, search_, graph_delay_calc_));
+}
+```
+
+Differences from the current implementation:
+
+- `#include "sta/LevelizeObserver.hh"` instead of
+  `#include "search/Levelize.hh"`.
+- `DbStaLevelizeObserver` inherits from `StaLevelizeObserver` (public class)
+  and calls `StaLevelizeObserver::levelsChangedBefore()` /
+  `levelChangedBefore()` instead of hand-replicating the Search +
+  GraphDelayCalc forwarding.
+- `setLevelizeObserver(...)` replaces the previous
+  `levelize_->setObserver(...)` reach-in.
+
+### `src/dbSta/src/CMakeLists.txt`
+
+Drop the private include hack:
+
+```diff
+ 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
+ )
+```
+
+`dbSta_lib`'s public dependency on the `OpenSTA` CMake target already
+exposes `include/`, and Bazel's `opensta_lib` `hdrs` already lists the new
+header — both pick up `sta/LevelizeObserver.hh` without extra wiring.
+
+### What does **not** change
+
+- `dbSta::levelizedDrvrVertices()` body, the cache invariant, the
+  `bool drvr_vertices_level_valid_` flag, and the `clear()` inside
+  `invalidateLevelizedDrvrVertices()` are all independent of the upstream
+  change. The `clear()` is still required because the cache vector must be
+  empty when the flag is `false` — otherwise the next rebuild's
+  `push_back` appends to stale (and possibly dangling) contents. See
+  `dbSta::invalidateLevelizedDrvrVertices()` doc.
+- `dbStaCbk::inDbInstCreate` / `inDbInstDestroy` calls to
+  `invalidateLevelizedDrvrVertices()` remain — they plug the gap where
+  `Sta::deleteLeafInstanceBefore` bypasses the `LevelizeObserver` path.
+- Existing rsz call sites (`Resizer.cc`, `RepairDesign.cc`, `Rebuffer.cc`)
+  are untouched.
+- Regression test `src/dbSta/test/levelize_drvr_vertices1.tcl` and its
+  golden remain valid.
+
+## Why this is worth it
+
+| Concern | Today | After upstream |
+|---|---|---|
+| `OPENSTA_HOME` PRIVATE include | required | removed |
+| Reach into `Sta::levelize_` | yes | no |
+| Replicate `StaLevelizeObserver` forwarding | yes (drift risk) | no (inherit) |
+| Upstream adds new work to `StaLevelizeObserver` | silently lost in dbSta | inherited automatically |
+
+

src/dbSta/include/db_sta/dbSta.hh

@@ -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"
@@ -83,6 +84,7 @@ class dbSta;
 class dbNetwork;
 class dbStaReport;
 class dbStaCbk;
+class DbStaLevelizeObserver;
 class PatternMatch;
 class TestCell;
 
@@ -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();
+  // 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,
@@ -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

src/dbSta/src/CMakeLists.txt

@@ -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
 )
 
 

src/dbSta/src/dbSta.cc

@@ -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"
@@ -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"
@@ -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));
+}
+
+void dbSta::invalidateLevelizedDrvrVertices()
+{
+  if (drvr_vertices_level_valid_) {
+    drvr_vertices_level_valid_ = false;
+    levelized_drvr_vertices_.clear();
+  }
+}
+
+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_;
+}
+
 void dbSta::postReadLef(odb::dbTech* tech, odb::dbLib* library)
 {
   if (library) {
@@ -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)