feat[next][dace]: iterator-view support to DaCe backend #1790

edopao · 2025-01-08T15:24:54Z

The lowering of scan to SDFG requires the support for iterator view. This PR introduces a subset of iterator features:

Local if_ with exclusive branch execution
Lowering of list_get, make_tuple and tuple_get in iterator view
Field operators returning a tuple of fields
Tuple of fields with different size

Iterator tests are enabled on dace CPU backend without SDFG transformations (auto_optimize=False).

This reverts commit 792a8eb.

This reverts commit 746f9d8.

philip-paul-mueller

It looks good, there are some issues I see, but it looks good.

src/gt4py/next/program_processors/runners/dace_common/dace_backend.py

src/gt4py/next/program_processors/runners/dace_common/workflow.py

src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py

src/gt4py/next/program_processors/runners/dace_fieldview/gtir_dataflow.py

This reverts commit 87b5bd5.

edopao

Thank you for the review comments, I tried to address most of them.

src/gt4py/next/program_processors/runners/dace_common/dace_backend.py

src/gt4py/next/program_processors/runners/dace_common/workflow.py

src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py

src/gt4py/next/program_processors/runners/dace_fieldview/gtir_dataflow.py

src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py

src/gt4py/next/program_processors/runners/dace_fieldview/gtir_dataflow.py

philip-paul-mueller

There is the aspect of naming variables, i.e. the centralized naming stuff, I am not really happy.
The result looks good.

src/gt4py/next/program_processors/runners/dace_fieldview/gtir_dataflow.py

src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py

philip-paul-mueller

I have two final remarks about, most importantly I think that an add_temp_array() function is missing and we should no longer generate __tmp names, but turn them differently.
Then there is something that I think is a bit inconsistent.

philip-paul-mueller · 2025-01-15T07:36:14Z

src/gt4py/next/program_processors/runners/dace_fieldview/gtir_sdfg.py

+        self, sdfg: dace.SDFG, dtype: dace.dtypes.typeclass
+    ) -> tuple[str, dace.data.Scalar]:
+        """Add a temporary scalar to the SDFG."""
+        temp_name = sdfg.temp_data_name()


To fully avoid the renaming problem, you should perform here the renaming, i.e. __tmp -> __gtit_tmp, also add a note to why this is done.

It looks rather strange that there is no add_temp_array() because they suffer from the same problem. Furthermore, it is just strange if that method is missing.

The renaming problem was solved, the renaming of DaCe data containers was removed in the first place. All temporaries that are added during the lowering are safe to be __tmp dace transients. The GTIR temp symbols generated by the SSA pass are renamed into gtir_tmp before starting the lowering.

I have added the helper method add_temp_array()

src/gt4py/next/program_processors/runners/dace_fieldview/gtir_dataflow.py

philip-paul-mueller

LGTM

src/gt4py/next/program_processors/runners/dace_fieldview/gtir_dataflow.py

…1824) The implementation of scan field operator introduced a bug in the lowering of if-statements to SDFG. The scan requires extended lowering support for iterator view, therefore #1790 introduced the lowering of local-if with exclusive branch execution. However, attention was not paid to enable the exclusive behavior of if-expressions only in iterator view, that is only in the scope of scan field operators. Regular field operators should instead lower if-expressions to a tasklet, because the field view behavior is that of a local select.

edopao added 30 commits December 3, 2024 15:30

scan - working draft

df1847a

Merge remote-tracking branch 'origin/main' into dace-gtir-scan

89ca8f7

Merge remote-tracking branch 'origin/main' into dace-gtir-scan

f22eb64

Improve utility functions for tuples

c26d906

Fix for empty field domain

ba0a9ba

Merge remote-tracking branch 'origin/main' into dace-gtir-scan

ac7acf8

Merge remote-tracking branch 'origin/main' into dace-gtir-scan

877d81e

Merge remote-tracking branch 'origin/main' into dace-gtir-scan

8baf6d1

Add exclusive if_ in dataflow

784b573

Better handling of isolated nodes

de9c9de

Fix field offset in nested SDFG context

14e66e8

fix problem with dereferencil of 1D vertical fields inside scan

fcfaf72

generalize previous fix to all scan input fields

79204ee

minor edit

5fe461a

fix out-of-bound access

a4bde3a

Better handling of isolated nodes

c75a8e4

Merge remote-tracking branch 'origin/main' into dace-gtir-scan

6f72cac

exclude scan tests on dace backend with optimizations

397acae

Merge remote-tracking branch 'origin/main' into dace-gtir-scan

acf5ac0

fix pre-commit

a706b27

fix doctest

c22cfc8

Merge remote-tracking branch 'origin/main' into dace-gtir-scan

59e0ed5

temporarily disable one optimize transformation

792a8eb

Revert "temporarily disable one optimize transformation"

61985f7

This reverts commit 792a8eb.

fix for scan output stride

aa236a2

fix previous commit

9bdc75b

converto scalar to array on nsdfg output

746f9d8

Revert "converto scalar to array on nsdfg output"

0d894ff

This reverts commit 746f9d8.

Split handling of let-statement lambdas from stencil body

440a474

minor edit

500590b

edopao marked this pull request as ready for review January 10, 2025 15:57

philip-paul-mueller self-requested a review January 13, 2025 07:38

better symbol mapping for lambda nested SDFG

d93a387

philip-paul-mueller reviewed Jan 13, 2025

View reviewed changes

edopao added 4 commits January 13, 2025 21:05

Revert "add tuple_get"

678b782

This reverts commit 87b5bd5.

Merge remote-tracking branch 'origin/main' into dace-gtir-iterator_view

eaaee4e

address review comments

d4599d2

fix

4a82810

edopao commented Jan 13, 2025

View reviewed changes

fix subset num_elements

81b5fd3

edopao requested a review from philip-paul-mueller January 13, 2025 22:27

philip-paul-mueller reviewed Jan 14, 2025

View reviewed changes

src/gt4py/next/program_processors/runners/dace_fieldview/gtir_dataflow.py Outdated Show resolved Hide resolved

src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py Outdated Show resolved Hide resolved

edopao added 3 commits January 14, 2025 12:28

address review comments (1)

ea44598

fix test markers

981f2c7

address review comments (2)

6035ccc

edopao requested a review from philip-paul-mueller January 14, 2025 16:09

fix previous commit

bed7e0d

philip-paul-mueller reviewed Jan 15, 2025

View reviewed changes

edopao added 5 commits January 15, 2025 09:16

better tuple symbol tree

3529964

rename sym_tree to symbol_tree

974d643

helper function add_temp_array

6f4ff65

make _visit_if_branch_result separate function

46879b7

fix doc test

4437108

edopao requested a review from philip-paul-mueller January 15, 2025 10:52

philip-paul-mueller approved these changes Jan 15, 2025

View reviewed changes

src/gt4py/next/program_processors/runners/dace_fieldview/gtir_dataflow.py Outdated Show resolved Hide resolved

address review comment (1)

d7671a7

edopao merged commit 17bae8e into GridTools:main Jan 15, 2025
31 checks passed

edopao deleted the dace-gtir-iterator_view branch January 15, 2025 11:20

edopao mentioned this pull request Jan 24, 2025

fix[next][dace]: handle if-expression as non-exclusive in field view #1824

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat[next][dace]: iterator-view support to DaCe backend #1790

feat[next][dace]: iterator-view support to DaCe backend #1790

edopao commented Jan 8, 2025 •

edited

Loading

philip-paul-mueller left a comment

edopao left a comment

philip-paul-mueller left a comment

philip-paul-mueller left a comment

philip-paul-mueller Jan 15, 2025

edopao Jan 15, 2025

philip-paul-mueller left a comment

feat[next][dace]: iterator-view support to DaCe backend #1790

feat[next][dace]: iterator-view support to DaCe backend #1790

Conversation

edopao commented Jan 8, 2025 • edited Loading

philip-paul-mueller left a comment

Choose a reason for hiding this comment

edopao left a comment

Choose a reason for hiding this comment

philip-paul-mueller left a comment

Choose a reason for hiding this comment

philip-paul-mueller left a comment

Choose a reason for hiding this comment

philip-paul-mueller Jan 15, 2025

Choose a reason for hiding this comment

edopao Jan 15, 2025

Choose a reason for hiding this comment

philip-paul-mueller left a comment

Choose a reason for hiding this comment

edopao commented Jan 8, 2025 •

edited

Loading