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

feature[next]: Extend Single Static Assignment (SSA) pass to support if statements #1250

Merged
merged 6 commits into from
May 9, 2023

Conversation

tehrengruber
Copy link
Contributor

This PR extends the SSA pass (operating on the Python AST) to support if statements such that every variable is assigned only once per branch, e.g. this

if True:
    a = 1
else:
    a = 2
    if True:
        a = 3
    a = 4
a = 5

is transformed into

if True:
    a__0 = 1
    a__2 = a__0
else:
    a__0 = 2
    if True:
        a__1 = 3
    else:
        a__1 = a__0
    a__2 = 4
a__3 = 5

Most of the code was written by @BenWeber42 reviewed by me. See #972 for the discussion. The PR is a prerequisite for #1079 and was spun off from there to ease review.

Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

The algorithm looks good but the implementation looks too verbose to me and unnecessarily hard to read. I recommend to simplify the code before approving the Pr.

@tehrengruber
Copy link
Contributor Author

Ready for another round.

Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

It looks almost ready to go. Just some comments about missing type annotations and docstrings.

Comment on lines 271 to 274
# An empty annotation always applies to the next assignment.
# So we need to use the correct versioning, but also ensure
# we restore the old versioning afterwards, because no assignment
# actually happens.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I understood the same core idea but don't really get the details. I think we should understand what the comment really means and rephrase it in a more comprehensible way before merging the PR, otherwise is useless.

@tehrengruber tehrengruber requested a review from egparedes May 9, 2023 09:09
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

ILGTM.

@tehrengruber tehrengruber merged commit 65c83c0 into GridTools:main May 9, 2023
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