rsz: replace BfsIterator pull-API usage with local DFS traversal#10862
rsz: replace BfsIterator pull-API usage with local DFS traversal#10862dsengupta0628 wants to merge 8 commits into
Conversation
Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
![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 new test case gcd_restructure_delay and replaces the pull-style sta::BfsFwdIterator and sta::BfsBkwdIterator traversals in Resizer.cc with a custom iterative DFS implementation (dfsSearch). It also updates findClkInverters to sort the discovered inverters by level. The reviewer feedback focuses on optimizing the performance of dfsSearch by templating it on the traversal direction and callback types to eliminate std::function overhead and runtime branching, as well as templating on the seed container type to avoid copying vertex sets into temporary vectors.
| static void dfsSearch(sta::Graph* graph, | ||
| sta::SearchPred& pred, | ||
| DfsDirection dir, | ||
| const std::vector<sta::Vertex*>& seeds, | ||
| bool visit_seeds, | ||
| const std::function<bool(sta::Vertex*)>& visit) | ||
| { | ||
| std::vector<sta::Vertex*> stack; | ||
| sta::VertexSet visited(graph); | ||
|
|
||
| auto expand = [&](sta::Vertex* vertex) { | ||
| if (dir == DfsDirection::fanout) { | ||
| if (pred.searchFrom(vertex)) { | ||
| sta::VertexOutEdgeIterator edge_iter(vertex, graph); | ||
| while (edge_iter.hasNext()) { | ||
| sta::Edge* edge = edge_iter.next(); | ||
| sta::Vertex* to_vertex = edge->to(graph); | ||
| if (pred.searchThru(edge) && pred.searchTo(to_vertex) | ||
| && visited.insert(to_vertex).second) { | ||
| stack.push_back(to_vertex); | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| if (pred.searchTo(vertex)) { | ||
| sta::VertexInEdgeIterator edge_iter(vertex, graph); | ||
| while (edge_iter.hasNext()) { | ||
| sta::Edge* edge = edge_iter.next(); | ||
| sta::Vertex* from_vertex = edge->from(graph); | ||
| if (pred.searchFrom(from_vertex) && pred.searchThru(edge) | ||
| && visited.insert(from_vertex).second) { | ||
| stack.push_back(from_vertex); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| for (sta::Vertex* seed : seeds) { | ||
| if (visit_seeds) { | ||
| if (visited.insert(seed).second) { | ||
| stack.push_back(seed); | ||
| } | ||
| } else { | ||
| expand(seed); | ||
| } | ||
| } | ||
|
|
||
| while (!stack.empty()) { | ||
| sta::Vertex* vertex = stack.back(); | ||
| stack.pop_back(); | ||
| if (visit(vertex)) { | ||
| expand(vertex); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Using std::function for the callback visit in dfsSearch introduces virtual dispatch and potential dynamic allocation overhead in a performance-critical timing graph traversal. Additionally, checking dir inside the hot expansion logic introduces a runtime branch on every visited vertex.
We can optimize this by templating dfsSearch on the SeedsContainer and VisitFunc types, and passing dir as a template parameter. This allows if constexpr to be used to eliminate the runtime branch entirely, and completely avoids std::function overhead. To comply with repository guidelines, the helper logic for expanding vertices is defined as a free function in an anonymous namespace rather than a local lambda.
namespace {
template <DfsDirection dir>
void dfsSearchExpand(sta::Vertex* vertex,
sta::Graph* graph,
sta::SearchPred& pred,
sta::VertexSet& visited,
std::vector<sta::Vertex*>& stack)
{
if constexpr (dir == DfsDirection::fanout) {
if (pred.searchFrom(vertex)) {
sta::VertexOutEdgeIterator edge_iter(vertex, graph);
while (edge_iter.hasNext()) {
sta::Edge* edge = edge_iter.next();
sta::Vertex* to_vertex = edge->to(graph);
if (pred.searchThru(edge) && pred.searchTo(to_vertex)
&& visited.insert(to_vertex).second) {
stack.push_back(to_vertex);
}
}
}
} else {
if (pred.searchTo(vertex)) {
sta::VertexInEdgeIterator edge_iter(vertex, graph);
while (edge_iter.hasNext()) {
sta::Edge* edge = edge_iter.next();
sta::Vertex* from_vertex = edge->from(graph);
if (pred.searchFrom(from_vertex) && pred.searchThru(edge)
&& visited.insert(from_vertex).second) {
stack.push_back(from_vertex);
}
}
}
}
}
} // namespace
template <DfsDirection dir, typename SeedsContainer, typename VisitFunc>
static void dfsSearch(sta::Graph* graph,
sta::SearchPred& pred,
const SeedsContainer& seeds,
bool visit_seeds,
const VisitFunc& visit)
{
std::vector<sta::Vertex*> stack;
stack.reserve(seeds.size());
sta::VertexSet visited(graph);
for (sta::Vertex* seed : seeds) {
if (visit_seeds) {
if (visited.insert(seed).second) {
stack.push_back(seed);
}
} else {
dfsSearchExpand<dir>(seed, graph, pred, visited, stack);
}
}
while (!stack.empty()) {
sta::Vertex* vertex = stack.back();
stack.pop_back();
if (visit(vertex)) {
dfsSearchExpand<dir>(vertex, graph, pred, visited, stack);
}
}
}References
- Helper logic should be defined as a free function in a namespace rather than as a local lambda within a function.
| const std::vector<sta::Vertex*> seeds(ends.begin(), ends.end()); | ||
| dfsSearch( | ||
| graph_, | ||
| pred, | ||
| DfsDirection::fanin, | ||
| seeds, | ||
| /*visit_seeds=*/false, | ||
| [&](sta::Vertex* vertex) { | ||
| if (isRegOutput(vertex) || network_->isTopLevelPort(vertex->pin())) { | ||
| return false; | ||
| } | ||
| fanins.insert(vertex->pin()); | ||
| return true; | ||
| }); |
There was a problem hiding this comment.
By templating dfsSearch on the SeedsContainer type, we can pass ends (which is a sta::VertexSet) directly to dfsSearch without copying its elements into a temporary std::vector.
dfsSearch<DfsDirection::fanin>(
graph_,
pred,
ends,
/*visit_seeds=*/false,
[&](sta::Vertex* vertex) {
if (isRegOutput(vertex) || network_->isTopLevelPort(vertex->pin())) {
return false;
}
fanins.insert(vertex->pin());
return true;
});| const std::vector<sta::Vertex*> seeds(ends.begin(), ends.end()); | ||
| dfsSearch( | ||
| graph_, | ||
| pred, | ||
| DfsDirection::fanin, | ||
| seeds, | ||
| /*visit_seeds=*/false, | ||
| [&](sta::Vertex* vertex) { | ||
| if (isRegOutput(vertex) || network_->isTopLevelPort(vertex->pin())) { | ||
| roots.insert(vertex); | ||
| return false; | ||
| } | ||
| return true; | ||
| }); |
There was a problem hiding this comment.
By templating dfsSearch on the SeedsContainer type, we can pass ends (which is a sta::VertexSet) directly to dfsSearch without copying its elements into a temporary std::vector.
dfsSearch<DfsDirection::fanin>(
graph_,
pred,
ends,
/*visit_seeds=*/false,
[&](sta::Vertex* vertex) {
if (isRegOutput(vertex) || network_->isTopLevelPort(vertex->pin())) {
roots.insert(vertex);
return false;
}
return true;
});| const std::vector<sta::Vertex*> seeds(reg_outs.begin(), reg_outs.end()); | ||
| dfsSearch(graph_, | ||
| pred, | ||
| DfsDirection::fanout, | ||
| seeds, | ||
| /*visit_seeds=*/false, | ||
| [&](sta::Vertex* vertex) { | ||
| if (isRegister(vertex)) { | ||
| return false; | ||
| } | ||
| fanouts.insert(vertex); | ||
| return true; | ||
| }); |
There was a problem hiding this comment.
By templating dfsSearch on the SeedsContainer type, we can pass reg_outs (which is a sta::VertexSet) directly to dfsSearch without copying its elements into a temporary std::vector.
dfsSearch<DfsDirection::fanout>(
graph_,
pred,
reg_outs,
/*visit_seeds=*/false,
[&](sta::Vertex* vertex) {
if (isRegister(vertex)) {
return false;
}
fanouts.insert(vertex);
return true;
});| dfsSearch( | ||
| graph_, | ||
| srch_pred, | ||
| DfsDirection::fanout, | ||
| seeds, | ||
| /*visit_seeds=*/true, | ||
| [&](sta::Vertex* vertex) { | ||
| const sta::Pin* pin = vertex->pin(); | ||
| sta::Instance* inst = network_->instance(pin); | ||
| sta::LibertyCell* lib_cell = network_->libertyCell(inst); | ||
| if (vertex->isDriver(network_) && lib_cell && lib_cell->isInverter()) { | ||
| inverters.emplace_back(vertex->level(), inst); | ||
| debugPrint(logger_, | ||
| RSZ, | ||
| "repair_clk_inverters", | ||
| 2, | ||
| "inverter {}", | ||
| network_->pathName(inst)); | ||
| } | ||
| return !vertex->isRegClk(); | ||
| }); |
There was a problem hiding this comment.
Update the call to dfsSearch to use the template argument for DfsDirection::fanout.
dfsSearch<DfsDirection::fanout>(
graph_,
srch_pred,
seeds,
/*visit_seeds=*/true,
[&](sta::Vertex* vertex) {
const sta::Pin* pin = vertex->pin();
sta::Instance* inst = network_->instance(pin);
sta::LibertyCell* lib_cell = network_->libertyCell(inst);
if (vertex->isDriver(network_) && lib_cell && lib_cell->isInverter()) {
inverters.emplace_back(vertex->level(), inst);
debugPrint(logger_,
RSZ,
"repair_clk_inverters",
2,
"inverter {}",
network_->pathName(inst));
}
return !vertex->isRegClk();
});
|
Gemini observations are technically accurate, but I profiled the priorities differently, so I'm declining most of this: std::function overhead: The indirect call is real but not measurable here. The per-vertex cost of this traversal is dominated by the visited set insert (std::set, O(log n) with a heap allocation per node) and the SearchPred calls, which are genuinely virtual and additionally iterate over all modes in their mode-less variants. On top of that, both call paths are cold: restructure is an opt-in command and repair_clock_inverters runs once per CTS. std::function visitors are also the established pattern in rsz (see RepairTargetCollector.hh, Rebuffer.cc), so I'd rather stay consistent than template for an unmeasurable win. dir branch in the expansion loop: It's loop-invariant, so it's perfectly predicted after the first iteration - if constexpr would eliminate a branch that's already effectively free. Anonymous-namespace free function "to comply with repository guidelines": I couldn't find such a guideline; the existing file-local helpers in Resizer.cc (SearchPredCombLogic, ClkArrivalSearchPred) don't use one either. Where I agree: if we want a performance pass on this helper, the actual lever is the visited container - sta::VertexSet is an ordered std::set, and since it's used purely for membership (never iterated), switching to std::unordered_set<Vertex*> is |
|
resizer_dfs_mig.pdf |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 585da5dfd1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| set tielo "LOGIC0_X1/Z" | ||
|
|
||
| ord::set_thread_count "3" | ||
| restructure -liberty_file Nangate45/Nangate45_typ.lib -target delay \ |
There was a problem hiding this comment.
In this test, -target delay is not a recognized mode: restructure documents area|timing, and Restructure::setMode only switches to delay optimization for "timing", leaving invalid modes in area mode while warning. The new golden output already includes Mode delay not recognized., so this regression runs the area path (findFaninFanouts) instead of the delay path (findFanins) that this commit is meant to cover, allowing the DFS fanin replacement to regress without the test failing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
actually I now get a seg fault with -target timing in this testcase! Fixing that now before I update this regression
|
Added a new test gcd_restructure_delay.tcl to cover findFanins, which the DFS work rewrote. findFanins sole caller = rmp::Restructure::getBlob in timing mode (-target timing). Zero ORFS exposure, no existing test. Existing rmp restructure test gcd_restructure.tcl uses -target area --> hits findFaninRoots/findFanouts (via findFaninFanouts), not findFanins. So before this, findFanins had no test at all. Why Restructure.cpp needs to be changed So we need a force arrival search like this in non area mode: ... So,
So: worstSlack() = "search ran at all"; null-guard = "this specific endpoint has no path." Without the guard, PathExpanded expanded(path, ...) derefs null --> segfault. So both fixes needed. |
Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
2 participants
Summary
OpenSTA plans to remove BfsIterator::hasNext()/next(), so this PR moves the four remaining rsz users off that API onto a file-local iterative DFS helper in Resizer.cc:
The helper (dfsSearch) reproduces Bfs{Fwd,Bkwd}Iterator::enqueueAdjacentVertices adjacency semantics exactly, including the mode-less SearchPred calls (OR across modes). Explicit stack + visited set: no recursion, terminates on cycles, and drops rsz's
dependence on BfsIndex::other and levelized queue state.
Result sets are unchanged (three functions return unordered sets). findClkInverters now sorts its result by graph level before returning — cloneClkInverter requires upstream-first clone order, previously guaranteed implicitly by BFS level order, now
enforced explicitly.
Also adds rmp/gcd_restructure_delay regression test: delay-mode restructure was the sole caller of findFanins and had zero coverage (existing tests only exercise area mode).
Type of Change
Impact
NA
Verification
./etc/Build.sh).