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.

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

feat[dace]: Custom SDFG inline pass #1649

Merged

Conversation

philip-paul-mueller
Copy link
Contributor

Added a custom pass for inlining SDFG.
The function builds upon the traditional inline pass (InlineSDFG), however, before it is run some cleaning steps are preformed (PruneConectors and PruneSymbols) which increase the likelihood that the inlining can be done.
The cost is that state fusing is performed.

Also the gt_simplify() function is modified, instead of the build in behaviour it will only run the GT4Py specific one.
This behaviour can not be changed.

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.

Nice! Just one minor comment.

Args:
sdfg: The SDFG to optimize.
validate: Perform validation after the pass has run.
validate_all: Perform extensive validation.
skip: List of simplify passes that should not be applied, defaults
to `GT_SIMPLIFY_DEFAULT_SKIP_SET`.
"""
return dace_passes_simplify.SimplifyPass(
if skip is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we should remove the Optional type, because anyway we have a default skip set. If instead we have to keep Optional, and I see then the need for this if-statement, then we can also move back the default set here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point, but keeping optional would allow for skip=None to tell the transformation to not skip anything.
Otherwise we would have to write skip=[] or skip=set() which is more cryptic. But I removed it, what do you think?
@edopao

Copy link
Contributor

Choose a reason for hiding this comment

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

I am OK with that, both options are OK. My only comment is that if we keep Optional, it makes more sense (I think) to have None as default and go back to the old way of initializing the set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@edopao I made the changes we discussed.

@edopao
Copy link
Contributor

edopao commented Sep 19, 2024

LGTM

@philip-paul-mueller philip-paul-mueller merged commit 2eed367 into GridTools:main Sep 19, 2024
31 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.

2 participants