mpl: check core containment against the post-orientation footprint - #11155
Open
sfmth wants to merge 2 commits into
Open
mpl: check core containment against the post-orientation footprint#11155sfmth wants to merge 2 commits into
sfmth wants to merge 2 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
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/contributing.html
- Build Instructions: https://openroad.readthedocs.io/en/latest/contrib/BuildWithCMake.html
Please ensure:
- CI passes
- Code is properly formatted
- Tests are included where applicable
A maintainer will review shortly!
![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 updates the macro containment validation in MacroPlacer::placeMacro to use the correct footprint of the macro after the requested orientation is applied. Specifically, it swaps the width and height if the requested orientation differs in right-angle rotation from the current orientation, preventing legal rotated placements from being rejected and illegal ones from being accepted. A new integration test, place_macro_rotated, has been added to verify this fix. There are no review comments, and I have no additional feedback to provide.
place_macro validated the core-containment rectangle using the
instance's
bounding box *before* the requested orientation was applied. The four
right-angle orientations (R90, R270, MXR90, MYR90)
swap width and height, so whenever the requested orientation differed
from the current one in right-angle-ness the check used the wrong
footprint: it rejected legal placements of rotated macros with
MPL-0034, and symmetrically would accept ones that do not fit.
Compute the width and height the macro will actually have once
`orientation` is applied, and validate that instead. The comparison is
made against the instance's current orientation rather than assuming
R0, so a macro that has already been rotated is handled correctly too.
The new test places a 100x400um macro at R90 in a 499.89x198.80um
core, where the rotated footprint (400x100) fits and the unrotated one
(100x400) does not. It also asserts the same origin is still rejected
at R0, so the fix cannot silently over-correct.
Signed-off-by: Farhad Modaresi <sfmth0@gmail.com>
The new test was added to COMPULSORY_TESTS but not to the per-test
resource dict. src/mpl/test/BUILD does not glob the testcases/
directory -- the filegroup globs only `test_name + ".*"`, which picks up
the .tcl, .ok and .defok, so each test's LEF and DEF have to be listed
explicitly.
The Bazel target therefore resolved and ran without
testcases/place_macro_rotated.{lef,def}, while the CMake build passed
because it collects test inputs differently.
bazel query deps(//src/mpl/test:place_macro_rotated_resources) now lists
all six required inputs, and
bazel test //src/mpl/test:place_macro_rotated-tcl_test passes.
Signed-off-by: Farhad Modaresi <sfmth0@gmail.com>
AcKoucher
requested changes
Aug 17, 2026
Comment on lines
+104
to
+119
|
|
||
| // The containment check below must use the footprint the macro will have | ||
| // once `orientation` is applied, not the one it has right now. The four | ||
| // right-angle orientations swap width and height, so validating the | ||
| // pre-setOrient bbox rejects legal placements of rotated macros -- and | ||
| // symmetrically accepts illegal ones -- whenever the requested orientation | ||
| // differs from the current one in right-angle-ness. | ||
| int width = inst->getBBox()->getDX(); | ||
| int height = inst->getBBox()->getDY(); | ||
| if (orientation.isRightAngleRotation() | ||
| != inst->getOrient().isRightAngleRotation()) { | ||
| std::swap(width, height); | ||
| } | ||
|
|
||
| const int x2 = x1 + width; | ||
| const int y2 = y1 + height; |
Contributor
There was a problem hiding this comment.
Just setOrient before getting the coordinates to avoid the check.
| fixed_covers | ||
| macros_without_pins1 | ||
| unfixed_cells_dont_fit_in_core | ||
| place_macro_rotated |
Contributor
There was a problem hiding this comment.
A more descriptive name would be place_macro_with_right_angle_rotation
Comment on lines
+15
to
+24
| # The regression: this is legal at R90 and must be accepted. | ||
| place_macro -macro_name macro -location {20 20} -orientation R90 | ||
|
|
||
| # The guard against over-correcting: the same origin is genuinely outside the | ||
| # core at R0, and must still be rejected. | ||
| if { [catch { place_macro -macro_name macro -location {20 20} -orientation R0 } msg] } { | ||
| puts "R0 correctly rejected" | ||
| } else { | ||
| puts "ERROR: R0 placement should have been rejected" | ||
| } |
Contributor
There was a problem hiding this comment.
Just a single place_macro call with the macro on R90 successfully passing is fine.
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
2 participants
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
MacroPlacer::placeMacrobuilt its core-containment rectangle frominst->getBBox()and raisedMPL-0034atsrc/mpl/src/rtl_mp.cpp:109, but didnot call
inst->setOrient(orientation)until 24 lines later. SincegetBBox()returns the bounding box cached for the instance's current orientation, any
request whose orientation differs in right-angle-ness from the current one was
validated against the wrong footprint — the four right-angle orientations
(
R90,R270,MXR90,MYR90) swap width and height.This computes the width and height the macro will actually have once
orientationis applied and validates that instead. The comparison is madeagainst the instance's current orientation rather than assuming
R0, so a macrothat has already been rotated — for example by an earlier attempt in a caller's
retry loop, which may reset placement status without resetting orientation — is
handled correctly rather than double-swapped.
The existing comment two lines below the check ("Orientation must be set before
location so we don't end up flipping and misplacing the macro") shows the
ordering of
setOrient/setLocationwas deliberate; only the containment testwas left above both.
Type of Change
Impact
place_macronow validates core containment against the footprint the macro willhave after the requested orientation is applied, rather than the one it
currently has. Only calls that change the instance's right-angle-ness are
affected: placements at
R0/R180/MX/MYon an unrotated instance take theidentical path as before, square macros are unaffected, and the existing
mplsuite passes unchanged (36/36). For the four right-angle orientations two
behaviours change, in opposite directions. A legal placement of a rotated
non-square macro is no longer rejected — previously it was measured against its
unrotated footprint, so a macro that fits only when rotated was refused with
MPL-0034; the new test is exactly this case, a 100×400 µm macro placed atR90in a 499.89×198.80 µm core. Symmetrically, a rotated macro that genuinely does
not fit is now caught, where before the swapped dimensions could let it pass the
check and be placed and
LOCKEDoutside the core. That second direction is theone to be aware of when upgrading: a flow that previously appeared to succeed may
now report
MPL-0034, but the placement it produced was already out of bounds andsimply went undetected.
Verification
./etc/Build.sh).Built at
cbc7678e45with the equivalent direct invocation:cmake -B build -G Ninja && ninja -C build openroad(Release, gcc).src/mpl/test/./regression— 36/36 mpl tests pass, including the new one.clang-format -i src/mpl/src/rtl_mp.cppapplied.place_macro_rotated, registered in bothCMakeLists.txtandBUILD.Evidence the test catches the bug
The same test, same testcase, against a binary built from unpatched source versus
the patched one:
26Q2-1164-g08f67ee5ec)R90call —[ERROR MPL-0034] Cannot place macro at (20, 20) (120, 420), outside of the core (0, 0) (499.89, 198.8).26Q3-1297-gcbc7678e45)[INFO MPL-0035] Macro macro placed. Bounding box (20.000um, 20.000um), (420.000um, 120.000um). Orientation R90The rejected rectangle in the failing case,
(120, 420), is the macro'sunrotated 100×400 footprint offset by the requested origin — the bug is visible
in the error message itself. The accepted rectangle is the rotated 400×100
footprint, which fits the core.
The test also asserts that the same origin is still rejected at
R0, wherethe macro genuinely does not fit, so the change cannot silently over-correct in
the permissive direction.
Related Issues
None filed — reporting directly via this PR. The defect is reachable from the
public
place_macrocommand for any caller that passes a right-angle orientationfor a non-square macro;
HierRTLMP::placeMacrosuses a separate commit path andis unaffected.