Repository navigation
Conversation
Retain each generated-rule default during validation when no applicable cut-layer enclosure rule replaces it. Preserve signed deficits through half-margin calculation and manufacturing-grid snapping. Add actual-geometry regressions and retain valid override behavior. Update reviewed expected outputs to expose rejected malformed connections. Signed-off-by: Mahmoud Sub Laban <137659892+MahmoudNedalSubLaban@users.noreply.github.com>
![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/GettingInvolved.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 improves the handling of generated-via enclosures in the PDN generator. It updates snapping logic to prevent rounding enclosure deficits up to zero, changes double enclosure division to use floor division instead of truncating integer division, and ensures default VIARULE enclosures are retained when no cut-layer overrides apply. Additionally, several regression tests are introduced. The feedback suggests optimizing the floor division by replacing the floating-point std::floor conversions with highly efficient arithmetic right shifts (>> 1) on signed integers.
| bottom_enclosure_->setX(determine_enclosure( | ||
| use_bottom_min_enclosure, | ||
| true, | ||
| bottom_min_enclosure.getX(), | ||
| static_cast<int>(std::floor(double_enc_x / 2.0)), | ||
| lower_constraint_)); | ||
| bottom_enclosure_->setY(determine_enclosure( | ||
| use_bottom_min_enclosure, | ||
| false, | ||
| bottom_min_enclosure.getY(), | ||
| static_cast<int>(std::floor(double_enc_y / 2.0)), | ||
| lower_constraint_)); | ||
| top_enclosure_->setX(determine_enclosure( | ||
| use_top_min_enclosure, | ||
| true, | ||
| top_min_enclosure.getX(), | ||
| static_cast<int>(std::floor(double_enc_x / 2.0)), | ||
| upper_constraint_)); | ||
| top_enclosure_->setY(determine_enclosure( | ||
| use_top_min_enclosure, | ||
| false, | ||
| top_min_enclosure.getY(), | ||
| static_cast<int>(std::floor(double_enc_y / 2.0)), | ||
| upper_constraint_)); |
There was a problem hiding this comment.
Using floating-point division double_enc_x / 2.0, std::floor, and casting back to int introduces unnecessary overhead. Since double_enc_x and double_enc_y are signed integers, we can perform highly efficient integer-only floor division by 2 using the arithmetic right shift operator >> 1 (which is guaranteed to be arithmetic and perform floor division in C++20, and is universally supported as such in C++17). This avoids floating-point conversions entirely.
bottom_enclosure_->setX(determine_enclosure(
use_bottom_min_enclosure,
true,
bottom_min_enclosure.getX(),
double_enc_x >> 1,
lower_constraint_));
bottom_enclosure_->setY(determine_enclosure(
use_bottom_min_enclosure,
false,
bottom_min_enclosure.getY(),
double_enc_y >> 1,
lower_constraint_));
top_enclosure_->setX(determine_enclosure(
use_top_min_enclosure,
true,
top_min_enclosure.getX(),
double_enc_x >> 1,
upper_constraint_));
top_enclosure_->setY(determine_enclosure(
use_top_min_enclosure,
false,
top_min_enclosure.getY(),
double_enc_y >> 1,
upper_constraint_));| bottom_enclosure_->setX( | ||
| determine_enclosure(use_bottom_min_enclosure, | ||
| true, | ||
| bottom_min_enclosure.getX(), | ||
| static_cast<int>(std::floor(double_enc_x / 2.0)), | ||
| lower_constraint_)); | ||
| bottom_enclosure_->setY( | ||
| determine_enclosure(use_bottom_min_enclosure, | ||
| false, | ||
| bottom_min_enclosure.getY(), | ||
| static_cast<int>(std::floor(double_enc_y / 2.0)), | ||
| lower_constraint_)); | ||
| top_enclosure_->setX( | ||
| determine_enclosure(use_top_min_enclosure, | ||
| true, | ||
| top_min_enclosure.getX(), | ||
| static_cast<int>(std::floor(double_enc_x / 2.0)), | ||
| upper_constraint_)); | ||
| top_enclosure_->setY( | ||
| determine_enclosure(use_top_min_enclosure, | ||
| false, | ||
| top_min_enclosure.getY(), | ||
| static_cast<int>(std::floor(double_enc_y / 2.0)), | ||
| upper_constraint_)); |
There was a problem hiding this comment.
Using floating-point division double_enc_x / 2.0, std::floor, and casting back to int introduces unnecessary overhead. Since double_enc_x and double_enc_y are signed integers, we can perform highly efficient integer-only floor division by 2 using the arithmetic right shift operator >> 1 (which is guaranteed to be arithmetic and perform floor division in C++20, and is universally supported as such in C++17). This avoids floating-point conversions entirely.
bottom_enclosure_->setX(
determine_enclosure(use_bottom_min_enclosure,
true,
bottom_min_enclosure.getX(),
double_enc_x >> 1,
lower_constraint_));
bottom_enclosure_->setY(
determine_enclosure(use_bottom_min_enclosure,
false,
bottom_min_enclosure.getY(),
double_enc_y >> 1,
lower_constraint_));
top_enclosure_->setX(
determine_enclosure(use_top_min_enclosure,
true,
top_min_enclosure.getX(),
double_enc_x >> 1,
upper_constraint_));
top_enclosure_->setY(
determine_enclosure(use_top_min_enclosure,
false,
top_min_enclosure.getY(),
double_enc_y >> 1,
upper_constraint_));Apply the repository Tcl formatter and split long diagnostic output lines. Preserve geometry assertions and byte-identical reproduction outputs. Signed-off-by: Mahmoud Sub Laban <137659892+MahmoudNedalSubLaban@users.noreply.github.com>
|
@gadfort would you review this? The via code is involved as I recall |
The enclosure guard selects Via1Array-1 in the two Nangate45 RAM tests. Via1Array-0 requires 35 nm on both axes, but the metal1 landing provides only 5 nm on one axis. Update only the generated-via definition in each expected DEF. Keep signal-routing, placement and Verilog expectations unchanged. Independent OpenDB checks reduce invalid instances from 36/54 to zero while preserving both supply connectivity checks. Signed-off-by: Mahmoud Sub Laban <137659892+MahmoudNedalSubLaban@users.noreply.github.com>
|
I’ve pushed a69d08e to address the two RAM regression failures. Both tests now pass, and the Jenkins head and merge builds are successful. The expected-output changes were checked against actual via geometry and supply connectivity; the PR description includes the results and limitations. Could a maintainer approve the pending GitHub Actions workflows when convenient? |
Signed-off-by: Mahmoud Sub Laban <137659892+MahmoudNedalSubLaban@users.noreply.github.com>
Signed-off-by: Mahmoud Sub Laban <137659892+MahmoudNedalSubLaban@users.noreply.github.com>
|
Pushed 1ec9fa0 to address merge #12. The resizer failure is the upstream golden-log issue fixed by 53b91d5, now included by merging current master. The new pad fixture's expected DEF retained 374 vias with 195 nm enclosure where metal9 requires 200 nm. The rectangle audit confirms their rejection, with metal shapes and instance poses unchanged. Only that fixture's expected DEF and log change; all rejection warnings remain. Fresh local checks: 216/216 PDN tests, the resizer test and single-port RAM pass. Defect controls fail before/pass after; the valid control is unchanged. The separate local multiport RAM route-order mismatch occurs with both executables, so its golden remains unchanged. The pad fixture still fails both supply checks and reports more disconnected shapes after illegal connections are removed. This is not a connectivity repair. The PR description includes the geometry, source/executable bindings and limitations. New-head CI and review remain pending. |
Signed-off-by: Mahmoud Sub Laban <137659892+MahmoudNedalSubLaban@users.noreply.github.com>
Exercise per-landing defaults, applicable enclosure alternatives, signed rounding, and rejection of insufficiently enclosed cuts with the native via fixture. Six defect tests fail consistently on unpatched upstream; the applicable-override control passes. Document the native Bazel target and retain the existing integration geometry checks. Signed-off-by: Mahmoud Sub Laban <137659892+MahmoudNedalSubLaban@users.noreply.github.com>
Use legal landings for containment, tapered-stack, extension and ripup controls. Check emitted rectangles and retain the undersized landings as rejection regressions. Signed-off-by: Mahmoud Sub Laban <137659892+MahmoudNedalSubLaban@users.noreply.github.com>
|
I've updated this PR to current master ( The same tests reproduce six enclosure defects and three caller-level rejection failures on unpatched upstream, consistently across three runs. They all pass with the fix. The complete PDN, PSM and RAM suites, plus The merge exposed five connection fixtures that relied on under-enclosed vias. I kept their containment, extension and ripup coverage using valid landings, added checks against the emitted rectangles, and retained the undersized cases as rejection tests. There are no additional production changes or expected-output updates. @gadfort, could you review the per-landing default fallback and signed rounding when you have time? The updated description and reproduction notes include the results and remaining limits. Rejecting an illegal via still does not guarantee a replacement connection. |
|
@MahmoudNedalSubLaban I'll take a look this weekend. Thanks for updating the tests. |
Use variant so the content scan does not mistake a shell variable for a restricted technology name. Commands and output paths are unchanged. Signed-off-by: Mahmoud Sub Laban <137659892+MahmoudNedalSubLaban@users.noreply.github.com>
gadfort
left a comment
gadfort
left a comment
There was a problem hiding this comment.
Overall looks good, if the added tcl tests are not checking anything more than the CPP test, then I would suggest removing them. If they do, then they should mirror the other tests so its a little easier to manage.
Keep one integration test for rejection reporting and supply connectivity, using the standard PDN test helpers. Remove the redundant Tcl cases and custom runner; enclosure assertions remain in the C++ tests. Signed-off-by: Mahmoud Sub Laban <137659892+MahmoudNedalSubLaban@users.noreply.github.com>
|
Thanks for reviewing, @gadfort. Pushed b5d6449. I removed the two redundant Tcl cases and the custom runner and audit scripts. I kept one integration test because it checks the rejected-connection report and VDD/VSS connectivity after No production changes. All 287 affected test targets passed three fresh runs locally. Could you take another look? |
Resolve the sole modify/delete conflict by accepting upstream removal of src/pdn/test/CMakeLists.txt. Preserve the reviewed enclosure fix, regression fixtures, expected outputs, and Bazel test registration. Signed-off-by: Mahmoud Sub Laban <137659892+MahmoudNedalSubLaban@users.noreply.github.com>
|
@maliberty can you approve for the maintainers? |
Summary
Generated vias can bypass enclosure validation when no applicable cut-layer
rule replaces the VIARULE default. One reproducer has a 140 nm cut and a
20 nm landing: −60 nm enclosure on each side.
Retain the default independently for each landing without an applicable
override. Preserve negative margins through half-margin calculation and
manufacturing-grid snapping. Applicable overrides remain alternatives;
legal landing-extension attempts remain available. The only production file
changed is
src/pdn/src/via.cpp.Fixes #11538.
Current validation - 24 October
Head:
0eaf08712f2bee52fbbd2493ed67cd9f53d559e6.Merged upstream:
6f3f28cc85407d71ba64a550cca194d8a6a01c1c.The enclosure fix remains the only production change relative to upstream.
This update adds native C++ regressions in the existing PDN test targets.
It adds no Tcl tests and makes no additional expected-output changes.
RAM, and
repair_setup_reroute1. Test-result reuse was disabled.six defect tests fail in all three runs. With the fix, 211/211 pass.
upstream: three enclosure-rejection tests fail in all three runs.
With the fix, 77/77 pass.
tapered stacks, legal landing extension, obstruction ripup, and
permitted/forbidden nonpreferred-direction widening.
clang-format --dry-run --Werrorandgit diff --checkpass.Five recently added connection fixtures expected successful construction
using insufficient enclosure. An independent check of their actual OpenDB
rectangles found margins of 0, 5, or -5 DBU against 10-DBU requirements.
The positive fixtures now use adequate landings, and the original undersized
locked/pinned, outer-stack, and widening cases are rejection regressions.
The rectangle checks do not call the generator's enclosure predicate.
The previously noted multiport RAM local mismatch does not occur in this
Bazel run. Its existing expected output was not changed for this update.
These are local results; CI and maintainer review of the new head are pending.
Current reproduction commands and rule precedence.
Earlier validation - 11 October
Head:
1ec9fa04d61aaba2bfb0672c100b3bc0676617df.Upstream base:
b931a33de66ae1956279d47eda0cc26df25dedb7.Fresh local CMake/GCC checks:
repair_setup_reroute1andmake_7x7_nangate45pass.The valid control's DEF and geometry tables are byte-identical.
git diff --checkpasses. All contributor commits carry DCO sign-off.The additional multiport RAM shell-runner check still has a local route-order
mismatch with its golden. Fresh unpatched and patched outputs differ only
in the previously reviewed generated-via definition; their signal routes
match each other. That unrelated golden is unchanged. This is not a claim
that every local regression passes or that a full local Bazel build was run.
Public reproduction commands and rule precedence.
Existing override tests, including
stacked_via_merged_enclosureand theIHP cases, remain unchanged. New-head CI and maintainer review are pending.
Merge #12 follow-up
Merge #12
had two regression failures as well as infrastructure errors.
53b91d5
fixes its expected log. The complete failing log matches that correction.
The current upstream merge includes it; this PR adds no resizer change.
Its old expected DEF includes 374 generated vias with 195 nm metal9
enclosure against a 200 nm requirement. An independent OpenDB rectangle
audit finds 374 invalid instances before and zero after the fix
(756 → 382 generated instances; 187 rejected on each supply).
Metal shapes, instance poses and net-terminal records are unchanged.
The follow-up changes only that fixture's expected DEF and log: one invalid
via definition and its 374 instances disappear, and all 374 rejection warnings
remain visible. No production code, test assertion or technology rule is
changed in this follow-up.
Both supply checks already fail in this fixture. After rejection, native
unconnected-shape messages increase 468 → 1,001 and unconnected-instance
messages 200 → 266. These are message counts, not a sign-off metric.
Consequences and limits
Rejecting an illegal via does not guarantee a replacement connection.
In the earlier, already-disconnected
pads_black_parrot_macro_gridfixture,the retained audit reports disconnected-shape markers increasing from
1,595 to 9,349 for VDD and 1,035 to 9,133 for VSS.
This does not solve DFT68's separate PDN-0179 construction problem or
establish full-chip PPA, IR-drop, electromigration or universal PDN repair.
Local results are not upstream CI or maintainer acceptance.
Historical validation and tested executable identities
The September 28 local PDN result was 207/207. The earlier RAM correction
(
a69d08ebb) changed one via-definition line per expected DEF: the existinglegal
Via1Array-1replaces the insufficiently enclosedVia1Array-0.The retained OpenDB audits removed 36/54 invalid generated instances from
the two RAM fixtures, with supply checks passing before and after.
Jenkins head #3
and merge #3
passed on September 28 (4,467 passing, four skipped; respectively 10 and
1,768 fresh executions). These historical runs do not certify the current head.
Fresh October 11 executable SHA-256 values:
d4ea7743b5c232cc3583831e6918a644347b21be54660d832690b83ede23defceba45f730c88e5adad31ead83cdc9ecff0ecdb02c4493993dbec67dad651165bThe builds use identical dependencies. Their production-source difference
is only
via.cpp. The corrected executable was built from3396a194ae;the subsequent
1ec9fa04d6changes only the two reviewed expected-outputfiles, used by the final regression run.
Review of the eleven existing PDN expected-output changes
These counts come from the retained original-versus-corrected OpenDB review,
not a recount by the latest regression run. They count generated-array
instances, not individual cuts or independent designs. Existing test scripts
and assertions are unchanged, and macro/cell poses are preserved.
macros_cells_via_failurepads_black_parrotpads_black_parrot_flipchippads_black_parrot_flipchip_connect_bumpspads_black_parrot_flipchip_connect_overpadspads_black_parrot_grid_definepads_black_parrot_macro_gridpads_black_parrot_offsetpads_black_parrot_strapspolygon_cut_corners_padspolygon_two_macrosThe metal-cleanup explanation is an inference from geometry differences, not a
per-operation trace. These expected outputs document rejection consequences;
they do not certify these fixtures as fully connected or physically signed off.
Related earlier work: #1828, #11193 and #11212. Their functionality and
attribution are preserved.