fix[dace][next]: Fix for DistributedBufferRelocator #1799
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The bug was because the `DistributedBufferRelocator` transformation did not check if its insertion would create a read-write conflict. This commit adds such a check, that is, however, not very sophisticated and needs some improvements. However, the example /`model/atmosphere/dycore/tests/dycore_stencil_tests/test_compute_exner_from_rhotheta.py`) where it surfaced, does hold more challenges. The main purpose of this commit is to unblock further development in ICON4Py. Link to ICON4Py PR: C2SM/icon4py#638
edopao
reviewed
Jan 15, 2025
| ) -> bool: | ||
| """Tests if read-write conflict would be created for a single location. | ||
|
|
||
| Args: |
There was a problem hiding this comment.
Maybe also add the Returns section to make clear what True means.
There was a problem hiding this comment.
Good idea.
Actually when I wrote the thing I was also confused.
| target_locations: Location where the new write back should be performed. | ||
|
|
||
| Todo: | ||
| Refine this checks later. |
There was a problem hiding this comment.
Suggested change
| Refine this checks later. | |
| Refine this check later. |
There was a problem hiding this comment.
There is a grammar but the function actually performs two different checks, so correct would be these.
src/gt4py/next/program_processors/runners/dace_fieldview/transformations/simplify.py
Outdated
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/transformations/simplify.py
Show resolved
Hide resolved
| state_to_inspect: dace.SDFGState = target_location[1] | ||
|
|
||
| # This is the access not on which we will perform the write back. | ||
| def_location_of_intermediate: dace_nodes.AccessNode = target_location[0] |
There was a problem hiding this comment.
I imagine you do not like this:
def_location_of_intermediate, state_to_inspect = *target_location
There was a problem hiding this comment.
Technically I am not a fan of it, but here it actually makes sense.
src/gt4py/next/program_processors/runners/dace_fieldview/transformations/simplify.py
Outdated
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/transformations/simplify.py
Outdated
Show resolved
Hide resolved
| if len(access_to_global_data_in_this_state) == 0: | ||
| return False | ||
|
|
||
| # For the second test we look if `global_data_name` is referred to in |
There was a problem hiding this comment.
This second case should never appear in the original SDFG, from the lowering. Can it be an artifact of other transformations?
There was a problem hiding this comment.
Actually the Exner test case produces this kind of SDFGs.
This was actually also the reason why it is implemented.
But you are right, it is very unlikely that they appear but we should keep them.
|
Please merge the latest main to avoid the failure in cartesian tests. |
It is now refined and also explcitly checks side streams. It was actually doing that the whole time, just the description was wrong.
philip-paul-mueller
added a commit
to philip-paul-mueller/gt4py
that referenced
this pull request
Jan 22, 2025
…rved in ICON4Py's `TestUpdateThetaAndExner` test. In essence there was an `assert` that assumed that checked if this temporary was a sink node. but, the code that finds all write backs was never excluding such cases, i.e. the temporaries that were selected might not be sink nodes in the state where they are defined. The `assert` was not part of the original implementation and is not a requirement of the transformation, instead it was introduced by [PR#1799](GridTools#1799), that fixed some issues in the analysis of read write dependencies. There are two solutions for this, either removing the `assert` or prune these kinds of temporaries. After some consideration, it was realized that handling such cases will not lead to invalid SDFG, as long as the other restrictions on the global data are respected. For that reason the `assert` was removed. However, we should thinking of doing something more intelligent in that case.
philip-paul-mueller
added a commit
that referenced
this pull request
Jan 22, 2025
This PR fixes a bug in `DistributedBufferRelocator` that was observed in ICON4Py's `TestUpdateThetaAndExner` test. In essence there was an `assert` that assumed that checked if this temporary was a sink node, but, the code that finds all write backs was never excluding such cases, i.e. the temporaries that were selected might not be sink nodes in the state where they are defined. The `assert` was not part of the original implementation and is not a requirement of the transformation, instead it was introduced by [PR#1799](#1799), that fixed some issues in the analysis of read write dependencies. There are two solutions for this, either removing the `assert` or prune these kinds of temporaries. After some consideration, it was realized that handling such cases will not lead to invalid SDFG, as long as the other restrictions on the global data are respected. For that reason the `assert` was removed. However, we should thinking of doing something more intelligent in that case.
edopao
pushed a commit
to edopao/gt4py
that referenced
this pull request
Jan 22, 2025
This PR fixes a bug in `DistributedBufferRelocator` that was observed in ICON4Py's `TestUpdateThetaAndExner` test. In essence there was an `assert` that assumed that checked if this temporary was a sink node, but, the code that finds all write backs was never excluding such cases, i.e. the temporaries that were selected might not be sink nodes in the state where they are defined. The `assert` was not part of the original implementation and is not a requirement of the transformation, instead it was introduced by [PR#1799](GridTools#1799), that fixed some issues in the analysis of read write dependencies. There are two solutions for this, either removing the `assert` or prune these kinds of temporaries. After some consideration, it was realized that handling such cases will not lead to invalid SDFG, as long as the other restrictions on the global data are respected. For that reason the `assert` was removed. However, we should thinking of doing something more intelligent in that case.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes an error that was reported by Edoardo (@edopao).
The bug was because the
DistributedBufferRelocatortransformation did not check if its insertion would create a read-write conflict. This commit adds such a check, that is, however, not very sophisticated and needs some improvements. However, the example /model/atmosphere/dycore/tests/dycore_stencil_tests/test_compute_exner_from_rhotheta.py) where it surfaced, does hold more challenges. The main purpose of this PR is to unblock further development in ICON4Py.Link to ICON4Py PR: C2SM/icon4py#638