Fix/balance pad to core connections#10295
Fix/balance pad to core connections#10295alokkumardalei-wq wants to merge 4 commits intoThe-OpenROAD-Project:masterfrom
Conversation
Signed-off-by: alokkumardalei-wq <alokkumardalei2@gmail.com>
Signed-off-by: alokkumardalei-wq <alokkumardalei2@gmail.com>
![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 updates the PDN grid setup to improve pad connection distribution by implementing a round-robin approach for VDD and VSS pads, which helps prevent uneven connection density. The code changes also include significant updates to the test files to reflect these routing adjustments. I have reviewed the logic and suggest that the nested loop structure in grid.cpp could be further simplified to improve readability and reduce indentation.
| for (size_t i = 0; i < max_count; ++i) { | ||
| for (const auto& iterms : per_net_iterms) { | ||
| if (i >= iterms.size()) { | ||
| continue; | ||
| } | ||
| auto* iterm = iterms[i]; | ||
| auto pad_connect = std::make_unique<PadDirectConnectionStraps>( | ||
| this, iterm, connect_pad_layers); | ||
| if (pad_connect->canConnect()) { |
There was a problem hiding this comment.
The nested loop structure for round-robin iteration is clear, but it could be simplified by using a single loop over the maximum size and checking bounds. This would reduce the indentation level and improve readability.
There was a problem hiding this comment.
Done in 20ecd0a, split the round-robin into phase 1 (build ordered_iterms interleaved) and phase 2 (create straps from that flat list). Strap-creation loop is now single-level.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Spatial sort: same comparator across nets so position i in each net | ||
| // refers to a pad in roughly the same region of the die. | ||
| std::sort( | ||
| iterms.begin(), iterms.end(), [](odb::dbITerm* lhs, odb::dbITerm* rhs) { |
There was a problem hiding this comment.
warning: use a ranges version of this algorithm [modernize-use-ranges]
| iterms.begin(), iterms.end(), [](odb::dbITerm* lhs, odb::dbITerm* rhs) { | |
| std::ranges::sort( | |
| iterms,, [](odb::dbITerm* lhs, odb::dbITerm* rhs) { |
| per_net_iterms.push_back(std::move(iterms)); | ||
| } | ||
|
|
||
| size_t max_count = 0; |
There was a problem hiding this comment.
warning: no header providing "size_t" is directly included [misc-include-cleaner]
src/pdn/src/grid.cpp:7:
- #include <cstdint>
+ #include <cstddef>
+ #include <cstdint>Signed-off-by: alokkumardalei-wq <alokkumardalei2@gmail.com>
Signed-off-by: alokkumardalei-wq <alokkumardalei2@gmail.com>
alokkumardalei-wq
force-pushed
the
fix/balance-pad-to-core-connections
branch
from
20ecd0a to
ad10f41
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
Fixes #9994.
Problem
-connect_to_padsonadd_pdn_ringproduces imbalanced pad-to-core ringconnections — e.g. mole99's report of 1 VSS vs 5 VDD per side. Root cause
identified by @gadfort:
CoreGrid::setupDirectConnectadds all VDD strapsto the grid component list before any VSS strap.
Grid::makeShapesiterates components in append order and accumulates obstructions, so VSS
straps face a more obstructed environment than VDD and get dropped
disproportionately by core PDN straps.
Fix
In
CoreGrid::setupDirectConnect:spatial order, identical comparator across nets so position
iofeach net refers to a pad in the same region of the die).
straps now interleave in the grid component list, giving fair
geometric competition during shape generation.
This is static interleave balance, not deficit-tracking
(success/fail feedback) balance. Chose static interleave because:
Grid::makeShapeschanges)scope guidance
@gadfort: if you'd prefer feedback-driven deficit tracking, happy to
escalate to that as a follow-up PR.
Test results (verified locally)
Built openroad on macOS arm64 with
--linkopt=-fuse-ld=lld(Appleld64segfaults on link, LLVM's
lldworks) and ran every affected test:pads_black_parrotpads_black_parrot_flipchippads_black_parrot_flipchip_connect_bumpspads_black_parrot_flipchip_connect_overpadspads_black_parrot_grid_definepads_black_parrot_limit_connectpads_black_parrot_max_widthpads_black_parrot_no_connect-connect_to_pads)pads_black_parrot_offsetpads_black_parrot_strapspads_connect_from_non_pref_edgeEvery change is purely additive — every affected test gains exactly
2 new pad-to-core ring stripes, zero stripes removed, zero geometric
regressions. The
pads_black_parrot_no_connecttest (the onlypads_*test that doesn't use
-connect_to_pads) is unchanged, confirming thechange is correctly scoped.
Why 10 golden file updates and not 1?
@gadfort, you wrote "only impact the tests that rely on the
connectOverPads code". Strictly, my change touches every test using
add_pdn_ring -connect_to_padsordefine_pdn_grid -connect_to_pads,not only the test named
connect_overpads. Hence 10 goldens updated,not 1. Large line counts in the
.defokdiffs are SWIRES re-shufflingin the DEF output (the strap order changed, so the SWIRES section is
re-emitted in different order) , not real geometric changes. Each
test's semantic diff against its old golden is just
+2 stripesasshown above; the line shuffle is an artifact of DEF emission order.
If this scope is broader than you intended, happy to discuss narrowing
the change.