Skip to content

gpl, pdn: fix NaN clamp bypass and -min_width_layers edge cases#10735

Closed
ranaumarnadeem wants to merge 1 commit into
The-OpenROAD-Project:masterfrom
ranaumarnadeem:pdn-fix-min-width-layers
Closed

gpl, pdn: fix NaN clamp bypass and -min_width_layers edge cases#10735
ranaumarnadeem wants to merge 1 commit into
The-OpenROAD-Project:masterfrom
ranaumarnadeem:pdn-fix-min-width-layers

Conversation

@ranaumarnadeem

Copy link
Copy Markdown

Summary

Two bugs in the -min_width_layers feature added by #10689:

1. Off-grid via metal when -min_width_layers is active
generateMinEnclosureViaRects computed min-width via rect edges as
center ± min_width/2 with no manufacturing-grid snap. When a layer is
listed in -min_width_layers the min-width rect becomes the sole via
rect (the full-overlap rect is dropped via std::move), so unsnapped
edges produced off-grid metal. Fixed by snapping with
TechLayer::snapToManufacturingGrid, matching the existing pattern in
generateComplexStackedViaRects.

2. Invalid -min_width_layers layer silently ignored
setMinWidthLayers inserted any layer unconditionally. A layer that is
not an intermediate routing layer of the connect pair (wrong routing
level, cut layer) never matches inside generateMinEnclosureViaRects,
so the user's constraint had no effect with no diagnostic. Fixed by
validating each entry against intermediate_routing_layers_; invalid
layers now emit [WARNING PDN-0501] and are skipped.

Type of Change

  • Bug fix

Impact

  • Via metal constrained by -min_width_layers is now guaranteed to land
    on the manufacturing grid. No change to behavior for connects that do
    not use -min_width_layers.
  • Misconfigured -min_width_layers arguments (e.g. a cut layer or a
    layer outside the connect pair's stack) now produce a
    [WARNING PDN-0501] instead of being silently ignored.

Verification

  • I have verified that the local build succeeds (./etc/Build.sh).
  • I have run the relevant tests and they pass.
  • My code follows the repository's formatting guidelines.
  • I have included tests to prevent regressions.
  • I have signed my commits (DCO).

Related Issues

Introduced by #10689 (pdn: bridge stacked vias to on-grid neighbors and
add -min_width_layers).

Signed-off-by: ranaumarnadeemgit config --global user.name ranaumarnadeem <ranaumarnadeem632@gmail.com>
@ranaumarnadeem ranaumarnadeem requested review from a team as code owners June 22, 2026 04:36
@ranaumarnadeem ranaumarnadeem requested review from gadfort and gudeh June 22, 2026 04:36

@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 introduces NaN handling for the skew_fraction parameter in replace.cpp, defaulting to 0.10f when NaN is encountered. In connect.cpp, it adds validation to ensure that layers specified in setMinWidthLayers are intermediate routing layers, and updates generateMinEnclosureViaRects to snap coordinates to the manufacturing grid. The reviewer suggests replacing the hardcoded magic number 0.10f with a named constant to improve maintainability.

Comment thread src/gpl/src/replace.cpp
Comment on lines +335 to +337
skew_fraction = std::isnan(skew_fraction)
? 0.10f
: std::clamp(skew_fraction, 0.0f, 1.0f);

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

The default skew fraction value 0.10f is hardcoded as a magic number. To improve maintainability and adhere to the general rule of defining tunable parameters or default values as named constants, please define a named constant for this default value.

      constexpr float default_skew_fraction = 0.10f;
      skew_fraction = std::isnan(skew_fraction)
                          ? default_skew_fraction
                          : std::clamp(skew_fraction, 0.0f, 1.0f);
References
  1. Define tunable parameters as named constants instead of using hardcoded magic numbers.

@maliberty

Copy link
Copy Markdown
Member

duplicate of #10734

@maliberty maliberty closed this Jun 22, 2026
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