Deterministic dace toolchain draft#17
Deterministic dace toolchain draft#17tehrengruber wants to merge 2 commits intobranch_gt4py-next-integration_2026_02_12from
Conversation
philip-paul-mueller
left a comment
philip-paul-mueller
left a comment
There was a problem hiding this comment.
This is a first high level review of the changes.
| self._dst = dst | ||
| self._data: T = data | ||
|
|
||
| def id(self): |
There was a problem hiding this comment.
There are several things here.
First of all this type of edge is only for edges between states or to be precise control flow regions.
Inside a state, there is a second kind of graph, the dataflow graph.
down in the file you will find the MultiEdge (which is probably irrelevant) and the MultiConnectorEdge (which is the relevant), which are used for the dataflow graph.
In addition these other kind of edges are actually missing the id() function.
Then, you have added the id property to the Node, however, this is only the base class for the the nodes of the dataflow graph.
Thus, calling this function will fail, because the nodes, which are control flow regions do not have that attribute.
You must add them either to AbstractControlFlowRegion or what is probably better to ControlFlowBlock, but in that regard I am not fully sure.
Furthermore, two nodes can have multiple connections between them.
This is more relevant for the dataflow graph, however, it is technically still allowed for the state graph, although less relevant.
Thus, edge.id() is not an unique key, as there could be several edges between any two nodes.
To make them unique, you need to include the .data property, which is either a Memlet or an InterstateEdge.
| @@ -215,7 +218,7 @@ def __getitem__(self, node: NodeT) -> Iterable[NodeT]: | |||
|
|
|||
| def all_edges(self, *nodes: NodeT) -> Iterable[Edge[EdgeT]]: | |||
There was a problem hiding this comment.
| def all_edges(self, *nodes: NodeT) -> Iterable[Edge[EdgeT]]: | |
| def all_edges(self, *nodes: NodeT) -> List[Edge[EdgeT]]: |
| return current | ||
|
|
||
|
|
||
| # TODO: check if the edges need an id. If source and dest, are equal they should be interchangable right? |
There was a problem hiding this comment.
No, see my comment above.
Essentially the code:
a[3] = b[4]
a[6] = b[5]Will lead to an SDFG with two nodes one for a and one for b and two edges between them, each does an assignment.
There are similar situations with Maps.
| "type": typestr, | ||
| "label": labelstr, | ||
| "attributes": dace.serialize.all_properties_to_json(self), | ||
| # TODO(tehrengruber): This id looks very similar to the ID I introduced. |
There was a problem hiding this comment.
This is not stable it is essentially the position in the sorted node array, see here.
This design has some very nasty consequences in the way how transformations have to be implemented.
This is the transformation why your transformations look like:
def apply():
matched_node = self.pattern_node
# Never use `self.pattern_node` from here always use `matched_node` and pass it to all functions!
| ############################## | ||
|
|
||
| # TODO(tehrengruber): This could also be done using the counter. | ||
| def _find_new_name(self, name: str): |
There was a problem hiding this comment.
Yes it would probably better, because not all symbol (names) are stored in self.symbols, for example the symbols used as Map parameters are not inside it.
| """ | ||
| return set() | ||
|
|
||
| # TODO(tehrengruber): should we make this an ordered set? |
There was a problem hiding this comment.
I am actually in favour of that, however, there are some issues.
This function exists because SymPy had it and a lot of other classes have such a method (although it is most likely implemented by used_symbols()).
Thus you will have to change them all.
Furthermore, while here the return type is set[str] this is actually wrong.
Some objects, especially the ones that are very close to SymPy, return a set of dace.symbolic.symbol (which extends (but does not wrap) SymPy's symbol) and sometimes you even get a mixture of them.
So this is a big set of work.
| block_reach: Dict[int, Dict[ControlFlowBlock, OrderedSet[ControlFlowBlock]]]) -> Set[SDFGState]: | ||
| closure: Set[SDFGState] = OrderedSet() |
There was a problem hiding this comment.
| block_reach: Dict[int, Dict[ControlFlowBlock, OrderedSet[ControlFlowBlock]]]) -> Set[SDFGState]: | |
| closure: Set[SDFGState] = OrderedSet() | |
| block_reach: Dict[int, Dict[ControlFlowBlock, OrderedSet[ControlFlowBlock]]]) -> OrderedSet[SDFGState]: | |
| closure: OrderedSet[SDFGState] = OrderedSet() |
| """ | ||
| single_level_reachable: Dict[int, Dict[ControlFlowBlock, | ||
| Set[ControlFlowBlock]]] = defaultdict(lambda: defaultdict(set)) | ||
| OrderedSet[ControlFlowBlock]]] = defaultdict(lambda: defaultdict(set)) |
There was a problem hiding this comment.
| OrderedSet[ControlFlowBlock]]] = defaultdict(lambda: defaultdict(set)) | |
| OrderedSet[ControlFlowBlock]]] = defaultdict(lambda: defaultdict(OrderedSet)) |
| # By definition, data that is referenced by the conditions (branching condition, | ||
| # loop condition, ...) is not single use data, also remove that. | ||
| for cfr in sdfg.all_control_flow_regions(): | ||
| single_use_data.difference_update(cfr.used_symbols(all_symbols=True, with_contents=False)) |
There was a problem hiding this comment.
used_symbols() also returns a set.
| array_name = an.data | ||
| write_subsets = set(e.data.dst_subset for e in st.in_edges(an)) | ||
| write_subsets = OrderedSet(e.data.dst_subset for e in st.in_edges(an)) | ||
| wss = str(write_subsets) |
There was a problem hiding this comment.
I know outside of the scope of this PR, but serializing a set and then use it as keys does not make sense to me.
2 participants
Just a basis for further discussions.