dpl: debug deep iterative and new paintings#9441
dpl: debug deep iterative and new paintings#9441maliberty merged 20 commits intoThe-OpenROAD-Project:masterfrom
Conversation
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>
…ion changes Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
the incremental mode initiate legalized instances as placed instead of everything unplaced 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
The pull request introduces new debug options for the DPL tool, including -paint_pixels for visualizing occupied pixels, and -deep_iterative for more granular pausing during iterative placement. It also adds an -incremental mode to initialize cells as placed if they are already legal and collision-free. The changes involve modifications to the Opendp.h, Opendp.cpp, Opendp.i, Opendp.tcl, and Graphics.cpp files to support these new functionalities. The searchNearestSite function has been renamed to diamondSearch across the codebase. Overall, the changes enhance the debugging capabilities and provide a new placement initialization mode.
| if (iterative_placement_) { | ||
| logger_->report( | ||
| "Total counters:\n" | ||
| "\t#cells: {}\n" | ||
| "\t#Failed mapMove(): {}, #Success mapMove(): {}\n" | ||
| "\t% Success mapMove(): {:.2f}\n" | ||
| "\t#Failed shiftMove(): {}\n" | ||
| "\t% Success shiftMove(): {:.2f}", | ||
| sorted_cells.size(), | ||
| failed_map_move, | ||
| success_map_move, | ||
| success_map_move > 0 | ||
| ? 100.0 * success_map_move / (failed_map_move + success_map_move) | ||
| : 0, | ||
| failed_shift_move, | ||
| failed_map_move > 0 | ||
| ? 100.0 * (failed_map_move - failed_shift_move) / (failed_map_move) | ||
| : 0); | ||
|
|
||
| logger_->report("Placement failures: {}", placement_failures_.size()); |
There was a problem hiding this comment.
The logging block for total counters is quite large and contains complex calculations for percentages. This could be encapsulated in a separate helper function, e.g., reportPlacementSummary(size_t total_cells, int failed_map, int success_map, int failed_shift) to improve readability and reduce the size of the place function. This would make the main placement logic easier to follow.
| const GridY boundary_margin{3}; | ||
| const GridX margin_width{grid_->gridPaddedWidth(cell).v * boundary_margin.v}; | ||
| const GridX margin_width{grid_->gridPaddedWidth(target_cell).v | ||
| * (1 + boundary_margin.v)}; |
There was a problem hiding this comment.
The margin_width calculation uses a "magic number" (1 + boundary_margin.v). It's generally good practice to define such numbers as named constants to improve readability and maintainability. This makes the code easier to understand and modify in the future.
References
- Define tunable parameters as named constants instead of using hardcoded magic numbers.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
…on another PR 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! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
10 similar comments
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
maliberty
changed the title
src/dpl/src/Opendp.cpp
Outdated
|
|
||
| for (auto& node : network_->getNodes()) { | ||
| if (node->getType() == Node::CELL && !node->isFixed() && node->isPlaced()) { | ||
| if ((node->getLeft() % site_width != 0) || !checkInRows(*node)) { |
There was a problem hiding this comment.
The row might not start on a multiple of site_width so this may be incorrect.
There was a problem hiding this comment.
what if we do the following, the snapping to the grid pixels has to be same as the cell position.
const GridX x = grid_->gridX(node.get());
const GridY y = grid_->gridSnapDownY(node.get());
if (node->getLeft() != gridToDbu(x, site_width)
|| node->getBottom() != grid_->gridYToDbu(y)
| if (!pixel->cell->isFixed()) { | ||
| conflicted.insert(pixel->cell); |
There was a problem hiding this comment.
Is it not a conflict to be on a fixed object?
There was a problem hiding this comment.
This ensures we do not attempt to unplace fixed cells. In other words, when !pixel->cell->isFixed() is true (the cell is movable), we mark both cells as conflicted. But if the cell already in the pixel is fixed, the condition is false, and only the current node is marked for unplacement.
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
…sting placement Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
2 participants
This PR is no-op.
It includes the following changes to debug mode:
-paint_pixels-deep_iterativeThis PR also includes a new mode for DPL:
-incremental, which initiates all cells in a legal position and without collisions as placed, instead of the current default which initiates all cells as unplaced (this option has functional implications, but is not used by default currently).