Skip to content

gpl: divergence handling - #11560

Open
gudeh wants to merge 12 commits into
The-OpenROAD-Project:masterfrom
gudeh:gpl-routability-diverge
Open

gudeh wants to merge 12 commits into
The-OpenROAD-Project:masterfrom
gudeh:gpl-routability-diverge

Conversation

@gudeh

@gudeh gudeh commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

This PR includes the following changes:

  • Use two snapshots saving, one for divergences and another for routability snapshot.
  • Allow reverting when a divergence happens during routability mode.
  • Use momentum reset when divergences occur, for default and routability divergences.
  • Make routability mode less verbose, demoting multiple messages to debug.

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

…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>
@gudeh
gudeh requested a review from a team as a code owner September 28, 2026 19:29
@gudeh
gudeh requested a review from LucasYuki September 28, 2026 19:29

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 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.

Comment thread src/gpl/src/nesterovBase.cpp
Comment thread src/gpl/src/nesterovBase.cpp
Comment thread src/gpl/src/nesterovPlace.cpp Outdated
Comment thread src/gpl/src/routeBase.cpp
Comment on lines +287 to 293
debugPrint(log_,
GPL,
"routability",
1,
"Target density at minimum routing congestion: {:.4f}{}",
minRcTargetDensity_[j],
nbVec_[j]->getGroup()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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()

Comment thread src/gpl/src/routeBase.cpp
Comment on lines +1046 to +1048
nbVec_[nb_index]->getGroup()
? " in group " + string(nbVec_[nb_index]->getGroup()->getName())
: "");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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())
: "");

@gudeh

gudeh commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants