Togglable-rebased GPL/RSZ #9155
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new, togglable resizer logic for NG45, gated by the ORFS_ENABLE_NEW_OPENROAD environment variable. The changes are extensive, touching global placement, timing-driven placement, and the resizer itself. My review focuses on improving code maintainability by addressing significant code duplication of helper functions across multiple files. I've also pointed out areas where the control flow could be simplified for better readability. Finally, a minor cleanup in a Tcl script is suggested to remove unreachable code. Overall, the changes seem to be a good step towards a more advanced resizer, but would benefit from some refactoring to improve code quality.
| namespace { | ||
| bool envVarTruthy(const char* name) | ||
| { | ||
| const char* raw = std::getenv(name); | ||
| if (raw == nullptr || *raw == '\0') { | ||
| return false; | ||
| } | ||
|
|
||
| std::string value(raw); | ||
| const size_t start = value.find_first_not_of(" \t\n\r"); | ||
| if (start == std::string::npos) { | ||
| return false; | ||
| } | ||
| const size_t end = value.find_last_not_of(" \t\n\r"); | ||
| value = value.substr(start, end - start + 1); | ||
| std::transform( | ||
| value.begin(), value.end(), value.begin(), [](unsigned char c) { | ||
| return static_cast<char>(std::tolower(c)); | ||
| }); | ||
| return value == "1" || value == "true" || value == "yes" || value == "on"; | ||
| } | ||
|
|
||
| bool useOrfsNewOpenroad() | ||
| { | ||
| return envVarTruthy("ORFS_ENABLE_NEW_OPENROAD"); | ||
| } | ||
|
|
||
| std::optional<float> getEnvFloat(const char* name) | ||
| { | ||
| const char* raw = std::getenv(name); | ||
| if (raw == nullptr || *raw == '\0') { | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| char* end = nullptr; | ||
| const float value = std::strtof(raw, &end); | ||
| if (end == raw || (end != nullptr && *end != '\0')) { | ||
| return std::nullopt; | ||
| } | ||
| return value; | ||
| } | ||
| } // namespace |
There was a problem hiding this comment.
These helper functions (envVarTruthy, useOrfsNewOpenroad, getEnvFloat) are duplicated across multiple files (src/gpl/src/replace.cpp, src/gpl/src/timingBase.cpp, src/rsz/src/RepairSetup.cc, src/rsz/src/Resizer.cc). This duplication increases maintenance overhead and can lead to inconsistencies.
To improve maintainability, these functions should be consolidated into a common utility file. For example, they could be moved to a new header/source pair within the utl directory.
Additionally, the version of getEnvFloat in src/gpl/src/timingBase.cpp is more robust as it includes logging for invalid float values. This improved version should be the one that is centralized.
| if (!useOrfsNewOpenroad()) { | ||
| int weighted_net_count = 0; | ||
| for (auto& gNet : nbc_->getGNets()) { | ||
| // default weight | ||
| gNet->setTimingWeight(1.0); | ||
| if (gNet->getGPins().size() > 1) { | ||
| auto net_slack_opt = rs_->resizeNetSlack(gNet->getPbNet()->getDbNet()); | ||
| if (!net_slack_opt) { | ||
| continue; | ||
| } | ||
| auto net_slack = net_slack_opt.value(); | ||
| if (net_slack < slack_max) { | ||
| if (slack_max == slack_min) { | ||
| gNet->setTimingWeight(1.0); | ||
| } else { | ||
| // weight(min_slack) = net_weight_max_ | ||
| // weight(max_slack) = 1 | ||
| const float weight = 1 | ||
| + (net_weight_max_ - 1) | ||
| * (slack_max - net_slack) | ||
| / (slack_max - slack_min); | ||
| gNet->setTimingWeight(weight); | ||
| } | ||
| weighted_net_count++; | ||
| } | ||
| debugPrint(log_, | ||
| GPL, | ||
| "timing", | ||
| 1, | ||
| "net:{} slack:{} weight:{}", | ||
| gNet->getPbNet()->getDbNet()->getConstName(), | ||
| net_slack, | ||
| gNet->getTotalWeight()); | ||
| } | ||
| } | ||
|
|
||
| debugPrint(log_, | ||
| GPL, | ||
| "timing", | ||
| 1, | ||
| "Timing-driven: weighted {} nets.", | ||
| weighted_net_count); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The control flow here, with if (!useOrfsNewOpenroad()) { ... return true; } followed by the new logic, can be confusing to follow. It would be more readable and maintainable to refactor this into a clear if/else structure. This would make the separation between the old and new logic paths more explicit.
A similar pattern is present in src/rsz/src/Resizer.cc in the getSwappableCells method, which would also benefit from this refactoring.
| if (!useOrfsNewOpenroad()) { | ||
| LibertyCellSeq swappable_cells; | ||
| LibertyCellSeq* equiv_cells = sta_->equivCells(source_cell); | ||
|
|
||
| if (equiv_cells) { | ||
| int64_t source_cell_area = master->getArea(); | ||
| std::optional<float> source_cell_leakage; | ||
| if (sizing_leakage_limit_) { | ||
| source_cell_leakage = cellLeakage(source_cell); | ||
| } | ||
| for (LibertyCell* equiv_cell : *equiv_cells) { | ||
| if (dontUse(equiv_cell) || !isLinkCell(equiv_cell)) { | ||
| continue; | ||
| } | ||
|
|
||
| dbMaster* equiv_cell_master = db_network_->staToDb(equiv_cell); | ||
| if (!equiv_cell_master) { | ||
| continue; | ||
| } | ||
|
|
||
| if (sizing_area_limit_.has_value() && (source_cell_area != 0) | ||
| && (equiv_cell_master->getArea() | ||
| / static_cast<double>(source_cell_area) | ||
| > sizing_area_limit_.value())) { | ||
| continue; | ||
| } | ||
|
|
||
| if (sizing_leakage_limit_ && source_cell_leakage) { | ||
| std::optional<float> equiv_cell_leakage = cellLeakage(equiv_cell); | ||
| if (equiv_cell_leakage | ||
| && (*equiv_cell_leakage / *source_cell_leakage | ||
| > sizing_leakage_limit_.value())) { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| if (sizing_keep_site_) { | ||
| if (master->getSite() != equiv_cell_master->getSite()) { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| if (sizing_keep_vt_) { | ||
| if (cellVTType(master).vt_index | ||
| != cellVTType(equiv_cell_master).vt_index) { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| if (match_cell_footprint_) { | ||
| const bool footprints_match = sta::stringEqIf( | ||
| source_cell->footprint(), equiv_cell->footprint()); | ||
| if (!footprints_match) { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| if (source_cell->userFunctionClass()) { | ||
| const bool user_function_classes_match | ||
| = sta::stringEqIf(source_cell->userFunctionClass(), | ||
| equiv_cell->userFunctionClass()); | ||
| if (!user_function_classes_match) { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| swappable_cells.push_back(equiv_cell); | ||
| } | ||
| } else { | ||
| swappable_cells.push_back(source_cell); | ||
| } | ||
|
|
||
| swappable_cells_cache_[source_cell] = swappable_cells; | ||
| return swappable_cells; | ||
| } |
There was a problem hiding this comment.
Similar to other parts of this PR, the control flow here with if (!useOrfsNewOpenroad()) { ... return ...; } followed by the new logic can be confusing. Refactoring this into a clear if/else structure would improve readability and maintainability by making the separation between the old and new logic paths more explicit.
| switch -- $normalized { | ||
| 1 - true - yes - on { | ||
| set value 1 | ||
| } | ||
| 0 - false - no - off { | ||
| set value 0 | ||
| } | ||
| default { | ||
| set value 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
The [string is boolean -strict $enabled] check on line 492 ensures that $enabled is one of 0, 1, true, false, yes, no, on, off (case-insensitive). The switch statement on line 498 handles all of these possible values after they are converted to lowercase. Consequently, the default case on line 505 is unreachable and can be removed for code clarity.
switch -- $normalized {
1 - true - yes - on {
set value 1
}
0 - false - no - off {
set value 0
}
}
1 participant
This PR is 1 half of the PR that will introduce a better resizer for NG45. Placeholder description for build checks.