feat[next][dace]: Added `DistributedGlobalSelfCopyElimination` #1890

philip-paul-mueller · 2025-02-28T06:09:14Z

The transformation DistributedGlobalSelfCopyElimination is very similar to the GT4PyGlobalSelfCopyElimination but they target slightly different cases.
The transformation GT4PyGlobalSelfCopyElimination, which is already there, handles the pattern (G) -> (T) -> (G), i.e. the global data G is copied into the transient T is then immediately copied back into G.
Because of ADR-18 we know that this has no effect, because G is used as input and output and must therefore be point wise, so G[i, j] in the output can only be G[i, j] at the beginning.

The new transformation GT4PyGlobalSelfCopyElimination handles a different case, it looks for patterns (G) -> (T) and (T) -> (G), which is essentially the same, but this time the definition of T and the write back of T into G does not need to be in the same state.

In the long run, the two transformation should be combined.

…s own file.

… but this time in multiple states. Note that this transformation does not look at the subsets when it removes it. This is in accordance with ADR-18.

…) -> (T) -> (G)` which is handled by the previous self copy elimination transformation. However, it is something that we should do. The main problem is that deleting the (unnecessary) writes to `G` is a lot harder.

…fication pipeline.

…remover

edopao

LGTM

src/gt4py/next/program_processors/runners/dace/transformations/simplify.py

edopao · 2025-02-28T06:49:01Z

src/gt4py/next/program_processors/runners/dace/transformations/simplify.py

@@ -58,7 +58,11 @@ def gt_simplify(

    Further, the function will run the following passes in addition to DaCe simplify:
    - `GT4PyGlobalSelfCopyElimination`: Special copy pattern that in the context
-        of GT4Py based SDFG behaves as a no op.
+        of GT4Py based SDFG behaves as a no op, i.e. `(G) -> (T) -> (G)`.
+    - `DistributedGlobalSelfCopyElimination`: Very similar to


I propose different names: SingleStateGlobalSelfCopyElimination and MultiStateGlobalSelfCopyElimination, but this is just a matter of taste. Please do as you prefer.
Also, I wonder why only one includes "GT4Py" in the name.

I agree the names are very bad and your names are very good.
They are actually so good that I now want to keep SingleStateGlobalSelfCopyElimination such that I can keep MultiStateGlobalSelfCopyElimination and do not have to rename it to GlobalSelfCopyElimination.

src/gt4py/next/program_processors/runners/dace/transformations/redundant_array_removers.py

edopao · 2025-02-28T08:39:15Z

src/gt4py/next/program_processors/runners/dace/transformations/redundant_array_removers.py

+                        #  This is because the local `G` node could become isolated.
+                        #  We do not need to consider the outgoing edges, because
+                        #  they are reads which we have handled above.
+                        neigbourhood.update((state, iedge.src) for iedge in state.in_edges(dnode))


Am I wrong or here we could assert isinstance(iedge.src, dace_nodes.AccessNode) and iedge.src.data == gname ?

You are right.
The assert is there, but down on line 405, inside the loop that actually removes the isolated nodes.
My problem with the assert the location you suggested is/was that I would either have to use a for loop or go through the set of edges twice.
The for loop adds more indentation and the going through the neighbours is potentially expensive.
So I originally put the assert later.

However, I agree that having the assert where the problem originates is better.
I settled with the for loop.

I see, please do as you prefer.

edopao · 2025-02-28T08:40:31Z

src/gt4py/next/program_processors/runners/dace/transformations/redundant_array_removers.py

+        removed_isolated_nodes: set[dace_nodes.Node] = set()
+        for state, nh_node in neigbourhood:
+            assert isinstance(nh_node, dace_nodes.AccessNode) and nh_node.data == gname
+            if (nh_node not in removed_isolated_nodes) and (state.degree(nh_node) == 0):


and we could assert state.in_degree(nh_node) == 0 and only check state.out_degree(nh_node) == 0

I do not really understand that comment.

I am not sure if I understand you correctly, but nh_node is not necessaraly isolated, as some other nodes might read from it.

However, your question made me question the current implementation and I updated it and made it a bit cleaner.
However, because Python does not have curly brace and we do not use 8 space indentation I am not sure if it has become better, what do you think?
I like this one a little bit more.

My suggestion was to assert on input degree, so the node can have successors reading from it.

I mean: there should never be a write to the global, since this is a candidate. Instead, there might be a read, that is why we should check the output degree.

You can not assert on degree, because it might be that the neighbour node nh_node has not become isolated.
It might be that it is read somewhere else.
Consider this example, when w_concorr_c is G and __tmp18 is T, then by deleting the T node will not make the G node isolated.

I wrote input degree above, but maybe it is too restrictive:

if (nh_node not in removed_isolated_nodes) and (state.out_degree(nh_node) == 0): assert state.in_degree(nh_node) == 0

Not fully sure.
I do not have a counter example, but in most cases you might be right.
However, I would keep the degree == 0 test since we are interested in removing isolated nodes.

Co-authored-by: edopao <[email protected]>

edopao · 2025-02-28T13:07:54Z

src/gt4py/next/program_processors/runners/dace/transformations/redundant_array_removers.py

+        removed_isolated_nodes: set[dace_nodes.Node] = set()
+        for state, nh_node in neigbourhood:
+            assert isinstance(nh_node, dace_nodes.AccessNode) and nh_node.data == gname
+            if (nh_node not in removed_isolated_nodes) and (state.degree(nh_node) == 0):


philip-paul-mueller added 12 commits February 27, 2025 09:41

Fixed a stray import.

ad31947

I need to merge master.

76e5848

Relocated the GT4PyGlobalSelfCopyElimination transformation into it…

6f651a3

…s own file.

Created a new transformation that is able to remove the self copying,…

8382d9a

… but this time in multiple states. Note that this transformation does not look at the subsets when it removes it. This is in accordance with ADR-18.

Cleaned up the interface.

df9ffe7

Added a function to call DistributedGlobalSelfCopyElimination nicely.

c1eca66

Integrated DistributedGlobalSelfCopyElimination into GT4Py's simpli…

5b9ac22

…fication pipeline.

Merge remote-tracking branch 'gt4py/main' into distributed-self-copy-…

652369d

…remover

Updated the distributed self copy elimination to also respect branches.

08e521d

Added a to do.

73fe4e8

Better file names.

f150691

philip-paul-mueller requested a review from edopao February 28, 2025 06:09

Merge remote-tracking branch 'gt4py/main' into distributed-self-copy-…

f59ac25

…remover

edopao reviewed Feb 28, 2025

View reviewed changes

philip-paul-mueller and others added 2 commits February 28, 2025 12:53

Apply suggestions from code review

3c656e5

Co-authored-by: edopao <[email protected]>

Applied Edoardo's suggestions.

c5086e2

edopao approved these changes Feb 28, 2025

View reviewed changes

philip-paul-mueller merged commit 9d1bdd6 into GridTools:main Feb 28, 2025
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat[next][dace]: Added `DistributedGlobalSelfCopyElimination` #1890

feat[next][dace]: Added `DistributedGlobalSelfCopyElimination` #1890

philip-paul-mueller commented Feb 28, 2025

edopao left a comment

edopao Feb 28, 2025

philip-paul-mueller Feb 28, 2025

edopao Feb 28, 2025

philip-paul-mueller Feb 28, 2025 •

edited

Loading

edopao Feb 28, 2025

edopao Feb 28, 2025

philip-paul-mueller Feb 28, 2025 •

edited

Loading

edopao Feb 28, 2025

edopao Feb 28, 2025

philip-paul-mueller Feb 28, 2025

edopao Feb 28, 2025

philip-paul-mueller Feb 28, 2025

edopao Feb 28, 2025

edopao Feb 28, 2025

feat[next][dace]: Added DistributedGlobalSelfCopyElimination #1890

feat[next][dace]: Added DistributedGlobalSelfCopyElimination #1890

Conversation

philip-paul-mueller commented Feb 28, 2025

edopao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philip-paul-mueller Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philip-paul-mueller Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat[next][dace]: Added `DistributedGlobalSelfCopyElimination` #1890

feat[next][dace]: Added `DistributedGlobalSelfCopyElimination` #1890

philip-paul-mueller Feb 28, 2025 •

edited

Loading

philip-paul-mueller Feb 28, 2025 •

edited

Loading