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

Fixing a (likely) bug in MapFusion. #1673

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pratyai
Copy link
Collaborator

@pratyai pratyai commented Oct 4, 2024

When checking for the "array usage" criteria that can prevent map-fusion, check only within the current state. Otherwise, any "use" of the array globally (i.e., in the entire SDFG) will cancel the fusion.

map-fusion, check only within the current state. Otherwise, any "use" of
the array _globally_ (i.e., in the entire SDFG) will cancel the fusion.
@tbennun
Copy link
Collaborator

tbennun commented Oct 5, 2024

This should not be the correct behavior. Eliminating an access node that corresponds to a data container has to be done safely: if a data container is used in another state (via an access node with the same name), its memory must be preserved because it can be reused in a subsequent read (which is a case that happens often when conserving memory). If you want map fusion to work, you would need to use a unique array name that can be removed, or rename arrays that you know they will be overwritten later.

Checking the SDFG for other instances of the access node is the correct way to check for a safe removal of an access node:

>>> sdfg = dace.SDFG('test')
>>> state = sdfg.add_state()
>>> state2 = sdfg.add_state_after(state)
>>> state.add_access('A')
>>> state2.add_access('A')
# Checking access nodes:
>>> state.data_nodes()
[AccessNode (A)]
>>> sdfg.data_nodes()
[AccessNode (A), AccessNode (A)]

@pratyai
Copy link
Collaborator Author

pratyai commented Oct 5, 2024

I see that my test case (of applying fusion twice) has this problem: the corresponding transient access nodes in two copies have the same name. I was assuming that the transient access nodes are scoped only within the state, and states cannot rely on other states' transient access nodes.
Just to confirm, must the transient access nodes have globally unique names?

Even then, I think that MapFusion's current way sometimes unnecessarily chooses to avoid the fusion. Could we just not remove such access nodes and just proceed with the fusion where both versions would produce identical output? In other words, what if we allow the fusion without any access-node-removal, and then remove any redundant transient nodes?

[ By the way, I've been looking at this since only very recently, so chances are that I have some misunderstanding about access nodes. Apologies in advance, if that's indeed the case here. ]

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