dpl: fix one site gap false positive #9227
Conversation
previously it was wrongfully checking more rows than necessary Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
…imes, the test also include a one site gap handling Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
|
A secure-CI is running at: https://jenkins.openroad.tools/blue/organizations/jenkins/OpenROAD-flow-scripts-Private/detail/secure-dpl-fix-one-site-gap/1/pipeline I expect no-op on public designs, but private designs may need metrics update. |
|
@jfgava FYI |
![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 correctly fixes a false positive in the one-site gap checker by restricting the check to the rows occupied by the instance. The logic change in Place.cpp is sound and addresses the issue described. The addition of debug logs is also helpful.
I've added a new test case to verify this fix. However, I found an issue with the test data where the input and golden output files are identical, which means the test doesn't currently validate that the gap is being fixed. Please see my specific comment on the test file. Once that is addressed, this PR should be good to merge.
|
clang-tidy review says "All clean, LGTM! 👍" |
|
tcl lint needs fixing. @osamahammad21 please review/merge |
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
src/dpl/src/Place.cpp
Outdated
| const GridX x_begin = max(GridX{0}, x - 1); | ||
| const GridY y_begin = max(GridY{0}, y - 1); | ||
| const GridY y_begin = max(GridY{0}, y); | ||
| // inclusive search, so we don't add 1 to the end | ||
| const GridX x_finish = min(x_end, grid_->getRowSiteCount() - 1); | ||
| const GridY y_finish = min(y_end, grid_->getRowCount() - 1); | ||
|
|
||
| auto isAbutted = [this](const GridX x, const GridY y) { | ||
| const Pixel* pixel = grid_->gridPixel(x, y); | ||
| return (pixel == nullptr || pixel->cell); | ||
| }; | ||
|
|
||
| auto cellAtSite = [this](const GridX x, const GridY y) { | ||
| const Pixel* pixel = grid_->gridPixel(x, y); | ||
| return (pixel != nullptr && pixel->cell); | ||
| }; | ||
| for (GridY y = y_begin; y <= y_finish; ++y) { | ||
| for (GridY y = y_begin; y < y_finish; ++y) { | ||
| // left side | ||
| if (!isAbutted(x_begin, y) && cellAtSite(x_begin - 1, y)) { | ||
| debugPrint(logger_, | ||
| DPL, | ||
| "one_site_gap", | ||
| 1, | ||
| "One site gap left of {} at ({}, {})", | ||
| cell->name(), | ||
| x, | ||
| y); | ||
| return false; | ||
| } | ||
| // right side | ||
| if (!isAbutted(x_finish, y) && cellAtSite(x_finish + 1, y)) { | ||
| debugPrint(logger_, | ||
| DPL, | ||
| "one_site_gap", | ||
| 1, | ||
| "One site gap right of {} at ({}, {})", | ||
| cell->name(), | ||
| x, | ||
| y); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
@gudeh Do you think we should move this outside the y for loop? It seems weird we are doing it for every y
There was a problem hiding this comment.
don't we have to do so for multirow cells?
There was a problem hiding this comment.
we do, but this whole block of code exists in an outer loop that iterates over the y indices of the cell.
Currently it is something like this:
for (GridY y1 = y; y1 < y_end; y1++) { // line 907
for (GridY y = y1; y < y_end; ++y) {
...
}
}
This is repetitive. It's not caused by PR and has always been there, but it's incorrect.
There was a problem hiding this comment.
Ah, I see! We are checking for one site gap for every row, although we should require to check only once. I will move it out of the loop.
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
This requires updates on metrics for private designs. I have it running at the moment to fetch the values. A PR for it should follow soon. |
3 participants
The checker in this part of the code was incorrectly examining more rows than intended. This appears to have been an oversight while trying to handle multi-row instances.
The issue surfaced with a private PDK that disallows one-site gaps between adjacent instances. In this case, instances were being moved even when no actual modifications were applied to the design. It happened because we were checking the one site gap horizontally in a row that the instance was not present.
The problem was reproducible with gcd during GRT when repair_design made no changes, and also when invoking DPL multiple times in sequence using the disallows one-site gaps.
I added a test that performs multiple consecutive DPL calls in the presence of a one-site gap. The test ensures that the one site gap is fixed on the first pass and no further changes are made in subsequent runs.
Finally, this issue occurs because the one-site gap check is implemented independently in both Place.cpp and CheckPlacement.cpp, without a shared helper function. A potential follow-up improvement would be to factor this logic into a common function used by both.