dpl: remove negotiation standalone padding check - #11089
Conversation
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>
![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 refactors the negotiation legalizer so that movable cells only claim their actual footprint during legalization, leaving padding checks to PlacementDRC, while fixed cells continue to use their padded footprint. The helper functions have been renamed to paddedXBegin and paddedXEnd to reflect this. The review feedback suggests a minor optimization in isCellLegal to hoist the row coordinate calculation out of the inner loop.
| for (int dy = 0; dy < cell.height; ++dy) { | ||
| for (int gx = xBegin; gx < xEnd; ++gx) { | ||
| if (gridAt(gx, cell.y + dy).capacity == 0 | ||
| || gridAt(gx, cell.y + dy).overuse() > 0) { | ||
| for (int gx = cell.x; gx < cell.x + cell.width; ++gx) { | ||
| const Pixel& g = gridAt(gx, cell.y + dy); | ||
| if (g.capacity == 0 || g.overuse() > 0) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
We can optimize this loop by hoisting the calculation of gy = cell.y + dy to the outer loop. This avoids recomputing cell.y + dy for every pixel in the inner loop.
for (int dy = 0; dy < cell.height; ++dy) {
const int gy = cell.y + dy;
for (int gx = cell.x; gx < cell.x + cell.width; ++gx) {
const Pixel& g = gridAt(gx, gy);
if (g.capacity == 0 || g.overuse() > 0) {
return false;
}
}
…ation (check only with native checkDRC()). Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
| const int xBegin = effXBegin(cell); | ||
| const int xEnd = effXEnd(cell); |
There was a problem hiding this comment.
Wouldn't that count for padding twice? here and in PlacementDRC?
There was a problem hiding this comment.
I think this should also use the real x range instead of the padded one to allow PlacementDRC::checkPadding to do the check on its own and for allowPaddingOverlap to decide if padding overlap is allowed
| const int xBegin = effXBegin(cell); | ||
| const int xEnd = effXEnd(cell); |
There was a problem hiding this comment.
I think this should also use the real x range instead of the padded one to allow PlacementDRC::checkPadding to do the check on its own and for allowPaddingOverlap to decide if padding overlap is allowed
2 participants
Summary
We don't need this check since it's already performed in checkDRC() and countDRCViolations(). Removing it eliminates redundancy.
Adding padding during negotiation was creating a corner-case bug for an upcoming branch that modifies negotiation's initial snapping (PR incoming soon). A mismatch existed between negotiation and check_placement padding checks because the latter allows endcaps to disregard padding (allowPaddingOverlap()). Padding isn't needed in the negotiation algorithm—it's a legacy remnant from before DRCs were integrated into negotiation.
Specific bug details: The issue occurred on a private design containing a vertical channel with the exact number of sites required for certain buffers. Two buffers were initially snapped into the same site with overlap. Negotiation never moved them because of padding (+1 on each side) combined with fixed instances (endcaps) on both sides. These buffers were eventually resolved by falling back to a diamond search and moved to valid (DRC-clean) positions. Meanwhile, check_placement correctly did not report a DRC because padding overlap with endcaps is allowed.
Type of Change
Impact
Waiting for secure-CI to finish. No-op so far, most PDKs don't use padding.
Verification
./etc/Build.sh).Related Issues
[Link issues here]