Refactor full_selection #4768
Open
Refactor full_selection #4768
Conversation
This file contains 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
KrystalDelusion
force-pushed
the
krys/refactor_selections
branch
from
be3c542 to
c0a04b8
Compare
KrystalDelusion
requested review from
eddiehung and
Ravenslofty
as code owners
The `Design::selected_*()` methods no longer unconditionally skip boxed modules. Instead, selections are now box and design aware.
The selection constructor now optionally takes a design pointer, and has a new `selects_boxes` flag. If the selection has an assigned design, then `Selection::selected_*()` will only return true for boxed modules if the selects_boxes flag is set. A warning is raised if a selection is checked and no design is set. Selections can change design via the `Selection::optimize()` method.
Most places that iterate over `Design::modules()` and check `Selection::selected_module()` should instead use `Design::selected_modules()`.
Since boxed modules should only ever be selected explicitly, and `full_selection` (now) refers to all non-boxed modules, `Selection::optimize()` will clear the `full_selection` flag if the `selects_boxes` flag is enabled, and instead explicitly selects all modules (including boxed modules). This also means that `full_selection` will only get automatically applied to a design without any boxed modules.
These changes necessitated a number of changes to `select.cc` in order to support this functionality when operating on selections, in particular when combining selections (e.g. by union or difference).
To minimize redundancy, a number of places that previously iterated over `design->modules()` now push the current selection to the design, use `design->selected_modules()`, and then pop the selection when done.
Introduce `RTLIL::NamedObject`, to allow for iterating over all members of a module with a single iterator instead of needing to iterate over wires, cells, memories, and processes separately.
Also implement `Module::selected_{memories, processes, members}()` to match wires and cells methods. The `selected_members()` method combines each of the other `selected_*()` methods into a single list.
Now uses two enums, one to control whether or not to include partially selected
modules (and what to do if they are encountered), and one to control whether or
not to include boxed modules (and what to do if they are encountered).
Mark Design::selected{modules, whole_modules}() deprecated and make them
provide warnings on boxes. There are a lot of places that use them and I can't
always tell which ones support boxed modules and which don't.
If a selection contains a boxed module, but does not select boxes, it should be removed from the selection.
New methods on Design to push/pop selection instead of accessing the selection stack directly. Includes methods for pushing a full/complete/empty selection. Also helper methods on modules to check `is_selected` and `is_selected_whole`.
Catch more uses of selection constructor without assigning a design.
Since it doesn't change anything and is just a lookup.
If the current selection is not the provided selection, push the provided selection.
cxxrtl `test_unconnected_output` and simple_abc9 `abc9.v` both expect boxed modules in the outputs, so make sure they work as expected.
Used to select all modules including boxes, set when both `full` and `boxes` are true in the constructor, pulling down `full_selection`. Add `Selection::selects_all()` method as short hand for `full_selection || complete_selection`. Update selection operations to account for complete selections. Add static methods to `Selection` for creating a new empty/full/complete selection to make it clearer to users when doing so. Use said static methods to replace most instances of the `Selection` constructor. Update `Selection::optimize` to use
Or rather, say we're calling `select =*`, but actually bypass the select command to avoid the warning that can pop up if there is nothing to select.
Fixes quicklogic/pp3 problem with `dffepc` including processes. Also means the preceding `proc` is safe to remove (and may result in some small speedup by doing so).
Add a `log_assert` for it in `Design::check()`. Remove unneeded checks in other places.
Change `top` pointer default to `nullptr` to avoid issues with `Design->top_module()` only operating on the current selection. Calls to other passes (`bmuxmap` etc) will only operate on the current selection, and may cause problems when those cells are unprocessed, but this is consistent with the other backends that only operate on the full designs and will hopefully be fixed in another PR soon :)
Instead, change the default `Design::selected_modules()` to match the behaviour (i.e. `selected_unboxed_modules_warn()`) because it's a lot of files to touch and they don't really _need_ to be updated. Also change `Design::selected_whole_modules()` users over to `Design::selected_unboxed_whole_modules()`, except `attrmap` because I'm not convinced it should be ignoring boxes. So instead, leave the deprecation warning for that one use and come back to the pass another time.
KrystalDelusion
force-pushed
the
krys/refactor_selections
branch
from
4c0f68b to
a3968d4
Compare
|
Rebase on v |
Because the use of `RTLIL::AttrObject::get_blackbox_attribute()` is deprecated, but the assert is needed in case users are doing weird things.
If the selection stack only has one element (which it normally does), then `design->pop_selection()` automatically resets to the default full selection. This is a problem for `select [-none | -clear]` which were trying to replace the current selection, but because the pop added an extra element when the `execute` returned, the extra selection (the one we actually wanted) gets popped too. So instead, reassign `design->selection()` in the same way as if we called `select [selection]`. Also adds selection stack tests, and removes the accidentally-committed `boxes_dummy.ys`.
KrystalDelusion
added
the
status-needs-review
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.
What are the reasons/motivation for this change?
A
full_selectioncurrently includes boxed modules, butDesign::selected_modules()silently ignores boxes. This has lead to a number of workarounds in the code for getting all unboxed selected modules.Explain how this is achieved.
full_selectionto not include (black)boxes. Non-full selections can contain blackboxes (but this will only happen with =patterns that ask for them explicitly).complete_selectionflag that has the previous meaning offull_selection, i.e. all modules including boxes for optimisation purposes and to simplify nested commands that expect boxed modules, likeabc9.Note:
Design::selected_whole_modules()is marked deprecated because I'm not convincedattrmapshould be ignoring boxes, even though that is the current behaviour.Optional (but probably for a separate pr):
Design::modules()to useDesign::selected_modules()where relevant.-selectedwhere possible, otherwise working on the full design).If applicable, please suggest to reviewers how they can test the change.
New tests are added in
tests/selectto check the selection semantics w.r.t boxes that have been changed here, but I guess double check that the semantics when applied to boxes match their stated intention inhelp select.