Skip to content

added driver vertex levelization#327

Merged
maliberty merged 3 commits into
masterfrom
driver-vertex-level-cache
Apr 2, 2026
Merged

added driver vertex levelization#327
maliberty merged 3 commits into
masterfrom
driver-vertex-level-cache

Conversation

@precisionmoon
Copy link
Copy Markdown
Contributor

No description provided.

Signed-off-by: Cho Moon <cmoon@precisioninno.com>
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 31, 2026

CLA assistant check
All committers have signed the CLA.

Signed-off-by: Cho Moon <cmoon@precisioninno.com>
@dsengupta0628
Copy link
Copy Markdown
Contributor

Hi @precisionmoon I was looking at OpenSTA PRs and came across this one. I have some comments regarding this change (got reviewed by AI)

  1. Encapsulation violation — Sta::levelize() breaks the facade pattern

Sta is explicitly documented as a FACADE (see Sta.hh:98):
"This class is a FACADE used to present an API to the collection of objects..."

Adding Levelize* levelize() const { return levelize_; } leaks the internal component. Instead, add a delegating method on Sta:

// In Sta.hh public section:
const VertexSeq &levelizedDrvrVertices();

// In Sta.cc:

  const VertexSeq &
  Sta::levelizedDrvrVertices()
  {
    return levelize_->levelizedDrvrVertices();
  }

This keeps the facade intact and doesn't require callers to know about Levelize at all.

--
2. Non-deterministic sort - no secondary key

The sort comparator is:

  sort(levelized_drvr_vertices_,
       [](const Vertex *a, const Vertex *b) {
         return a->level() < b->level();
       });

std::sort is not stable. Vertices at the same level will appear in arbitrary order, which can vary between runs. This is inconsistent with the existing codebase pattern — see Levelize.cc:258-259 where sortedRootsWithFanout() uses VertexNameLess(network_) for determinism, and findCycleBackEdges() at line 308-309 does the same.

If any consumer iterates this for delay calculation or reporting, results could be non-reproducible. Fix by either:

  • Using std::stable_sort, or
  • Adding a secondary sort key (e.g., vertex name or pin name) for tie-breaking
  sort(levelized_drvr_vertices_,
       [&](const Vertex *a, const Vertex *b) {
         if (a->level() != b->level())
           return a->level() < b->level();
         return VertexNameLess(network_)(a, b);
       });

  1. clear() doesn't clear the cached vector

The clear() method sets drvr_vertices_level_valid_ = false but doesn't clear levelized_drvr_vertices_. Every other container in clear() is explicitly cleared (roots_.clear(), loops_.clear(), loop_edges_.clear()). The cached vector should be too:

  drvr_vertices_level_valid_ = false;
  levelized_drvr_vertices_.clear();

This also avoids holding stale/dangling pointers between a clear() and the next rebuild.


  1. deleteVertexBefore — dangling pointer risk

After deleteVertexBefore(vertex), the vertex is about to be destroyed, but levelized_drvr_vertices_ may still contain the pointer. The flag is set false, so levelizedDrvrVertices() would rebuild before returning — but the raw vector member is accessible to subclasses (it's protected). At minimum, also clear the vector:

  void
  Levelize::deleteVertexBefore(Vertex *vertex)
  {
    if (levelized_) {
      roots_.erase(vertex);
      relevelize_from_.erase(vertex);
      drvr_vertices_level_valid_ = false;
      levelized_drvr_vertices_.clear();  // prevent dangling pointers
    }
  }

  1. Missing invalidation in invalid() when !levelized_

In invalid(), the drvr_vertices_level_valid_ = false is inside the if (levelized_) guard. This is technically correct today because drvr_vertices_level_valid_ can only be true when levelized_ is true — the only path that sets it true (levelizeDrvrVertices()) goes through ensureLevelized() first, which guarantees levelized_ == true.

However, this makes the correctness non-obvious and dependent on call ordering across multiple methods. Consider moving it outside the guard:

  void Levelize::invalid()
  {
    drvr_vertices_level_valid_ = false;
    if (levelized_) {
      levelized_ = false;
      levels_valid_ = false;
    }
  }

  1. Minor: reserve() for performance

levelizeDrvrVertices() iterates all vertices, filtering for drivers. On large designs this could cause multiple vector reallocations. A reserve() based on a fraction of total vertex count would help:

  void Levelize::levelizeDrvrVertices() {
    levelized_drvr_vertices_.clear();
    // Rough estimate: ~half of vertices are drivers
    levelized_drvr_vertices_.reserve(graph_->vertexCount() / 2);
    ...
  }

@dsengupta0628
Copy link
Copy Markdown
Contributor

DESCRIPTION

This PR adds a way to get all driver vertices from the timing graph, sorted by their level (topological depth), with lazy caching.

What it adds

One new data structure — a cached VertexSeq (std::vector<Vertex*>) of driver vertices sorted by level, plus a bool dirty flag.

Two new methods on Levelize:

  • levelizeDrvrVertices() (protected) — iterates every vertex in the graph, filters for those where vertex->isDriver(network_) is true, collects them into the vector, and sorts by vertex->level().
  • levelizedDrvrVertices() (public) — the accessor. Calls ensureLevelized() to make sure graph levels are up to date, then rebuilds the cache only if the dirty flag is set. Returns a const reference to the
    sorted vector.

Cache invalidation — sets drvr_vertices_level_valid_ = false in every method that can change the graph structure or vertex levels: clear(), invalid(), deleteVertexBefore(), relevelizeFrom(),
deleteEdgeBefore().

One new accessor on Sta — Levelize* levelize() const to let external code reach the Levelize object and call levelizedDrvrVertices().

What it does NOT do

  • It doesn't change any existing behavior or timing results.
  • It doesn't add any callers of the new API — this is purely infrastructure for some future consumer.
  • It doesn't add tests.

In plain terms

Before this PR, if you wanted all driver pins in topological order, you'd have to walk the entire graph, filter for drivers, and sort them yourself — every time. This PR does that once and caches the result,
automatically invalidating the cache whenever the graph changes.

@dsengupta0628 dsengupta0628 self-requested a review April 2, 2026 02:04
Copy link
Copy Markdown
Contributor

@dsengupta0628 dsengupta0628 left a comment

Choose a reason for hiding this comment

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

Added in detailed comment section

Signed-off-by: Cho Moon <cmoon@precisioninno.com>
@maliberty maliberty merged commit 94bb37b into master Apr 2, 2026
7 of 9 checks passed
@maliberty maliberty deleted the driver-vertex-level-cache branch April 2, 2026 21:22
eder-matheus pushed a commit to eder-matheus/OpenSTA that referenced this pull request Apr 11, 2026
…roject#327

Signed-off-by: James Cherry <cherry@parallaxsw.com>
maliberty added a commit that referenced this pull request May 10, 2026
…vr_level_to_dbsta

Revert driver-vertex levelization (PR #327): move to OpenROAD/dbSta
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants