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

Fix bug in map_fusion transformation #1553

Merged
merged 5 commits into from
Mar 26, 2024
Merged

Conversation

edopao
Copy link
Collaborator

@edopao edopao commented Mar 20, 2024

Four-lines bugfix and associated test case for map_fusion transformation.

Without this change, the test would fail in SDFG validation with error:
dace.sdfg.validation.InvalidSDFGEdgeError: Memlet data does not match source or destination data nodes) (at state state, edge __s0_n1None_n3IN_T[0] (V:None -> numeric:_inp))

@edopao edopao marked this pull request as ready for review March 21, 2024 10:37
@edopao edopao requested a review from alexnick83 March 21, 2024 10:56
@edopao
Copy link
Collaborator Author

edopao commented Mar 22, 2024

@alexnick83 I can give you some context here. The problem addressed in this PR is different from the problem addressed in #1535. The problem addressed here blocks a delivery on gt4py main branch (for reference GridTools/gt4py#1502), which simplifies the icon4py SDFGs.

Copy link
Contributor

@alexnick83 alexnick83 left a comment

Choose a reason for hiding this comment

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

The fix looks correct for your use case, but I am unsure if it will always lead to correct results due to how it is applied. I explain in more detail in my comment. The fact that this bug exists in the first place shows that we just don't have enough tests for a lot of different cases regarding MapFusion. Therefore, I suggest that you limit the applicability of your fix until we can study related issues in more detail. Please find an example in my comment and let me know if it is something you can work with. If it is urgent, I can check again on short notice.

dace/transformation/dataflow/map_fusion.py Outdated Show resolved Hide resolved
@edopao
Copy link
Collaborator Author

edopao commented Mar 25, 2024

Thank you @alexnick83 for reviewing this PR. I have limited the application of this fix to the known valid case, as you suggested.
However, I skipped this line:
if current_data is not None and current_data not in test_data and list(edge.data.subset) == [0]:
because it is always true: the current_data is set at line 463-464.

@edopao edopao requested a review from alexnick83 March 25, 2024 08:10
@alexnick83 alexnick83 added this pull request to the merge queue Mar 26, 2024
Merged via the queue into spcl:master with commit d0db188 Mar 26, 2024
11 checks passed
@edopao edopao deleted the fix-map_fusion branch March 26, 2024 13:13
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.

2 participants