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.

Sign up for GitHub

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

Jump to bottom

InlineMultistateSDFG Destroys SSA Invariant #1959

Open
philip-paul-mueller opened this issue Mar 3, 2025 · 0 comments
Open

InlineMultistateSDFG Destroys SSA Invariant #1959

philip-paul-mueller opened this issue Mar 3, 2025 · 0 comments

Comments

@philip-paul-mueller
Copy link
Collaborator

This is not a bug per se, as the resulting SDFGs are correct, however, the InlineMultistateSDFG transformation destroys the SSA invariant of SDFGs, which is a less than optimal behaviour.

Consider the following SDFG:

Image

The first nested SDFG writes into T1 which is then written into T3[0:a], the second nested SDFG writes its output into T2 which is then copied into T3[a:N].
T3 is then finally written into the global memory G.
As you can see al transients are written to exactly once, for certain reason (I am looking at you FORTRAN) we can not enforce this on global data, but this is not the issue here.

After we run InlineMultistateSDFG we get the following SDFG:

Image

As you can see now the writing of the output, i.e. {(T1), (T2)} -> (T3) -> (G) is now split and put into different states.
Thus the transients are no longer SSA.

Since SSA is a very nice and useful property I would think/expect/hope that the transformation would maintain this invariant.
Thus from this perspective it is a bug.
philip-paul-mueller added a commit to philip-paul-mueller/gt4py that referenced this issue Mar 4, 2025
@philip-paul-mueller
This commit mitigates an issue with DaCe's InlineSDFGs pass.
fff8498 
The DaCe issue is described [here](spcl/dace#1959), essentially running `InlineMultistateSDFG` destroys the SSA properties, i.e. that in any control path there is only one write to a transient.
An example for this can be found in `TestFusedSolveNonhydroStencil39To40` an ICON4Py test.
It was noticed that `InlineSDFG`, which only handles nested SDFGs with a single state is not affected (because it does not create additional states).
The fix, which has to be revisited later, is to run the `InlineSDFG` transformation.
Note that we still allow and perform multi state inlining, because we need it.
Furthermore, another help is once we have the chain remover for `concat_where`.
@philip-paul-mueller philip-paul-mueller mentioned this issue Mar 4, 2025
Merged
philip-paul-mueller added a commit to GridTools/gt4py that referenced this issue Mar 10, 2025
@philip-paul-mueller @edopao
feat[dace][next]: Added CopyChainRemover (#1901)
2f4db72 
This PR adds the `CopyChainRemover` transformation.

The introduction of `concat_where` introduced a new pattern, which is
essentially a chain of copies.
As an example imagine the case that a domain is split into 3 subdomain.
The result on the first subdomain is stored in `T1`, the one of the
second in `T2` and the one for the third domain in `T3`.
`T1` and `T2` are then copied into `T4`, finally `T4` together with `T3`
are then copied into `T5`.
This transformation will remove `T1`, `T2`, `T3` and `T4` thus the
results will be written into `T5` directly.

There are some limitation, if we have the pattern `(A1) -> (A2)`, then
we eliminate `A1` only if:
- `A1` is fully read; this is to avoid some nasty adjustments of map
bounds.
- There can only be one connection between `A1` and `A2`.

The transformation was added twice to the simplify pass, which allows us
to mitigate DaCe [issue#1959](spcl/dace#1959).

---------

Co-authored-by: edopao <edoardo16@gmail.com>
@philip-paul-mueller philip-paul-mueller mentioned this issue Mar 19, 2025
Merged
philip-paul-mueller added a commit to philip-paul-mueller/gt4py that referenced this issue Mar 19, 2025
@philip-paul-mueller
Removed the workaround for DaCe issue 1959.
f6ef897 
This [issue](spcl/dace#1959) leaded to some problem, especially in ICON4Py stencil `39_to_40`.
However, through an [update](GridTools#1915) in GT4Py this leads to now to other problems (`ConstantPropagation` involving AcccessNodes) in `19_to_20`.
The underlying issue in DaCe was solved in [PR#1980](spcl/dace#1980) and this PR now deactivate the workaround.

Only merge if the DaCe dependency in GT4Py was updated to include that fix.
@philip-paul-mueller philip-paul-mueller mentioned this issue Mar 19, 2025
Merged
github-merge-queue bot pushed a commit that referenced this issue Mar 21, 2025
@philip-paul-mueller
Updated MultistateInlining (#1980)
4245396 
This PR addresses
[issue#1959](#1959), the issue was
about that the inlining transformation destroys the SSA invariant we
maintain inside GT4Py.
This invariant states that in every code path there is exactly one
AccessNode with that is used to write to a data container.

The PR solves this issue by modifying how the nested SDFG is isolated,
before this was done in two steps and is now performed in a single step.
The main idea is that the number of writes is preserved.
philip-paul-mueller added a commit to GridTools/gt4py that referenced this issue Mar 24, 2025
@philip-paul-mueller
fix[next][dace]: Removed Workaround For DaCe Issue 1959 (#1921)
701fc01 
This [issue](spcl/dace#1959) leaded to some
problem, especially in ICON4Py stencil `39_to_40`. However, through an
[update](#1915) in GT4Py this
leads to now to other problems (`ConstantPropagation` involving
AcccessNodes) in `19_to_20`. The underlying issue in DaCe was solved in
[PR#1980](spcl/dace#1980) and this PR now
deactivate the workaround.

It also updates the version of DaCe.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@philip-paul-mueller