-
Notifications
You must be signed in to change notification settings - Fork 15
orfs_synth: filter non-synth stage arguments, avoid rebuilds #490
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
minimizes surprises, same behavior as orfs_flow() Signed-off-by: Øyvind Harboe <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors orfs_synth to filter stage-specific arguments before passing them to the underlying rule implementation, matching the behavior of orfs_flow(). This prevents unnecessary rebuilds by ensuring only relevant arguments are passed to each stage.
Key changes:
- Introduced
_filter_stage_args()helper function to filter arguments per stage - Converted
orfs_synthfrom a rule to a macro wrapper that calls the new_orfs_synthrule - Applied the same argument filtering pattern to synthesis and other stages in
_orfs_pass()
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| openroad.bzl | Renamed orfs_synth rule to _orfs_synth, added _filter_stage_args() function, created new orfs_synth() macro wrapper, and updated _orfs_pass() to use filtered arguments |
| BUILD | Added test comment with PLACE_DENSITY argument to verify synth stage doesn't rebuild when non-synth arguments change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Filter and prepare the arguments for a specific stage.""" | ||
|
|
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _filter_stage_args function lacks a docstring explaining its parameters and return value. While line 1178 provides a brief description, it should document the stage parameter and the **kwargs structure including expected keys like arguments, data, settings, etc.
| """Filter and prepare the arguments for a specific stage.""" | |
| """ | |
| Filter and prepare the keyword arguments for a specific flow stage. | |
| This helper normalizes and combines generic and per-stage arguments, | |
| sources, and data before invoking a particular stage of the flow. | |
| Args: | |
| stage: String name of the stage to prepare arguments for (for example | |
| "synth", "place", or "route"). This value is used to select | |
| per-stage entries from the structures in ``**kwargs``. | |
| **kwargs: Keyword arguments controlling how stage arguments are built. | |
| Recognized keys include: | |
| * ``arguments``: dict mapping generic argument names to values that | |
| apply to all stages. | |
| * ``data``: list of data targets or files that apply to all stages. | |
| * ``settings``: dict of generic settings (similar structure to | |
| ``arguments``) that apply to all stages. | |
| * ``extra_configs``: dict mapping stage name to a list of additional | |
| configuration files or labels for that stage. | |
| * ``sources``: dict or structure containing source files or labels | |
| that are shared across stages. | |
| * ``stage_arguments``: dict mapping stage name to a dict of | |
| arguments specific to that stage. | |
| * ``stage_sources``: dict mapping stage name to additional source | |
| entries specific to that stage. | |
| * ``stage_data``: dict mapping stage name to a list of data entries | |
| (targets or files) specific to that stage. | |
| Any additional keyword arguments are passed through unchanged to the | |
| returned dictionary. | |
| Returns: | |
| A dict containing the prepared keyword arguments for ``stage`` with at | |
| least the following keys: | |
| * ``arguments``: merged generic and per-stage arguments. | |
| * ``data``: combined generic and per-stage data. | |
| * ``extra_configs``: list of extra configuration entries for the | |
| given stage. | |
| * ``settings``: merged generic and per-stage settings. | |
| Any additional keys provided in ``**kwargs`` are also included in the | |
| returned dict. | |
| """ |
| def _args(**kwargs): | ||
| return kwargs |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nested _args() helper function simply returns its arguments as a dictionary. This adds unnecessary indirection - you can directly return a dictionary literal in the outer function's return statement instead.
minimizes surprises, same behavior as orfs_flow()