dpl: enhance initial snap - #11121
Conversation
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>
…to dpl-enhance-initial-snap
…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>
…to dpl-enhance-initial-snap
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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.
| 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); |
| for (int budget = site_width_; !found; | ||
| budget = std::min(budget * 2, max_budget)) { |
There was a problem hiding this comment.
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.
| 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) { |
…uncating Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
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:
New behavior for initial snapping:
NegotiationLegalizer::respectsFence(). Previous behavior did not account for top-level instances.Type of Change
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.

Also, now we avoid bad snappings like this one:

Now we allow diagonal snapping such as these:

Verification
./etc/Build.sh).Related Issues
This branch includes changes from #11089, and should be merged after it.
This PR addresses issues: