feat[dace][next]: Added `CopyChainRemover` #1901

philip-paul-mueller · 2025-03-04T14:56:57Z

This PR adds the CopyChainRemover transformation.

The introduction of concat_where introduced a new pattern, which is essentially a chain of copies.
As an example imagine the case that a domain is split into 3 subdomain.
The result on the first subdomain is stored in T1, the one of the second in T2 and the one for the third domain in T3.
T1 and T2 are then copied into T4, finally T4 together with T3 are then copied into T5.
This transformation will remove T1, T2, T3 and T4 thus the results will be written into T5 directly.

There are some limitation, if we have the pattern (A1) -> (A2), then we eliminate A1 only if:

A1 is fully read; this is to avoid some nasty adjustments of map bounds.
There can only be one connection between A1 and A2.

The transformation was added twice to the simplify pass, which allows us to mitigate DaCe issue#1959.

…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

Merge branch 'distributed-self-copy-remover' into redundant-array-remover-chain.

Co-authored-by: edopao <edoardo16@gmail.com>

…over-chain

… that we need to handle concat_where expressions.

…r-chain

I do not fully understand it, it is a DaCe issue. It is, however, interesting that it worked for me yesterday but not today, and for Edoardo it worked yesterday.

The funny thing is that `can_be_applied()` can handle it, but the `apply()` method can not. The case is not handled at all and even the retrival of teh edge can not be handled.

…specified.

It now also handles the case that `a1` has other outpust. There are some tests missing, which I will now add.

There is one case in teh cycle detection that is not handled correctly, but it is a rather edgy case.

Because then it would be cyclic anyway.

The issue was that we also must propagate the strides into scopes, i.e. the case when a NestedSDFG is located inside a nested SDFG.

I do not want it, but I will allow it for now.

…r-chain

edopao

Great job! I imagined it would have been much simpler..

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

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

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

edopao · 2025-03-07T12:49:33Z

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

+
+        # If the subset we care about, which is always on the `a1` side, was not
+        #  specified we assume that the whole `a1` has been written.
+        # TODO(edopao): Fix lowering that this does not happens, it happens for example


Does it happen in docstring tests?

I have not really looked into it, I just know that in that file and that test there is a Memlet without other_subset.
It is not a high priority, but validation complains, and this case only has three access nodes, so it should be simple to fix, once we have time.

I've found the place, that's because I call sdfg.make_array_memlet(field.dc_node.data) which only sets subset, not other_subset.

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

Co-authored-by: edopao <edoardo16@gmail.com>

edopao · 2025-03-07T14:03:35Z

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

+
+        # If the subset we care about, which is always on the `a1` side, was not
+        #  specified we assume that the whole `a1` has been written.
+        # TODO(edopao): Fix lowering that this does not happens, it happens for example


I've found the place, that's because I call sdfg.make_array_memlet(field.dc_node.data) which only sets subset, not other_subset.

philip-paul-mueller and others added 27 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

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

f59ac25

…remover

Added the file structure 'distributed-self-copy-remover' introduced.

63567fa

Merge branch 'distributed-self-copy-remover' into redundant-array-remover-chain.

Apply suggestions from code review

3c656e5

Co-authored-by: edopao <edoardo16@gmail.com>

Applied Edoardo's suggestions.

c5086e2

Merge branch 'distributed-self-copy-remover' into redundant-array-rem…

5abcb8a

…over-chain

Implemented a first version of the redundant array removal for chains…

3cacc5f

… that we need to handle concat_where expressions.

Made some fixes and completed it, now starting testing.

b234cfc

Removed some stray import.

65c4b9e

Made some last fixes to the transformation.

2121e85

Added tests for the chain copy remover.

ec40730

Fixed something with the return value.

0305853

Integrated the CopyChainRemover into simplify.

5025c8a

Added the case to handle nested SDFGs.

21beb11

Updated some notes.

43b52ec

Merge remote-tracking branch 'gt4py/main' into redundant-array-remove…

6f49a25

…r-chain

philip-paul-mueller requested a review from edopao March 4, 2025 14:56

philip-paul-mueller added 2 commits March 5, 2025 07:28

Fixed the issue with the pattern matching.

71c1a3e

I do not fully understand it, it is a DaCe issue. It is, however, interesting that it worked for me yesterday but not today, and for Edoardo it worked yesterday.

Realized that the case that a1 has outgoing edges is not handled.

0e6f500

The funny thing is that `can_be_applied()` can handle it, but the `apply()` method can not. The case is not handled at all and even the retrival of teh edge can not be handled.

philip-paul-mueller added 12 commits March 5, 2025 07:48

Made it a requirement that source and destination subsets have to be …

d542517

…specified.

Updated the chain remover.

7a683c1

It now also handles the case that `a1` has other outpust. There are some tests missing, which I will now add.

Fixed an issue with a wrong subset correction.

d26da7e

Updated the test, there is still one missing.

7a1a842

Temporary fix for Edoardo.

7d7bdbb

There is one case in teh cycle detection that is not handled correctly, but it is a rather edgy case.

I am thinking that we do not have to check the incomming connections.

61766a5

Because then it would be cyclic anyway.

Added a todo about propagation.

ab6859a

Fixed the stride propagation in copy chain remover.

99ea6c9

The issue was that we also must propagate the strides into scopes, i.e. the case when a NestedSDFG is located inside a nested SDFG.

Allowed some undefined subsets.

736b9d4

I do not want it, but I will allow it for now.

Added a test that also uses a NestedSDFG.

869e8ef

The chain remover now properly handles viewes.

9e69173

Merge remote-tracking branch 'gt4py/main' into redundant-array-remove…

cbdb836

…r-chain

edopao approved these changes Mar 7, 2025

View reviewed changes

philip-paul-mueller and others added 2 commits March 7, 2025 14:06

Apply suggestions from code review

dd68139

Co-authored-by: edopao <edoardo16@gmail.com>

Applied the rest of Edoardo's point.

0f53152

philip-paul-mueller requested a review from edopao March 7, 2025 13:29

edopao approved these changes Mar 7, 2025

View reviewed changes

philip-paul-mueller merged commit 2f4db72 into GridTools:main Mar 10, 2025
23 checks passed

philip-paul-mueller deleted the redundant-array-remover-chain branch March 19, 2025 11:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat[dace][next]: Added `CopyChainRemover` #1901

feat[dace][next]: Added `CopyChainRemover` #1901

philip-paul-mueller commented Mar 4, 2025

edopao left a comment

edopao Mar 7, 2025

philip-paul-mueller Mar 7, 2025

edopao Mar 7, 2025

edopao Mar 7, 2025

feat[dace][next]: Added CopyChainRemover #1901

feat[dace][next]: Added CopyChainRemover #1901

Conversation

philip-paul-mueller commented Mar 4, 2025

edopao left a comment

Choose a reason for hiding this comment

edopao Mar 7, 2025

Choose a reason for hiding this comment

philip-paul-mueller Mar 7, 2025

Choose a reason for hiding this comment

edopao Mar 7, 2025

Choose a reason for hiding this comment

edopao Mar 7, 2025

Choose a reason for hiding this comment

feat[dace][next]: Added `CopyChainRemover` #1901

feat[dace][next]: Added `CopyChainRemover` #1901