Conversation
…the synth passes that need them.
It's pretty common for `opt_clean` to find no wires to remove. In that case, there is no point scanning the entire design, which can be significantly expensive for huge designs.
Currently when `s1` and `s2` are different bits of the same wire, it is possible for both `compare_signals(s1, s2)` and `compare_signals(s2, s1)` to return false. This means the calling code will call `assign_map.add()` for both `s1` and `s2`, which doesn't make much sense --- one of `s1` or `s2` should be consistently preferred. So fix that by preferring the `SigBit` with the smaller bit offset.
The negation here is confusing. The intent of the code is "if `s1` is preferred over `s2` as the canonical `SigBit` for this signal, make `s1` the canonical `SigBit` in `assign_map`", so write the code that way instead of "if `s2` is not preferred over `s1` ...". This doesn't change any behavior now that `compare_signals()` is a total order, i.e. `s1` is preferred over `s2`, `s2` is preferred over `s1`, or `s1` and `s2` are equal. Now, when `s1` and `s2` are equal, we don't call `assign_map.add(s1)`, but that's already a noop in that case.
Remove `Design::sort()` calls from optimization passes
Avoid scanning entire module in `Module::remove()` if there are no wires to remove
Clean up `compare_signals()` in `opt_clean`
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 8bbde80 in 14 seconds. Click for details.
- Reviewed
113lines of code in7files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_Fq3eL7e8TRrFjQuG
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Greptile OverviewGreptile SummaryThis PR is a merge from upstream that shifts sorting to be explicit in scripts/passes and tweaks a few internals. Key changes:
No must-fix issues were found in the diff: the new Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User/Script
participant Pass as ScriptPass/Pass
participant Design as RTLIL::Design
participant Module as RTLIL::Module
User->>Pass: "Invoke prep / synth_*"
Pass->>Design: "optimize()"
Note over Pass: "PR removes implicit Design::sort() from opt/opt_clean"
Pass->>Pass: "run('sort') at key stages"
Pass->>Design: "sort() (explicit)"
Pass->>Pass: "run('opt_clean')"
Pass->>Module: "rmunused_module_signals()"
Module->>Module: "compare_signals() picks representative"
Module->>Module: "remove(wires)"
Note over Module: "remove() returns early on empty pools"
Pass->>Design: "check()"
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If your work is part of a larger effort, please discuss your general plans on Discourse first to align your vision with maintainers.
What are the reasons/motivation for this change?
Explain how this is achieved.
Make sure your change comes with tests. If not possible, share how a reviewer might evaluate it.
These template prompts can be deleted when you're done responding to them.
Important
Add
sortcommand to synthesis scripts and remove redundantsortcalls from optimization passes, with updates to signal comparison logic inopt_clean.cc.sortcommand toprep.ys,prep.cc,synth_gowin.cc, andsynth_xilinx.ccto ensure sorting is done at specific stages.sortcalls fromopt.ccandopt_clean.ccto streamline optimization passes.opt_clean.cc, modifycompare_signals()to ensure correct signal comparison by adding a condition to compare offsets when wires are the same.opt_clean.cc, changermunused_module_signals()to use the updatedcompare_signals()logic.opt_clean.ccto improve readability and maintainability.This description was created by
for 8bbde80. You can customize this summary. It will automatically update as commits are pushed.