Skip to content

Commit dbe5123

Browse files
dsengupta0628dsengupta0628
committed
add todo item post upstream change
Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
1 parent d6ec358 commit dbe5123

1 file changed

Lines changed: 223 additions & 0 deletions

File tree

‎docs/user/LevelizeObserver-upstream-plan.md‎

Lines changed: 223 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
1+
# Proposed OpenSTA upstream changes — `LevelizeObserver` + `Sta::setLevelizeObserver`
2+
3+
## Status
4+
5+
Proposed. Not yet submitted upstream.
6+
7+
- OpenSTA branch staged at: `parallaxsw/OpenSTA` fork
8+
- OpenROAD dependency: blocked until upstream lands.
9+
10+
## Motivation
11+
12+
OpenROAD's `dbSta` needs to attach its own `LevelizeObserver` so the
13+
driver-vertex cache (`dbSta::levelizedDrvrVertices()`) is invalidated when
14+
the timing graph levels change. Today this requires three workarounds:
15+
16+
1. **`OPENSTA_HOME` private include path.** `LevelizeObserver` is declared in
17+
`sta/search/Levelize.hh`, which is not part of OpenSTA's public include.
18+
`src/dbSta/src/CMakeLists.txt` adds `${OPENSTA_HOME}` and
19+
`${OPENSTA_HOME}/include/sta` as `PRIVATE` includes on `dbSta_lib` to
20+
reach it.
21+
2. **Reaching into `Sta::levelize_`.** `dbSta::makeObservers` calls
22+
`levelize_->setObserver(...)` directly. `levelize_` is `protected` on
23+
`StaState`, so it works, but it is a private contract.
24+
3. **Replicating `StaLevelizeObserver` behavior.**
25+
`Levelize::setObserver` overwrites and deletes any prior observer (single
26+
slot, no chain). The observer that `Sta::makeObservers` installs —
27+
`StaLevelizeObserver` — forwards level-change events to `Search` and
28+
`GraphDelayCalc` so their incremental-update iterators stay consistent.
29+
Because `dbSta` must replace that observer, it has to duplicate the
30+
forwarding by hand inside `DbStaLevelizeObserver`. If upstream ever adds
31+
work to `StaLevelizeObserver`, the dbSta copy silently drifts.
32+
33+
Reviewer feedback (gemini) on the OpenROAD PR called out both points:
34+
35+
> The `dbSta::makeObservers` override replaces the `StaLevelizeObserver`
36+
> created by the base class. ... it would be safer to verify if OpenSTA
37+
> could be updated to support observer chaining or multiple observers.
38+
39+
> It would make more sense for this observer to inherit from the existing
40+
> observer and then call the parent class function instead of duplicating
41+
> the contents.
42+
43+
> I think it makes more sense to add a set function for the levelize
44+
> observer on the `Sta` class than to change the include search path to get
45+
> access to `Levelize.hh`.
46+
47+
## Proposed OpenSTA changes
48+
49+
Five small changes, ~12 LOC of behavior. No functional change to existing
50+
flows.
51+
52+
### 1. New public header `include/sta/LevelizeObserver.hh`
53+
54+
Holds both observer classes that previously lived in
55+
`search/Levelize.hh` and `search/Sta.cc`.
56+
57+
```cpp
58+
namespace sta {
59+
60+
class Vertex;
61+
class Search;
62+
class GraphDelayCalc;
63+
64+
class LevelizeObserver {
65+
public:
66+
virtual ~LevelizeObserver() = default;
67+
virtual void levelsChangedBefore() = 0;
68+
virtual void levelChangedBefore(Vertex *vertex) = 0;
69+
};
70+
71+
class StaLevelizeObserver : public LevelizeObserver {
72+
public:
73+
StaLevelizeObserver(Search *search, GraphDelayCalc *graph_delay_calc);
74+
void levelsChangedBefore() override;
75+
void levelChangedBefore(Vertex *vertex) override;
76+
private:
77+
Search *search_;
78+
GraphDelayCalc *graph_delay_calc_;
79+
};
80+
81+
} // namespace sta
82+
```
83+
84+
### 2. `search/Levelize.hh`
85+
86+
Drop the inline `LevelizeObserver` class body. Include the new public
87+
header.
88+
89+
```diff
90+
+#include "sta/LevelizeObserver.hh"
91+
...
92+
-class LevelizeObserver
93+
-{ ... };
94+
```
95+
96+
### 3. `search/Sta.cc`
97+
98+
Drop the file-local `StaLevelizeObserver` class body. Keep the function
99+
definitions (they now define the public class).
100+
101+
### 4. `include/sta/Sta.hh`
102+
103+
Add public setter.
104+
105+
```diff
106+
+class LevelizeObserver;
107+
...
108+
+ // Replace the Levelize observer. Takes ownership; deletes any prior
109+
+ // observer. Subclass StaLevelizeObserver to extend the default behavior
110+
+ // (Search + GraphDelayCalc forwarding) without re-implementing it.
111+
+ void setLevelizeObserver(LevelizeObserver *observer);
112+
```
113+
114+
### 5. `search/Sta.cc`
115+
116+
One-line implementation.
117+
118+
```cpp
119+
void Sta::setLevelizeObserver(LevelizeObserver *observer) {
120+
levelize_->setObserver(observer);
121+
}
122+
```
123+
124+
CMake / Bazel pick up the new header automatically:
125+
- CMake `install(DIRECTORY include/sta DESTINATION include)`.
126+
- Bazel `opensta_lib` already globs `include/sta/*.hh`.
127+
128+
## OpenROAD follow-up after upstream lands
129+
130+
### `src/dbSta/src/dbSta.cc`
131+
132+
Replace the current observer + override with the cleaner version that
133+
inherits from `StaLevelizeObserver` and uses `Sta::setLevelizeObserver`:
134+
135+
```cpp
136+
#include "sta/LevelizeObserver.hh" // public — no OPENSTA_HOME hack
137+
138+
class DbStaLevelizeObserver : public StaLevelizeObserver
139+
{
140+
public:
141+
DbStaLevelizeObserver(dbSta* sta, Search* s, GraphDelayCalc* gdc)
142+
: StaLevelizeObserver(s, gdc), sta_(sta) {}
143+
144+
void levelsChangedBefore() override {
145+
StaLevelizeObserver::levelsChangedBefore(); // parent: Search + GDC
146+
sta_->invalidateLevelizedDrvrVertices();
147+
}
148+
void levelChangedBefore(Vertex* v) override {
149+
StaLevelizeObserver::levelChangedBefore(v);
150+
sta_->invalidateLevelizedDrvrVertices();
151+
}
152+
153+
private:
154+
dbSta* sta_;
155+
};
156+
157+
void dbSta::makeObservers() {
158+
Sta::makeObservers();
159+
setLevelizeObserver(
160+
new DbStaLevelizeObserver(this, search_, graph_delay_calc_));
161+
}
162+
```
163+
164+
Differences from the current implementation:
165+
166+
- `#include "sta/LevelizeObserver.hh"` instead of
167+
`#include "search/Levelize.hh"`.
168+
- `DbStaLevelizeObserver` inherits from `StaLevelizeObserver` (public class)
169+
and calls `StaLevelizeObserver::levelsChangedBefore()` /
170+
`levelChangedBefore()` instead of hand-replicating the Search +
171+
GraphDelayCalc forwarding.
172+
- `setLevelizeObserver(...)` replaces the previous
173+
`levelize_->setObserver(...)` reach-in.
174+
175+
### `src/dbSta/src/CMakeLists.txt`
176+
177+
Drop the private include hack:
178+
179+
```diff
180+
target_include_directories(dbSta_lib
181+
PUBLIC
182+
../include
183+
${PROJECT_SOURCE_DIR}/include
184+
- PRIVATE
185+
- # Needed for search/Levelize.hh and the unprefixed transitive
186+
- # OpenSTA headers it includes (e.g. Graph.hh) which are not part of
187+
- # OpenSTA's public include.
188+
- ${OPENSTA_HOME}
189+
- ${OPENSTA_HOME}/include/sta
190+
)
191+
```
192+
193+
`dbSta_lib`'s public dependency on the `OpenSTA` CMake target already
194+
exposes `include/`, and Bazel's `opensta_lib` `hdrs` already lists the new
195+
header — both pick up `sta/LevelizeObserver.hh` without extra wiring.
196+
197+
### What does **not** change
198+
199+
- `dbSta::levelizedDrvrVertices()` body, the cache invariant, the
200+
`bool drvr_vertices_level_valid_` flag, and the `clear()` inside
201+
`invalidateLevelizedDrvrVertices()` are all independent of the upstream
202+
change. The `clear()` is still required because the cache vector must be
203+
empty when the flag is `false` — otherwise the next rebuild's
204+
`push_back` appends to stale (and possibly dangling) contents. See
205+
`dbSta::invalidateLevelizedDrvrVertices()` doc.
206+
- `dbStaCbk::inDbInstCreate` / `inDbInstDestroy` calls to
207+
`invalidateLevelizedDrvrVertices()` remain — they plug the gap where
208+
`Sta::deleteLeafInstanceBefore` bypasses the `LevelizeObserver` path.
209+
- Existing rsz call sites (`Resizer.cc`, `RepairDesign.cc`, `Rebuffer.cc`)
210+
are untouched.
211+
- Regression test `src/dbSta/test/levelize_drvr_vertices1.tcl` and its
212+
golden remain valid.
213+
214+
## Why this is worth it
215+
216+
| Concern | Today | After upstream |
217+
|---|---|---|
218+
| `OPENSTA_HOME` PRIVATE include | required | removed |
219+
| Reach into `Sta::levelize_` | yes | no |
220+
| Replicate `StaLevelizeObserver` forwarding | yes (drift risk) | no (inherit) |
221+
| Upstream adds new work to `StaLevelizeObserver` | silently lost in dbSta | inherited automatically |
222+
223+

0 commit comments

Comments
 (0)