Skip to content
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

mpl: fix fixed terminals for cluster of unplaced IOs and max cost #6689

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

AcKoucher
Copy link
Contributor

@AcKoucher AcKoucher commented Feb 11, 2025

Fix #6668

What is Wrong

In the context of the WL computation inside SA for when the target cluster is a cluster of unplaced IOs:

  1. The distance to boundary is being wrongly computed when the cluster of unplaced IOs is the the cluster of unconstrained pins, because we're using the shape of its Cluster object which does not take into account the offset w.r.t. the current outline.
  2. The max/limit cost which should be the die area HPWL is also being wrongly computed, because we always use the shape of the target io cluster that may not be the die area. The only situation in which that is true is if the cluster of unplaced IOs represents pins without constraints.

Context and Changes

Originally, a fixed terminal in HierRTLMP was just a Soft/Hard Macro with the shape of a point (area = 0).
With the new IOs' abstraction implemented in #5809, for clusters of unplaced IOs, we need to have a Soft/Hard Macro object with a proper shape inside SA in order to compute the distance to the closest boundary correctly.

As this needs to happen for both cluster placement (Soft) and macro placement (Hard) I made the constructors used to create fixed terminals in both Hard/Soft Macro have the same arguments to be able to template the function that uses these constructors.

I.e., this PR ensures that:

  • Inside SACore, for a WL computation in which one of the objects is a cluster of unplaced IOs, the Cluster object is used to identify to which boundary the cluster of unplaced IOs is constrained to.
  • We now cache the dimensions of the die area inside the PhysicalHierarchy struct during the ClusteringEngine initialization. This cache is passed on to the SA Core, which during its initialization will offset it to the current outline. The cache will be, then, used to compute the dist to the closest boundary in the case of the cluster of unconstrained IOs.
  • The cached die area shape will also be used to compute the max distance to generate the max cost.

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! 👍"

@AcKoucher
Copy link
Contributor Author

Running Secure-CI

@AcKoucher AcKoucher requested a review from maliberty February 11, 2025 17:47
@maliberty
Copy link
Member

Code is ok; waiting on CI to merge

@AcKoucher
Copy link
Contributor Author

AcKoucher commented Feb 12, 2025

The failure in the regression test seems to triggered by a bug that is not related to the changes here. However, it needs fixing before this PR gets merged. I'm working on it.

@AcKoucher AcKoucher changed the title mpl: fix fixed terminals for cluster of unplaced IOs mpl: fix fixed terminals for cluster of unplaced IOs and max cost Feb 28, 2025
@AcKoucher
Copy link
Contributor Author

AcKoucher commented Feb 28, 2025

@maliberty I found another bug in the same scope of the changes here. I pushed the fix and updated the description. Could you take another look?

PS this is still needs #6697 to make the regression tests consistent.

  1. Cache actual offset die area instead of just the half
     perimeter and use the cached die to compute the max
     dist for max cost.
  2. Some renaming.

Signed-off-by: Arthur Koucher <[email protected]>
@AcKoucher
Copy link
Contributor Author

AcKoucher commented Mar 6, 2025

I pushed one more change that will help me rebasing second phase of the IO constraints' project branch with the changes here.
I updated the description accordingly.

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.

mpl: the distance to boundary is wrongly computed from level != 0 inside SA
2 participants