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.
src/gpl/src/replace.cpp
Outdated
| 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.
src/gpl/src/timingBase.cpp
Outdated
| 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.
src/rsz/src/Resizer.cc
Outdated
| 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
}
}
Signed-off-by: PrecisEDAnon <[email protected]>
Signed-off-by: PrecisEDAnon <[email protected]>
Signed-off-by: PrecisEDAnon <[email protected]>
Signed-off-by: PrecisEDAnon <[email protected]>
PrecisEDAnon
force-pushed
the
OpenROAD-toggle-rebased
branch
from
e33b0bc to
addabb5
Compare
1 participant
This PR is 1 half of the PR that will introduce a better resizer for NG45.
The following content was AI generated.
Togglable (Rebased) GPL/RSZ QoR Improvements for Nangate45 —
ORFS_ENABLE_NEW_OPENROADSummary
This PR adds an opt-in QoR path across global placement (GPL) and the resizer (RSZ) intended to improve results on Nangate45 flows. All behavioral changes are gated behind
ORFS_ENABLE_NEW_OPENROAD; with the toggle unset/false, behavior matches the existing implementation.Motivation / Why a toggle?
This work is used for Nangate45 QoR experiments and needs a clean A/B switch:
What This PR Changes
Toggle-gated behavior (when
ORFS_ENABLE_NEW_OPENROADis truthy)-match_cell_footprinttorepair_design/repair_timingto enforce footprint matching when selecting replacements.set_opt_config,reset_opt_config,report_opt_configfor sizing/leakage/site/VT limits and early-sizing cap ratio knobs.report_equiv_cellsto inspect equivalent/swap candidates.RSZ_DECR_MAX_PASSEScan override the decreasing-slack pass limit (read once per process).repair_timing: under toggle, chooses parasitics source based onNG45_USE_DETAILED_PARA(defaults described below).Always-on maintenance/cleanup
GPL 0107collision).utl/Environment.h) and simplify toggle control-flow for readability.defaultbranch in boolean normalization.Usage / Toggle Behavior
ORFS_ENABLE_NEW_OPENROADis treated as truthy if set to one of:1,true,yes,on(case-insensitive). Otherwise it is off.Enable:
export ORFS_ENABLE_NEW_OPENROAD=1Disable:
Environment knobs (optional, experimental; only read when toggle is on)
GPL timing-driven net weights
GPL_WEIGHT_MAX(float > 0): maximum timing net weight.GPL_WEIGHT_EXP(float > 0): exponent applied to normalized criticality.GPL_WEIGHT_USE_ZERO_REF(int): use 0-slack reference instead of best slack.GPL_WEIGHT_COVERAGE(float in (0,100]): percent of "worst nets" used for cutoff reference.GPL_WEIGHT_USE_LENGTH_FACTOR(int): enable length factor.GPL_WEIGHT_LENGTH_ALPHA(float, clamped [0,1]): mix between base and length-scaled criticality.RSZ
RSZ_DECR_MAX_PASSES(int > 0): override decreasing-slack max passes.NG45_USE_DETAILED_PARA(0/1): when design is routed and toggle is on, choose parasitics source:1=>detailed_routing0=>global_routingValidation Recommendation
CI Note
Default remains off, so existing regressions should be unaffected. To keep the new path covered, consider adding an additional CI lane with
ORFS_ENABLE_NEW_OPENROAD=1enabled for at least one representative test.Related Work
This PR is designed to pair cleanly with OpenROAD-flow-scripts branches that set/use the same toggle for Nangate45 experiments.