-
Notifications
You must be signed in to change notification settings - Fork 131
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
Loop Regions #1407
Loop Regions #1407
Conversation
2d4c318
to
5573575
Compare
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, looks good.
@@ -1156,10 +1181,9 @@ def __init__(self, label=None, sdfg=None, debuginfo=None, location=None): | |||
""" | |||
from dace.sdfg.sdfg import SDFG # Avoid import loop | |||
OrderedMultiDiConnectorGraph.__init__(self) | |||
ControlFlowBlock.__init__(self, label) | |||
ControlFlowBlock.__init__(self, label, 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.
In case there is a discussion about using a single super
call to initialize everything:
This is a good read: https://stackoverflow.com/questions/3277367/how-does-pythons-super-work-with-multiple-inheritance
My understanding is that super(SDFGState, self).__init__()
will call in a sequence the inits of all the superclasses (recursively even if they also call super
). To address the issue of different parameters, you could change the signatures of the inits to self, args, **kwargs.
However, my suggestion would be NOT to CHANGE anything, because IMHO it is unintuitive, and most people would just be confused.
|
||
@make_properties | ||
class LoopRegion(ControlFlowRegion): | ||
|
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 would add some documentation here. Some notes:
- I don't like the name
inverted
because it doesn't sound to me to match the description. I also don't have a suggestion, but ask around for opinions. - Break and continue happen after completing execution of a break/continue state?
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.
Correct on the break / continue. I will make sure to add the necessary documentation, thanks!
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.
Added documentation
@@ -2170,6 +2171,10 @@ def compile(self, output_file=None, validate=True) -> 'CompiledSDFG': | |||
# if the codegen modifies the SDFG (thereby changing its hash) | |||
sdfg.build_folder = build_folder | |||
|
|||
# Convert any loop constructs with hierarchical loop regions into simple 1-level state machine loops. | |||
# TODO (later): Adapt codegen to deal with hierarchical CFGs instead. | |||
sdutils.inline_loop_blocks(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.
This is not the right place to put this code. API calls such as generate_code will skip this. It’s better to add this to the codegen module, right after we validate before generating the code
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.
Moved.
dace/sdfg/state.py
Outdated
@@ -99,6 +99,10 @@ def in_degree(self, node: NodeT) -> int: | |||
def out_degree(self, node: NodeT) -> int: | |||
... | |||
|
|||
@property | |||
def sdfg(self) -> 'dace.sdfg.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.
you should probably import dace.sdfg.SDFG in the TYPE_CHECKING clause (line 28)
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.
Done
init_edge = InterstateEdge() | ||
if self.loop.init_statement is not None: | ||
init_edge.assignments = { | ||
self.loop.loop_variable: self.loop.init_statement.as_string.rpartition('=')[2].strip() |
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 am not super happy with this. There can be more than one statement in a codeblock, or a different statement entirely (i.e., that does not assign anything). Perhaps verify that there is only one ast.Assign in the can_be_applied
method.
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.
Fixed - I'm allowing any number of assignments (but nothing else!) since this can be expressed with a valid SDFG (isedges can have an arbitrary number of assignments).
update_edge = InterstateEdge() | ||
if self.loop.update_statement is not None: | ||
update_edge.assignments = { | ||
self.loop.loop_variable: self.loop.update_statement.as_string.rpartition('=')[2].strip() |
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.
same here
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.
same here
Co-authored-by: Tal Ben-Nun <[email protected]>
Co-authored-by: Tal Ben-Nun <[email protected]>
Co-authored-by: Tal Ben-Nun <[email protected]>
Co-authored-by: Tal Ben-Nun <[email protected]>
This PR lets the Python and Fortran frontends (optionally) generate `LoopRegion`s for DaCe programs. This forms the third core element of the [plan to make loops first class citizens of SDFGs](https://github.com/orgs/spcl/projects/10). This PR is fully backwards compatible. `LoopRegion`s are always generated from new Python DaCe programs, and the legacy way of constructing a while / for loop is gone to remove complexity. To provide backwards compatibility, these `LoopRegion`s are by default immediately inlined into a traditional single level state machine loop as soon as program parsing is completed, before simplification and / or validation. However, an optional boolean parameter `use_experimental_cfg_blocks` can be set to True when declaring a DaCe program in Python to enable their use, which skips this inlining step. Example use: ```Python import dace import numpy N = dace.symbol('N') @dace.program(use_experimental_cfg_blocks=True): def mat_mult(A: dace.float64[N, N], B: dace.float64[N, N]): return A @ B # OR: mat_mult.use_experimental_cfg_blocks = True sdfg = mat_mult.to_sdfg() ``` The Fortran frontend similarly only utilizes `LoopRegions` if an additional parameter `use_experimenatl_cfg_blocks` is passed to the parser together with the program. Many passes and transformations (including in simplify) do not yet have the capability of handling the new, hierarchical SDFGs. To not break the pipeline and to provide backwards compatibility, a new decorator `@single_level_sdfg_only` has been added, which can be (and has been) placed over any pass or transformation that is not compatible with the new style SDFGs. Passes annotated with this decorator are skipped in all pipelines where they occur and instead generate warnings that they were skipped due to compatibility issues. For more information on `LoopRegion`s please refer to the [PR that introduced them](#1407). **Important Note about disabled tests:** Certain Python frontend loop tests have been disabled. Specifically, this concerns tests where either the loop structure (using continue/break) or other conditional statements cause the generation of control flow that looks irregular before the simplification pass is applied. The reason being that the frontend generates branches with one branch condition being set to constant `False` when generating continue / break / return, or while/for-else clauses. These branches are trivially removed during simplification, but not running simplification (as part of our CI does) leads to irregular control flow which is handled poorly by our codegen-time control flow detection. This error has so far gone unnoticed in these tests because of sheer luck, but is now exposed through a ever so slightly different state machine being generated by control flow region and loop inlining. The goal is for a subsequent PR to completely adapt codegen to make use of the control flow region constructs, thereby fixing this issue and re-enabling the tests. For more information about the issue, see #635 and #1586. Linked to: https://github.com/orgs/spcl/projects/10/views/4?pane=issue&itemId=42047238 and https://github.com/orgs/spcl/projects/10/views/4?pane=issue&itemId=42151188
This PR adds
LoopRegion
s to SDFGs. This forms the second core element of the plan to make loops first class citizens of SDFGs.LoopRegion
s are a special class ofControlFlowRegion
s that represent different types of loops, meaning the control flow region inside them is executed a parametric number of times.A
LoopRegion
must have a conditional expression that determines whether the region is executed or not. It may additionally have:break
in C/Python)continue
in C/Python)For more general information on control flow regions please refer to the documentation or the PR that introduced them.
An example of a tripple loop nest of regular for loops can be seen in the GEMM program below, showing that a proof-of-concept visualization for the introduced concepts is already available in the latest release version of the VSCode extension (version 1.6.0 and upwards):
As outlined by the project plan, these
LoopRegion
s are currently not being used by any frontend. They can, however, already be manually used through the SDFG builder API. According to plan, most passes and transformations are not currently able to handle the use of suchLoopRegion
s yet, so their use is still highly experimental. To allow traditional transformations and passes to work, as well as to be able to generate codes for SDFGs containingLoopRegion
s, a compatibility pass (InlineLoopRegions
) is available, together with a utility functiondace.sdfg.utils.inline_loop_blocks
that inlines anyLoopRegion
s to traditional state machine loops.In summary, the PR includes:
LoopRegion
s to SDFGs, a special control flow region to represent loops