Skip to content

rsz: preserve buffers on hierarchical module boundaries in canRemoveBuffer#10985

Open
oharboe wants to merge 1 commit into
The-OpenROAD-Project:masterfrom
oharboe:resizer-hier-buffer-removal-check
Open

rsz: preserve buffers on hierarchical module boundaries in canRemoveBuffer#10985
oharboe wants to merge 1 commit into
The-OpenROAD-Project:masterfrom
oharboe:resizer-hier-buffer-removal-check

Conversation

@oharboe

@oharboe oharboe commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

When global placement performs timing-driven buffer removal (Resizer::removeBuffers), canRemoveBuffer checks whether a candidate buffer can be safely removed. If the buffer is situated on a hierarchical module boundary where its input pin and output pin belong to different dbModNets, removing the buffer flattens the net without updating hierarchical module port/net bindings, leaving downstream module inputs disconnected.

Fix

Check whether input_modnet != output_modnet in Resizer::canRemoveBuffer and return false to preserve buffers located on hierarchical module boundaries.

@oharboe
oharboe requested a review from a team as a code owner July 25, 2026 05:18
@oharboe
oharboe requested a review from precisionmoon July 25, 2026 05:18

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

Copy link
Copy Markdown
Contributor

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 modifies Resizer::canRemoveBuffer to prevent buffer removal when the input and output hierarchical nets differ. The reviewer correctly identifies a critical side-effect where this change causes Resizer::removeBuffer to merge a hierarchical net with itself, potentially leading to undefined behavior or crashes in OpenDB. The reviewer suggests updating the merging logic to only merge distinct nets.

Comment thread src/rsz/src/Resizer.cc
@oharboe
oharboe force-pushed the resizer-hier-buffer-removal-check branch from 0ba1c68 to 9783ce2 Compare July 25, 2026 05:21
@oharboe
oharboe requested review from jhkim-pii and removed request for precisionmoon July 25, 2026 05:25
@oharboe
oharboe force-pushed the resizer-hier-buffer-removal-check branch from 9783ce2 to 6ae97e6 Compare July 25, 2026 05:27
@oharboe

oharboe commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

@precisionmoon Heads up! I changed reviewer because reviewer was automatically assigned and this is a continuation of a conversation between me and @jhkim-pii

…uffer

When global placement performs timing-driven buffer removal
(Resizer::removeBuffers), canRemoveBuffer checks whether a candidate
buffer can be removed. If the buffer is situated on a hierarchical module
boundary where its input pin and output pin belong to different dbModNets,
removing the buffer flattens the net without updating hierarchical module
port/net bindings, disconnecting downstream module inputs.

Check whether input_modnet != output_modnet in canRemoveBuffer and return
false to preserve hierarchical boundary buffers. Also ensure removeBuffer
only merges distinct dbModNets (survivor_modnet != removed_modnet).

Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
@oharboe
oharboe force-pushed the resizer-hier-buffer-removal-check branch from 6ae97e6 to bac7e38 Compare July 25, 2026 05:28
@oharboe

oharboe commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

/gemini review

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

Copy link
Copy Markdown
Contributor

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 modifies Resizer.cc to prevent buffer removal when the input and output hierarchical nets differ, and updates the buffer removal logic to avoid merging identical hierarchical nets. The reviewer pointed out that the updated condition in removeBuffer causes a fall-through to the else if branch when both survivor_modnet and removed_modnet are identical and non-null, leading to a redundant call to connectTermsOf. Nesting the inequality check inside the non-null check is suggested to resolve this issue.

Comment thread src/rsz/src/Resizer.cc
Comment on lines +2910 to 2913
if (survivor_modnet != nullptr && removed_modnet != nullptr
&& survivor_modnet != removed_modnet) {
survivor_modnet->mergeModNet(removed_modnet);
} else if (survivor_modnet != nullptr) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

If survivor_modnet == removed_modnet (and both are non-null), the current condition survivor_modnet != removed_modnet evaluates to false. This causes the execution to fall through to the else if (survivor_modnet != nullptr) branch, which will then execute survivor_modnet->connectTermsOf(db_removed). Since both nets are already part of the same hierarchical module net, calling connectTermsOf is redundant and potentially problematic.

Nesting the inequality check inside the if (survivor_modnet != nullptr && removed_modnet != nullptr) block avoids falling through to the else if branches when both modnets are identical.

  if (survivor_modnet != nullptr && removed_modnet != nullptr) {
    if (survivor_modnet != removed_modnet) {
      survivor_modnet->mergeModNet(removed_modnet);
    }
  } else if (survivor_modnet != nullptr) {

@oharboe

oharboe commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

@jhkim-pii I don't know what I'm doing here... I don't know if I should follow Gemini's directions... Please advice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant