Skip to content

Ifp log messages#9410

Open
gudeh wants to merge 15 commits intoThe-OpenROAD-Project:masterfrom
gudeh:ifp-log-messages
Open

Ifp log messages#9410
gudeh wants to merge 15 commits intoThe-OpenROAD-Project:masterfrom
gudeh:ifp-log-messages

Conversation

@gudeh
Copy link
Contributor

@gudeh gudeh commented Feb 2, 2026

This PR is no-op. And just adds and changes some log messages

  • New log messages showing core area, instances area, number of instances and final utilization.
  • Also new messages for stating which of the 4 types we have for initiating the floorplan.
  • Rearrange error messages IDs to start at 300.

gudeh added 3 commits February 2, 2026 15:29
also reorganize error message to start from ID 300

Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Copy link
Contributor

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

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 improves logging in the initial floorplan tool and reorganizes error message IDs. The changes in the TCL scripts are correct, including a bug fix where a variable was being checked incorrectly. The C++ changes introduce a new reporting function, reportAreas, which logs various floorplan metrics. However, I've identified a potential issue in this new function related to data type conversions that could lead to overflow or precision loss. I've provided a suggestion to address this, also incorporating a repository guideline regarding defensive checks for non-zero area.

Comment on lines +1368 to +1386
logger_->info(IFP,
16,
"{:27} {:15.3f} um^2",
"Core area:",
block_->dbuAreaToMicrons(core.area()));
int64_t design_area = static_cast<int64_t>(designArea());
logger_->info(IFP,
17,
"{:27} {:15.3f} um^2",
"Instances area:",
block_->dbuAreaToMicrons(design_area));
double core_area_um = block_->dbuAreaToMicrons(core.area());
if (core_area_um > 0) {
logger_->info(IFP,
20,
"{:27} {:15.3f}",
"Result utilization:",
block_->dbuAreaToMicrons(design_area) / core_area_um);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The designArea() function returns a double, and core.area() returns a uint64_t. Casting these values to int64_t to use with dbuAreaToMicrons can lead to overflow if the area is very large, or precision loss due to truncation. A safer approach is to perform the area conversion to microns using floating-point arithmetic directly. This also avoids redundant calculations of core_area_um. Additionally, the defensive check if (core_area_um > 0) has been removed from the suggestion, as dbMaster's height (and by extension, core area) is assumed to be non-zero, making such checks unnecessary according to repository guidelines.

  const double dbu_per_micron = block_->getDbUnitsPerMicron();
  const double dbu_per_micron_sq = dbu_per_micron * dbu_per_micron;

  const double core_area_um = static_cast<double>(core.area()) / dbu_per_micron_sq;
  logger_->info(IFP,
                    16,
                    "{:27} {:15.3f} um^2",
                    "Core area:",
                    core_area_um);

  const double design_area_dbu = designArea();
  const double design_area_um = design_area_dbu / dbu_per_micron_sq;
  logger_->info(IFP,
                    17,
                    "{:27} {:15.3f} um^2",
                    "Instances area:",
                    design_area_um);

  logger_->info(IFP,
                    20,
                    "{:27} {:15.3f}",
                    "Result utilization:",
                    design_area_um / core_area_um);
References
  1. A dbMaster's height is assumed to be non-zero. A zero height indicates a more fundamental issue elsewhere in the system, so defensive checks for zero height before division are unnecessary.

gudeh added 6 commits February 5, 2026 17:43
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2026

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

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2026

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

Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2026

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

gudeh added 2 commits February 5, 2026 19:59
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2026

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

gudeh added 3 commits February 7, 2026 10:36
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
…is function call

Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2026

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant