-
Notifications
You must be signed in to change notification settings - Fork 49
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[next][dace]: Fix lowering of nested let-statements #1697
Conversation
…dace-gtir-fix_let_stmts
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.
There are some points that needs clarification.
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_sdfg.py
Outdated
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_sdfg.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.
For symmetry I would keep the parenthesis on the true case too.
But this is a matter of style.
LGTM.
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.
There are two suggestions but it looks good.
# nested SDFG as transient array/scalar storage. The exception is given by | ||
# input arguments that are just passed through and returned by the lambda, | ||
# e.g. when the lambda is constructing a tuple: in this case, the result | ||
# data is non-transient, because it corresponds to an input node. |
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 would add that this is corrected by make_temps()
.
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.
Done.
1st if-branch: | ||
Transient nodes actually contain some result produced by the dataflow | ||
itself, therefore these nodes are changed to non-transient and an output | ||
edge will write the result from the nested SDFG to a new intermediate | ||
data node in the parent context. | ||
|
||
2nd and 3rd if-branch: | ||
Non-transient nodes are just input nodes that are immediately returned | ||
by the lambda expression. Therefore, these nodes are already available | ||
in the parent context and can be directly accessed there. |
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 would put this description as comments.
Because, a docstring is supposed to be viewed without the source code.
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.
Good point.
This PR fixes one corner case of nested let-statements, discovered in
test_tuple_unpacking_star_multi
during GTIR integration. Test case added.Additionally, fixed handling of symbol already defined in SDFG for #1695.