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 state replication and if raising transformations #1639

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

luigifusco
Copy link
Contributor

This PR adds the StateReplication and IfRaising transformations.
StateReplication performs a normalization step. It takes a state with n incoming edges and creates an equivalent SDFG with n replicas of the original state, each with a single incoming edge.

IfRaising takes an if guard and 'raises' the evaluation of the conditional. The condition needs to be independent of the computations done in the if guard. The if guard is replicated in each branch and the conditional is evaluated earlier in a new empty state.

The combination of these two transformations unlocks extra dataflow in the case of "diamond" patterns that can be found in some weather codes. In particular, it targets the case where some flag changes what computation is performed.

Example before:
image
After:
image
After State, Map, and Tasklet fusion:
image

return False

# make sure this is not a loop guard
if len(out_edges) == 2:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Application on loops results in the addition of useless states (but the SDFG is still correct). This will not get rid of the loop, meaning that apply_transformations_repeated will never halt.

if if_guard.is_empty():
return False

# check that condition does not depend on computations in the state
Copy link
Contributor Author

Choose a reason for hiding this comment

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

might be missing something I do not know about here. Also I don't know if this can result in spurious failures

Copy link
Contributor

@acalotoiu acalotoiu left a comment

Choose a reason for hiding this comment

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

LGTM, however, this is missing a couple of "negative" tests. Please add tests where the expected behavior is that the transformation is not applied.

@phschaad
Copy link
Collaborator

Apart from @acalotoiu 's comment, this looks good to me. However, I want to add: I believe that the presence of more and more niche transformations like this should make us prioritize the idea of a 'community transformation repository' a bit more.

@luigifusco
Copy link
Contributor Author

Apart from @acalotoiu 's comment, this looks good to me. However, I want to add: I believe that the presence of more and more niche transformations like this should make us prioritize the idea of a 'community transformation repository' a bit more.

I really see this as a member of a potential set of "transformation building blocks", towards having composable transformations. This also lies in between a utility method (e.g. in the SDFG class) and an actual transformation

sdfg.apply_transformations_repeated([OTFMapFusion])

assert len(sdfg.nodes()) == 4
assert sdfg.start_state.is_empty()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the tests fails here but it works on my machine. Is there something specific to the setup in the pipeline?

@pratyai
Copy link
Collaborator

pratyai commented Oct 29, 2024

Added one "negative test" (besides the existing one) where the expected behaviour is that the transformation is not applied. Gave each of the two transformations its own test file. PTAL.

@phschaad
Copy link
Collaborator

phschaad commented Oct 29, 2024

Thank you for addressing the comments @pratyai! In general, this looks good to me. However, I would prefer if this PR could be part of a brief discussion in the DaCe weekly this week before it is merged. For that reason I am not pressing 'accept' yet. I would prefer to discuss it due to two key points:

  1. Preparation for release 1.0 which is currently underway for the next few days
  2. Potential duplication of work for when the Complete Transition to Control Flow Regions #1676 is merged, as that should change the pattern the transformation matches from an if-guard to a conditional block and consequently slightly changes the raising being performed.

@phschaad phschaad added question Further information is requested 2.0 labels Oct 29, 2024
@phschaad
Copy link
Collaborator

Leaving a note to revisit when #1676 is merged.

@phschaad
Copy link
Collaborator

@acalotoiu ping for re-review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants