mpl: check core containment against the post-orientation footprint - #11155
Merged
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.
AcKoucher
requested changes
Aug 17, 2026
Contributor
Author
|
@AcKoucher |
AcKoucher
self-requested a review
August 21, 2026 21:26
AcKoucher
requested changes
Aug 21, 2026
AcKoucher
left a comment
AcKoucher
left a comment
Contributor
There was a problem hiding this comment.
Please, adapt the names of the files to be place_macro_right_angle_rotation so that we have everything consistent.
Contributor
Author
|
@AcKoucher |
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: sfmth <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: sfmth <sfmth0@gmail.com>
Set the orientation before building the containment rectangle instead of computing the post-orientation footprint by hand, per review. getBBox() then already reports the footprint the macro will have, so the explicit width/height swap and its right-angle-ness comparison are unnecessary. setOrient() had to precede setLocation() in any case; it now precedes the core-containment check as well. Rename the test to place_macro_with_right_angle_rotation and reduce it to the single R90 placement. The negative case it also asserted is no longer sound: with setOrient() ahead of the check, a rejected placement leaves the instance rotated, so catching MPL-0034 and then writing the DEF would serialize the macro at the rejected orientation. Signed-off-by: sfmth <sfmth0@gmail.com>
Rename the test and all five of its files to place_macro_right_angle_rotation, so the CMake test name, the Bazel targets, the .tcl/.ok/.defok and the testcases LEF/DEF agree. The half-applied rename currently on the PR branch renamed only the Bazel entries, which fails the package load -- "Error in glob: glob pattern 'place_macro_with_right_angle_rotation.*' didn't match anything" -- and takes all of Jenkins down with it. Reword the comment above setOrient() and cut the test down to its header line, both verbatim as requested in review. Signed-off-by: sfmth <sfmth0@gmail.com>
Contributor
Author
|
fixed DCO check |
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.