Skip to content

dpl: fix topological sort cycle with multi-height cells in shift lega… #10137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Divinesoumyadip wants to merge 3 commits into
base: master
Choose a base branch
from
+12 −1
Open

dpl: fix topological sort cycle with multi-height cells in shift lega… #10137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc6dc6d
dpl: fix topological sort cycle with multi-height cells in shift lega…
Divinesoumyadip Apr 15, 2026
7b5d315
dpl: fix non-deterministic sort in compareNodesX with cell ID tie-bre…
Divinesoumyadip Apr 15, 2026
39a00e7
dpl: fix clang-format braces warning in compareNodesX
Divinesoumyadip Apr 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/dpl/src/legalize_shift.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,18 @@ bool ShiftLegalizer::legalize(DetailedMgr& mgr)
outgoing_.resize(size);
for (size_t i = 0; i < mgr.getNumSegments(); i++) {
const int segId = mgr.getSegment(i)->getSegId();
const int rowId = mgr.getSegment(i)->getRowId();
const DbuY rowBottom{arch_->getRow(rowId)->getBottom()};
for (size_t j = 1; j < mgr.getNumCellsInSeg(segId); j++) {
const Node* prev = mgr.getCellsInSeg(segId)[j - 1];
const Node* curr = mgr.getCellsInSeg(segId)[j];

// Only add edges from a cell's bottom (primary) row to avoid
// cycles in the topological sort caused by multi-height cells
// appearing in multiple row segments with conflicting orderings.
if (prev->getBottom() != rowBottom || curr->getBottom() != rowBottom) {
continue;
}
Comment on lines +132 to +134
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Apr 15, 2026

Choose a reason for hiding this comment

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

high

The logic introduced here to avoid cycles by only adding edges when both cells' bottom matches the current row's bottom is problematic for correctness. It effectively ignores dependency constraints between cells of different heights or cells that start at different rows.

For example, if a multi-height cell $M$ (spanning rows 0 and 1) is to the left of a single-height cell $S$ (in row 1), the edge $M \to S$ will never be added:

  • In Row 0: $S$ is not present.
  • In Row 1: $M.getBottom()$ (Row 0) does not match rowBottom (Row 1), so the edge is skipped.

This lack of constraints can lead to overlaps during the shifting process, as the legalizer will not maintain the required relative ordering of these cells.

The root cause of the topological sort cycles is likely the non-deterministic sorting in DetailedMgr::compareNodesX (in detailed_manager.h). When two cells have the same X center, std::sort may produce different relative orders in different row segments, creating a cycle. A more robust fix would be to make the cell comparison deterministic by adding a tie-breaker, such as the cell ID:

struct compareNodesX {
  bool operator()(Node* p, Node* q) const {
    if (p->getCenterX() != q->getCenterX())
      return p->getCenterX() < q->getCenterX();
    return p->getId() < q->getId();
  }
};

Additionally, note that this PR only modifies the first loop that builds the dependency graph for the topological sort check, but leaves the second loop in ShiftLegalizer::shift (lines 273-282) unchanged. This inconsistency means the clump logic will still operate on the original, potentially cyclic/inconsistent graph.

References
  1. Iteration or sorting over pointer-based collections requires a custom comparator or a unique tie-breaker (like an ID) to ensure deterministic behavior across different runs.

incoming_[curr->getId()].push_back(prev->getId());
outgoing_[prev->getId()].push_back(curr->getId());
}
Expand Down
5 changes: 4 additions & 1 deletion src/dpl/src/optimization/detailed_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,10 @@ class DetailedMgr
// Needs cell centers.
bool operator()(Node* p, Node* q) const
{
return p->getCenterX() < q->getCenterX();
if (p->getCenterX() != q->getCenterX()) {
return p->getCenterX() < q->getCenterX();
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Apr 15, 2026

Choose a reason for hiding this comment

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

warning: statement should be inside braces [google-readability-braces-around-statements]

Suggested change
return p->getCenterX() < q->getCenterX();
if (p->getCenterX() != q->getCenterX()) {
return p->getCenterX() < q->getCenterX();
}

}
return p->getId() < q->getId();
}
bool operator()(Node*& s, DbuX i) const { return s->getCenterX() < i; }
bool operator()(DbuX i, Node*& s) const { return i < s->getCenterX(); }
Expand Down
Loading