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

[Divinesoumyadip]

Summary

improve_placement fails with DPL-0400 Cells incorrectly ordered during shifting on designs with multi-height cells. Multi-height cells appear in segments across multiple rows. When a single-height cell sits inside the row span of two multi-height cells, the ordering differs per row, creating a cycle in the topological sort. Fix: in legalize_shift.cxx, only add dependency edges when both cells' bottom Y matches the current segment's row bottom. Multi-height cells contribute edges only from their primary (bottom) row, eliminating the cycle.

Type of Change

Bug fix

Impact

improve_placement no longer crashes with DPL-0400 on designs containing multi-height cells such as SRAMs. No behavioral change for single-height-only designs.

Related Issues

Fixes #9932

[gemini-code-assist]

Code Review

This pull request modifies the dependency graph construction in ShiftLegalizer::legalize by only adding edges between cells that align with the current row's bottom, aiming to prevent cycles in the topological sort. The reviewer pointed out that this logic is flawed as it ignores necessary constraints between cells of varying heights, which could lead to overlaps. A more robust solution was suggested: making the cell comparison deterministic by using cell IDs as a tie-breaker in the sorting logic, while also noting that the current changes create an inconsistency with the shifting logic in the same class.

[gemini-code-assist]

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.

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[maliberty]

FYI, our efforts are mostly going towards the new negotiation based legalizer which will become the new default.

[Divinesoumyadip]

FYI, our efforts are mostly going towards the new negotiation based legalizer which will become the new default.

should I still pursue this or skip given the new legalizer?

[gudeh]

The new legalizer is executed before improve placement. So I say you should go for the fix!

We should still run improve placement even with the new legalizer.

Legalization is performed with detailed_placement command

[github-actions]

clang-tidy made some suggestions

[github-actions]

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();
	}

[gudeh]

LGTM

OR tests seem to indicate no-op. I also triggered a secure-CI to be sure.

@Divinesoumyadip have you confirmed this solves the issue with the reproducible ?

[gudeh]

You also need to solve the clang-format and clang-tidy

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"