mpl: change objects that ared considered during notch calculation #9449

openroad-ci · 2026-02-10T16:39:12Z

This PR changes which objects are used to detect notches, changing from "not a std cell clusters" to "mixed clusters and macro clusters". This is necessary since it doesn't really make sense to include IO pins and PADs in notch detection (and some designs such as bp_quad have a huge amount of them, causing MPL to take way too long to finish).

The PR also includes suggestions made in PR #9282.

github-actions · 2026-02-10T16:43:12Z

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

gemini-code-assist

Code Review

This pull request refactors the notch calculation logic, primarily by changing which objects are considered. This is a good optimization. However, I've identified a critical bug in the isSegmentEmpty function that could lead to incorrect notch detection. There's also a significant, and likely unintentional, change in how notches are defined in calNotchPenalty, where fully enclosed empty spaces might no longer be penalized. I've also pointed out a minor typo in a function declaration. Please address these issues to ensure the correctness of the implementation.

gemini-code-assist · 2026-02-10T16:45:26Z

src/mpl/src/SACoreSoftMacro.cpp

+bool SACoreSoftMacro::isSegmentEmpty(const std::vector<std::vector<bool>>& grid,
+                                     const int start_row,
+                                     const int start_col,
+                                     const int end_row,
+                                     const int end_col)
 {
  for (int i = start_row; i <= end_row; i++) {
-    for (int j = start_col; j <= end_col; j++) {
-      if (grid[i][j]) {
-        return false;
-      }
+    if (grid[i][end_col]) {
+      return false;
+    }
+  }
+
+  for (int j = start_col; j <= end_col; j++) {
+    if (grid[end_row][j]) {
+      return false;
    }
  }



The new implementation of isSegmentEmpty is incorrect. It no longer checks if the entire specified segment is empty, but only its rightmost column and bottommost row. This can lead to incorrect behavior. For example, if grid[start_row][start_col] is occupied and the segment is larger than 1x1, this function will incorrectly return true.

The previous implementation was correct. If this change is for optimization, it should be implemented differently. A correct optimization would be to check only the newly added row or column. This could be done with helper functions like isRowEmpty and isColEmpty and calling them appropriately from the expansion loop in calNotchPenalty.

As it is, this is a critical bug that will cause incorrect notch detection. I recommend reverting this function to its previous correct implementation.

bool SACoreSoftMacro::isSegmentEmpty(const std::vector<std::vector<bool>>& grid, const int start_row, const int start_col, const int end_row, const int end_col) { for (int i = start_row; i <= end_row; i++) { for (int j = start_col; j <= end_col; j++) { if (grid[i][j]) { return false; } } } return true; }

gemini-code-assist · 2026-02-10T16:45:26Z

src/mpl/src/SACoreSoftMacro.cpp

      bool is_notch = false;
-      if (current_neighbors.total() == 4) {
-        is_notch = true;
-      } else if (current_neighbors.top && current_neighbors.bottom) {
+      if (current_vicinity.top && current_vicinity.bottom) {
        if (height < notch_h_th_) {
          is_notch = true;
        }
-      } else if (current_neighbors.left && current_neighbors.right) {
+      }
+
+      if (current_vicinity.left && current_vicinity.right) {
        if (width < notch_v_th_) {
          is_notch = true;
        }


The logic for identifying a notch has been changed significantly. The original code considered any fully enclosed empty space as a notch (current_neighbors.total() == 4). This condition has been removed.

The new logic is:

bool is_notch = false; if (current_vicinity.top && current_vicinity.bottom) { if (height < notch_h_th_) { is_notch = true; } } if (current_vicinity.left && current_vicinity.right) { if (width < notch_v_th_) { is_notch = true; } }

For a fully enclosed space, this will only flag it as a notch if it is thin in at least one dimension (height < notch_h_th_ or width < notch_v_th_). A large, fully enclosed empty area will no longer be considered a notch, which seems incorrect as it represents wasted space that should be penalized.

This change is not mentioned in the PR description and appears to be a bug. If this change is intentional, it should be documented. Otherwise, the original logic for fully enclosed spaces should be restored.

bool is_notch = false; if (current_vicinity.total() == 4) { is_notch = true; } else if (current_vicinity.top && current_vicinity.bottom) { if (height < notch_h_th_) { is_notch = true; } } else if (current_vicinity.left && current_vicinity.right) { if (width < notch_v_th_) { is_notch = true; } }

gemini-code-assist · 2026-02-10T16:45:26Z

src/mpl/src/SACoreSoftMacro.h

+  static bool isSegmentEmpty(const std::vector<std::vector<bool>>& grid,
+                             int start_row,
+                             int tart_col,
+                             int end_row,
+                             int end_col);


There is a typo in the parameter name tart_col. It should be start_col.

Suggested change

static bool isSegmentEmpty(const std::vector<std::vector<bool>>& grid,

int start_row,

int tart_col,

int end_row,

int end_col);

static bool isSegmentEmpty(const std::vector<std::vector<bool>>& grid,

int start_row,

int start_col,

int end_row,

int end_col);

github-actions · 2026-02-10T18:21:15Z

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

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

github-actions · 2026-02-11T02:47:17Z

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

joaomai · 2026-02-11T13:16:24Z

Paired with PR #3881 on ORFS.

maliberty · 2026-02-11T16:57:34Z

src/mpl/src/graphics.cpp

+    // painter.setBrush(gui::Painter::kYellow, gui::Painter::kNone);
+


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

src/mpl/src/SACoreSoftMacro.cpp

-    if (macro.isStdCellCluster()) {
+    if (!macro.isMacroCluster() && !macro.isMixedCluster()) {


What is the semantic difference?

Using isMacroCluster keeps PADs, IOs, etc., which is undesired. I'll be opening an issue to better categorize the cluster "types" beyond standard cells, mixed and macro.

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

AcKoucher · 2026-02-12T18:27:08Z

src/mpl/src/SACoreSoftMacro.cpp

  int num_x = grid.front().size();

-  Neighbors neighbors;
+  NotchVicinity vicnity;


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

github-actions · 2026-02-12T19:12:23Z

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

github-actions · 2026-02-12T19:20:01Z

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

openroad-ci assigned joaomai Feb 10, 2026

gemini-code-assist bot reviewed Feb 10, 2026

View reviewed changes

joaomai added 8 commits February 10, 2026 19:45

mpl: include only macro and mixed clusters for notch calculation

f75c77f

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

mpl: improve calNotchPenalty performance

b143b41

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

mpl: draw notches last

e50af31

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

mpl: visited flag and avoid points collapse for notch calc

23b46fe

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

mpl: better naming and comments for notch code

d599b29

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

mpl: add const and static to funcs

e0965f4

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

mpl: update tests

c0bc1a3

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

mpl: split isSegmentEmpty in two functions

78ccfcf

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

openroad-ci force-pushed the mpl-notch-fix branch from cf84dd4 to 78ccfcf Compare February 11, 2026 02:42

joaomai mentioned this pull request Feb 11, 2026

flow: update metrics for MPL notch changes The-OpenROAD-Project/OpenROAD-flow-scripts#3881

Open

joaomai requested a review from maliberty February 11, 2026 15:03

maliberty reviewed Feb 11, 2026

View reviewed changes

joaomai added 2 commits February 12, 2026 14:44

Merge branch 'master' into mpl-notch-fix

e0abf02

mpl: remove unused comment

2d91bc4

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

AcKoucher suggested changes Feb 12, 2026

View reviewed changes

src/mpl/src/SACoreSoftMacro.cpp Outdated

int num_x = grid.front().size();

Neighbors neighbors;

NotchVicinity vicnity;

Copy link

Contributor

AcKoucher Feb 12, 2026

Choose a reason for hiding this comment

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

typo

fix typo

5a105a1

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

		// painter.setBrush(gui::Painter::kYellow, gui::Painter::kNone);

		if (macro.isStdCellCluster()) {
		if (!macro.isMacroCluster() && !macro.isMixedCluster()) {

mpl: change objects that ared considered during notch calculation #9449

Are you sure you want to change the base?

mpl: change objects that ared considered during notch calculation #9449

Conversation

openroad-ci commented Feb 10, 2026

Uh oh!

github-actions bot commented Feb 10, 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 Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 10, 2026

Uh oh!

github-actions bot commented Feb 11, 2026

Uh oh!

joaomai commented Feb 11, 2026

Uh oh!

maliberty Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

maliberty Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

joaomai Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

AcKoucher Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 12, 2026

Uh oh!

github-actions bot commented Feb 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants