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

Cutout Improvements and Transformation Affected Nodes API #1079

Closed
wants to merge 11 commits into from

Conversation

phschaad
Copy link
Collaborator

@phschaad phschaad commented Aug 4, 2022

Multistate Cutouts

Allow cutouts from SDFG where the cutout consists of one or more entire states that form a coherent, self-contained SDFG. In practice, that means that there is a single distinct entry state to the cutout's state machine. This mode of constructing cutouts will always create a copy of states and does not contain a reference-cutout mode like in-state cutouts, since states hold a back reference to the SDFG they're contained in and that reference must point to the new cutout SDFG for the cutout to validate.

Transformation Affected Nodes API

This gives transformations a get_affected_nodes API method where the set of nodes touched by the transformation is returned. For most pattern transformations, this set of nodes is equivalent to the matched pattern nodes. However, some transformations (like MapToLoop) may modify additional nodes (or states) that are NOT matched by the pattern nodes, like loop bodies. This makes it impossible for anything other than the transformation itself to report what nodes are affected. With this API call, a transformation author can (and must) provide this information transparently iff the set of affected nodes diverges from the patched pattern nodes. If not overridden, the standard pattern node matches are used.

"Alibi Nodes" in Cutouts

If a cutout inside of a scope region borders one of the scope nodes (exit or entry), the cutout attempts to cross the scope via each memlet path to connect the corresponding data containers. In the example image below, a cutout of the tasklet prog_39 would result in the cutout procedure attempting to add the map exit and the access node to C to the cutout. Having only the map exit in the cutout, but not the entry trivially leads to a fault. Including the entry node would require including the entire content of the scope. Both these solutions are not what the cutout should intuitively contain.

image

To address this, this PR adds "Alibi Nodes" to cutouts that cross scope borders. What this means, is that in the example above, the access across the map exit border is detected and instead of inserting both the exit node and the outside access node to the cutout, the outgoing memlet from prog_39 is analyzed, and a new data container with a shape corresponding to the accessed subset is inserted into the cutout SDFG. The memlet is then attached to that new alibi data container instead. This further solves the problem that the memlet crossing the scope border may have symbols in the index term, which can only be defined by the map itself.

Miscellaneous Other Points

  • Changes the assignments property on InterstateEdges to a DictProperty (from a regular property with dtype=dict). This is necessary to get proper metadata which can be used to infer what type of information this property contains.

@phschaad phschaad changed the title Multistate cutouts and xform get_affected_nodes Multistate Cutouts and Transformation Affected Nodes API Aug 5, 2022
@phschaad phschaad changed the title Multistate Cutouts and Transformation Affected Nodes API Multistate Cutouts, Transformation Affected Nodes API, and Alibi Nodes in Cutouts Aug 13, 2022
@phschaad phschaad changed the title Multistate Cutouts, Transformation Affected Nodes API, and Alibi Nodes in Cutouts Cutout improvements and Transformation Affected Nodes API Aug 13, 2022
@phschaad phschaad changed the title Cutout improvements and Transformation Affected Nodes API Cutout Improvements and Transformation Affected Nodes API Aug 13, 2022
@phschaad phschaad removed the wip label Aug 13, 2022
@phschaad phschaad marked this pull request as ready for review August 13, 2022 07:28
@lukastruemper
Copy link
Contributor

Haven‘t looked into it yet, but we should add at least simple two positive tests for cutout. But no over-engineering, can do this after deadlines thoroughly.

@phschaad
Copy link
Collaborator Author

Agreed. Particularly multistate cutouts and alibi nodes themselves currently have no coverage. This has to be added, I'll take thosw two specific ones up asap after deadlines.

@lukastruemper
Copy link
Contributor

lukastruemper commented Aug 23, 2022

Some points from earlier discussions:

  • Alibi cutouts: I think it makes total sense to stay within the scope defined by the given nodes. Including temporary data containers makes total sense.
  • Affected nodes: I don't have a very strong opinion on it, but I think we should not introduce this concept in the same PR. Maybe we can separate this into a draft PR? Even if it is not harmful right now because it is not used within dace and defaults to something reasonable, we should have a broad agreement on it with more developers. It changes how we write transformations. Also how does this compare to "annotate_memlets"? Is the enclosing scope affected? Maybe the state/nested SDFG? I am thinking about a more general API. I see more applications for this: When I fuse two nodes, I don't know what happened to the nodes. Is it gone? Are both nodes gone? Did we create a new one that replaces another? This is quite crucial to know when you want to pin a sequence of transformations to a subgraph, you want to know how it is transformed like a trace

@lukastruemper
Copy link
Contributor

Some points from earlier discussions:

  • Alibi cutouts: I think it makes total sense to include to stay within the scope defined by the given nodes. Including temporary data containers makes total sense.
  • Affected nodes: I don't have a very strong opinion on it, but I think we should not introduce this concept in the same PR. Maybe we can separate this into a draft PR? Even if it is not harmful right now because it is not used within dace and defaults to something reasonable, we should have a broad agreement on it with more developers. It changes how we write transformations. Also how does this compare to "annotate_memlets"? Is the enclosing scope affected? Maybe the state/nested SDFG? I am thinking about a more general API. I see more applications for this: When I fuse two nodes, I don't know what happened to the nodes. Is it gone? Are both nodes gone? Did we create a new one that replaces another? This is quite crucial to know when you want to pin a sequence of transformations to a subgraph, you want to know how it is transformed like a trace

So maybe separate the PRs?

@lukastruemper
Copy link
Contributor

Some points from earlier discussions:

  • Alibi cutouts: I think it makes total sense to include to stay within the scope defined by the given nodes. Including temporary data containers makes total sense.
  • Affected nodes: I don't have a very strong opinion on it, but I think we should not introduce this concept in the same PR. Maybe we can separate this into a draft PR? Even if it is not harmful right now because it is not used within dace and defaults to something reasonable, we should have a broad agreement on it with more developers. It changes how we write transformations. Also how does this compare to "annotate_memlets"? Is the enclosing scope affected? Maybe the state/nested SDFG? I am thinking about a more general API. I see more applications for this: When I fuse two nodes, I don't know what happened to the nodes. Is it gone? Are both nodes gone? Did we create a new one that replaces another? This is quite crucial to know when you want to pin a sequence of transformations to a subgraph, you want to know how it is transformed like a trace

Sorry, but it would be nice to know what the graph translation of a transformation was in general, maybe we can actually solve this by some form of "graph changes instrumentation" instead of manual specification.

@phschaad phschaad marked this pull request as draft August 23, 2022 17:51
@phschaad phschaad removed the request for review from tbennun December 27, 2022 09:46
@phschaad phschaad removed the request for review from lukastruemper December 27, 2022 09:46
phschaad added a commit that referenced this pull request Feb 8, 2023
@phschaad phschaad mentioned this pull request Mar 1, 2023
phschaad added a commit that referenced this pull request Mar 1, 2023
@phschaad
Copy link
Collaborator Author

phschaad commented Mar 9, 2023

This has now been handled by #1201 and #1208. No longer relevant.

@phschaad phschaad closed this Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request transformations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants