dace icon indicating copy to clipboard operation
dace copied to clipboard

Add state replication and if raising transformations

Open luigifusco opened this issue 1 year ago • 6 comments

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

luigifusco avatar Sep 05 '24 11:09 luigifusco

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.

phschaad avatar Sep 11 '24 07:09 phschaad

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

luigifusco avatar Sep 11 '24 07:09 luigifusco

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.

pratyai avatar Oct 29 '24 15:10 pratyai

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 https://github.com/spcl/dace/pull/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 avatar Oct 29 '24 17:10 phschaad

Leaving a note to revisit when https://github.com/spcl/dace/pull/1676 is merged.

phschaad avatar Nov 25 '24 09:11 phschaad

@acalotoiu ping for re-review.

phschaad avatar Nov 25 '24 09:11 phschaad