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

fix(ssa refactor): Reset condition value during flattening pass #1811

Merged
merged 5 commits into from
Jun 23, 2023

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Jun 23, 2023

Description

Problem*

Resolves #1792 / #1806

Summary*

When flattening the cfg there is an optimization to set the value of an if's condition to a constant true/false within the then/else branch respectively since there is no other way to get to these branches without this being true.

This means for code such as:

fn main(c: bool) {
    if c {
        constrain c;
    }
}

We can optimize it to:

fn main(c: bool) {
    if c {
        constrain true;
    }
}

Since we know c is always true within the then branch. This allows us to remove this particular constrain entirely afterward.

The problem in #1792 arised when we forgot to reset the condition c back to c, so even after the if ended we still had c = true or c = false in the context. This PR fixes this by simply resetting c to its old value after finishing an if statement.

There is another, related fix in this PR that we now avoid setting c = true or c = false if c is already a known constant. If this happened, we could be setting true = false or false = true which would not be good.

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@jfecher
Copy link
Contributor Author

jfecher commented Jun 23, 2023

@ludamad do you think I should include the integration test 1792_bad_equality from #1806 in this PR? I thought it was a bit repetitive to include all 3 of the tests in that PR for the same issue so I only included the smallest one initially.

@ludamad ludamad closed this Jun 23, 2023
@ludamad ludamad reopened this Jun 23, 2023
ludamad
ludamad previously approved these changes Jun 23, 2023
Copy link
Collaborator

@ludamad ludamad left a comment

Choose a reason for hiding this comment

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

LGTM

@jfecher jfecher enabled auto-merge June 23, 2023 16:04
@jfecher jfecher added this pull request to the merge queue Jun 23, 2023
Merged via the queue into master with commit 2e330e0 Jun 23, 2023
@jfecher jfecher deleted the jf/fix-1792 branch June 23, 2023 16:36
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.

BadConstantEquality with conditionals in SSA refactor
2 participants