Fixed RedundantArray's handling of "reshaping" Memlets
#1603
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
philip-paul-mueller
force-pushed
the
fixed_redundant_array
branch
from
e3f6cfe
to
59120ae
Compare
I also verified that without the fix the test will fail.
philip-paul-mueller
force-pushed
the
fixed_redundant_array
branch
from
8e4ca85
to
7ea9a62
Compare
It essentially creates a special case for it and applies then the correct way. This is not very good but it works and I have no better solution.
This is for investigate why the CI fails. After this commit all tests except the one I had added should pass. And NOAA which should also fail. This reverts commit 742568d.
This reverts commit 8ec3c58.
|
@philip-paul-mueller please rename the title, this is unclear. Even "Fix for issue #1595" is a better name. |
philip-paul-mueller
changed the title
RedundantArray's handling of "reshaping" Memlets
alexnick83
requested changes
Jul 3, 2024
There was a problem hiding this comment.
What happens if the first/input array has multiple incoming edges? It looks like the transformation will apply as before your fix and produce a similar error. You don't need to make the transformation work on such corner cases, but maybe it should not match those cases in the first place. How about refactoring the new login into a separate helper method and calling it also in can_be_applied?
|
I like that idea. |
… handles the reshaping case but simply ignore it. I put everything inside a helper function.
In the previous versions I tried to simply ignore the reshaping Memlet case, however, this break other tests. In this version reshaping Memlets are not turned into a view, i.e. the source array is not removed but turned into a view. This fixed the other tests again. It is not the best solution but it works.
alexnick83
approved these changes
Jul 3, 2024
A reshaping memlet must change the shape of a datacontainer. However, in the old version also the total size was considered. Which is not fully correct, as the total size is more related to the memory layout and not to the "computational domain".
philip-paul-mueller
added a commit
to philip-paul-mueller/jace
that referenced
this pull request
Sep 25, 2024
I enabled the simplify pass in commit `411bd7bd` and it worked locally. However, this was because I was not running it inside nox and using my own version of DaCe. The bug in simplify was fixed in [PR#1603](spcl/dace#1603) which was merged _after_ 16.1 was released, thus the fix is not avaliable.
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 issue that was reported as issue 1595, that was traced back to
RedundantArray.The commit adds a deterministic test, unlike the one in the issue, that fails without the fix.
The underlying problem is, that the transformation does not correctly handle a Memlet that performs a reshaping.
This commit does not really solves the issue, instead it adds a special case for this particular case and then handles it correctly as I was unable to modify the code to handle it correctly.
It is not a nice solution, but it works.