mpl: check Pusher moves overlaps correctly by openroad-ci · Pull Request #10332 · The-OpenROAD-Project/OpenROAD

openroad-ci · 2026-05-04T19:12:43Z

Summary

Fixes #10330.
This PR fixes a bug where the Pusher would overlap macros during the push the boundaries if they were diagonally aligned. This was fixed by keeping the cluster_box rectangle updated between pushes instead of updating it per push.

The PR also includes a small performance improvement by:
-Checking if a macro is in a cluster using the cluster ID instead of comparing against the hard macro list of the cluster
-Only moving the hard macros during a push if the cluster_box doesn't overlap with anything

Type of Change

Bug fix

Impact

Macros don't overlap now.

Verification

I have verified that the local build succeeds (./etc/Build.sh).
I have run the relevant tests and they pass.
My code follows the repository's formatting guidelines.
I have signed my commits (DCO).

Related Issues

#10330

Signed-off-by: João Mai <jmai@precisioninno.com>

gemini-code-assist

Code Review

This pull request refactors the macro cluster movement logic in hier_rtlmp.cpp to improve efficiency. The implementation now moves the cluster's bounding box and performs overlap checks using the cluster ID before committing the movement to individual hard macros. The overlapsWithHardMacro function signature was also updated to reflect this change. I have no feedback to provide as the review comment regarding the unused variable is incorrect; the variable is still required for the final movement commit.

github-actions · 2026-05-04T19:37:12Z

clang-tidy review says "All clean, LGTM! 👍"

AcKoucher

Code wise LGTM.

Is it possible to include a test to prevent regressions? My hope is that it can be done in Cpp.

Signed-off-by: João Mai <jmai@precisioninno.com>

github-actions

clang-tidy made some suggestions

github-actions · 2026-05-05T17:44:07Z

+    std_cell->setClusterType(StdCellCluster);
+    auto std_soft = std::make_unique<SoftMacro>(std_cell.get());
+    std_soft->setShapeF(macro_width_, macro_height_);
+    std_cell->setSoftMacro(std::move(std_soft));


warning: no header providing "std::move" is directly included [misc-include-cleaner]

src/mpl/test/cpp/TestPusher.cpp:5:

- #include <vector> + #include <utility> + #include <vector>

github-actions · 2026-05-05T17:44:07Z

+    auto std_soft = std::make_unique<SoftMacro>(std_cell.get());
+    std_soft->setShapeF(macro_width_, macro_height_);
+    std_cell->setSoftMacro(std::move(std_soft));
+    root->addChild(std::move(std_cell));


warning: no header providing "std::move" is directly included [misc-include-cleaner]

root->addChild(std::move(std_cell)); ^

github-actions · 2026-05-05T17:44:07Z

+    auto hard_macro = std::make_unique<HardMacro>(
+        odb::Point(x, y), name + "_hard", width, height, cluster.get());
+    HardMacro* macro_ptr = hard_macro.get();
+    hard_macro_storage_.push_back(std::move(hard_macro));


warning: no header providing "std::move" is directly included [misc-include-cleaner]

hard_macro_storage_.push_back(std::move(hard_macro)); ^

Signed-off-by: João Mai <jmai@precisioninno.com>

joaomai · 2026-05-05T18:19:05Z

CI finishes with the same issues as master (nightly).

joaomai · 2026-05-05T18:20:59Z

@AcKoucher
Writing the tests made me realize that there is a bias to which boundary a macro is pushed if a single move doesn't overlap, but doing both moves overlaps with something. Currently, it follows the Boundary enum ordering (B -> L -> T -> R), maybe it would be better to use the closest boundary instead?

github-actions · 2026-05-05T18:23:52Z

clang-tidy review says "All clean, LGTM! 👍"

AcKoucher · 2026-05-05T21:48:54Z

@AcKoucher
Writing the tests made me realize that there is a bias to which boundary a macro is pushed if a single move doesn't overlap, but doing both moves overlaps with something. Currently, it follows the Boundary enum ordering (B -> L -> T -> R), maybe it would be better to use the closest boundary instead?

Good catch. Sure. That makes sense.

There are failures in the BAZEL build regarding include:

src/mpl/test/cpp/TestPusher.cpp:9:10: error: use of private header from outside its module: '../../src/hier_rtlmp.h' [-Wprivate-header]
     9 | #include "../../src/hier_rtlmp.h"
       |          ^
src/mpl/test/cpp/TestPusher.cpp:10:10: error: use of private header from outside its module: '../../src/object.h' [-Wprivate-header]
   10 | #include "../../src/object.h"
      |          ^

Three new patterns from the dogfood survey: 1. `Failed to run image '<image>'` — the Jenkins docker-workflow plugin couldn't `docker run` the build container. Appears in three wrappers on PR The-OpenROAD-Project#10340 (`Caught exception: …`, `ERROR: A fatal error occurred …`, and a bare `java.io.IOException: …`); match the common substring rather than any one wrapper. Maps to the existing `container-registry` source. 2. `Also: org.jenkinsci.plugins.workflow.actions.ErrorAction$ErrorId:` — Jenkins-side workflow exception, distinct from a step that crashed inside user code. New `jenkins-pipeline-error` source. 3. New `pipeline_outcome` parser surfaces the `Finished: <UNSTABLE|ABORTED|NOT_BUILT|SUCCESS>` marker from the tail of the Jenkins log: - UNSTABLE / ABORTED / NOT_BUILT → info finding "pipeline ended X" so the contributor knows it wasn't a hard FAILURE. - SUCCESS → info finding "GitHub status is likely stale" because observing this with a `--pr` that GitHub flagged failed almost always means the commit-status got stuck (PR The-OpenROAD-Project#10332 case). - FAILURE → no finding (already covered by the parsers that captured the actual failing step). Wired into both `repos.toml` lists and the parser registry. Tests cover all three new shapes plus the SUCCESS / UNSTABLE / no-outcome cases. 30/30 Bazel tests pass. Live dogfood diff vs. P0: - PR The-OpenROAD-Project#10286 (UNSTABLE) now surfaces the outcome explicitly. - All other PRs unchanged in outcome. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>

Signed-off-by: João Mai <jmai@precisioninno.com>

github-actions

clang-tidy made some suggestions

github-actions · 2026-05-06T13:53:59Z

+
+Pusher::Pusher(utl::Logger* logger,
+               Cluster* root,
+               odb::dbBlock* block,


warning: no header providing "odb::dbBlock" is directly included [misc-include-cleaner]

src/mpl/src/pusher.cpp:10:

- #include "utl/Logger.h" + #include "odb/db.h" + #include "utl/Logger.h"

github-actions · 2026-05-06T13:53:59Z

+Pusher::Pusher(utl::Logger* logger,
+               Cluster* root,
+               odb::dbBlock* block,
+               const std::vector<odb::Rect>& io_blockages)


warning: no header providing "odb::Rect" is directly included [misc-include-cleaner]

src/mpl/src/pusher.cpp:10:

- #include "utl/Logger.h" + #include "odb/geom.h" + #include "utl/Logger.h"

github-actions · 2026-05-06T13:53:59Z

+               "Macro Cluster {}",
+               macro_cluster->getName());
+
+    std::map<Boundary, int> boundaries_distance


warning: no header providing "mpl::Boundary" is directly included [misc-include-cleaner]

src/mpl/src/pusher.cpp:9:

- #include "object.h" + #include "mpl-util.h" + #include "object.h"

github-actions · 2026-05-06T13:53:59Z

+      logger_->report("Distance to Close Boundaries:");
+
+      for (auto& [boundary, distance] : boundaries_distance) {
+        logger_->report("{} {}", toString(boundary), distance);


warning: no header providing "mpl::toString" is directly included [misc-include-cleaner]

logger_->report("{} {}", toString(boundary), distance); ^

Signed-off-by: João Mai <jmai@precisioninno.com>

github-actions · 2026-05-06T15:25:02Z

clang-tidy review says "All clean, LGTM! 👍"

mpl: check Pusher moves overlaps correctly

c8360b3

Signed-off-by: João Mai <jmai@precisioninno.com>

openroad-ci assigned joaomai May 4, 2026

github-actions Bot added the size/S label May 4, 2026

gemini-code-assist Bot reviewed May 4, 2026

View reviewed changes

joaomai requested a review from AcKoucher May 4, 2026 19:29

AcKoucher reviewed May 4, 2026

View reviewed changes

mpl: add unit tests for Pusher

fda67ec

Signed-off-by: João Mai <jmai@precisioninno.com>

github-actions Bot added size/M and removed size/S labels May 5, 2026

github-actions Bot reviewed May 5, 2026

View reviewed changes

mpl: include missing header

bc924e5

Signed-off-by: João Mai <jmai@precisioninno.com>

mpl: extract Pusher for better testing

1a975cf

Signed-off-by: João Mai <jmai@precisioninno.com>

github-actions Bot added size/XL and removed size/M labels May 6, 2026

github-actions Bot reviewed May 6, 2026

View reviewed changes

mpl: fix includes

69e8cab

Signed-off-by: João Mai <jmai@precisioninno.com>

joaomai requested a review from AcKoucher May 6, 2026 19:02

Conversation

openroad-ci commented May 4, 2026

Summary

Type of Change

Impact

Verification

Related Issues

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

github-actions Bot commented May 4, 2026

Uh oh!

AcKoucher left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 5, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 5, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 5, 2026

Choose a reason for hiding this comment

Uh oh!

joaomai commented May 5, 2026

Uh oh!

joaomai commented May 5, 2026

Uh oh!

github-actions Bot commented May 5, 2026

Uh oh!

AcKoucher commented May 5, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants