Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat[next][dace]: GTIR-to-SDFG lowering of neighbors and reduce #1597

Merged
merged 207 commits into from
Jul 30, 2024
Merged

feat[next][dace]: GTIR-to-SDFG lowering of neighbors and reduce #1597

merged 207 commits into from
Jul 30, 2024

Conversation

edopao
Copy link
Contributor

@edopao edopao commented Jul 29, 2024

This PR adds lowering of neighbors and reduce operators to DaCe SDFG.

edopao added 30 commits April 18, 2024 11:36
@edopao
Skeleton for ITIR translation
b36fcb3
@edopao
Minor edit
2020182
@edopao
Use Python callstack as a context stack for the ITIR visitor
5c6b6ba
@edopao
Format error
60e1c69
@edopao
Refactor tasklet codegen
073a0a4
@edopao
Code refactoring
50be68f
@edopao
Add domain to field operator
4e2dc15
@edopao
Minor edit
ea9da35
@edopao
Remove hard-coded field shape
daf7827
@edopao
Remove hard-coded target domain
9672b3b
@edopao
Merge remote-tracking branch 'origin/main' into dace-fieldview
b6326b8
@edopao
Refactoring
26f3790
@edopao
Merge remote-tracking branch 'origin/main' into dace-fieldview
1efffa7
@edopao
Fix formatting
f99fa84
@edopao
More refactoring
d6e1088
@edopao
Minor edit
9854497
@edopao
Fix formatting
37d83d7
@edopao
Use callable to build taskgraph
29986ef
@edopao
Add draft of select operator
390f3b4
@edopao
Remove node mapping
de27419
@edopao
Remove node mapping (fix + test case)
cd900f5
@edopao
Add test case for inlined mathematic builtins
326cbb5
@edopao
Go full functional (remove SDFGState member var)
a10b614
@edopao
Minor edit
aef4265
@edopao
Minor edit (1)
9e67dfe
@edopao
Fix state handling
4b4109e
@edopao
Edit comments based on review
495fd0a
@edopao
Add test case for nested select
0085194
@edopao
Separate builtin translation from driver logic
41e2a44
@edopao
Improve code comments
7148c5f
edopao added 17 commits July 26, 2024 14:53
@edopao
Review comments + new testcase
3eced05
@edopao
Working solution
9ae57d1
@edopao
Import changes from let-lambdas branch
94f1529
@edopao
First working prototype for neighbors-reduce
015b924
@edopao
Merge remote-tracking branch 'origin/dace-fieldview-neighbors-let_red…
4cc1c82 
…uce_identity' into dace-fieldview-neighbors-let_reduce_identity
@edopao
Utility for common pattern matching
4ac22e9
@edopao
Merge fixes
fd737c8
@edopao
Minor edit
676d49b
@edopao
Merge remote-tracking branch 'origin/dace-fieldview-let_lambdas-rebas…
0f904ae 
…e' into dace-fieldview-neighbors-let_reduce_identity
@edopao
Remove assert on KeyError
88e4574
@edopao
Merge remote-tracking branch 'origin/dace-fieldview-let_lambdas-rebas…
7e414f2 
…e' into dace-fieldview-neighbors-let_reduce_identity
@edopao
Add code comment
37afe2d
@edopao
Merge remote-tracking branch 'origin/dace-fieldview-let_lambdas-rebas…
8472719 
…e' into dace-fieldview-neighbors-let_reduce_identity
@edopao
Use let-symbol for reduce identity
6183305
@edopao
Merge remote-tracking branch 'origin/main' into dace-fieldview-neighb…
924e413 
…ors-let_reduce_identity
@edopao
Minor edit
ff8064a
@edopao
Edit code comments
ab68406
@edopao edopao marked this pull request as ready for review July 29, 2024 15:17
philip-paul-mueller
philip-paul-mueller reviewed Jul 30, 2024
View reviewed changes
Copy link
Contributor

@philip-paul-mueller philip-paul-mueller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A first round of comments.

src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py Outdated Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_to_tasklet.py Outdated Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_to_tasklet.py Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_to_tasklet.py Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_to_tasklet.py Outdated Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_to_tasklet.py Show resolved Hide resolved
edopao
edopao commented Jul 30, 2024
View reviewed changes
Copy link
Contributor Author

@edopao edopao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comments, I will soon update my PR.

src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py Outdated Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_to_tasklet.py Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_to_tasklet.py Outdated Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_to_tasklet.py Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_to_tasklet.py Outdated Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_to_tasklet.py Show resolved Hide resolved
philip-paul-mueller
philip-paul-mueller approved these changes Jul 30, 2024
View reviewed changes
Copy link
Contributor

@philip-paul-mueller philip-paul-mueller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some small things, but nothing that essentially blocks merging.
However, maybe someone else should also take a small look, for example Till.

src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py Outdated Show resolved Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_to_sdfg.py Show resolved Hide resolved
edopao
edopao commented Jul 30, 2024
View reviewed changes
tests/next_tests/unit_tests/program_processor_tests/runners_tests/test_dace_fieldview.py
@@ -776,6 +785,447 @@ def test_gtir_connectivity_shift_chain():
assert np.allclose(e_out, ref)


def test_gtir_neighbors_as_input():
# FIXME[#1582](edopao): Enable testcase when type inference is working
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tehrengruber I have added 6 testcases for neighbors and reduce but 3 of them are currently skipped, because of the issues we discussed about fields of lists as input/output to programs or in combination with map operations. Later we can try to enable them, when GTIR type inference has support.

@edopao edopao merged commit 719c487 into GridTools:main Jul 30, 2024
31 checks passed
@edopao edopao deleted the dace-fieldview-neighbors-let_reduce_identity branch July 30, 2024 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philip-paul-mueller philip-paul-mueller philip-paul-mueller approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@edopao @philip-paul-mueller