dpl: remove negotiation standalone padding check - #11089
Open
gudeh wants to merge 5 commits into
Open
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}
Contributor
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.
Comment on lines
1097
to
1103
| 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; | ||
| } | ||
| } |
Contributor
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;
}
}
Contributor
Author
…ation (check only with native checkDRC()). Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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]