Skip to content

dpl: enhance initial snap - #11121

Open
gudeh wants to merge 22 commits into
The-OpenROAD-Project:masterfrom
gudeh:dpl-enhance-initial-snap
Open

dpl: enhance initial snap#11121
gudeh wants to merge 22 commits into
The-OpenROAD-Project:masterfrom
gudeh:dpl-enhance-initial-snap

Conversation

@gudeh

@gudeh gudeh commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR modifies the initial snapping performed by the Negotiation legalizer at DPL. With these modifications we attempt to avoid issues coming from other tools.

Issues and motivations:

  • RSZ may introduce new instances on top of macros which require snapping to the closest valid row.
  • MPL may create vertical/horizontal small channels with only a few sites/rows.
    • In addition, a small space may be crowded with fixed instances. Which is usually the case around macros (endcaps).
  • GPL may place top-level (regionless) instances inside regions.

New behavior for initial snapping:

  • Use standalone diamond search for closer snapping.
  • Snap after site capacity is calculated to account for fixed instances. Do not snap to fixed instances positions (capacity == 0).
  • Snap after regions are initiated. Consider instance regions properly with NegotiationLegalizer::respectsFence(). Previous behavior did not account for top-level instances.

Type of Change

  • New feature

Impact

Avoid other tools generated issues.

Old and new behavior on the upf_aes test. Previously, top-level (regionless) instances were only snapped out of the regions at the very end, by the diamond search fallback, once negotiation had failed to find valid positions for them.

Now we snap them first, so negotiation can make the necessary moves properly and account more fairly for the competition for space, since all top-level instances are present from the start.
image

Also, now we avoid bad snappings like this one:
image

Now we allow diagonal snapping such as these:
image

Verification

  • I have verified that the local build succeeds (./etc/Build.sh).
  • I have run the relevant tests and they pass.
  • My code follows the repository's formatting guidelines.
  • I have signed my commits (DCO).

Related Issues

This branch includes changes from #11089, and should be merged after it.

This PR addresses issues:

gudeh added 19 commits July 31, 2026 16:25
consider available space for initial snapping

Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
should provide better QoR (instances snapped to closer position), with controlled runtime.

Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
The helpers return the cell's footprint extended by its left/right padding.
The 'eff' prefix did not convey that, which made the padded-vs-footprint
distinction in the negotiation grid loops hard to follow.

Pure rename, no functional change.

Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
negotiationCost, addUsage, isCellLegal and the history/sort bookkeeping all
scanned the cell's padded span (footprint plus left/right padding) and
tested Pixel::capacity.  capacity is a plain int, so the class of the
blocking instance is lost: an endcap and a CORE cell look identical.
PlacementDRC::checkPadding does have the class and waives padding entirely
for SP-class neighbours (CORE_SPACER, ENDCAP*) via allowPaddingOverlap, so
the legalizer demanded one more site per SP-class neighbour than
check_placement does.

Worse, capacity == 0 produced kInfCost, a veto rather than a cost.  A cell
whose padded span fits nowhere in its search window gets every candidate
rejected, so findBestLocation returns the incumbent (best_x/best_y are
initialised to the cell's current position) and the cell never moves.
History cost cannot break the deadlock because the alternatives are vetoed,
not merely expensive.  Two cells seeded into the same narrow channel stayed
fully overlapped for the whole run.

Observed on gf12/ca53_cpu with CELL_PAD_IN_SITES_DETAIL_PLACEMENT=1: a
12-site cell in a 12-site channel flanked by ENDCAPTIE12 instances needed a
14-column run of free sites that exists nowhere nearby, so it was stranded.

Movable cells now claim and test only their footprint; their padding is
enforced solely by PlacementDRC, through the checkDRC call in isCellLegal
and the countDRCViolations penalty in findBestLocation.  usage/overuse
consequently means site contention only, which is what the
negotiated-congestion cost model expects.

Padding against fixed instances is unaffected and still hard: buildGrid
continues to blockade each fixed cell's padded range, and that range is
already class-correct because Padding::isPaddedType reports no padding for
SP-class masters.  An endcap therefore blockades only its footprint while a
CORE or CORE_WELLTAP neighbour blockades its padding too.

No change to the dpl regression results: the same 23 pre-existing master
failures, before and after.

Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
…ation (check only with native checkDRC()).

Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
previous commit had leftover unecessary padding consideration

Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
…m negotiation algorithm

Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
@gudeh
gudeh requested a review from a team as a code owner August 11, 2026 00:48
@gudeh
gudeh requested a review from osamahammad21 August 11, 2026 00:48

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the NegotiationLegalizer by centralizing the snapping logic of movable cells into a new initialSnap() method, and updates how cell padding is handled during negotiation. Feedback on the changes highlights two potential issues in the new initialSnap() method: a potential out-of-bounds array access if the grid height is smaller than the cell height, and a potential signed integer overflow when doubling the search budget.

Comment on lines +618 to +622
const int max_budget
= grid_w_ * site_width_
+ std::max(init_y_dbu - dpl_grid->gridYToDbu(GridY{0}).v,
dpl_grid->gridYToDbu(GridY{grid_h_ - cell.height}).v
- init_y_dbu);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

If grid_h_ is less than cell.height (e.g., in extremely small grids or with exceptionally tall cells), grid_h_ - cell.height will be negative. Passing a negative value to GridY and then to gridYToDbu will result in an out-of-bounds array access, causing undefined behavior or a crash. Consider guarding this calculation to ensure a non-negative row index is used.

Suggested change
const int max_budget
= grid_w_ * site_width_
+ std::max(init_y_dbu - dpl_grid->gridYToDbu(GridY{0}).v,
dpl_grid->gridYToDbu(GridY{grid_h_ - cell.height}).v
- init_y_dbu);
const int max_budget
= grid_w_ * site_width_
+ std::max(init_y_dbu - dpl_grid->gridYToDbu(GridY{0}).v,
(grid_h_ >= cell.height)
? dpl_grid->gridYToDbu(GridY{grid_h_ - cell.height}).v - init_y_dbu
: 0);

Comment on lines +629 to +630
for (int budget = site_width_; !found;
budget = std::min(budget * 2, max_budget)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

If max_budget is very large (e.g., in extremely large designs), budget * 2 can exceed INT_MAX, leading to signed integer overflow (undefined behavior in C++). If it overflows to a negative value, std::min will select the negative value, potentially causing an infinite loop. Capping the budget before multiplying avoids this potential overflow.

Suggested change
for (int budget = site_width_; !found;
budget = std::min(budget * 2, max_budget)) {
for (int budget = site_width_; !found;
budget = (budget > max_budget / 2) ? max_budget : budget * 2) {

@gudeh
gudeh requested review from a team as code owners August 17, 2026 18:53
@gudeh
gudeh requested a review from jfgava August 17, 2026 18:53
@gudeh
gudeh requested a review from arthurjolo August 17, 2026 18:53
@github-actions github-actions Bot added size/L and removed size/M labels Aug 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant