-
Notifications
You must be signed in to change notification settings - Fork 130
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
Add if extraction transformation #1641
base: main
Are you sure you want to change the base?
Conversation
new_state = outer_sdfg.add_state() | ||
sdutil.change_edge_dest(outer_sdfg, if_branch, new_state) | ||
|
||
# take the old state as the if branch, and create a copy to act as the else branch |
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.
I feel like "duplicate_element" should become a utility method of the sdfg
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.
How is this different from a deepcopy?
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.
It is already used in some other transformation because according to some comment "it is faster than deepcopy". Consider that deepcopy will copy the entire SDFG as it is referenced in the state. Deepcopying nodes can also lead to invalid state -> deepcopy shoud be used on very small things (edge data, ...) or avoided completely, and some state/node duplication utility should likely be provided in the state/node itself
from dace.properties import make_properties | ||
|
||
|
||
def eliminate_branch(sdfg: sd.SDFG, initial_state: sd.SDFGState): |
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.
Is there an analogous method in the sdfg? Should I make it one?
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.
I do not know of one, if this has potential applicability anywhere else, I would add a good docstring and put it on the ControlFlowRegion
or SDFG Utilities.
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.
In general happy to accept this, but we need at least one or two unit tests
Agreeing with @phschaad here - tests for when this applies and when it does not apply are needed. Apart from that, it LGTM. |
0d7af88
to
96706d3
Compare
…ome cosmetic rephrasing of expressions.
as inconvenient as it is.
bit more of corner cases).
Added more unit tests, added more explanation. PTAL. |
Leaving a note here to revisit this transformation for slight adaptation after #1676 is merged. |
As part of the series of SDFG rewriting transformations, IfExtraction targets nested SDFGs that begin with an if statement whose condition is independent of symbols defined or updated in the outer state (these are either writes to arrays or definitions in maps). The if is "extracted": the outers state is duplicated and a new if guard is created in the outer SDFG with the appropriately translated condition. If the edges contain symbol definitions these become definitions in the nested SDFG
symbol_mapping
.Example of application:
becomes: