Repository navigation
MPL: keep snapped macros inside the core - #11844
sainadh812 wants to merge 3 commits into
Conversation
Signed-off-by: sainadh812 <58074126+sainadh812@users.noreply.github.com>
There was a problem hiding this comment.
Welcome to OpenROAD! Thanks for opening your first PR.
Before we review:
- Contribution Guide: https://openroad.readthedocs.io/en/latest/contrib/GettingInvolved.html
- Build Instructions: https://openroad.readthedocs.io/en/latest/user/Build.html
Please ensure:
- CI passes
- Code is properly formatted
- Tests are included where applicable
A maintainer will review shortly!
There was a problem hiding this comment.
Code Review
This pull request improves macro placement and snapping logic to ensure macros remain within the core area. It refactors core containment checks in rtl_mp.cpp into a reusable lambda, fixes a self-overlap bug in findOverlappedMacros, and updates the snapper to validate that snapped origins lie within the core. Feedback was provided regarding a potential division-by-zero vulnerability in snapper.cpp if the manufacturing grid size is zero or negative.
Signed-off-by: sainadh812 <58074126+sainadh812@users.noreply.github.com>
Signed-off-by: sainadh812 <58074126+sainadh812@users.noreply.github.com>
| const int initial_origin = origin; | ||
| alignWithManufacturingGrid(origin); | ||
|
|
||
| if (!isOriginInsideCore(origin, target_direction)) { |
There was a problem hiding this comment.
What's the reasoning behind snapping a macro that is outside the core region?
Also, a design should not have a non-positive manufacturing grid, if that happens the tech information is malformed and should be fixed.
(Gemini trends to be overprotective about nullptr handling and divisions by zero, you can ignore its suggestion)
| @@ -61,14 +95,33 @@ void Snapper::snap(const odb::dbTechLayerDir& target_direction) | |||
| starting_position_index -= 1; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The removal makes sense since it is redone at the first step in attemptSnapToExtraPatterns, good catch.
| void Snapper::alignWithManufacturingGrid(int& origin) | ||
| { | ||
| const int manufacturing_grid | ||
| = inst_->getDb()->getTech()->getManufacturingGrid(); |
There was a problem hiding this comment.
Not needed, as I commented above.
|
|
||
| odb::dbITerm* snap_pin = layers_data_list[0].pins[0]; | ||
| const std::vector<int>& positions = layers_data_list[0].available_positions; | ||
| const int initial_origin = target_direction == odb::dbTechLayerDir::VERTICAL |
There was a problem hiding this comment.
I'll add a comment about the approach in general here: I see the idea, but I feel like this could be implemented in a cleaner manner. Instead of testing snapping and testing every positions (the code does this currently) and check its legality (your inclusion), maybe it should only try legal positions to begin with?
You could try an approach where you fetch the 50 positions to left and right of start_index from available_positions and cull illegal, and then try to snap to these positions.
| { | ||
| int origin = position - getPinOffset(pin, direction); | ||
| alignWithManufacturingGrid(origin); | ||
| if (!isOriginInsideCore(origin, direction)) { |
There was a problem hiding this comment.
snapPinToPosition does two things now: snaps and checks legality. Remove this from the method and add after the calls, if necessary (check my other comment about the approach you're using).
| snapper.snapMacro(); | ||
| } | ||
|
|
||
| check_core_containment(inst->getBBox()->getBox()); |
There was a problem hiding this comment.
If the Snapper cannot snap macros to outside the core region now, isn't this check redundant?
|
|
||
| for (odb::dbInst* inst : block->getInsts()) { | ||
| if (!inst->isBlock() || !inst->isPlaced()) { | ||
| if (inst == macro || !inst->isBlock() || !inst->isPlaced()) { |
There was a problem hiding this comment.
The macro that call this method is not placed (its status has not changed to LOCKED yet) at the time of call, so it flagged by !inst->isPlaced(). It should not cause problem for this PR, but its a nice fix since it could lead to problems if a different function calls it in the future, move this change to a separate PR please.
|
I reworked this in #11901 under my correct contributor account. The 50 positions on either side of the starting track are filtered for core legality before trying a snap, and snapPinToPosition now only moves the macro. The extra post-snap placeMacro check and unrelated nonpositive-grid handling are removed. The self-overlap change stays separate in #11322. The outside-core case was the rounded candidate, not an initially outside input: grid rounding can move a valid boundary placement past the edge. The new tests make that precondition explicit. All 51 MPL/repository targets passed, and all 56 unit cases passed three shuffled runs. Eight-orientation tests include non-square macros and the no-legal-candidate path; mandatory pin-grid failures are still reported without moving the macro. |
Fixes #11278
Fixes #11269
Snapper could move a macro past the core boundary after
place_macrochecked the requested location. That made a placement script written byrtl_macro_placer -write_macro_placementfail when it was sourced again.Only snap candidates whose resulting macro box stays inside the core are now considered. If a macro without track data has no legal manufacturing-grid coordinate, it keeps its requested legal location. The overlap scan also skips the macro being placed, so a placed macro does not report itself as a collision.
The review also caught a missing-grid case. Both manufacturing-grid calculations now use integer DBU coordinates when the technology grid is zero or negative. This keeps a legal origin unchanged and lets an outside origin be brought back into the core without dividing by zero.
Tested on the GCP Linux VM:
snapMacro().//src/mpl/test/...,//:lint_test, and//:dup_id_testpassed with test-result caching disabled.orientation_improve1placement-script round trip also passed.