[dbSta] driver cache: key by canonical flat dbNet (root-cause fix for #10327)#10344
[dbSta] driver cache: key by canonical flat dbNet (root-cause fix for #10327)#10344oharboe wants to merge 1 commit into
Conversation
Resizer::eliminateDeadLogic was dominated on hierarchical designs by
dbNetwork::disconnectPinBefore -> dbNet::findRelatedModNets, an
unmemoized DFS through the modnet hierarchy fired on every pin
disconnect.
The DFS only existed to keep the inherited net_drvr_pin_map_ driver
cache in sync when a single physical net was named by multiple
dbModNets in the hierarchy: each modnet had its own cache entry, and
disconnecting a pin had to walk the full reachable component to evict
all of them. The cache content for every such entry was identical
(dbNetwork::drivers populated each from the flat net's
getFirstDriverTerm()), so the per-modnet entries were redundant.
Make the cache canonical: dbNetwork::drivers translates any input net
to its flat dbNet via findFlatDbNet() and keys the cache by that flat
dbNet only. disconnectPinBefore can then evict in O(1) -- just one
flat-net entry -- with no findRelatedModNets call. For hierarchical
pins (case 2) the modnet-to-flat-net translation is now memoized in
dbNetwork (modnet_to_flat_net_cache_), invalidated by dbStaCbk on the
events that can change the mapping (modnet/modITerm/modBTerm
create/destroy/connect/disconnect, plus flat iterm/bterm disconnects
that touch a modnet, plus dbNet destroy).
No new public API and no specialized "begin/end" mode: every caller of
sta::deleteInstance benefits, not just eliminateDeadLogic. Existing
rsz eliminate_dead_logic{1,2} tests pass unchanged; full //src/...
test suite (1547 targets) passes.
This supersedes the rejected approach in PR The-OpenROAD-Project#10327 and closes the
underlying complaint in issue The-OpenROAD-Project#10329.
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@povik I don't think it makes sense for me to drive this. I have no idea what this means. |
![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 introduces a memoization cache for mapping hierarchical nets to flat nets and refactors the driver cache to use canonical flat nets as keys, improving performance and simplifying invalidation logic. Feedback highlights a potential bug where disconnecting hierarchical pins might fail to clear stale driver cache entries and a thread-safety issue caused by using a static local variable initialized with an instance pointer.
| if (dbNet* db_net = findFlatDbNet(dbToSta(modnet))) { | ||
| removeDriverFromCache(dbToSta(db_net), pin); | ||
| } |
There was a problem hiding this comment.
When disconnecting a hierarchical pin (moditerm), removeDriverFromCache is called with the pin object representing the moditerm. However, the driver cache (net_drvr_pin_map_) stores Pin* objects representing flat driver pins (dbITerm or dbBTerm).
The Pin* for a dbModITerm is different from the Pin* for a dbITerm/dbBTerm, so the call to entry->second->erase(drvr) inside removeDriverFromCache will fail to find and remove the driver. This will leave stale entries in the cache.
A safer approach would be to invalidate the entire cache entry for the flat net, forcing it to be recomputed on the next access. This can be done by using the one-argument version of removeDriverFromCache.
if (dbNet* db_net = findFlatDbNet(dbToSta(modnet))) {
removeDriverFromCache(dbToSta(db_net));
}| static PinSet empty(this); | ||
| return ∅ |
There was a problem hiding this comment.
The use of a function-local static variable empty initialized with this can lead to incorrect behavior when multiple dbNetwork objects are used. The static PinSet is initialized only once, during the first call to this function path. Subsequent calls from different dbNetwork instances will receive a PinSet associated with the Network* of the first instance, not their own. This can cause subtle bugs or crashes.
To fix this, empty should be an instance member of dbNetwork rather than a static local variable.
For example, you could add a private member to dbNetwork in src/dbSta/include/db_sta/dbNetwork.hh:
class dbNetwork : public ConcreteNetwork
{
// ...
private:
// ...
PinSet empty_pin_set_{this};
};And then change this function to return a pointer to it:
if (db_net == nullptr) {
return &empty_pin_set_;
}
|
This PR removes flat nets only in the cache, and does not remove hier nets, which is incomplete. |
Yes: out of my depth... |
2 participants
Summary
Follow-up to #10327, addressing review feedback from @povik:
This PR fixes the root cause in
dbNetwork::disconnectPinBeforeso plainsta::deleteInstanceis fast on hierarchical designs without any new"begin/end bulk delete" API. Closes #10329.
Root cause
Each
sta_->deleteInstance(leaf)triggers, per pin disconnect, thecallback chain
dbStaCbk::inDbITermPreDisconnect→dbNetwork::disconnectPinBefore(pin)(src/dbSta/src/dbNetwork.cc:2782).For driver pins,
disconnectPinBeforepreviously calleddbNet::findRelatedModNets(src/odb/src/db/dbNet.cpp:2486) — anunmemoized DFS through the modnet hierarchy graph allocating a fresh
std::set<dbModNet*>per call. For hierarchical pins it additionallycalled
dbModNet::findRelatedNet.Why those DFS calls existed: the inherited driver cache
net_drvr_pin_map_(declared in upstream OpenSTAsrc/sta/network/Network.cc— must NOT be modified per CLAUDE.md rule #1) was keyed by raw
Net*pointer, accepting both
dbNet*(flat) anddbModNet*(hier) keys. Asingle physical net named by N modnets in the hierarchy had up to N
redundant entries with identical content (each populated by
findFlatDbNet(net)->getFirstDriverTerm()); evicting them all requiredwalking the modnet graph.
Per-disconnect work was O(reachable modnets); across an
eliminate_dead_logicsweep deleting M leaf instances with ~P pins each,total cost was O(M × P × reachable_modnets). On the hierarchical asap7
design measured in #10327, this dominated
synth_odbwall time.Change
Canonicalize the cache key to flat
dbNet*.dbNetwork::drivers(const Net* net)translates any input net to itsflat
dbNetviafindFlatDbNet()and uses that as the cache key.Modnet inputs converge on the same canonical entry; the cache holds
at most one entry per flat net.
dbNetwork::disconnectPinBeforeevicts one flat-net entry per pin —no DFS through the modnet hierarchy. The
findRelatedModNetscallsare removed.
findFlatDbNetmemoizes thedbModNet → dbNettranslation(
modnet_to_flat_net_cache_);dbStaCbkinvalidates it on the eventsthat can change a modnet's flat-net mapping (modnet/modITerm/modBTerm
create/destroy/connect/disconnect, plus
dbNetdestroy and flatiterm/bterm connects/disconnects that touch a modnet).
No new public API. No specialized "begin/end bulk delete" mode. Every
caller of
sta::deleteInstancebenefits, not justeliminateDeadLogic.Why this is safe
drivers()consume the returnedPinSet*immediately and do not rely on per-modnet cache identity (verified
across rsz, dbSta, est, gui, drt, and upstream sta callers).
PinSetcontent for a modnet was always equal to the flat net'scontent; sharing one entry produces identical observable behavior.
removeDriverFromCache(net, pin)(the two-arg form) erases a singlePin and composes correctly with multi-driver flat nets.
dbNet::findRelatedModNetsitself is unchanged — sanity checks, dump,GUI, and
dbInsertBuffercontinue to use it.Test plan
eliminate_dead_logic{1,2}.tclregressions pass.Performance
Measured on `asap7/swerv_wrapper` (`OPENROAD_HIERARCHICAL=1`,
`SYNTH_KEEP_MODULES` per design config), one run each:
Honest caveat: at this scale (1264 dead instances, ~70× smaller than
the S size in #10327's reported benchmark) the modnet DFS is not the
dominant cost — even #10327's specialized bulk-delete path doesn't
move the needle here. The savings show up at larger scale (per #10327:
S=87k objs/753s → L=574k objs/1675s on a hierarchical design with many
`keep_hierarchy` boundaries). Could a maintainer with access to such a
workload re-time on this branch? The structural change should match
#10327's gains by construction (same DFS calls eliminated) without
adding the specialized API.
Out of scope
`levelize_->relevelizeFrom` cost in `Sta::disconnectPinBefore`
(issue Speed up synth_odb.tcl eliminate dead logic #10329's "STA cache update" theory, @jhkim-pii Theory B).
Touching `src/sta/` is forbidden per CLAUDE.md rule Update install target to also install the /etc directory with the flu… #1; would need
a separate upstream conversation.
`dbModNet`. Cleaner long-term but not needed for this PR's gains.
🤖 Generated with Claude Code