Skip to content

fix[next-dace]: Map stride of local dimension in nested SDFG#2593

Merged
edopao merged 11 commits into
GridTools:mainfrom
edopao:dace_fix_stride_if_region
May 12, 2026
Merged

fix[next-dace]: Map stride of local dimension in nested SDFG#2593
edopao merged 11 commits into
GridTools:mainfrom
edopao:dace_fix_stride_if_region

Conversation

@edopao
Copy link
Copy Markdown
Contributor

@edopao edopao commented May 6, 2026

This PR contains 3 kinds of changes:

  1. A bugfix in mapping the stride of local dimension into the nested SDFG of if expressions.
inner_desc = dace.data.Array(
    dtype=arg_desc.dtype,
    shape=(arg_desc.shape[local_dim_pos],),
    strides=(arg_desc.strides[local_dim_pos],),
)
  1. Addition of connectivities. This is needed to support the pattern with can_deref expressions (not part of this PR, see feat[next-dace]: Lowering mixed-GTIR to SDFG #2467). After GTIR transformations, which unroll the neighbors loops, a common pattern in grids with skip values is if(can_deref(a, V2E), deref(a(V2E)) + 2, 0).

  2. Renaming of function parameters.

Blueline performance is in pair with baseline.

@edopao edopao requested a review from philip-paul-mueller May 7, 2026 11:44
Copy link
Copy Markdown
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 am not fully happy with this change because it has potentially big implication for the IfMover and the condition fuser (which will stop working, since it can no longer identifying which If-Blocks can be moved together.
Because of these implication we must absolutely make sure to also run MuPhys, because, it will be very much impacted by these changes.

I am actually not sure if the data locality is actually improved, because in the almost everything has a restrict keyword, so the compiler should be smart enough tio figuring that out.
But this is just my concern or have you different opinions @iomaganaris

Comment thread src/gt4py/next/program_processors/runners/dace/lowering/gtir_dataflow.py Outdated
Comment thread src/gt4py/next/program_processors/runners/dace/lowering/gtir_dataflow.py Outdated
@edopao
Copy link
Copy Markdown
Contributor Author

edopao commented May 7, 2026

I am not fully happy with this change because it has potentially big implication for the IfMover and the condition fuser (which will stop working, since it can no longer identifying which If-Blocks can be moved together. Because of these implication we must absolutely make sure to also run MuPhys, because, it will be very much impacted by these changes.

I am actually not sure if the data locality is actually improved, because in the almost everything has a restrict keyword, so the compiler should be smart enough tio figuring that out. But this is just my concern or have you different opinions @iomaganaris

I see your point, makes sense. This change is not part of the bugfix, so I could revert it.

@edopao edopao force-pushed the dace_fix_stride_if_region branch from cb60549 to f560107 Compare May 8, 2026 07:13
@edopao
Copy link
Copy Markdown
Contributor Author

edopao commented May 8, 2026

I have applied the review comments from @philip-paul-mueller, I agree totally. I have reverted the modification to the lowering of the condition: this change introduced a performance regression. Below the blueline performance plot:

  • gt4py_timers_fix_stride: the current state of the PR, after reverting the modification to the lowering of the condition.
  • gt4py_timers_fix_stride_and_move_cond: the previous state of the PR, which moved the evaluation of the if-condition inside the nested SDFG.
bench_blueline_stencil_compute

@edopao edopao requested a review from philip-paul-mueller May 8, 2026 07:18
Copy link
Copy Markdown
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.

LGTM, although I have two smallish points.

Comment on lines -722 to +742
if param_name in if_sdfg.arrays:
if param_name in sdfg.arrays:
# the data desciptor was added by the visitor of the other branch expression
assert if_sdfg.data(param_name) == inner_desc
assert sdfg.data(param_name) == inner_desc
else:
if_sdfg.add_datadesc(param_name, inner_desc)
if_sdfg_input_memlets[param_name] = arg_expr
sdfg.add_datadesc(param_name, inner_desc)
input_memlets[param_name] = arg_expr
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Question: If this function is called in a uniform way I would expect that the data descriptor is either already added to the sdfg or not in all cases. So this if locks a bit fishy to me but maybe I miss something.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The origin of this if is that I am adding globals as I encounter them while visiting the if branches. I am visiting one branch at a time, so it could be that some globals have already been added in the previous branch.

Comment thread src/gt4py/next/program_processors/runners/dace/lowering/gtir_dataflow.py Outdated
@edopao edopao merged commit 437088c into GridTools:main May 12, 2026
23 of 24 checks passed
@edopao edopao deleted the dace_fix_stride_if_region branch May 12, 2026 13:25
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.

2 participants