dpl: PDN aware detailed placement legalization - #11120
Conversation
Index fixed supply via constituents on cell-used routing and cut layers, and materialize 64-site conflict recipes on demand. Use Euclidean spacing for routing and cut geometry while exempting supply pins connected to the same special net. Reject conflicting negotiation candidates and mirrored orientations before they are committed. Signed-off-by: Philippe Sauter <phsauter@iis.ee.ethz.ch>
Add an opt-in -pdn_aware flag for both legalization engines. Limit fixed supply via initialization to requested detailed placement and same-block check_placement or optimize_mirroring calls so unrelated DPL commands retain their existing path. Report fixed supply via failures separately from blocked-layer failures. Signed-off-by: Philippe Sauter <phsauter@iis.ee.ethz.ch>
Cover default-off and same-block mode reset behavior, both legalization engines, standalone mirroring, routing and cut constituents, same-net exemptions, finite via arrays, Euclidean spacing boundaries, and core-boundary handling. Register all cases in CMake and Bazel. Signed-off-by: Philippe Sauter <phsauter@iis.ee.ethz.ch>
Signed-off-by: Philippe Sauter <phsauter@iis.ee.ethz.ch>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to OpenROAD! Thanks for opening your first PR.
Before we review:
- Contribution Guide: https://openroad.readthedocs.io/en/latest/contrib/contributing.html
- Build Instructions: https://openroad.readthedocs.io/en/latest/contrib/BuildWithCMake.html
Please ensure:
- CI passes
- Code is properly formatted
- Tests are included where applicable
A maintainer will review shortly!
![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 PDN-aware detailed placement mode (-pdn_aware) to prevent cell pin and obstruction spacing conflicts with fixed vias on special power and ground nets. The changes include adding fixed supply via checking to the DRC engine, updating grid initialization, and integrating these checks into detailed placement, check placement, and mirroring optimization. The review feedback highlights several safety and robustness improvements, including adding null checks to prevent potential crashes from null pointers in FixedSupplyVias and NegotiationLegalizer, and ensuring deleteGrid() is called in optimizeMirroring to avoid memory leaks.
| FixedSupplyVias(utl::Logger* logger, Grid* grid, odb::dbBlock* block) | ||
| : logger_(logger), grid_(grid) | ||
| { | ||
| for (odb::dbNet* net : block->getNets()) { |
There was a problem hiding this comment.
If block is null, dereferencing it to call getNets() or getInsts() will cause a segmentation fault. Add a null check at the beginning of the constructor to prevent potential crashes when no design is loaded.
| FixedSupplyVias(utl::Logger* logger, Grid* grid, odb::dbBlock* block) | |
| : logger_(logger), grid_(grid) | |
| { | |
| for (odb::dbNet* net : block->getNets()) { | |
| FixedSupplyVias(utl::Logger* logger, Grid* grid, odb::dbBlock* block) | |
| : logger_(logger), grid_(grid) | |
| { | |
| if (block == nullptr) { | |
| return; | |
| } | |
| for (odb::dbNet* net : block->getNets()) { |
| for (const RecipeEntry& entry : recipe.entries) { | ||
| if ((entry.mask & query_mask) != 0 | ||
| && cell->getDbInst()->getITerm(entry.term)->getNet() != entry.net) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
In a corrupted or inconsistent database state, getITerm(entry.term) could potentially return nullptr. Dereferencing it directly to call getNet() would cause a crash. It is safer to use a defensive null check.
for (const RecipeEntry& entry : recipe.entries) {
if ((entry.mask & query_mask) != 0) {
odb::dbITerm* iterm = cell->getDbInst()->getITerm(entry.term);
if (iterm == nullptr || iterm->getNet() != entry.net) {
return false;
}
}
}| void Opendp::optimizeMirroring() | ||
| { | ||
| OptimizeMirroring opt(logger_, db_); | ||
| if (isPdnAwareForCurrentBlock()) { | ||
| importDb(); | ||
| adjustNodesOrient(); | ||
| initGrid(true); | ||
| opt.setLegalPredicate( | ||
| [this](odb::dbInst* inst, const odb::dbOrientType& orient) { | ||
| Node* cell = network_->getNode(inst); | ||
| return drc_engine_->checkFixedSupplyVias( | ||
| cell, grid_->gridX(cell), grid_->gridRoundY(cell), orient); | ||
| }); | ||
| } | ||
| opt.run(); | ||
| } |
There was a problem hiding this comment.
When isPdnAwareForCurrentBlock() is true, initGrid(true) is called to initialize the grid and fixed supply vias. However, the grid is never deleted at the end of optimizeMirroring(), which can lead to memory leaks and inconsistent state. Call deleteGrid() at the end of the function if the grid was initialized.
void Opendp::optimizeMirroring()
{
OptimizeMirroring opt(logger_, db_);
if (isPdnAwareForCurrentBlock()) {
importDb();
adjustNodesOrient();
initGrid(true);
opt.setLegalPredicate(
[this](odb::dbInst* inst, const odb::dbOrientType& orient) {
Node* cell = network_->getNode(inst);
return drc_engine_->checkFixedSupplyVias(
cell, grid_->gridX(cell), grid_->gridRoundY(cell), orient);
});
}
opt.run();
if (isPdnAwareForCurrentBlock()) {
deleteGrid();
}
}| if (node != nullptr | ||
| && !opendp_->drc_engine_->checkFixedSupplyVias( | ||
| node, GridX{tx}, GridY{ty}, targetOrient)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Ensure opendp_ and opendp_->drc_engine_ are non-null before calling checkFixedSupplyVias to prevent potential null pointer dereferences.
| if (node != nullptr | |
| && !opendp_->drc_engine_->checkFixedSupplyVias( | |
| node, GridX{tx}, GridY{ty}, targetOrient)) { | |
| return; | |
| } | |
| if (node != nullptr && opendp_ && opendp_->drc_engine_ | |
| && !opendp_->drc_engine_->checkFixedSupplyVias( | |
| node, GridX{tx}, GridY{ty}, targetOrient)) { | |
| return; | |
| } |
|
@osamahammad21 FYI |
|
Wonderful, this is a known limitation which we were planning to work with! I should review your code on the upcoming days. I would also like to investigate your test case. Have you dived deeply into what Codex implemented, had a look with the GUI? I also assume you can't share the design where you require this fix, right? |
I did have a look in the GUI and in Siemens Calibre and they seemed reasonable and correct to me, though I do not want to make any statement regarding coverage, might be bad. Another thing that could be considered: From my understanding sroute etc is already covered, hence the focus on vias. In principle this could be expanded to cover all PDN-related placement legalization cases. |
|
@phsauter what's the runtime penalty to leave this on by default? it seems like something we would always want to check (maybe there is a way to determine if its needed automatically?) |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d623071fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Fixed supply via constituent checks cover width/PRL routing spacing and | ||
| // default cut spacing. Detailed routing remains authoritative for other | ||
| // LEF58 rules. | ||
| if (layer->getType() == odb::dbTechLayerType::CUT) { | ||
| return layer->getSpacing(); |
There was a problem hiding this comment.
For PDKs where the applicable separation is defined by LEF58 EOL or cut-spacing-table rules rather than the legacy/default spacing, this function returns a smaller or even zero spacing and -pdn_aware can accept a cell that still violates DRC. The comment's reliance on detailed routing does not close the gap: the corresponding fixed-to-fixed checks are explicitly skipped in src/drt/src/gc/FlexGC_eol.cpp and src/drt/src/gc/FlexGC_cut.cpp, so these violations can remain entirely unreported. Include the applicable LEF58 routing and cut rules in the cached keepout calculation.
Useful? React with 👍 / 👎.
Merge standard-cell geometry into per-row vertical ranges before building the fixed supply via index. Skip candidate checks when every via constituent is separated by the maximum applicable spacing at all legal sites. Retain the collected geometry for a lazy fallback when check_placement sees an invalid site or orientation. Signed-off-by: Philippe Sauter <phsauter@iis.ee.ethz.ch>
Fall back to the exact fixed supply via checks for rows with right-angle rotations. Preserve cell geometry beyond the top and bottom core boundaries so the vertical envelope cannot miss a conflict. Keep the maximum spacing local and describe skipped checks in terms of spacing. Signed-off-by: Philippe Sauter <phsauter@iis.ee.ethz.ch>
Previously once you activate it, you will always have close to the full overhead no matter if you have PDN geometry and cells that can cause conflicts or not (since it still needs to check). |
4 participants
Summary
detailed_placement -pdn_awareadds PDN routing awareness to the placement legalizer and makes it so standard-cell to pdn-shape spacing rules are checked and satisfied.The rough procedure is:
Initial implementation is written by me, Codex Sol was used to improve performance from a 12x slowdown to a 1.5x overhead, to lint and cleanup code and to construct test cases.
Type of Change
Impact
Adds a new flag
-pdn_awaresince activating this features increases legalization runtime by ~1.5x (35s to 55s) on a 500k cell design in GF22FDX. Compared to the total runtime of the flow the time increase is small though so its debatable if it should be active by default or even always active.Using this it is possible to avoid cell internals to PDN via spacing violations.
Without this change we observed spacing violation in GF22FDX both with minimal-size vias (mainly related to coloring and the more complex spacing rules there) and especially with recommended via arrays following design-for-manufacturability (DFM) guidelines. The new pdn-awareness completely prevents these violations at a a very minimal runtime and QoR cost (~0.2% HPWL increase)
Note: These DRC violations are currently not reported by TritonRoute because the geometry checker suppress fixed-to-fixed checks, which includes PDN geometry against placed cells internal geometry.
Verification
./etc/Build.sh). Cannot on workstation, it does build without Bazel though