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

Updated MultistateInlining #1980

Merged
merged 17 commits into from
Mar 21, 2025

Conversation

philip-paul-mueller
Copy link
Collaborator

This PR addresses issue#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 philip-paul-mueller/gt4py that referenced this pull request Mar 19, 2025
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.
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.

This looks really nice and useful! Good idea with the three states! Thank you!

Copy link
Collaborator

@phschaad phschaad left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Should be orthogonal to my fixes too mostly

@phschaad
Copy link
Collaborator

Weird failing test - can that be reproduced locally? It seems like a possible missing reshape / view when inlining, but seems a bit strange that only one test fails then.

@philip-paul-mueller philip-paul-mueller added this pull request to the merge queue Mar 20, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 20, 2025
@philip-paul-mueller philip-paul-mueller added this pull request to the merge queue Mar 20, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 20, 2025
@philip-paul-mueller philip-paul-mueller added this pull request to the merge queue Mar 20, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 20, 2025
@phschaad phschaad added this pull request to the merge queue Mar 21, 2025
Merged via the queue into spcl:main with commit 4245396 Mar 21, 2025
10 of 11 checks passed
philip-paul-mueller added a commit to GridTools/gt4py that referenced this pull request Mar 24, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants