Skip to content

Conversation

@openroad-ci
Copy link
Collaborator

This PR changes the behavior of the calculaMacroTilings function. Now, the function enumerates the possible tilings for a given cluster instead of using SA to find the tilings. This enables tighter results, since the generated options are guaranteed to be minimum area.

This PR also removes the size_tolerance variable. It was used to change the maximum and minimum values for the thresholds, reducing the minimum and increasing the maximum. In practice, it made the use o MPL thresholds variables unreliable, with the only justification being a comment saying that it made the macro placer "more robust".

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

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

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 is a great simplification, replacing the Simulated Annealing approach for macro tiling with a more direct and deterministic enumeration method. This should lead to more predictable and optimal results for macro cluster shapes. The removal of the size_tolerance variable is also a good cleanup, making the clustering thresholds more reliable. I have a few suggestions to improve the implementation further, mainly focusing on performance, robustness, and code clarity.

return;
}
int number_of_macros = hard_macros.size();
int macro_width = hard_macros.front()->getWidth();
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

There's a potential for a crash here if cluster->getHardMacros() returns an empty vector, as hard_macros.front() would be undefined behavior. Although the call chain seems to prevent this, adding a defensive check for an empty vector would make the code more robust. For example:

std::vector<HardMacro*> hard_macros = cluster->getHardMacros();
if (hard_macros.empty()) {
  return;
}
int number_of_macros = hard_macros.size();
// ...

Comment on lines 645 to 649
if (tilings.size() == 0) {
TilingList extra_tilings = generateTilingsForMacroCluster(
macro_width, macro_height, number_of_macros + 1);
tilings.insert(tilings.end(), extra_tilings.begin(), extra_tilings.end());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This logic of trying to generate tilings for number_of_macros + 1 is a clever heuristic to find more aspect ratios, especially when number_of_macros is a prime number. However, this intent is not immediately obvious. Adding a comment would greatly improve code readability and maintainability.

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

}
}
remaining_runs -= run_thread;
if (tilings.size() == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]

Suggested change
if (tilings.size() == 0) {
if (tilings.empty()) {
Additional context

/usr/include/c++/13/bits/stl_vector.h:1087: method 'vector'::empty() defined here

      empty() const _GLIBCXX_NOEXCEPT
      ^

}

if (tilings_set.empty()) {
if (tilings.size() == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]

Suggested change
if (tilings.size() == 0) {
if (tilings.empty()) {
Additional context

/usr/include/c++/13/bits/stl_vector.h:1087: method 'vector'::empty() defined here

      empty() const _GLIBCXX_NOEXCEPT
      ^

Signed-off-by: João Mai <jmai@precisioninno.com>
@github-actions
Copy link
Contributor

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

@joaomai
Copy link
Contributor

joaomai commented Jan 13, 2026

Paired with PR #3781 on ORFS.

@joaomai
Copy link
Contributor

joaomai commented Jan 13, 2026

Secure CI is ok, public CI has some fails that are addressed in the ORFS PR.

Copy link
Contributor

@AcKoucher AcKoucher left a comment

Choose a reason for hiding this comment

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

Nice!

Comment on lines 646 to 648
TilingList extra_tilings = generateTilingsForMacroCluster(
macro_width, macro_height, number_of_macros + 1);
tilings.insert(tilings.end(), extra_tilings.begin(), extra_tilings.end());
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for std::vector::insert here. Just assign tilings directly.

Signed-off-by: João Mai <jmai@precisioninno.com>
@github-actions
Copy link
Contributor

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

@AcKoucher AcKoucher self-requested a review January 13, 2026 16:36
Copy link
Contributor

@AcKoucher AcKoucher left a comment

Choose a reason for hiding this comment

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

LGTM.

@AcKoucher AcKoucher requested a review from maliberty January 13, 2026 16:37
Comment on lines +685 to +686
if (number_of_macros % cols == 0) {
rows = number_of_macros / cols;
Copy link
Member

Choose a reason for hiding this comment

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

If we only allow perfect tiling we will have more limited choices (e.g. for an 9 macros you won't try 2x5, only 1x9 and 3x3). Is this intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. The previous code kept only the smallest area tilings too. Doing it this way also avoids notches.

}
int number_of_macros = hard_macros.size();
int macro_width = hard_macros.front()->getWidth();
int macro_height = hard_macros.front()->getHeight();
Copy link
Member

Choose a reason for hiding this comment

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

Where does this check that all macros are of a single master? What if they are not?

Copy link
Contributor

Choose a reason for hiding this comment

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

The clusters are always made of macros of the same size. The function that creates/merges macro clusters checks for their sizes (ClusteringEngine::groupSingleMacroCluster using the size_class variable).

@maliberty maliberty merged commit b421595 into The-OpenROAD-Project:master Jan 13, 2026
13 checks passed
@maliberty maliberty deleted the mpl-tight-packing branch January 13, 2026 19:32
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.

4 participants