Skip to content

mpl: print macro locations in microns - #11318

Closed
naveenvenk17 wants to merge 5 commits into
The-OpenROAD-Project:masterfrom
naveenvenk17:nv-mpl-report-locations
Closed

naveenvenk17 wants to merge 5 commits into
The-OpenROAD-Project:masterfrom
naveenvenk17:nv-mpl-report-locations

Conversation

@naveenvenk17

@naveenvenk17 naveenvenk17 commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #11241.

The location report passed integer DBU values to floating-point format specifiers, which could fail instead of printing the table. Coordinates and dimensions now use the block's DBU-to-micron conversion, matching the outline report.

The fixed-terminal loop starts at the number of movable macros, as requested in review. This also includes the first fixed terminal, which the old boundary check skipped.

Tested on the GCP Linux VM on this exact branch tree:

  • All 44 MPL targets passed with test-result caching disabled.
  • A separate VM-only probe of the actual hard/soft reporter instantiations passed five shuffled repetitions, covering 140 scenario checks. It covers empty and mixed lists, ordering, the first fixed terminal, negative coordinates and both 1000/2000 DBU-per-micron units.
  • Formatting and whitespace checks passed.

Upstream was merged additively; existing review history is unchanged. DCO still flags two older sign-off trailers. The new commits have matching sign-offs, but do not retroactively fix those old trailers. The missing upstream registry checksum is now fixed in d61ef5a.

Signed-off-by: Naveen Venkat <archgen.guest@nyayanidhi.in>
Signed-off-by: Naveen Venkat <archgen.guest@nyayanidhi.in>
@naveenvenk17
naveenvenk17 requested a review from a team as a code owner September 2, 2026 19:18
@naveenvenk17
naveenvenk17 requested a review from joaomai September 2, 2026 19:18
@github-actions github-actions Bot added the size/S label Sep 2, 2026

@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 updates SimulatedAnnealingCore::reportLocations to convert macro coordinates and dimensions from DBU to microns using block_->dbuToMicrons. It also corrects a loop condition to properly process fixed terminals. The reviewer suggested simplifying the loop logic by starting the iteration directly from number_of_moveable_macros instead of starting from zero and skipping elements.

Comment thread src/mpl/src/SimulatedAnnealingCore.cpp Outdated
Comment on lines 696 to 699
for (int i = 0; i < macros_.size(); ++i) {
if (i <= number_of_moveable_macros) {
if (i < number_of_moveable_macros) {
continue;
}

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

Instead of starting the loop from 0 and skipping indices less than number_of_moveable_macros, you can start the loop directly from number_of_moveable_macros. This simplifies the logic and avoids unnecessary iterations and conditional checks.

Suggested change
for (int i = 0; i < macros_.size(); ++i) {
if (i <= number_of_moveable_macros) {
if (i < number_of_moveable_macros) {
continue;
}
for (int i = number_of_moveable_macros; i < macros_.size(); ++i) {

@joaomai joaomai Sep 10, 2026 •

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.

This helps with readability, you can incorporate it in the PR.

@maliberty

Copy link
Copy Markdown
Member

DCO needs fixing

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-10T05:14:04.424298Z 588caed Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: 588caedb1e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@joaomai joaomai 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.

@naveenvenk17 as Matt mentioned, you gotta sign your commits for it to be approved. Gemini's suggestion is also nice, add it to the PR. Other than that, LGTM.

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

Updated the loop to start at the first fixed terminal, as suggested. I also merged upstream without rewriting the reviewed commits. The exact updated tree passed all 44 MPL targets on the VM; the focused hard/soft reporter probe passed 140 scenario checks across five shuffled repetitions.

DCO still flags the two old sign-off trailers. The new commits are signed off correctly. Could we use an additive DCO remediation or maintainer override so the existing review history can stay intact?

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>
@naveenvenk17

Copy link
Copy Markdown
Contributor Author

Replaced by #11910 with the same source tree and a corrected commit sign-off. The reporting changes and fixed-terminal loop from this review are both preserved. The VM validation results above apply to that unchanged tree; the replacement will run fresh CI. Closing this one to keep the discussion in one place.

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.

mpl: reportLocations prints "invalid format specifier" instead of macro locations

3 participants