Skip to content

Conversation

@SF-N
Copy link
Contributor

@SF-N SF-N commented Aug 22, 2025

This PR builds on PR 2209. It enables the handling of multiple output domains in program and field_operator calls, 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:

@gtx.field_operator
def fop(a: IField, b: JField) -> tuple[JField, IField]:
    return b, a

@gtx.program
def prog(
    a: IField,
    b: JField,
    out_a: IField,
    out_b: JField,
    i_size: gtx.int32,
    j_size: gtx.int32,
):
    fop(a, b, out=(out_b, out_a), domain=({JDim: (0, j_size)}, {IDim: (0, i_size)}))

@gtx.program
def prog_no_domain(
    a: IField,
    b: JField,
    out_a: IField,
    out_b: JField,
):
    fop(a, b, out=(out_b, out_a))

Copy link
Contributor

@tehrengruber tehrengruber left a 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,

Copy link
Contributor

@havogt havogt left a 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.

@edopao
Copy link
Contributor

edopao commented Oct 29, 2025

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

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.

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.

Copy link
Contributor

@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 @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.

@edopao
Copy link
Contributor

edopao commented Oct 31, 2025

@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.
bench_blueline_stencil_compute

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.

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.

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.

The lowering part looks good to me.

@SF-N SF-N merged commit 9369482 into GridTools:main Nov 3, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants