Skip to content

Downsize move #7410

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
May 31, 2025
Merged

Downsize move #7410

merged 20 commits into from
May 31, 2025

Conversation

mguthaus
Copy link
Contributor

Added a new move to downsize off-critical gates to improve setup slack. It respects timing by trying to not make the off-ciritical fanouts into critical nets. In general, this reduces area in many cases, but can have no impact on some other cases.

mguthaus added 5 commits May 21, 2025 12:26
Added skip option.
Separated sizeup and sizedown in move sequence string.
Added logger output for accept/reject move stats.
Added opt_moves logger tag for all moves.

Signed-off-by: Matthew Guthaus <[email protected]>
Signed-off-by: Matthew Guthaus <[email protected]>
Signed-off-by: Matthew Guthaus <[email protected]>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

mguthaus added 2 commits May 21, 2025 14:43
Signed-off-by: Matthew Guthaus <[email protected]>
Signed-off-by: Matthew Guthaus <[email protected]>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Copy link
Contributor

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

1 similar comment
Copy link
Contributor

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

Copy link
Contributor

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

Signed-off-by: Matthew Guthaus <[email protected]>
Copy link
Contributor

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

Signed-off-by: Matthew Guthaus <[email protected]>
Copy link
Contributor

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

Copy link
Contributor

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

@maliberty
Copy link
Member

Ready for review?

@mguthaus
Copy link
Contributor Author

I think so

@maliberty
Copy link
Member

There are still some CI failures to resolve.

Copy link
Contributor

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

1 similar comment
Copy link
Contributor

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

Signed-off-by: Matthew Guthaus <[email protected]>
Copy link
Contributor

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

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

using sta::LibertyCell;
using sta::LibertyPort;
using sta::LoadPinIndexMap;
using sta::Net;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: using decl 'Net' is unused [misc-unused-using-decls]

using sta::Net;
           ^
Additional context

src/rsz/src/SizeDownMove.cc:25: remove the using

using sta::Net;
           ^

Copy link
Contributor

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

Signed-off-by: Matthew Guthaus <[email protected]>
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

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

@mguthaus mguthaus requested a review from maliberty May 30, 2025 20:35
@maliberty maliberty merged commit 01aa43f into The-OpenROAD-Project:master May 31, 2025
11 checks passed
@maliberty
Copy link
Member

Not currently active due to increased ERCs.

[INFO RSZ-0094] Found 2 endpoints with setup violations.
[INFO RSZ-0099] Repairing 2 out of 2 (100.00%) violating endpoints...
Iter | Removed | Resized | Inserted | Cloned | Pin | Area | WNS | TNS | Viol | Worst
| Buffers | Gates | Buffers | Gates | Swaps | | | | Endpts | Endpt
--------------------------------------------------------------------------------------------------------------
0 | 0 | 0 | 0 | 0 | 0 | +0.0% | -0.118 | -0.2 | 2 | y2
5 | 0 | 0 | 0 | 0 | 0 | +0.0% | -0.118 | -0.2 | 2 | y2
10 | 0 | 3 | 0 | 0 | 1 | +53.7% | -0.118 | -0.2 | 2 | y2
10 | 0 | 0 | 0 | 0 | 0 | +53.7% | -0.118 | -0.2 | 2 | y2
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems suspicious, no? 0 changes but +53% area?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an existing bug in the area report, but this may be another. Right now, the area includes moves that aren't "accepted" but pending whereas the other stats are only the accepted moves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Earlier implementations would present data that could be reversed later. I agree it should be consistent though. This isn't limited to my PR but can happen in general.

[INFO RSZ-0094] Found 2 endpoints with setup violations.
[INFO RSZ-0099] Repairing 2 out of 2 (100.00%) violating endpoints...
Iter | Removed | Resized | Inserted | Cloned | Pin | Area | WNS | TNS | Viol | Worst
| Buffers | Gates | Buffers | Gates | Swaps | | | | Endpts | Endpt
--------------------------------------------------------------------------------------------------------------
0 | 0 | 0 | 0 | 0 | 0 | +0.0% | -0.118 | -0.2 | 2 | y2
5 | 0 | 0 | 0 | 0 | 0 | +0.0% | -0.118 | -0.2 | 2 | y2
10 | 0 | 3 | 0 | 0 | 1 | +53.7% | -0.118 | -0.2 | 2 | y2
10 | 0 | 0 | 0 | 0 | 0 | +53.7% | -0.118 | -0.2 | 2 | y2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants