Skip to content

gpl: fix options and cleanup readme#10406

Open
TheMightyDuckOfDoom wants to merge 1 commit into
The-OpenROAD-Project:masterfrom
TheMightyDuckOfDoom:gpl-fix-timing_driven_nets_percentage
Open

gpl: fix options and cleanup readme#10406
TheMightyDuckOfDoom wants to merge 1 commit into
The-OpenROAD-Project:masterfrom
TheMightyDuckOfDoom:gpl-fix-timing_driven_nets_percentage

Conversation

@TheMightyDuckOfDoom
Copy link
Copy Markdown
Contributor

This fixes a few issues with GPL options:

  • Outdated README.md: Some options where not present
  • Remove non-functional -disable_timing_driven and -disable_routability_driven
  • Fix -timing_driven_nets_percentage
  • Validate options range to match what is stated in the README.md

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Comment thread src/gpl/src/replace.cpp Outdated
Comment thread src/gpl/src/replace.cpp Outdated
Copy link
Copy Markdown
Contributor

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

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 a new timing-driven net percentage parameter to the global placement configuration, updates the documentation, and adds corresponding validation logic and tests. The review identified several issues, including incorrect validation ranges for padding and coefficient parameters, typos in documentation and parameter names, and inconsistencies in the newly added test scripts. All review comments provide actionable feedback and have been retained.

Comment thread src/gpl/src/replace.cpp Outdated
Comment thread src/gpl/README.md Outdated
Comment thread src/gpl/README.md Outdated
Comment thread src/gpl/README.md Outdated
Comment thread src/gpl/src/replace.cpp
Comment thread src/gpl/src/replace.cpp Outdated
Comment thread src/gpl/src/replace.cpp Outdated
Comment thread src/gpl/test/simple01-td-net-percentage.py Outdated
Comment thread src/gpl/test/simple01-td-net-percentage.py Outdated
Comment thread src/gpl/test/simple01-td-net-percentage.tcl Outdated
@TheMightyDuckOfDoom TheMightyDuckOfDoom force-pushed the gpl-fix-timing_driven_nets_percentage branch from 760149f to 5ac8b2d Compare May 12, 2026 13:39
@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@TheMightyDuckOfDoom TheMightyDuckOfDoom force-pushed the gpl-fix-timing_driven_nets_percentage branch from 5ac8b2d to 3d25a22 Compare May 12, 2026 14:54
@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@TheMightyDuckOfDoom TheMightyDuckOfDoom force-pushed the gpl-fix-timing_driven_nets_percentage branch from 3d25a22 to 10d37bd Compare May 12, 2026 15:28
@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@TheMightyDuckOfDoom TheMightyDuckOfDoom force-pushed the gpl-fix-timing_driven_nets_percentage branch from 10d37bd to ce1bbd9 Compare May 12, 2026 16:30
@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@TheMightyDuckOfDoom TheMightyDuckOfDoom force-pushed the gpl-fix-timing_driven_nets_percentage branch from ce1bbd9 to 7ff19c4 Compare May 12, 2026 17:39
@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@TheMightyDuckOfDoom TheMightyDuckOfDoom marked this pull request as ready for review May 12, 2026 19:32
Signed-off-by: Tobias Senti <git@tsenti.li>
@TheMightyDuckOfDoom TheMightyDuckOfDoom force-pushed the gpl-fix-timing_driven_nets_percentage branch from 7ff19c4 to 0487a95 Compare May 14, 2026 10:14
@TheMightyDuckOfDoom TheMightyDuckOfDoom requested a review from a team as a code owner May 14, 2026 10:14
@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Comment thread src/gpl/README.md
| `-init_density_penalty` | Set initial density penalty. The default value is `8e-5`. Allowed values are floats `[1e-6, 1e6]`. |
| `-init_wirelength_coef` | Set initial wirelength coefficient. The default value is `0.25`. Allowed values are floats. |
| `-min_phi_coef` | Set `pcof_min` ($\mu_k$ Lower Bound). The default value is `0.95`. Allowed values are floats `[0.95, 1.05]`. |
| `-min_phi_coef` | Set `pcof_min` ($\mu_k$ Lower Bound). The default value is `0.95`. Suggested values are positive floats `[0.95, 1.05]`. |
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.

Does it work if we do values outside of the suggested range? the max_phi_coef was kept with "allowed"

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