Skip to content

mpl: ignore the macro being placed in overlap checks - #11322

Closed
naveenvenk17 wants to merge 7 commits into
The-OpenROAD-Project:masterfrom
naveenvenk17:nv-mpl-skip-self-clean2
Closed

naveenvenk17 wants to merge 7 commits into
The-OpenROAD-Project:masterfrom
naveenvenk17:nv-mpl-skip-self-clean2

Conversation

@naveenvenk17

@naveenvenk17 naveenvenk17 commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #11269.

place_macro updates the target location before checking overlaps. If that macro is already placed, the scan can report the instance as overlapping itself. This skips only the instance being moved and keeps checks against other placed macros.

The dedicated regression loads three already-placed R0 macros from a DEF. It moves the target successfully, then independently checks rejection against each of the other two macros. It does not pass an orientation argument or modify the existing rotation regression. The target is restored to PLACED before each rejection attempt so those calls reach overlap checking rather than the LOCKED-origin guard.

Tested on the GCP Linux VM on the exact updated commit:

  • All 45 MPL targets and six repository checks passed with test-result caching disabled.
  • The focused test matches its committed golden output.
  • The same regression fails on the before-fix control with the expected self-overlap MPL-0041.
  • The new test is registered in Bazel only.

Existing review history is preserved; no force-push was used. The older DCO failure still requires maintainer remediation. New commits have matching sign-offs. The missing upstream registry checksum is now fixed in d61ef5a.

@naveenvenk17
naveenvenk17 requested a review from a team as a code owner September 2, 2026 20:43
@github-actions github-actions Bot added the size/M label Sep 2, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request prevents a macro from checking overlap with itself in findOverlappedMacros inside rtl_mp.cpp. It also updates Bazel test targets and introduces several new tests, including place_macro_right_angle_rotation. The review feedback points out that the golden file place_macro_right_angle_rotation.defok was placed in the wrong directory (testcases/), which will cause Bazel test failures, and should be moved to the parent test directory.

set def_file [make_result_file place_macro_right_angle_rotation.def]
write_def $def_file

diff_files place_macro_right_angle_rotation.defok $def_file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The golden file place_macro_right_angle_rotation.defok is added under src/mpl/test/testcases/, but this diff_files call expects it in the current directory (src/mpl/test/). Additionally, the Bazel glob in src/mpl/test/BUILD (glob(["place_macro_right_angle_rotation.*"])) will not find it inside testcases/, which will cause the Bazel test to fail. To resolve this and align with the repository's conventions, please move the golden file from src/mpl/test/testcases/place_macro_right_angle_rotation.defok to src/mpl/test/place_macro_right_angle_rotation.defok.

@naveenvenk17
naveenvenk17 force-pushed the nv-mpl-skip-self-clean2 branch from 6a5edfd to a25e3af Compare September 4, 2026 06:43
@naveenvenk17

Copy link
Copy Markdown
Contributor Author

Updated for current master. The golden output now comes from the existing parent test location, so there is no separate misplaced .defok file.

The regression now first marks the target macro as placed, then moves it. That exercises the self-overlap path directly. I also squashed the branch into one signed commit.

Validation on the GCP VM:

  • //src/mpl/test:place_macro_right_angle_rotation-tcl_test passed three consecutive runs
  • //src/mpl/test:all passed 42 of 42 tests
  • clang-format-18 --dry-run --Werror and git diff --check passed

@github-actions github-actions Bot added size/XS and removed size/M labels Sep 4, 2026

@AcKoucher AcKoucher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@naveenvenk17 Thanks for fixing this!

@naveenvenk17
naveenvenk17 force-pushed the nv-mpl-skip-self-clean2 branch from a25e3af to 0578744 Compare September 4, 2026 20:32
@github-actions github-actions Bot added size/S and removed size/XS labels Sep 4, 2026
Naveen Venkat added 2 commits September 4, 2026 13:33
@naveenvenk17
naveenvenk17 force-pushed the nv-mpl-skip-self-clean2 branch from 0578744 to 73d1ab9 Compare September 4, 2026 20:47
@naveenvenk17

Copy link
Copy Markdown
Contributor Author

Thanks, I split that setup out.

place_macro_right_angle_rotation is back to checking only the R90 placement. The new place_macro_already_placed_overlap test first marks the macro placed, then requests the same legal R90 placement without allowing overlap. That path only succeeds when the target macro is excluded from its own overlap search.

The new regression has its own expected log and DEF files, plus Bazel and CMake registration.

I rebased this on current master and validated it on the GCP VM:

  • both focused placement tests passed three uncached runs
  • the full //src/mpl/test:all suite passed 43 of 43 targets
  • clang-format --dry-run --Werror and git diff --check passed

I also had the final test split reviewed separately before the last test run; there were no blocking findings.

@AcKoucher AcKoucher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@naveenvenk17 Please, do not force-push after a review, because we lose history.

$macro setLocation 40000 40000
$macro setPlacementStatus PLACED

place_macro -macro_name macro -location {20 20} -orientation R90

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, use an input DEF that does not require setting -orientation that way we keep the test case narrower.

@naveenvenk17

Copy link
Copy Markdown
Contributor Author

You are right. I rebased and force-pushed to keep the branch current, but that removed review history. I will keep any further updates as additive commits and will not force-push this PR again.

@maliberty maliberty added the mpl Macro Placement label Sep 15, 2026
@maliberty

Copy link
Copy Markdown
Member

DCO needs fixing along with lint

Load already-placed R0 macros from a dedicated DEF instead of rotating the target in Tcl. Check that the target can move without reporting itself and that overlaps with each of two other placed macros still fail.

Order the scoped Bazel resource entry lexicographically to fix the reported Buildifier lint failure. Keep the existing reviewed history unchanged.

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>
Merge bf5a76a using the verified explicit base 372189e. Preserve b58e359 as the first parent and reviewed history unchanged.

The delta from upstream is only the self-skip condition and the dedicated placed-overlap regression. MODULE.bazel, MODULE.bazel.lock and submodule IDs match upstream.

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>
Remove the historical CMake test registration to follow the current repository testing guidance. Keep the existing orientation regression and all reviewed history unchanged.

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>
The actual regression run showed that the successful placement locks the target. Restore PLACED status before each rejection check so the test reaches MPL overlap validation instead of failing on the OpenDB LOCKED-origin guard.

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>
@naveenvenk17

Copy link
Copy Markdown
Contributor Author

The test now loads the placed R0 macros from a dedicated DEF and does not pass -orientation. It checks a valid move and separate overlaps with both neighbours. The actual run also caught the target becoming LOCKED after a successful move; the test now restores PLACED between independent attempts so it checks MPL-0041, not the OpenDB origin guard.

All 45 MPL targets and six repository checks passed on the final VM-tested commit. The before-fix control fails on self-overlap as expected. I kept the reviewed history intact and did not force-push.

DCO still flags the older commit. Could we handle that through an additive remediation or maintainer override rather than rewriting it?

Keep the feature tree unchanged and refresh pull-request CI after the missing registry checksum was added to master.

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>
@AcKoucher

Copy link
Copy Markdown
Contributor

The test being added here is overly complicated. I opened #11907 with your fix preserved and a simpler, straight forward test.

@naveenvenk17

Copy link
Copy Markdown
Contributor Author

Thanks, the shorter test makes sense. #11907 preserves the fix, so I'm closing this one in its favor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mpl Macro Placement size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mpl: should place_macro treat the macro being placed as an overlap with itself?

3 participants