gpl: ignore nets with high fanout#10430
Conversation
There was a problem hiding this comment.
Code Review
This pull request replaces the hardcoded fanout limit in the global placement process with a dynamic check based on Static Timing Analysis (STA) slack. It removes the -initial_place_max_fanout parameter and updates the net filtering logic to include RESET signals while ignoring nets with negative fanout slack. Feedback identifies critical issues in the new fanout check, specifically the omission of IO port drivers (dbBTerm), redundant calls to dbToSta, and a lack of null pointer safety. Additionally, it is recommended to provide a pin-count fallback for cases where timing libraries are missing to prevent placement degradation and to rename the check_fanout variable for better clarity.
| float fanout_slack = std::numeric_limits<float>::max(); | ||
| for (dbITerm* iTerm : db_net->getITerms()) { | ||
| bool is_driver = iTerm->isOutputSignal(true); | ||
| sta::Pin* sta_pin = sta_->getDbNetwork()->dbToSta(iTerm); | ||
| if (is_driver) { | ||
| for (sta::Mode* mode : sta_->modes()) { | ||
| float pin_fanout = 0.0; | ||
| float pin_fanout_slack = 0.0; | ||
| float pin_fanout_limit = 0.0; | ||
| sta_->checkFanout(sta_pin, | ||
| mode, | ||
| sta::MinMax::max(), | ||
| pin_fanout, | ||
| pin_fanout_limit, | ||
| pin_fanout_slack); | ||
|
|
||
| if (pin_fanout_slack < fanout_slack) { | ||
| fanout_slack = pin_fanout_slack; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current fanout check has several issues:
- Missing Drivers: It only checks
dbITerms for drivers. High-fanout nets like clocks and resets are frequently driven by IO ports (dbBTerm), which are currently skipped. This means such nets will not be ignored even if they have violations. - Efficiency:
dbToStais called for every pin in the net, but its result is only used if the pin is a driver. For high-fanout nets, this results in many redundant calls. - Robustness:
sta_->checkFanoutis called without verifying thatsta_pinis non-null. WhiledbToStausually returns a valid pin for signal nets, a null check is safer to prevent potential crashes if the pin is not in the STA graph.
I suggest refactoring the logic to check both dbITerm and dbBTerm drivers efficiently with proper null checks.
float fanout_slack = std::numeric_limits<float>::max();
auto check_driver = [&](auto* pin, bool is_driver) {
if (is_driver) {
if (sta::Pin* sta_pin = sta_->getDbNetwork()->dbToSta(pin)) {
for (sta::Mode* mode : sta_->modes()) {
float pin_fanout = 0.0, pin_fanout_slack = 0.0, pin_fanout_limit = 0.0;
sta_->checkFanout(sta_pin, mode, sta::MinMax::max(), pin_fanout, pin_fanout_limit, pin_fanout_slack);
fanout_slack = std::min(fanout_slack, pin_fanout_slack);
}
}
}
};
for (dbITerm* iTerm : db_net->getITerms()) {
check_driver(iTerm, iTerm->isOutputSignal(true));
}
for (dbBTerm* bTerm : db_net->getBTerms()) {
check_driver(bTerm, bTerm->isInputSignal());
}| const bool check_fanout | ||
| = (sta_->network() != nullptr && sta_->network()->isLinked() | ||
| && sta_->network()->defaultLibertyLibrary() != nullptr); |
There was a problem hiding this comment.
The removal of the hardcoded fanout limit (previously 200) in favor of an STA-based check reduces robustness when timing information (Liberty) is missing. If check_fanout is false (e.g., no Liberty files loaded), high-fanout nets like clocks will no longer be ignored, which can severely degrade placement quality. Consider keeping a simple pin-count based fallback when check_fanout is false. Additionally, rename the boolean variable check_fanout to a more descriptive name like enable_fanout_check to clarify its purpose.
References
- Avoid vague boolean parameter names like 'check'. Use a more descriptive name that clarifies its purpose, such as 'check_density' or 'enableDensityCheck'.
| || net_type == dbSigType::RESET) { | ||
| // Check fanout of the net. | ||
| if (check_fanout) { | ||
| float fanout_slack = std::numeric_limits<float>::max(); |
There was a problem hiding this comment.
warning: no header providing "std::numeric_limits" is directly included [misc-include-cleaner]
src/gpl/src/placerBase.cpp:9:
- #include <memory>
+ #include <limits>
+ #include <memory>| bool is_driver = iTerm->isOutputSignal(true); | ||
| sta::Pin* sta_pin = sta_->getDbNetwork()->dbToSta(iTerm); | ||
| if (is_driver) { | ||
| for (sta::Mode* mode : sta_->modes()) { |
There was a problem hiding this comment.
warning: no header providing "sta::Mode" is directly included [misc-include-cleaner]
src/gpl/src/placerBase.cpp:21:
- #include "utl/Logger.h"
+ #include "sta/Scene.hh"
+ #include "utl/Logger.h"| float pin_fanout_limit = 0.0; | ||
| sta_->checkFanout(sta_pin, | ||
| mode, | ||
| sta::MinMax::max(), |
There was a problem hiding this comment.
warning: no header providing "sta::MinMax" is directly included [misc-include-cleaner]
src/gpl/src/placerBase.cpp:21:
- #include "utl/Logger.h"
+ #include "sta/MinMax.hh"
+ #include "utl/Logger.h"|
|
||
| if (pin_fanout_slack < fanout_slack) { | ||
| fanout_slack = pin_fanout_slack; | ||
| } |
There was a problem hiding this comment.
warning: use std::min instead of < [readability-use-std-min-max]
| } | |
| fanout_slack = std::min(pin_fanout_slack, fanout_slack); |
Signed-off-by: Tobias Senti <git@tsenti.li>
2f9b322 to
4eb796a
Compare
| #include <cmath> | ||
| #include <cstdint> | ||
| #include <limits> | ||
| #include <memory> |
There was a problem hiding this comment.
warning: included header limits is not used directly [misc-include-cleaner]
| #include <memory> | |
| #include <memory> |
Previously the following nets where ignored:
-inital_place_max_fanout(default 200) during initial placementThis meant that clocks and other high fanout nets were still considered during placement.
We now have a single
-fanout_limitfor placement and also check OpenSTA for fanout violations.Nets with higher fanout than the limit or negative fanout slack are ignored during initial placement and global placement force calculation.