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>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
…to dpl-enhance-initial-snap
…to dpl-enhance-initial-snap
![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 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); |
There was a problem hiding this comment.
Not a bad suggestion to follow
| 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>
…to dpl-enhance-initial-snap
|
@eder-matheus Secure-CI shows a routing congestion DRC at 5-1 with nangate45/swerv for this change: |
|
I found a bug! Here is what DPL is doing at CTS (Stage 5-1 fails before the first DPL call): We obviously should be moving this instance on the closer available site by the left, not all the way to the top. Extra: |
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
|
No more fatals now |
|
We are getting some nice improvements in displacement and HPWL with this branch: And it does reflect in improvement in HPWL later on. GRT HPWL: |
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
The merged master reports [INFO GPL-1018] Final HPWL after global placement. All six occurrences in this test come from "global_placement -incremental" runs that follow detailed_placement, so their values track the negotiation legalizer's result and the numbers master recorded no longer apply. Regenerated from the bazel run of the merged tree; the rest of the golden is unchanged. Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
save_flow_metrics_limits re-derives every limit from the run it is given (limit = value * 1.2 and friends), so running it over a local ctest/CMake run ratchets the limits down to whatever that build happened to produce. The CMake and Bazel builds differ substantially on flow QoR -- aes_nangate45 reports 1 hold buffer under CMake and 74 under Bazel -- so the regenerated limits were unreachable for the Bazel build CI runs: the new hold buffer limit of 1 fails there, where the previous limit of 93 passes. Restore the seven limits files. The remaining flow failures under Bazel are then real QoR deltas against master rather than lost headroom, and can be judged on their merits. Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
maliberty
left a comment
maliberty
left a comment
There was a problem hiding this comment.
Approving on Cho's behalf for rsz. I didn't look at the rest
|
@arthurjolo @osamahammad21 we need your reviews on this one for merging |
| cell.init_x = best_x; | ||
| cell.init_y = best_y; |
There was a problem hiding this comment.
Why do we overwrite the initial positions here? Can't we just update the cell.x& cell.y?
| // positions | ||
| cell.init_x = dpl_grid->gridX(DbuX{db_x - die_xlo_}).v; | ||
| // Snap to grid, findBestLocation() iterates over grid positions | ||
| cell.init_x = dpl_grid->gridRoundX(DbuX{db_x - die_xlo_}).v; |
There was a problem hiding this comment.
What motivates this 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); |
There was a problem hiding this comment.
Not a bad suggestion to follow
5 participants
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.Additions after PR creation:
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: