Skip to content
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 loop regions to the frontend's capabilities #1475

Merged
merged 77 commits into from
Jun 26, 2024
Merged

Conversation

phschaad
Copy link
Collaborator

@phschaad phschaad commented Dec 11, 2023

This PR lets the Python and Fortran frontends (optionally) generate LoopRegions for DaCe programs. This forms the third core element of the plan to make loops first class citizens of SDFGs.

This PR is fully backwards compatible. LoopRegions 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 LoopRegions 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:

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 LoopRegions please refer to the PR that introduced them.

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

@phschaad phschaad added enhancement New feature or request frontend core labels Dec 11, 2023
@phschaad phschaad marked this pull request as ready for review December 11, 2023 19:21
@phschaad phschaad marked this pull request as draft December 11, 2023 19:26
@phschaad phschaad linked an issue Jun 13, 2024 that may be closed by this pull request
Copy link
Collaborator

@tbennun tbennun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet, need to discuss. Please answer questions in the meantime

dace/frontend/python/newast.py Outdated Show resolved Hide resolved
dace/frontend/python/newast.py Show resolved Hide resolved
dace/frontend/python/parser.py Show resolved Hide resolved
dace/sdfg/state.py Outdated Show resolved Hide resolved
dace/sdfg/state.py Outdated Show resolved Hide resolved
dace/transformation/interstate/fpga_transform_sdfg.py Outdated Show resolved Hide resolved
tests/python_frontend/loop_regions_test.py Outdated Show resolved Hide resolved
tests/python_frontend/loops_test.py Show resolved Hide resolved
tests/state_propagation_test.py Show resolved Hide resolved
tests/transformations/loop_to_map_test.py Outdated Show resolved Hide resolved
@phschaad phschaad requested a review from tbennun June 19, 2024 07:29
Copy link
Collaborator

@tbennun tbennun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make minor changes and feel free to merge.

@@ -303,7 +308,7 @@ def replace(self, name: str, new_name: str):
:param name: Name to find.
:param new_name: Name to replace.
"""
raise NotImplementedError()
pass

@abc.abstractmethod
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They may have empty contents bur are still @abc.abstractmethods. What was the intention?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially the idea was to never instantiate a ControlFlowBlock - only subclasses of it, each of which being a graph (States and Control flow regions). However, with the change to use break/return/continue blocks, we now have blocks being used which should NOT contain further elements - so are no longer a graph. That's why these methods now all of a sudden need to not throw NotImplementedErrors. Or rather, they could be left as throwing those errors, but then at each point where we iterate over nodes of a graph we would need to make sure to not try and iterate into a control flow block, only into states and regions etc.

So that's the background - and now why I left the abstractmethod decorator: It should still be the case that you never instantiate a BlockGraphView directly and always explicitly subclass it. In 90% of use cases, that would mean implementing these methods here, since you would usually be making a graph type object.

dace/sdfg/state.py Outdated Show resolved Hide resolved
dace/transformation/dataflow/map_for_loop.py Show resolved Hide resolved
dace/transformation/pass_pipeline.py Outdated Show resolved Hide resolved
dace/transformation/pass_pipeline.py Outdated Show resolved Hide resolved
dace/transformation/passes/pattern_matching.py Outdated Show resolved Hide resolved
dace/transformation/passes/pattern_matching.py Outdated Show resolved Hide resolved
dace/transformation/passes/pattern_matching.py Outdated Show resolved Hide resolved
dace/transformation/passes/pattern_matching.py Outdated Show resolved Hide resolved
dace/transformation/passes/simplify.py Outdated Show resolved Hide resolved
- Improve CFG incompatibility warning message
- Adhere to LLVM loop terminology (tail -> latch)
- Minor type safety improvements
@phschaad phschaad enabled auto-merge June 26, 2024 08:23
@phschaad phschaad added this pull request to the merge queue Jun 26, 2024
Merged via the queue into master with commit ecae262 Jun 26, 2024
10 checks passed
@phschaad phschaad deleted the loop_architecture_pt_3 branch June 26, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core enhancement New feature or request frontend
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Simplify raises an exception on LoopRegion
3 participants