feat[next]: Lowering foast/past to GTIR #1569

havogt · 2024-07-03T14:29:58Z

Adds lowering of foast and past to GTIR.

The backend that goes through the new lowering is not enabled in this PR as it is missing the domain propagation pass which comes in a different PR. The lowering is tested with the test_foast_to_gtir (which is an extended duplication of test_foast_to_itir which will eventually be deleted).

Notes:

Lowering of scan is not yet implemented.

havogt · 2024-08-14T15:03:52Z

src/gt4py/next/ffront/foast_to_gtir.py

+        common_symbols: dict[str, foast.Symbol] = node.annex.propagated_symbols
+
+        if return_kind is foast_introspection.StmtReturnKind.NO_RETURN:
+            # TODO document why this case should be handled in this way, not by the more general CONDITIONAL_RETURN


question to @tehrengruber (probably smaller tree?)

egparedes

It looks overall good to me. Requested changes are mostly minor style issues.

egparedes · 2024-08-21T15:14:15Z

src/gt4py/eve/utils.py

@@ -1651,3 +1651,5 @@ def to_set(self) -> Set[T]:


 xiter = XIterable
+
+__all__ = ["toolz"]


I guess this has been added to avoid linter errors but I don't think it's the right solution (and additionally __all__ should always go at the top of the file).

I don't remember. My question would be, do you think it's ok to import toolz from eve (to have the import check for the ctoolz not repeated everywhere).

src/gt4py/next/ffront/lowering_utils.py

src/gt4py/next/ffront/past_to_itir.py

tests/next_tests/unit_tests/ffront_tests/test_foast_to_gtir.py

egparedes · 2024-08-22T14:47:45Z

tests/next_tests/unit_tests/ffront_tests/test_foast_to_gtir.py

+
+
+def test_annotated_assignment():
+    pytest.xfail("Annotated assignments are not properly supported at the moment.")


Just a question: why?

it's blindly copied, I can check

First error "gtx not defined", after importing Field directly, I get "TDim not defined". Not sure if hard to fix.

(first part might be fixed by @nfarabullini 's work...)

Not sure, but I think the reason this doesn't work is because in a from __future__ import annotations file the annotations are no evaluated and hence don't land in the closure vars. This might be something to take into consideration in @nfarabullini works.

tests/next_tests/unit_tests/ffront_tests/test_foast_to_gtir.py

egparedes · 2024-08-22T14:56:00Z

tests/next_tests/unit_tests/ffront_tests/test_past_to_gtir.py

+    )
+
+
+def test_copy_lowering(copy_program_def, gtir_identity_fundef):


Slightly outside of the scope of the PR and not strictly necessary, but in the spirit of the previous tests, I'd suggest to rename all the copy_ names (e.g. fixture, test_function, ...) should be renamed to something like identity or pass_through,

This is actually a copy, we use the identity fieldop but assign the result into another field.

tests/next_tests/unit_tests/ffront_tests/test_past_to_gtir.py

havogt · 2024-08-23T06:48:45Z

src/gt4py/eve/utils.py

@@ -1651,3 +1651,5 @@ def to_set(self) -> Set[T]:


 xiter = XIterable
+
+__all__ = ["toolz"]


I don't remember. My question would be, do you think it's ok to import toolz from eve (to have the import check for the ctoolz not repeated everywhere).

havogt · 2024-08-23T06:49:31Z

src/gt4py/next/ffront/foast_to_gtir.py

+        common_symbols: dict[str, foast.Symbol] = node.annex.propagated_symbols
+
+        if return_kind is foast_introspection.StmtReturnKind.NO_RETURN:
+            # TODO document why this case should be handled in this way, not by the more general CONDITIONAL_RETURN


This is meant to be resolved in this PR, probably removed.

havogt · 2024-08-23T06:50:49Z

src/gt4py/next/type_system/type_info.py

@@ -136,7 +136,6 @@ class TupleConstructorType(Protocol, Generic[_R]):
    def __call__(self, *args: Any) -> _R: ...


-# TODO(havogt): the complicated typing is a hint that this function needs refactoring


this is the Till's tree_map version. this removal is acknowledging that the problem is not simple and typing is complicated for this kind of function.

tests/next_tests/unit_tests/ffront_tests/test_foast_to_gtir.py

havogt · 2024-08-23T06:52:34Z

tests/next_tests/unit_tests/ffront_tests/test_foast_to_gtir.py

+
+
+def test_annotated_assignment():
+    pytest.xfail("Annotated assignments are not properly supported at the moment.")


it's blindly copied, I can check

Co-authored-by: Enrique González Paredes <[email protected]>

edopao · 2024-08-23T09:03:36Z

src/gt4py/next/iterator/ir_utils/ir_makers.py

@@ -356,6 +363,18 @@ def lifted_neighbors(offset, it) -> itir.Expr:
    return lift(lambda_("it")(neighbors(offset, "it")))(it)


+def as_fieldop_neighbors(offset, it) -> itir.Expr:


We need to also pass the optional domain of as_fieldop, at least for unit testing of gtir to SDFG. Anyway, I could also add it in my PR.

egparedes

It looks good to me.

src/gt4py/next/ffront/foast_to_itir.py

src/gt4py/next/ffront/past_to_itir.py

src/gt4py/next/ffront/foast_to_gtir.py

src/gt4py/next/ffront/lowering_utils.py

havogt · 2024-08-26T07:57:15Z

src/gt4py/next/ffront/foast_to_gtir.py

+        common_symbols: dict[str, foast.Symbol] = node.annex.propagated_symbols
+
+        if return_kind is foast_introspection.StmtReturnKind.NO_RETURN:
+            # FIXME[#1582](havogt): document why this case should be handled in this way, not by the more general CONDITIONAL_RETURN


Suggested change

# FIXME[#1582](havogt): document why this case should be handled in this way, not by the more general CONDITIONAL_RETURN

# FIXME[#1582](tehrengruber): document why this case should be handled in this way, not by the more general CONDITIONAL_RETURN

havogt and others added 8 commits June 30, 2024 10:08

nothing

aeff778

basic opbinary

6919596

all tests updated

87883a6

test cleanup

01a6e07

add to_gtir path to past_to_itir

b949ebd

update past_to_gtir_test

be21b78

gtir_embedded backend

be00112

Merge remote-tracking branch 'upstream/main' into gtir_lowering

5230aeb

tehrengruber changed the title ~~feat[next] foast to gtir~~ feat[next]: foast to gtir Jul 17, 2024

havogt and others added 15 commits August 2, 2024 14:33

Merge branch 'main' into gtir_lowering

96efba2

add broadcast, max_over, min_over

5d0d16e

math builtins

bca403f

cleanup merge conflict

ce8ea56

add where lowering

17f891d

cond cases

c77380a

implement astype lowering

3b4c4a2

fix process_elements for itir lowering

ab42da2

fix pure scalar op lowering

ef40997

Merge remote-tracking branch 'upstream/main' into gtir_lowering

bff0278

improve typing, unary

1bd0797

as_offset

f7792ff

remove old license

0ea0f79

disable gtir embedded

70dd170

cleanup use of dimension

99f86c6

havogt commented Aug 14, 2024

View reviewed changes

cleanup

3458aee

havogt changed the title ~~feat[next]: foast to gtir~~ feat[next]: Lowering foast/past to GTIR Aug 15, 2024

havogt marked this pull request as ready for review August 15, 2024 10:25

havogt requested a review from tehrengruber August 15, 2024 10:25

egparedes requested changes Aug 22, 2024

View reviewed changes

havogt commented Aug 23, 2024

View reviewed changes

havogt and others added 5 commits August 23, 2024 08:55

Apply suggestions from code review

d46f308

Co-authored-by: Enrique González Paredes <[email protected]>

Merge remote-tracking branch 'upstream/main' into gtir_lowering

960e173

fix import

cdc8963

address review comments

e17ef1c

undo unneeded change

789a215

edopao reviewed Aug 23, 2024

View reviewed changes

havogt added 2 commits August 23, 2024 11:23

domain in itir maker

156b884

TODO to FIXME

9d2c034

egparedes approved these changes Aug 23, 2024

View reviewed changes

tehrengruber requested changes Aug 23, 2024

View reviewed changes

havogt added 2 commits August 26, 2024 09:46

address review comments

efd3587

add testcase for tuple in frozen namespace

c94e1e9

havogt commented Aug 26, 2024

View reviewed changes

havogt requested a review from tehrengruber August 26, 2024 07:59

tehrengruber approved these changes Aug 26, 2024

View reviewed changes

havogt merged commit 659c38c into GridTools:main Aug 26, 2024
31 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat[next]: Lowering foast/past to GTIR #1569

feat[next]: Lowering foast/past to GTIR #1569

havogt commented Jul 3, 2024 •

edited

Loading

havogt Aug 14, 2024

egparedes left a comment

egparedes Aug 21, 2024

havogt Aug 23, 2024

egparedes Aug 22, 2024

havogt Aug 23, 2024

havogt Aug 23, 2024

havogt Aug 23, 2024

tehrengruber Aug 26, 2024

egparedes Aug 22, 2024

havogt Aug 23, 2024

havogt Aug 23, 2024

havogt Aug 23, 2024

havogt Aug 23, 2024

havogt Aug 23, 2024

edopao Aug 23, 2024

egparedes left a comment

havogt Aug 26, 2024

		@@ -1651,3 +1651,5 @@ def to_set(self) -> Set[T]:


		xiter = XIterable

		__all__ = ["toolz"]



		def test_annotated_assignment():
		pytest.xfail("Annotated assignments are not properly supported at the moment.")

		)


		def test_copy_lowering(copy_program_def, gtir_identity_fundef):

		@@ -136,7 +136,6 @@ class TupleConstructorType(Protocol, Generic[_R]):
		def __call__(self, *args: Any) -> _R: ...


		# TODO(havogt): the complicated typing is a hint that this function needs refactoring

		@@ -356,6 +363,18 @@ def lifted_neighbors(offset, it) -> itir.Expr:
		return lift(lambda_("it")(neighbors(offset, "it")))(it)


		def as_fieldop_neighbors(offset, it) -> itir.Expr:

	# FIXME[#1582](havogt): document why this case should be handled in this way, not by the more general CONDITIONAL_RETURN
	# FIXME[#1582](tehrengruber): document why this case should be handled in this way, not by the more general CONDITIONAL_RETURN

feat[next]: Lowering foast/past to GTIR #1569

feat[next]: Lowering foast/past to GTIR #1569

Conversation

havogt commented Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

egparedes 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

egparedes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

havogt commented Jul 3, 2024 •

edited

Loading