-
Notifications
You must be signed in to change notification settings - Fork 758
AI generated PR #3 #9165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
AI generated PR #3 #9165
Conversation
Signed-off-by: PrecisEDAnon <[email protected]>
Signed-off-by: PrecisEDAnon <[email protected]>
There was a problem hiding this 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 introduces a new power-saving resizer, RecoverPowerMore, which is gated behind a command-line flag and an environment variable. The changes are well-structured and consistent across the build system, C++ source code, and Tcl scripts.
My review focuses on the new implementation in RecoverPowerMore.cc and the Tcl integration. I've identified a few areas for improvement:
- A potential semantic issue in
instanceWorstSlack's return value for unconstrained paths. - An opportunity to simplify resistance value clamping.
- A suggestion to improve the safety of the new Tcl environment variable handling.
- A note on the use of
const_cast, which appears to be a pattern to work with a legacy API but is worth discussing.
Overall, this is a solid contribution that adds a significant new capability. The code is complex but generally clear. Addressing the suggested points will enhance its correctness and maintainability.
src/rsz/src/RecoverPowerMore.cc
Outdated
| count += sta_->checkSlewLimits(const_cast<Net*>(net), true, nullptr, max_) | ||
| .size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using const_cast can be risky and might indicate an issue with const correctness in the API. The checkSlewLimits function should ideally take a const Net*. If changing the sta API is not feasible, consider adding a comment explaining why const_cast is safe to use here (i.e., the function does not actually modify the Net object). This applies to other const_cast usages in this file as well.
src/rsz/src/RecoverPowerMore.cc
Outdated
| found = true; | ||
| } | ||
|
|
||
| return found ? worst_slack : -std::numeric_limits<Slack>::infinity(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function returns -std::numeric_limits<Slack>::infinity() when no driver pin is found. Semantically, an instance with no driver pins has no output paths to analyze, so its slack should be considered infinitely good (positive infinity), not infinitely bad. Returning std::numeric_limits<Slack>::infinity() would be more correct and align with the behavior for unconstrained paths.
| return found ? worst_slack : -std::numeric_limits<Slack>::infinity(); | |
| return found ? worst_slack : std::numeric_limits<Slack>::infinity(); |
src/rsz/src/RecoverPowerMore.cc
Outdated
| float ra = resizer_->cellDriveResistance(a); | ||
| float rb = resizer_->cellDriveResistance(b); | ||
| if (ra <= 0.0f) { | ||
| ra = 0.0f; | ||
| } | ||
| if (rb <= 0.0f) { | ||
| rb = 0.0f; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic to ensure drive resistances ra and rb are non-negative can be simplified using std::max. This also makes the intent clearer.
| float ra = resizer_->cellDriveResistance(a); | |
| float rb = resizer_->cellDriveResistance(b); | |
| if (ra <= 0.0f) { | |
| ra = 0.0f; | |
| } | |
| if (rb <= 0.0f) { | |
| rb = 0.0f; | |
| } | |
| float ra = std::max(0.0f, resizer_->cellDriveResistance(a)); | |
| float rb = std::max(0.0f, resizer_->cellDriveResistance(b)); |
| default { | ||
| set more_recover_power 1 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default case for the environment variable MORE_RECOVER_POWER sets more_recover_power to 1. This could be surprising if the user makes a typo when setting the environment variable (e.g., MORE_RECOVER_POWER=ture). It might be safer to default to 0 and issue a warning for unrecognized values.
default {
utl::warn RSZ 0 "Unrecognized value for MORE_RECOVER_POWER: '$env_more_recover_power'. Defaulting to false."
set more_recover_power 0
}
Signed-off-by: PrecisEDAnon <[email protected]>
This PR gates behind a toggle a power-saving resizer that cuts, on average, 8.5% power across various designs (range : 2-20%).
The following content was AI generated.
Optional “More Recover Power” engine for
repair_timing -recover_powerSummary
This PR adds an optional alternative implementation of
recover_powerin the resizer. The new engine (RecoverPowerMore) is designed to recover additional power by systematically trading slack headroom for power via controlled cell swaps (size and VT) and (non-clock) buffer removal, while enforcing timing and DRV guardrails.Default behavior is unchanged. The new engine is gated behind a runtime toggle so existing flows match the legacy
recover_powerunless explicitly enabled.Motivation / When It Helps
repair_timing -recover_poweris commonly used to reduce post-route power once timing is reasonably stable. In practice, additional power recovery is often available in non-critical regions (and sometimes within clock networks) but requires:The new engine targets these gaps while remaining opt-in.
What This PR Changes
RecoverPowerMore(C++) as an alternaterecover_powerimplementation.repair_timing:repair_timing ... -recover_power <pct> -more_recover_powerMORE_RECOVER_POWER=1enables the new engine when the Tcl flag is not set.1,true,yes,on0,false,no,off, emptyUsage / Toggle Behavior
This PR is intentionally modular:
repair_timing -recover_power, nothing changes.-recover_poweris used and the toggle is off, behavior matches the legacy engine.-recover_poweris used and the toggle is on, the newRecoverPowerMoreengine runs.Enable / Disable
Enable per call:
Enable via environment (useful for CI/harness A/B):
set ::env(MORE_RECOVER_POWER) 1 repair_timing -recover_power 100Disable:
Technical Details (High Level)
The new engine is a bounded, multi-pass greedy optimizer with rollback:
WNS ≥ 0.-recover_powerpercent.dont_touch) and includes clock-driving instances for swap opportunities.Resizerjournal, updates parasitics, and re-runs STA requireds.-verboseemits per-move accept/reject debug prints.Validation Recommendation
repair_timing -recover_power <pct>.-more_recover_powerenabled under identical conditions.-recover_powervalues (e.g. 25/50) if runtime is a concern, then scale up.CI Note
Default is off to preserve legacy behavior. It is recommended to exercise the new path in some CI configuration (e.g., nightly) by enabling
MORE_RECOVER_POWER=1so the alternate code path is continuously built and tested.