mpl: check Pusher moves overlaps correctly#10332
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
|
clang-tidy review says "All clean, LGTM! 👍" |
AcKoucher
left a comment
AcKoucher
left a comment
There was a problem hiding this comment.
Code wise LGTM.
Is it possible to include a test to prevent regressions? My hope is that it can be done in Cpp.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
CI finishes with the same issues as master (nightly). |
|
@AcKoucher |
|
clang-tidy review says "All clean, LGTM! 👍" |
Good catch. Sure. That makes sense. There are failures in the BAZEL build regarding |
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>
|
clang-tidy review says "All clean, LGTM! 👍" |
openroad-ci
force-pushed
the
mpl-pusher-overlap-bugfix
branch
from
69e8cab to
1fa1043
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: João Mai <jmai@precisioninno.com>
Signed-off-by: João Mai <jmai@precisioninno.com>
openroad-ci
force-pushed
the
mpl-pusher-overlap-bugfix
branch
from
1fa1043 to
384e394
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
3 participants
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
Impact
Macros don't overlap now.
Verification
./etc/Build.sh).Related Issues
#10330