Parallel Map Fusion #1987
Parallel Map Fusion #1987
Conversation
It is now able to handle serial and parallel maps. In addition it is now possible to pass the `single_use_data` as argument.
The transformation can now handle the case when the intermediate has multiple inputs.
There was a problem hiding this comment.
It is a great idea to have the standalone capability to fuse parallel Maps rather than relying on SubgraphFusion. Relaxing the single-consumer constraint for the serial fusion's intermediate also makes sense to me.
My main question is whether an implementation of parallel fusion as a separate transformation has been considered, and what the trade-offs are. My understanding is that the fused implementation was chosen to better accommodate the FullMapFusion pass, but I wonder if the alternative would be more flexible. If this has been discussed and decided already internally, then I am fine. Otherwise, I would also ask for a review from someone currently actively working on transformations/auto-optimizer for a second opinion.
| pass and `MapFusion` | ||
| pass and `MapFusion`. | ||
|
|
||
| By default this transformation only handles the case where to maps are right after each other, |
There was a problem hiding this comment.
| By default this transformation only handles the case where to maps are right after each other, | |
| By default, this transformation only handles the case where two maps are executed in sequence, |
|
|
||
| By default this transformation only handles the case where to maps are right after each other, | ||
| separated by an intermediate array. However, by setting `allow_parallel_map_fusion` to `True`, | ||
| the transformation will be _in addition_ also be able to handle the case where the Maps are |
There was a problem hiding this comment.
| the transformation will be _in addition_ also be able to handle the case where the Maps are | |
| the transformation will _in addition_ also be able to handle the case where the Maps are |
| By default this transformation only handles the case where to maps are right after each other, | ||
| separated by an intermediate array. However, by setting `allow_parallel_map_fusion` to `True`, | ||
| the transformation will be _in addition_ also be able to handle the case where the Maps are | ||
| parallel (parallel here means that neither of the two Map can be reached from the other; see |
There was a problem hiding this comment.
| parallel (parallel here means that neither of the two Map can be reached from the other; see | |
| parallel (parallel here means that neither of the two Maps can be reached from the other; see |
|
|
||
| :param only_inner_maps: Only match Maps that are internal, i.e. inside another Map. | ||
| :param only_toplevel_maps: Only consider Maps that are at the top. | ||
| :param strict_dataflow: Which dataflow mode should be used, see above. | ||
| :param assume_always_shared: Assume that all intermediates are shared. | ||
| :param allow_serial_map_fusion: Allow serial map fusion, by default `True`. |
There was a problem hiding this comment.
| :param allow_serial_map_fusion: Allow serial map fusion, by default `True`. | |
| :param allow_serial_map_fusion: Allow serial Map fusion, by default `True`. |
|
|
||
| :param only_inner_maps: Only match Maps that are internal, i.e. inside another Map. | ||
| :param only_toplevel_maps: Only consider Maps that are at the top. | ||
| :param strict_dataflow: Which dataflow mode should be used, see above. | ||
| :param assume_always_shared: Assume that all intermediates are shared. | ||
| :param allow_serial_map_fusion: Allow serial map fusion, by default `True`. | ||
| :param allow_parallel_map_fusion: Allow to merge parallel maps, by default `False`. |
There was a problem hiding this comment.
| :param allow_parallel_map_fusion: Allow to merge parallel maps, by default `False`. | |
| :param allow_parallel_map_fusion: Allow to merge parallel Maps, by default `False`. |
| :param node1: The first node to check. | ||
| :param node2: The second node to check. | ||
| """ | ||
| # In order to be parallel they must be in the same scope. |
There was a problem hiding this comment.
Maybe rephrase this so there is no misunderstanding in the future. Nodes that are not in the same scope can definitely be parallel; you just do not want to try to merge Maps that are not in the same scope for semi-obvious reasons.
| if scope[node1] != scope[node2]: | ||
| return False | ||
|
|
||
| # The `all_nodes_between()` function traverse the graph and returns `None` if |
There was a problem hiding this comment.
| # The `all_nodes_between()` function traverse the graph and returns `None` if | |
| # The `all_nodes_between()` function traverses the graph and returns `None` if |
| @@ -1273,6 +1529,7 @@ def has_read_write_dependency( | |||
| )) | |||
| if not self.test_if_subsets_are_point_wise(all_subsets): | |||
| return True | |||
| del all_subsets | |||
| @@ -1307,6 +1564,7 @@ def has_read_write_dependency( | |||
| # Now we can test if these subsets are point wise | |||
| if not self.test_if_subsets_are_point_wise(all_subsets): | |||
| return True | |||
| del all_subsets | |||
|
The parallel fusion of Maps (currently an open PR) is from an implementation point of view, a subset of the functionality you need to handle serial Map fusion. In the very first version of the PR that introduced the new version of MapFusion there was this distinction between parallel and serial MapFusion. This "subset aspect of the implementation" makes a split implementation very strange, you have a base, which essentially contains everything that is needed to implement However, my issue with splitting the implementation is, that I think there is no reason why a class that is called But I think we should discuss this aspect at the next DaCe meeting. PS: I will not apply your correction yet, because I have multiple PRs about that subject open, so it get's complicated. |
philip-paul-mueller
changed the title
This PR extends the
MapFusiontransformation such that it also handles maps that are parallel.The functionality that had to be added is actually quite small, as the transformation already implemented all functions that were needed.
The only thing that was needed is some dispatch logic.
While
MapFusionwill by default not perform parallel map fusion, for this a flag has to be specified, theFullMapFusepass will do that.However, in auto optimize this is disabled for compatibility.
This PR replaces PR#1965.
Initially this PR was a bugfix, but it kind of changed and become the real parallel Map fusion PR.