Improved GPU Copy #1976

philip-paul-mueller · 2025-03-14T13:45:54Z

Before some 2D copies (especially if they had FORTRAN order) were turned into Maps, see issue#1953.
This PR modifies the code generator in such a way that such copies are now handled.

There is some legacy stuff that should also be looked at.

…t more.

But I have to run the unit tests.

… make the tests.

…D copy to a 1d copy, because it happens to be continiously allocated.

…not where I though I found it.

…date_state()` function. The reason is that in some cases this is valid, for example if an edge connects an AccessNode and a MapEntry, because, in that case the map might not be executed. Since the Memlet does not have access to its source and destination node it can not check that, so the test was moved to a location that can do this check. However, it only does the check for AN to AN connections, which is a bit restrictive, but this is something for later.

Once [PR#1976](spcl/dace#1976) is merged in DaCe the code generator is able to handle more Memlets directly as Cuda `memcpy()` calls. This PR modifies the GPU transformation of GT4Py in such a way that these Memlets are no longer transformed into Maps. However, it can only be merged if the DaCe dependency was bumped to a version that includes PR#1976!

phschaad · 2025-03-19T18:19:14Z

This looks good to me, but would be great if @alexnick83 could quickly sanity check - GPU codegen is not my forté

alexnick83

I have some questions/comments, namely the following:

I share the concern that, for ndim > 2, the conditions tested are insufficient. I would suggest making an issue to revisit in the future.
I am seeing an issue in _emit_copy, for ndim == 2, around lines 1014-1015. Maybe have a look and confirm that this is intended.

I see that we were already referring to these copies as "continuous," but the correct term is "contiguous." Could you do a find and replace-all?

dace/codegen/targets/cuda.py

alexnick83 · 2025-03-21T13:39:45Z

dace/codegen/targets/cuda.py

+                        #  If we do not have them, then we have to turn this into a Map.
+                        is_fortran_order = src_strides[0] == 1 and dst_strides[0] == 1
+                        is_c_order = src_strides[-1] == 1 and dst_strides[-1] == 1
+                        if is_c_order or is_fortran_order:
                            continue
                    elif dims > 2:
                        if not (src_strides[-1] != 1 or dst_strides[-1] != 1):


Looking at this now, is this condition sufficient? What is there (multiple) discontinuities in other higher dimensions? I would leave a note to check later if any strange multidimensional copies are covered by _emit_copy.

It is correct, however, in your had you have to apply de Morgans law twice.
I have reformulated the condition.

I mean that just having the last strides be 1 in both arrays when ndim > 2 is not enough to assume that we do not need a Map. But this is how the code was already, so I would leave the exploration of such cases for another PR.

dace/codegen/targets/cuda.py

alexnick83 · 2025-03-21T13:48:59Z

dace/codegen/targets/cuda.py

-                    raise NotImplementedError('2D copy only supported with one stride')
+                    src_strides = [src_strides[1 if is_c_order else 0]]
+                    dst_strides = [dst_strides[1 if is_c_order else 0]]
+                    is_fortran_order = src_strides[0] == 1 and dst_strides[0] == 1


Isn't this already computed in line 983?

See next point.

alexnick83 · 2025-03-21T13:50:01Z

dace/codegen/targets/cuda.py

+                    src_strides = [src_strides[1 if is_c_order else 0]]
+                    dst_strides = [dst_strides[1 if is_c_order else 0]]
+                    is_fortran_order = src_strides[0] == 1 and dst_strides[0] == 1
+                    is_c_order = is_fortran_order


Is this expression missing a not?

This here is very strange.
This code seems to check (I am not sure myself because there is no explanation and I am very bad at reading the minds of people from several years ago) if a 2d copy can be reduced to a 1d copy, for example there is no padding in neither the source and destination arrays.
This means a 1d copy is performed, although a 2d object is copied.
Thus the difference between FORTRAN and C order is meaningless, thus they have the same value and indicate if the stride is 1.
If you look at the initial definition of is_{fortran, c}_order (on line ~984), you will see that this is the same behaviour you had if the copy was 1d in the first place.

I see now. The array will initially be marked as C/Fortran ordered based on the last/first stride. However, in the special case ndim=2, the other stride is checked to verify whether the array is indeed contiguous.

alexnick83 · 2025-03-21T13:50:56Z

dace/codegen/targets/cuda.py

-                if src_strides[-1] != 1 or dst_strides[-1] != 1:
+                # Currently we only support ND copies when they can be represented
+                #  as a 1D copy or as a 2D strided copy
+                # NOTE: Not sure if this test is enough, it should also be tested that


To what are you agreeing on?

That you are not sure what this code is doing.

That you agree that it is meaningless and can be removed.

That the test is not enough. it doesn't consider, e.g., Views where you can have multiple discontinuities, and I guess it is like that because the original code predates Views. I also think it is fine to leave it as-is for now, but it should be noted down as a possible source of errors.

dace/codegen/targets/cuda.py

alexnick83 · 2025-03-21T14:02:13Z

dace/codegen/targets/cuda.py

+                    state_id,
+                    [src_node, dst_node],
+                )
+            elif dims == 2 and is_fortran_order:


Considering lines 1014-1015, will this condition ever trigger, or will everything be caught by the previous one (line 1113)?

Please see my explanation above.
Line 1014-1015 are only executed if a 2d copy can be expressed (known at code generation time) as a 1d copy.
See also the discussion above about if we can remove these 2d -> 1d copy changer because the memlet_copy_to_absolute_strides() function is doing it anyway.

dace/sdfg/validation.py

alexnick83 · 2025-03-21T15:16:09Z

dace/codegen/targets/cuda.py

-                if src_strides[-1] != 1 or dst_strides[-1] != 1:
+                # Currently we only support ND copies when they can be represented
+                #  as a 1D copy or as a 2D strided copy
+                # NOTE: Not sure if this test is enough, it should also be tested that


That the test is not enough. it doesn't consider, e.g., Views where you can have multiple discontinuities, and I guess it is like that because the original code predates Views. I also think it is fine to leave it as-is for now, but it should be noted down as a possible source of errors.

alexnick83 · 2025-03-21T15:18:50Z

dace/codegen/targets/cuda.py

+                    src_strides = [src_strides[1 if is_c_order else 0]]
+                    dst_strides = [dst_strides[1 if is_c_order else 0]]
+                    is_fortran_order = src_strides[0] == 1 and dst_strides[0] == 1
+                    is_c_order = is_fortran_order


I see now. The array will initially be marked as C/Fortran ordered based on the last/first stride. However, in the special case ndim=2, the other stride is checked to verify whether the array is indeed contiguous.

tbennun

Since I wrote the original code, I want to make sure I review it properly before it’s in

philip-paul-mueller and others added 16 commits March 14, 2025 10:18

Started with the implementation of a better copy, but I have to fix i…

feea97f

…t more.

Made a first version of the new copy implementation.

9b49c9e

But I have to run the unit tests.

Updated the memlet copying, I think I now have all the cases will now…

d0a396f

… make the tests.

Now 2D copies works, more tests needed.

c931b91

Added now also test for testing strided 1d copy.

a67ad2a

Added a new test to the SDFG.

61ea7a6

Improved memlet checking.

322ecda

Added a note about wrong usage of eid in validation.

0b15a74

Added a new test for the pseudo 1d case, i.e. when we reduce a copy 2…

76a1a58

…D copy to a 1d copy, because it happens to be continiously allocated.

Simplified some check.

66b43f8

Added more verification.

801adb1

Merge remote-tracking branch 'spcl/main' into improved-2d-copy

02d87b5

Fixed some issue and made it more logical.

3166302

I am not sure why the printout of the edge is not correct, but it is …

2801967

…not where I though I found it.

Added tests to ensure that the new verification works as expected.

065e0d7

philip-paul-mueller mentioned this pull request Mar 17, 2025

feat[dace][next]: Adjusted the handling of non standard GPU Memlets. GridTools/gt4py#1913

Open

alexnick83 requested changes Mar 21, 2025

View reviewed changes

philip-paul-mueller added 2 commits March 21, 2025 16:06

Added Alexnicks's suggestions.

b0b9945

Merge remote-tracking branch 'spcl/main' into improved-2d-copy

ba97874

philip-paul-mueller requested a review from alexnick83 March 21, 2025 15:07

alexnick83 approved these changes Mar 21, 2025

View reviewed changes

tbennun requested changes Mar 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved GPU Copy #1976

Improved GPU Copy #1976

philip-paul-mueller commented Mar 14, 2025

phschaad commented Mar 19, 2025

alexnick83 left a comment

alexnick83 Mar 21, 2025

philip-paul-mueller Mar 21, 2025

alexnick83 Mar 21, 2025

alexnick83 Mar 21, 2025

philip-paul-mueller Mar 21, 2025

alexnick83 Mar 21, 2025

philip-paul-mueller Mar 21, 2025

alexnick83 Mar 21, 2025

alexnick83 Mar 21, 2025

philip-paul-mueller Mar 21, 2025

alexnick83 Mar 21, 2025

alexnick83 Mar 21, 2025

philip-paul-mueller Mar 21, 2025

alexnick83 Mar 21, 2025

alexnick83 Mar 21, 2025

tbennun left a comment

Improved GPU Copy #1976

Are you sure you want to change the base?

Improved GPU Copy #1976

Conversation

philip-paul-mueller commented Mar 14, 2025

phschaad commented Mar 19, 2025

alexnick83 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

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

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

tbennun left a comment

Choose a reason for hiding this comment