Conversation
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>
There was a problem hiding this comment.
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.
| 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); | ||
| } |
There was a problem hiding this comment.
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
- 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.
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>
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
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>
|
clang-tidy review says "All clean, LGTM! 👍" |
This PR is no-op. And just adds and changes some log messages