Skip to content

MPL: add a read-only macro placement feasibility check - #11896

Open
naveenvenk17 wants to merge 5 commits into
The-OpenROAD-Project:masterfrom
naveenvenk17:nv-mpl-preflight-11495
Open

naveenvenk17 wants to merge 5 commits into
The-OpenROAD-Project:masterfrom
naveenvenk17:nv-mpl-preflight-11495

Conversation

@naveenvenk17

@naveenvenk17 naveenvenk17 commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Add check_macro_placement so a flow can reject an infeasible macro floorplan before running clustering or annealing. It reuses the placer's existing feasibility checks and diagnostic IDs, without writing reports or changing the database.

Disjoint fences now use safe rectangle intersection and report MPL-0068 in both commands. Previously, the intersection assumed overlap, which could assert in debug builds or accept a disjoint fence in release builds.

The preflight command requires all four fence coordinates together and distinguishes an omitted fence from an explicitly empty one. It rejects nonfinite or out-of-range DBU values before casting. Macro-plus-channel dimensions are checked with widened arithmetic before creating placement objects, so extreme channels cannot wrap into negative dimensions.

This implements the feasibility-check portion of #11495. Placement-script replay and rollback, placement-legality checks, and routing-capacity checks remain separate follow-ups.

Validation on the GCP Linux VM:

  • Full MPL suite: 46 targets passed with cached test results disabled.
  • After the Tcl-formatting correction found by upstream CI, all 46 MPL targets and all 6 repository-wide lint targets passed again on the VM.
  • The preflight unit target has 26 passing cases.
  • Stress coverage includes 121 channel-size combinations, 36 invalid-fence combinations, every partial-coordinate combination, conversion and extreme-channel boundaries, pin-aware spacing, and byte-for-byte database preservation after passing and failing checks.
  • Independent review completed with no blocking findings.

Review carryover: The earlier review's partial-fence and invalid-fence cases are covered here. The intersection helper clears disjoint results; the related warning was checked against that contract.

Replaces #11845 to correct the contributor account. The implementation and test files are unchanged from the tested revision af1f88c52ca2e8ed9056ee922b004086ed1b50be. Commit authorship is corrected. The source and test files are unchanged.

CI note: upstream d61ef5a now records the missing registry checksum. No dependency versions changed. This CI refresh leaves the tested feature source unchanged.

Carry forward the reviewed changes from The-OpenROAD-Project#11845 under the correct contributor account.
The source tree matches af1f88c.

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>
@naveenvenk17
naveenvenk17 requested a review from a team as a code owner October 21, 2026 14:14
@naveenvenk17
naveenvenk17 requested a review from joaomai October 21, 2026 14:14

@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 introduces a new 'check_macro_placement' command to perform area-feasibility preflight checks for macro placement without modifying the database. The changes include adding necessary C++ logic in 'HierRTLMP' and 'ClusteringEngine', exposing the functionality through the Tcl interface, and adding comprehensive unit tests. The review feedback identified potential null pointer dereference issues in the new code and suggested an improvement for robust initialization of the 'has_unfixed_macros' flag, all of which are being addressed.

int min_channel_height,
bool pin_aware_channels) const
{
odb::dbBlock* block = db_->getChip()->getBlock();

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.

high

If no design is loaded, db_->getChip() can return nullptr. Dereferencing it directly with db_->getChip()->getBlock() will cause a segmentation fault. Check if the chip and block exist before proceeding.

  odb::dbChip* chip = db_->getChip();
  if (chip == nullptr || chip->getBlock() == nullptr) {
    logger_->error(MPL, 1, "No block found for Macro Placement.");
  }
  odb::dbBlock* block = chip->getBlock();

Comment thread src/mpl/src/mpl.i
bool pin_aware_channels,
bool fence_provided)
{
auto block = ord::OpenRoad::openRoad()->getDb()->getChip()->getBlock();

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.

high

If no design is loaded, ord::OpenRoad::openRoad()->getDb()->getChip() can return nullptr. Dereferencing it directly will cause a segmentation fault. Check if the chip and block exist before proceeding.

  auto chip = ord::OpenRoad::openRoad()->getDb()->getChip();
  if (chip == nullptr || chip->getBlock() == nullptr) {
    ord::getLogger()->error(utl::MPL, 1, "No block found for Macro Placement.");
  }
  auto block = chip->getBlock();

Comment on lines 121 to 125
if (unfixed_macros.empty()) {
tree_->has_unfixed_macros = false;
logger_->info(MPL, 17, "No unfixed macros.");
return;
return 0;
}

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

To ensure robustness and defensive programming, explicitly set tree_->has_unfixed_macros = true; when unfixed_macros is not empty. This prevents potential issues if tree_ is reused or if has_unfixed_macros is not default-initialized to true.

Suggested change
if (unfixed_macros.empty()) {
tree_->has_unfixed_macros = false;
logger_->info(MPL, 17, "No unfixed macros.");
return;
return 0;
}
if (unfixed_macros.empty()) {
tree_->has_unfixed_macros = false;
logger_->info(MPL, 17, "No unfixed macros.");
return 0;
}
tree_->has_unfixed_macros = true;

Regenerated against the current upstream merge and checked for idempotence on the GCP VM. Only the missing Bison registry checksum is added; no dependency versions change.

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>
Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>
@naveenvenk17

Copy link
Copy Markdown
Contributor Author

I checked the new review against the call path. check_macro_placement calls mpl::check_block_exists before the SWIG command; the regression starts with an empty database and checks for MPL-0001. Each preflight call also creates a fresh PhysicalHierarchy, whose has_unfixed_macros flag starts true. The placement path creates a new tree in init as well. I haven't added duplicate checks for those cases.

Keep the feature tree unchanged and refresh pull-request CI after the missing registry checksum was added to master.

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>

@vvbandeira vvbandeira left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

approved from devops team side

@osamahammad21

Copy link
Copy Markdown
Member

@maliberty @vvbandeira Why am I requested to review this PR? I think originally it had some changes related to openroad-maintainers but then those changes were removed. Do/Can we remove unneeded reviews when the PR content changes?

@maliberty
maliberty removed request for a team and osamahammad21 November 8, 2026 05:01
Signed-off-by: Matt Liberty <mliberty@precisioninno.com>

# Conflicts:
#	src/mpl/src/mpl.i
@maliberty

Copy link
Copy Markdown
Member

@joaomai please review

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.

4 participants