mpl: improved notches by openroad-ci · Pull Request #9282 · The-OpenROAD-Project/OpenROAD

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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

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

Signed-off-by: João Mai <[email protected]>

AcKoucher · 2026-01-28T18:18:27Z

src/mpl/src/SACoreSoftMacro.h


  bool enhancements_on_ = false;
  bool centralization_was_reverted_ = false;
+  bool is_single_array_single_std_cell_cluster = false;


I think it would make more readable for the perspective of HierRTLMP class if this bool has something like force_centralization_.

Then, when setting it we'd have:

if (single_array_single_std_cell_cluster) { sa->forceCentralization(); }

github-actions · 2026-01-28T18:25:07Z

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

Signed-off-by: João Mai <[email protected]>

joaomai · 2026-01-29T16:20:08Z

Currently blocked by #3846 on ORFS.

Signed-off-by: João Mai <[email protected]>

github-actions · 2026-02-03T23:52:37Z

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

Signed-off-by: João Mai <[email protected]>

github-actions · 2026-02-04T00:27:55Z

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

Signed-off-by: João Mai <[email protected]>

github-actions · 2026-02-04T02:43:19Z

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

joaomai · 2026-02-04T13:03:04Z

Paired with #3863 on ORFS.

maliberty · 2026-02-04T16:38:32Z

src/mpl/src/SACoreSoftMacro.cpp

+    if (i + 1 < x_point.size()
+        && std::abs(x_point[i + 1] - x_point[i]) <= epsilon) {


This has the potential to skip too many points. Imagine a series of points all 0.9 epsilon apart. You would collapse them all. It would be better to set a threshold from an accepted point and skip all points within that threshold.

Changing this will require another secure CI so I suggest you do it in a follow up PR.

maliberty · 2026-02-04T16:41:12Z

src/mpl/src/SACoreSoftMacro.cpp

+bool SACoreSoftMacro::isSegmentEmpty(std::vector<std::vector<bool>>& grid,
+                                     int start_row,
+                                     int start_col,
+                                     int end_row,
+                                     int end_col)


All arguments should be const and the method itself can be static.

maliberty · 2026-02-04T16:41:53Z

src/mpl/src/SACoreSoftMacro.cpp

+SACoreSoftMacro::Neighbors SACoreSoftMacro::findNeighbors(
+    std::vector<std::vector<bool>>& grid,
+    int start_row,
+    int start_col,
+    int end_row,
+    int end_col)


same: const + static

maliberty · 2026-02-04T16:59:22Z

src/mpl/src/SACoreSoftMacro.cpp

+      if (current_neighbors.total() == 4) {
+        is_notch = true;


Pathological but you could have neighbors on all sides of a region larger than notch_h/v_th_ in both directions (e.g. a ring of macros). Probably that shouldn't be a notch.

maliberty · 2026-02-04T17:00:24Z

src/mpl/src/SACoreSoftMacro.h

+  // Used to calculate notches
+  struct Neighbors


I had to read the code to understand the semantics of these bools. A better descriptive comment would be helpful.

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 <[email protected]>

mpl: consider fixed macros during notch penalty calculation

7d60fa8

Signed-off-by: João Mai <[email protected]>

mpl: grid quantization and channels for notch penalty

4764698

Signed-off-by: João Mai <[email protected]>

mpl: notch merging

47aede1

Signed-off-by: João Mai <[email protected]>

mpl: minor adjustments

13275b6

Signed-off-by: João Mai <[email protected]>

mpl: adjust graphics and notch openness consideration

3d18105

Signed-off-by: João Mai <[email protected]>

mpl: disallow expansion that changes the open direction

93d2b8d

Signed-off-by: João Mai <[email protected]>

mpl: small refactor and formatting

135cd5e

Signed-off-by: João Mai <[email protected]>

formatting

1bdae97

Signed-off-by: João Mai <[email protected]>

mpl: adjust calSegmentIndex function

407f91a

Signed-off-by: João Mai <[email protected]>

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 <[email protected]>

mpl: notches bugfixes

b735830

Signed-off-by: João Mai <[email protected]>

mpl: remove unecessary conversion to micron

3cb7632

Signed-off-by: João Mai <[email protected]>

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 <[email protected]>

Merge branch 'master' into mpl-improve-notches

47c2f9a

joaomai added 2 commits January 23, 2026 06:20

bazel: update mock_array MPL params

5086c09

Signed-off-by: João Mai <[email protected]>

mpl: update tests

3912863

Signed-off-by: João Mai <[email protected]>

joaomai added 4 commits January 24, 2026 22:04

Merge branch 'master' into mpl-improve-notches

c5bcb53

Merge branch 'master' into mpl-improve-notches

99f71e2

mpl: mock-array workaround

38bf6ed

Signed-off-by: João Mai <[email protected]>

Merge branch 'master' into mpl-improve-notches

0147564

joaomai requested a review from AcKoucher January 28, 2026 16:36

Merge branch 'master' into mpl-improve-notches

bfe2c4e

AcKoucher reviewed Jan 28, 2026

View reviewed changes

joaomai added 2 commits January 29, 2026 15:38

Merge branch 'master' into mpl-improve-notches

2a11a3e

mpl: rename variable for clarity

3cf07f2

Signed-off-by: João Mai <[email protected]>

joaomai added 5 commits January 31, 2026 16:47

Merge branch 'master' into mpl-improve-notches

2e83e45

Merge branch 'master' into mpl-improve-notches

a13a488

Merge branch 'master' into mpl-improve-notches

cae5a35

mpl: change static_cast in cal notch to avoid integer overflow

cd6f980

Signed-off-by: João Mai <[email protected]>

Merge branch 'master' into mpl-improve-notches

fc73bb5

mpl: update tests

33cc3b1

Signed-off-by: João Mai <[email protected]>

update tinyRocket metrics

60bd6e7

Signed-off-by: João Mai <[email protected]>

joaomai mentioned this pull request Feb 4, 2026

update metrics for MPL improve notches and RTLMP_NOTCH_WT default value The-OpenROAD-Project/OpenROAD-flow-scripts#3863

Merged

joaomai requested a review from maliberty February 4, 2026 13:03

maliberty approved these changes Feb 4, 2026

View reviewed changes

maliberty merged commit 33cde43 into The-OpenROAD-Project:master Feb 4, 2026
13 checks passed

maliberty deleted the mpl-improve-notches branch February 4, 2026 17:01

AcKoucher mentioned this pull request Feb 4, 2026

mpl: remove data flow driven mechanism #9328

Open

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

		if (i + 1 < x_point.size()
		&& std::abs(x_point[i + 1] - x_point[i]) <= epsilon) {

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 28, 2026

Uh oh!

joaomai commented Jan 29, 2026

Uh oh!

github-actions bot commented Feb 3, 2026

Uh oh!

github-actions bot commented Feb 4, 2026

Uh oh!

github-actions bot commented Feb 4, 2026

Uh oh!

joaomai commented Feb 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

AcKoucher left a comment •

edited

Loading

AcKoucher Jan 21, 2026 •

edited

Loading