-
Notifications
You must be signed in to change notification settings - Fork 53
feat[next]: Multiple output domains #2225
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.
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
Conversation
…multiple_output_domains
… if output is a tuple
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some cleanups in past_to_itir using two new arguments added to tree_map. Ready from my side as soon as we have decided and resolved on how to handle normalize_domains,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just want to mark this non-mergable, because we need to double-check first if these changes don't have unintended side-effects on icon4py.
# Conflicts: # src/gt4py/next/utils.py
…to multiple_output_domains
|
The dace lowering now works, also tested in ICON4Py CI. Tomorrow I will run a performance regression test on ICON4Py blueline. @philip-paul-mueller Could you please review only the changes in the dace backend? Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main point of concern that a lot of checks is None where introduced, which I do not like at all.
They should have been filtered out before, this is what I would do/suggest.
If this is not possible, then some explanation, in doc strings, are missing what this mean.
src/gt4py/next/program_processors/runners/dace/gtir_to_sdfg_scan.py
Outdated
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace/gtir_to_sdfg_scan.py
Outdated
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace/gtir_to_sdfg_types.py
Outdated
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace/gtir_to_sdfg_utils.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @philip-paul-mueller for the review. I will now push an updated version of the SDFG lowering, which addresses your comments. However, I will pull part of the changes into a separate PR, which can be merge separately, before this PR.
src/gt4py/next/program_processors/runners/dace/gtir_to_sdfg_scan.py
Outdated
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace/gtir_to_sdfg_scan.py
Outdated
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace/gtir_to_sdfg_scan.py
Outdated
Show resolved
Hide resolved
|
@philip-paul-mueller The dace part is ready for another round of review. @havogt Yesterday I ran a performance regression test on icon4py blueline and I see no issues, see plot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the lowering is kind of okay, some refactoring might be needed.
Although I still do not like the Nones but the explanation makes it clear what is going on.
I have some smallish suggestions.
src/gt4py/next/program_processors/runners/dace/gtir_to_sdfg_primitives.py
Outdated
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace/gtir_to_sdfg_scan.py
Outdated
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace/gtir_to_sdfg_types.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lowering part looks good to me.

This PR builds on PR 2209. It enables the handling of multiple output domains in
programandfield_operatorcalls, which can either be explicitly specified or will be deduced.Note: Prior to this commit, in the case of implicit domains (no domain argument), only a single domain was deduced which was assumed to be the same for all output fields. Now, every field gets its own domain, even when at runtime they are all the same.
Example: