Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions dace/sdfg/graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from dace.dtypes import deduplicate
import dace.serialize
from typing import Any, Callable, Generic, Iterable, List, Optional, Sequence, TypeVar, Union

from ordered_set import OrderedSet

class NodeNotFoundError(Exception):
pass
Expand All @@ -31,6 +31,9 @@ def __init__(self, src, dst, data: T):
self._dst = dst
self._data: T = data

def id(self):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

return (self._src.id, self._dst.id)

@property
def src(self):
return self._src
Expand Down Expand Up @@ -215,7 +218,7 @@ def __getitem__(self, node: NodeT) -> Iterable[NodeT]:

def all_edges(self, *nodes: NodeT) -> Iterable[Edge[EdgeT]]:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
def all_edges(self, *nodes: NodeT) -> Iterable[Edge[EdgeT]]:
def all_edges(self, *nodes: NodeT) -> List[Edge[EdgeT]]:

"""Returns an iterable to incoming and outgoing Edge objects."""
result = set()
result = OrderedSet()
for node in nodes:
result.update(self.in_edges(node))
result.update(self.out_edges(node))
Expand Down
47 changes: 44 additions & 3 deletions dace/sdfg/nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import ast
from copy import deepcopy as dcpy
from collections.abc import KeysView
import contextvars
import dace
import itertools
import dace.serialize
Expand All @@ -26,6 +27,34 @@

# -----------------------------------------------------------------------------

# Global context variable to store the node ID counter
_node_id_counter: contextvars.ContextVar[int] = contextvars.ContextVar('_node_id_counter', default=0)


def _get_next_node_id() -> int:
"""Get the next node ID and increment the counter."""
current = _node_id_counter.get()
_node_id_counter.set(current + 1)
return current


# TODO: check if the edges need an id. If source and dest, are equal they should be interchangable right?
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.


class reset_node_id_counter:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I am actually not a fan of resetting the values.
Because it makes it in principle possible that an ID is encountered multiple times.

However, since there is a legitimate use of it inside the tests I would propose to move it to the tests, such that it can not be used inside DaCe.

"""Context manager that resets the node ID counter to zero and restores the old value afterwards."""

def __init__(self):
self._old_value = None

def __enter__(self):
self._old_value = _node_id_counter.get()
_node_id_counter.set(0)
return self

def __exit__(self, exc_type, exc_val, exc_tb):
_node_id_counter.set(self._old_value)
return False


@make_properties
class Node(object):
Expand All @@ -38,6 +67,7 @@ class Node(object):
value_type=dtypes.typeclass,
desc="A set of output connectors for this node.")
guid = Property(dtype=str, allow_none=False)
id = Property(dtype=int, allow_none=False)

def __init__(self, in_connectors=None, out_connectors=None):
# Convert connectors to typed connectors with autodetect type
Expand All @@ -50,6 +80,7 @@ def __init__(self, in_connectors=None, out_connectors=None):
self.out_connectors = out_connectors or {}

self.guid = graph.generate_element_id(self)
self.id = _get_next_node_id()

def __str__(self):
if hasattr(self, 'label'):
Expand All @@ -62,9 +93,14 @@ def __deepcopy__(self, memo):
result = cls.__new__(cls)
memo[id(self)] = result
for k, v in self.__dict__.items():
if k == 'guid': # Skip ID
if k == 'guid': # Skip GUID
continue
if k == '_id': # Skip ID, will be assigned new one
continue
setattr(result, k, dcpy(v, memo))
# Assign new GUID and ID
result.guid = graph.generate_element_id(result)
result.id = _get_next_node_id()
Comment on lines +102 to +103
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why not move them into the loop?

return result

def validate(self, sdfg, state):
Expand Down Expand Up @@ -101,6 +137,7 @@ def to_json(self, parent):
"type": typestr,
"label": labelstr,
"attributes": dace.serialize.all_properties_to_json(self),
# TODO(tehrengruber): This id looks very similar to the ID I introduced.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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!

"id": parent.node_id(self),
"scope_entry": scope_entry_node,
"scope_exit": scope_exit_node
Expand Down Expand Up @@ -312,6 +349,7 @@ def __deepcopy__(self, memo):
node._debuginfo = dcpy(self._debuginfo, memo=memo)

node._guid = graph.generate_element_id(node)
node._id = _get_next_node_id()

return node

Expand Down Expand Up @@ -644,10 +682,13 @@ def __deepcopy__(self, memo):
result = cls.__new__(cls)
memo[id(self)] = result
for k, v in self.__dict__.items():
# Skip GUID.
if k in ('guid', ):
# Skip GUID and ID.
if k in ('guid', '_id'):
continue
setattr(result, k, dcpy(v, memo))
# Assign new GUID and ID
result.guid = graph.generate_element_id(result)
result.id = _get_next_node_id()
if result._sdfg is not None:
result._sdfg.parent_nsdfg_node = result
return result
Expand Down
7 changes: 4 additions & 3 deletions dace/sdfg/scope.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from dace.config import Config
from dace.sdfg import nodes as nd
from dace.sdfg.state import StateSubgraphView
from orderly_set import OrderedSet

ScopeDictType = Dict[nd.Node, List[nd.Node]]

Expand Down Expand Up @@ -62,16 +63,16 @@ def _scope_subgraph(graph, entry_node, include_entry, include_exit) -> ScopeSubg
raise TypeError("Received {}: should be dace.nodes.EntryNode".format(type(entry_node).__name__))
node_to_children = graph.scope_children()
if include_exit:
children_nodes = set(node_to_children[entry_node])
children_nodes = OrderedSet(node_to_children[entry_node])
else:
children_nodes = set(n for n in node_to_children[entry_node] if not isinstance(n, nd.ExitNode))
children_nodes = OrderedSet(n for n in node_to_children[entry_node] if not isinstance(n, nd.ExitNode))
map_nodes = [node for node in children_nodes if isinstance(node, nd.EntryNode)]
while len(map_nodes) > 0:
next_map_nodes = []
# Traverse children map nodes
for map_node in map_nodes:
# Get child map subgraph (1 level)
more_nodes = set(node_to_children[map_node])
more_nodes = OrderedSet(node_to_children[map_node])
# Unionize children_nodes with new nodes
children_nodes |= more_nodes
# Add nodes of the next level to next_map_nodes
Expand Down
2 changes: 1 addition & 1 deletion dace/sdfg/sdfg.py
Original file line number Diff line number Diff line change
Expand Up @@ -1730,7 +1730,7 @@ def from_file(filename: str) -> 'SDFG':

# Dynamic SDFG creation API
##############################

# TODO(tehrengruber): This could also be done using the counter.
def _find_new_name(self, name: str):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

""" Tries to find a new name by adding an underscore and a number. """

Expand Down
57 changes: 55 additions & 2 deletions dace/sdfg/state.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import sympy
from typing import (TYPE_CHECKING, Any, AnyStr, Callable, Dict, Iterable, Iterator, List, Optional, Set, Tuple, Type,
Union, overload)
from hashlib import sha256

import dace
from dace.frontend.python import astutils
Expand All @@ -31,6 +32,7 @@
from dace.sdfg.type_inference import infer_expr_type
from dace.sdfg.validation import validate_state
from dace.subsets import Range, Subset
import json

if TYPE_CHECKING:
import dace.sdfg.scope
Expand Down Expand Up @@ -235,6 +237,7 @@ def used_symbols(self,
"""
return set()

# TODO(tehrengruber): should we make this an ordered set?
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@property
def free_symbols(self) -> Set[str]:
"""
Expand Down Expand Up @@ -1390,6 +1393,52 @@ def __init__(self, label=None, sdfg=None, debuginfo=None, location=None):
self.location = location if location is not None else {}
self._default_lineinfo = None

# TODO(tehrengruber): unify with sdfg.hash()
def hash_state(self, jsondict: Optional[Dict[str, Any]] = None) -> str:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should probably be refactored such that there is only one keyword_remover() and also such that hash_sdfg() is using this function.
But this is for later.

"""
Returns a hash of the current SDFG, without considering IDs and attribute names.

:param jsondict: If not None, uses given JSON dictionary as input.
:return: The hash (in SHA-256 format).
"""

def keyword_remover(json_obj: Any, last_keyword=""):
# Makes non-unique in SDFG hierarchy v2
# Recursively remove attributes from the SDFG which are not used in
# uniquely representing the SDFG. This, among other things, includes
# the hash, name, transformation history, and meta attributes.
if isinstance(json_obj, dict):
if 'cfg_list_id' in json_obj:
del json_obj['cfg_list_id']

keys_to_delete = []
kv_to_recurse = []
for key, value in json_obj.items():
if (isinstance(key, str)
and (key.startswith('_meta_')
or key in ['name', 'hash', 'orig_sdfg', 'transformation_hist', 'instrument', 'guid'])):
keys_to_delete.append(key)
else:
kv_to_recurse.append((key, value))

for key in keys_to_delete:
del json_obj[key]

for key, value in kv_to_recurse:
keyword_remover(value, last_keyword=key)
elif isinstance(json_obj, (list, tuple)):
for value in json_obj:
keyword_remover(value)

# Clean SDFG of nonstandard objects
jsondict = (json.loads(json.dumps(jsondict)) if jsondict is not None else self.to_json())

keyword_remover(jsondict) # Make non-unique in SDFG hierarchy

string_representation = json.dumps(jsondict) # dict->str
hsh = sha256(string_representation.encode('utf-8'))
return hsh.hexdigest()

@property
def parent(self):
""" Returns the parent SDFG of this state. """
Expand Down Expand Up @@ -1750,9 +1799,13 @@ def add_nested_sdfg(
sdfg.update_cfg_list([])

# Make dictionary of autodetect connector types from set
if isinstance(inputs, (set, collections.abc.KeysView)):
# TODO(tehrengruber): Using sets here leads to a situation where self._nodes has a different
# ordering, but to_json from_json restores the order again. Investigate.
if isinstance(inputs, set) or isinstance(outputs, set):
warnings.warn("Using sets as inputs is discouraged as it leads to indeterministic behavior.")
if isinstance(inputs, (set, collections.abc.KeysView, collections.abc.Set)):
inputs = {k: None for k in inputs}
if isinstance(outputs, (set, collections.abc.KeysView)):
if isinstance(outputs, (set, collections.abc.KeysView, collections.abc.Set)):
Comment on lines +1802 to +1808
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actually, input/output are used for the in/out connector.
Furthermore, they are guaranteed to be strings and unique (regardless how they are passed).
Thus, thus you can essentially write:

final_inputs = {k: old_inputs[k] for k in sorted(old_inputs.keys())}

Alternatively, you could also do that in the constructor of Node.
This has the advantages of be used everywhere (okay you can still abuse add_{in, out}_connector(), but this is a different topic.

outputs = {k: None for k in outputs}

s = nd.NestedSDFG(
Expand Down
19 changes: 10 additions & 9 deletions dace/transformation/dataflow/map_fusion_vertical.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
from dace.sdfg import SDFG, SDFGState, graph, nodes, propagation
from dace.transformation.dataflow import map_fusion_helper as mfhelper
from dace.sdfg.type_inference import infer_expr_type

from dace.utils import print_sdfg_hash
from ordered_set import OrderedSet

@properties.make_properties
class MapFusionVertical(transformation.SingleStateTransformation):
Expand Down Expand Up @@ -405,9 +406,9 @@ def partition_first_outputs(
param_repl: Dict[str, str],
) -> Union[
Tuple[
Set[graph.MultiConnectorEdge[dace.Memlet]],
Set[graph.MultiConnectorEdge[dace.Memlet]],
Set[graph.MultiConnectorEdge[dace.Memlet]],
OrderedSet[graph.MultiConnectorEdge[dace.Memlet]],
OrderedSet[graph.MultiConnectorEdge[dace.Memlet]],
OrderedSet[graph.MultiConnectorEdge[dace.Memlet]],
],
None,
]:
Expand Down Expand Up @@ -447,9 +448,9 @@ def partition_first_outputs(
`require_all_intermediates` and by `self.require_exclusive_intermediates`.
"""
# The three outputs set.
pure_outputs: Set[graph.MultiConnectorEdge[dace.Memlet]] = set()
exclusive_outputs: Set[graph.MultiConnectorEdge[dace.Memlet]] = set()
shared_outputs: Set[graph.MultiConnectorEdge[dace.Memlet]] = set()
pure_outputs: Set[graph.MultiConnectorEdge[dace.Memlet]] = OrderedSet()
exclusive_outputs: Set[graph.MultiConnectorEdge[dace.Memlet]] = OrderedSet()
shared_outputs: Set[graph.MultiConnectorEdge[dace.Memlet]] = OrderedSet()

# Set of intermediate nodes that we have already processed.
processed_inter_nodes: Set[nodes.Node] = set()
Expand Down Expand Up @@ -703,7 +704,7 @@ def partition_first_outputs(

def handle_intermediate_set(
self,
intermediate_outputs: Set[graph.MultiConnectorEdge[dace.Memlet]],
intermediate_outputs: OrderedSet[graph.MultiConnectorEdge[dace.Memlet]],
state: dace.SDFGState,
sdfg: SDFG,
first_map_exit: nodes.MapExit,
Expand Down Expand Up @@ -870,7 +871,7 @@ def handle_intermediate_set(
# the input connectors on the MapEntry, such that we know where we
# have to reroute inside the Map.
# NOTE: Assumes that Map (if connected is the direct neighbour).
conn_names: Set[str] = set()
conn_names: OrderedSet[str] = OrderedSet()
for inter_node_out_edge in state.out_edges(inter_node):
if inter_node_out_edge.dst == second_map_entry:
assert inter_node_out_edge.dst_conn.startswith("IN_")
Expand Down
8 changes: 5 additions & 3 deletions dace/transformation/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -830,6 +830,7 @@ def isolate_nested_sdfg(
visited.add(node_to_process)
pre_nodes.add(node_to_process)
to_visit.extend(iedge.src for iedge in state.in_edges(node_to_process))
pre_nodes = list(sorted(pre_nodes, key=lambda n: n.id))

# These are the nodes of the middle state. Which are all access nodes that serves
# as input to the nested SDFG and the nested SDFG itself.
Expand All @@ -856,14 +857,15 @@ def isolate_nested_sdfg(
"Can only split if the out to the nested SDFG are AccessNodes to non view data and the AccessNodes are only connected to the nested SDFG."
)
middle_nodes.add(oedge.dst)
middle_nodes = list(sorted(middle_nodes, key=lambda n: n.id))

# These are the nodes that belongs to the Post State. There are two reasons why a
# node belongs to the set of post nodes.
# The first is that the node does not belong to any other set.
post_nodes: Set[nodes.Node] = {
post_nodes: list[nodes.Node] = [
node
for node in state.nodes() if (node not in pre_nodes) and (node not in middle_nodes)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This operation has become $O(n^2)$ because of the because of the in test.
Either switch to OrderedSet or sort them, either using your ID or you can use this idea.
You should sort them right before you start moving them.

Furthermore, middle_nodes do not need to be sorted since they remain in the state.

}
]

# The second reason, are read dependencies, for this we have to look at the incoming
# edges and add any node that we need.
Expand All @@ -876,7 +878,7 @@ def isolate_nested_sdfg(
if test_if_applicable:
return False
raise ValueError("Can not replicate non non-View AccessNodes into the post state.")
post_nodes.add(node)
post_nodes.append(node)

if test_if_applicable:
return True
Expand Down
Loading