mpl: improved notches #9282

openroad-ci · 2026-01-18T02:29:43Z

This PR improves the notch detection for the notch penalty calculation. It achieves it by considering closely aligned macros as aligned (in order to generate a more meaningful grid) and merging cells in order to find the notches actual size and amount.

It also enhances the debug visualization, showing the notches when using the mpl::mpl_debug command.

gemini-code-assist

Code Review

This pull request introduces an improved notch detection algorithm for penalty calculation, along with debug visualization for the detected notches. The core logic changes are in SACoreSoftMacro::calNotchPenalty. My review focuses on enhancing the performance and maintainability of this new algorithm. I've identified a significant performance bottleneck due to redundant computations and suggest a fix. Additionally, I've provided recommendations for refactoring to reduce code duplication, replacing magic numbers with constants, and removing a non-standard header for better portability.

gemini-code-assist · 2026-01-18T02:31:45Z

src/mpl/src/SACoreSoftMacro.cpp


 #include "SACoreSoftMacro.h"

+#include <bits/ranges_algo.h>


The header <bits/ranges_algo.h> is a non-standard, GCC-specific implementation detail. For better portability, it's best to rely on standard C++ headers. Since <algorithm> and <ranges> are already included, this header is likely unnecessary and should be removed.

gemini-code-assist · 2026-01-18T02:31:45Z

src/mpl/src/SACoreSoftMacro.cpp

+  std::vector<int> x_coords;
+  int epsilon = outline_.dx() / 100;
+  for (size_t i = 0; i < x_point.size(); i++) {
+    if (i + 1 < x_point.size()
+        && std::abs(x_point[i + 1] - x_point[i]) <= epsilon) {
      continue;
    }
-    x_point.insert(macro.getX());
-    x_point.insert(macro.getX() + macro.getWidth());
-    y_point.insert(macro.getY());
-    y_point.insert(macro.getY() + macro.getHeight());
+    x_coords.push_back(x_point[i]);
+  }
+
+  std::vector<int> y_coords;
+  epsilon = outline_.dy() / 100;
+  for (size_t i = 0; i < y_point.size(); i++) {
+    if (i + 1 < y_point.size()
+        && std::abs(y_point[i + 1] - y_point[i]) <= epsilon) {
+      continue;
+    }
+    y_coords.push_back(y_point[i]);
  }


The value 100 is used as a magic number in two places to calculate epsilon. It would be better to define it as a single named constant (e.g., constexpr int kCoordEpsilonDivisor = 100;) at a suitable scope to improve readability, maintainability, and ensure consistency.

References

Define tunable parameters as named constants instead of using hardcoded magic numbers.

gemini-code-assist · 2026-01-18T02:31:45Z

src/mpl/src/SACoreSoftMacro.cpp

+      if (n.total() == 4) {
+        notch_penalty_ += calSingleNotchPenalty(block_->dbuToMicrons(width),
+                                                block_->dbuToMicrons(height));
+        if (graphics_) {
+          graphics_->addNotch(odb::Rect(x_coords[start_col],
+                                        y_coords[start_row],
+                                        x_coords[end_col + 1],
+                                        y_coords[end_row + 1]));
+        }
+      } else if (n.top && n.bottom) {
+        if (height < notch_h_th_) {
+          notch_penalty_ += calSingleNotchPenalty(block_->dbuToMicrons(width),
+                                                  block_->dbuToMicrons(height));
+          if (graphics_) {
+            graphics_->addNotch(odb::Rect(x_coords[start_col],
+                                          y_coords[start_row],
+                                          x_coords[end_col + 1],
+                                          y_coords[end_row + 1]));
+          }
+        }
+      } else if (n.left && n.right) {
+        if (width < notch_v_th_) {
          notch_penalty_ += calSingleNotchPenalty(block_->dbuToMicrons(width),
                                                  block_->dbuToMicrons(height));
+          if (graphics_) {
+            graphics_->addNotch(odb::Rect(x_coords[start_col],
+                                          y_coords[start_row],
+                                          x_coords[end_col + 1],
+                                          y_coords[end_row + 1]));
+          }
        }
      }


The code block for calculating the notch penalty and adding the notch for visualization is duplicated in three places within the if-else if structure. This can be refactored to improve maintainability and reduce redundancy.

You could determine if a notch should be penalized using a boolean flag, and then have a single block of code that performs the penalty calculation and graphics update.

bool is_notch = false; if (n.total() == 4) { is_notch = true; } else if (n.top && n.bottom) { if (height < notch_h_th_) { is_notch = true; } } else if (n.left && n.right) { if (width < notch_v_th_) { is_notch = true; } } if (is_notch) { notch_penalty_ += calSingleNotchPenalty(block_->dbuToMicrons(width), block_->dbuToMicrons(height)); if (graphics_) { graphics_->addNotch(odb::Rect(x_coords[start_col], y_coords[start_row], x_coords[end_col + 1], y_coords[end_row + 1])); } }

github-actions

clang-tidy made some suggestions

github-actions · 2026-01-18T02:33:48Z

src/mpl/src/SACoreSoftMacro.cpp

 #include <cmath>
 #include <iterator>
+#include <limits>
+#include <ranges>


warning: included header limits is not used directly [misc-include-cleaner]

Suggested change

#include <ranges>

#include <ranges>

github-actions · 2026-01-18T02:33:48Z

src/mpl/src/SACoreSoftMacro.cpp

 #include <iterator>
+#include <limits>
+#include <ranges>
 #include <set>


warning: included header ranges is not used directly [misc-include-cleaner]

Suggested change

#include <set>

#include <set>

github-actions · 2026-01-18T02:33:48Z

src/mpl/src/SACoreSoftMacro.cpp

+
+    if (graphics_) {
+      graphics_->setNotchPenalty(
+          {"Notch", notch_weight_, notch_penalty_, norm_notch_penalty_});


warning: use designated initializer list to initialize 'PenaltyData' [modernize-use-designated-initializers]

Suggested change

{"Notch", notch_weight_, notch_penalty_, norm_notch_penalty_});

{.name="Notch", .weight=notch_weight_, .value=notch_penalty_, .normalization_factor=norm_notch_penalty_});

Additional context

src/mpl/src/mpl-util.h:89: aggregate type is defined here

struct PenaltyData ^

github-actions · 2026-01-18T02:33:48Z

src/mpl/src/SACoreSoftMacro.cpp

+
+  std::vector<int> x_coords;
+  int epsilon = outline_.dx() / 100;
+  for (size_t i = 0; i < x_point.size(); i++) {


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

src/mpl/src/SACoreSoftMacro.cpp:9:

- #include <iterator> + #include <cstddef> + #include <iterator>

github-actions · 2026-01-18T22:09:30Z

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

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

github-actions · 2026-01-21T13:40:33Z

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

AcKoucher

I think that the notch function is too big and should be broken into smaller pieces, that's why most of the comments. We've been making a large effort to a more granular code and it has been proven to be a very helpful thing.

I tend to prefer functions over lambdas if it starts to get too big. I think it does depend on the context (it looks that the idea here was to keep the notch detection code well encapsulated), but for concision and readability, let's keep things smaller. We can have a "section" of methods only for notch penalty.

A question: are the notches being erased when rendering after MPL is done? (see eraseDrawing).

AcKoucher · 2026-01-21T15:02:13Z

src/mpl/src/SACoreSoftMacro.cpp

+    bool left = true;
+    bool right = true;
+
+    int total() { return top + bottom + left + right; }


I'm not a fan of implicit casting. However the logic here is sound. @maliberty would you have an opinion on it?

AcKoucher · 2026-01-21T15:03:24Z

src/mpl/src/SACoreSoftMacro.cpp

-    for (int col = 0; col < num_x; col++) {
-      if (grid[row][col]) {
-        continue;
+  auto neighbors = [&](int row1, int col1, int row2, int col2) {


I think this should be a function `findNeighbors".

AcKoucher · 2026-01-21T15:05:44Z

src/mpl/src/SACoreSoftMacro.cpp

+      int end_row = start_row;
+      int end_col = start_col;
+
+      Neighbors n = neighbors(start_row, start_col, end_row, end_col);


Please, avoid this type of abbreviation.

AcKoucher · 2026-01-21T15:08:07Z

src/mpl/src/SACoreSoftMacro.cpp

+    return neighbors;
+  };
+
+  auto valid = [&](int row1, int col1, int row2, int col2) {


This should also be a function. Validity doesn't feel like the best quality here. Perhaps expansionOverlapped or something in this sense? I'm not attached to my suggestion.

AcKoucher · 2026-01-21T15:10:05Z

src/mpl/src/SACoreSoftMacro.cpp

    return;
  }

+  struct Neighbors


This can be a member of the annealer itself. I also think we should have a comment explaining what are Neighbors (seeing a boolean in the code doesn't tell us much).

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

github-actions · 2026-01-22T12:25:04Z

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

AcKoucher · 2026-01-22T14:47:29Z

src/mpl/src/graphics.cpp

+    painter.drawRect(rect);
+  }
+
+  // notches_.clear();


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

github-actions · 2026-01-22T18:07:27Z

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

openroad-ci assigned joaomai Jan 18, 2026

gemini-code-assist bot reviewed Jan 18, 2026

View reviewed changes

github-actions bot reviewed Jan 18, 2026

View reviewed changes

joaomai added 10 commits January 20, 2026 17:27

mpl: notches display for mpl_debug

7e3d07d

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

mpl: consider fixed macros during notch penalty calculation

7d60fa8

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

mpl: grid quantization and channels for notch penalty

4764698

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

mpl: notch merging

47aede1

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

mpl: minor adjustments

13275b6

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

mpl: adjust graphics and notch openness consideration

3d18105

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

mpl: disallow expansion that changes the open direction

93d2b8d

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

mpl: small refactor and formatting

135cd5e

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

formatting

1bdae97

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

mpl: adjust calSegmentIndex function

407f91a

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

openroad-ci force-pushed the mpl-improve-notches branch from 94f3969 to 407f91a Compare January 21, 2026 13:36

joaomai requested a review from AcKoucher January 21, 2026 13:54

AcKoucher suggested changes Jan 21, 2026

View reviewed changes

joaomai added 4 commits January 21, 2026 22:36

mpl: refactor calNotchPenalty

a46e9a6

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

mpl: notches bugfixes

b735830

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

mpl: remove unecessary conversion to micron

3cb7632

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

Merge branch 'master' into mpl-improve-notches

e98234c

joaomai requested a review from AcKoucher January 22, 2026 12:22

AcKoucher suggested changes Jan 22, 2026

View reviewed changes

src/mpl/src/graphics.cpp Outdated

painter.drawRect(rect);

}

// notches_.clear();

Copy link

Contributor

AcKoucher Jan 22, 2026

Choose a reason for hiding this comment

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

Leftover?

joaomai added 2 commits January 22, 2026 17:40

mpl: remove leftover comment

6868275

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

Merge branch 'master' into mpl-improve-notches

47c2f9a

	{"Notch", notch_weight_, notch_penalty_, norm_notch_penalty_});
	{.name="Notch", .weight=notch_weight_, .value=notch_penalty_, .normalization_factor=norm_notch_penalty_});

mpl: improved notches #9282

Are you sure you want to change the base?

mpl: improved notches #9282

Conversation

openroad-ci commented Jan 18, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jan 18, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 18, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 18, 2026

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 Jan 18, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jan 18, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jan 18, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jan 18, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 18, 2026

Uh oh!

github-actions bot commented Jan 21, 2026

Uh oh!

AcKoucher left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AcKoucher Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

AcKoucher Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

AcKoucher Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

AcKoucher Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AcKoucher Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 22, 2026

Uh oh!

AcKoucher Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AcKoucher left a comment •

edited

Loading

AcKoucher Jan 21, 2026 •

edited

Loading