gpl: divergence handling - #11560
gpl: divergence handling#11560gudeh wants to merge 12 commits into
Conversation
…that may extend the routability mode end of a revert below 30% Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
…ebug messages Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
…vate design Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Introduce a secondary slot for saving a snapshot, now we have one for routability and another for general divergence. Sequence of attempts if a divergence keeps happening with reverts: 1. Revert to min congestion and reset momentum. Routability still on. 2. Same, but disable routability. 3. Revert to min HPWL and reset momentum. Retry up to 3x, descent can record a better snapshot in between. 4. Error 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>
There was a problem hiding this comment.
Code Review
This pull request refactors snapshot management in the Nesterov global placement engine to support multiple snapshot slots (Routability and Diverge), allowing separate recovery paths. It also transitions several verbose log messages to debug prints and refactors congestion tracking to use consistent tile-based overflow statistics. The review feedback highlights critical safety and correctness issues, including potential out-of-bounds accesses when indexing snapshot vectors and target density arrays, a short-circuit evaluation bug in divergence recovery that causes unintended state mutations, and a minor namespace qualification issue.
| debugPrint(log_, | ||
| GPL, | ||
| "routability", | ||
| 1, | ||
| "Target density at minimum routing congestion: {:.4f}{}", | ||
| minRcTargetDensity_[j], | ||
| nbVec_[j]->getGroup() |
There was a problem hiding this comment.
On line 282, a check is performed to see if minRcTargetDensity_ is empty: minRcTargetDensity_.empty() ? 0.0f : minRcTargetDensity_[0]. However, inside the subsequent loop (lines 286-293), minRcTargetDensity_[j] is accessed directly without any bounds check. If minRcTargetDensity_ is indeed empty or has fewer elements than nbVec_.size(), this will result in an out-of-bounds access and potential crash. A bounds check should be added inside the loop, or the loop should be guarded.
| debugPrint(log_, | |
| GPL, | |
| "routability", | |
| 1, | |
| "Target density at minimum routing congestion: {:.4f}{}", | |
| minRcTargetDensity_[j], | |
| nbVec_[j]->getGroup() | |
| debugPrint(log_, | |
| GPL, | |
| "routability", | |
| 1, | |
| "Target density at minimum routing congestion: {:.4f}{}", | |
| j < minRcTargetDensity_.size() ? minRcTargetDensity_[j] : 0.0f, | |
| nbVec_[j]->getGroup() |
| nbVec_[nb_index]->getGroup() | ||
| ? " in group " + string(nbVec_[nb_index]->getGroup()->getName()) | ||
| : ""); |
There was a problem hiding this comment.
Use std::string instead of string to be consistent with the rest of the codebase and to avoid potential compilation errors if std::string is not imported into the global namespace.
| nbVec_[nb_index]->getGroup() | |
| ? " in group " + string(nbVec_[nb_index]->getGroup()->getName()) | |
| : ""); | |
| nbVec_[nb_index]->getGroup() | |
| ? " in group " + std::string(nbVec_[nb_index]->getGroup()->getName()) | |
| : ""); |
|
This should be a much better solution than the closed PR #11505. |
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
This PR includes the following changes:
Previously if a divergence happens we would just revert to min HPWL and stop (not reaching 10% overflow target). Now we keep reverting and reseting the momentum up to 3x, allowing us to try and reach the overflow target.
Previously if a divergence happens inside routability we just error and didn't active the "revert if diverge" feature. Now we do revert if a divergence happens during routability. Also, we revert to the routability snapshot resetting the momentum if a divergence happened, if the divergence persists after such revert, we use the default "revert if diverge", reverting to the min HPWL.
This PR addresses issue https://github.com/The-OpenROAD-Project-private/OpenROAD-flow-scripts/issues/1883