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

Calculate which functions require predicates in SSA CFG construction #5498

Open
michaeljklein opened this issue Jul 11, 2024 · 0 comments
Open
Labels
enhancement New feature or request

Comments

@michaeljklein
Copy link
Contributor

michaeljklein commented Jul 11, 2024

Problem

Currently, we don't explicitly identify side-effect-free functions. This can make it difficult to identify all of the sources of side effects individually and is related to multiple bugs where side effects run in non-executed branches, for example #5462

  • The branch condition is a simple function applied to a dynamic/unknown input
  • There are side effects inside the branch
  • The side effects erroneously run when the condition is false
if foo(x) {
  // side effects in `bar`
  bar();
}

Happy Case

Identify side-effects in functions when calculating the control-flow graph (CFG) during SSA.

  • Add side-effect metadata to the CFG during construction
  • Add a method to check whether a function has side effects from the completed CFG
  • Use this check to ensure predicates (i.e. EnableSideEffectsIf conditions) are handled properly
  • Evaluate whether this allows us to remove the no_predicates attribute

See @jfecher's comment for a partial list of side effects to handle.

Project Impact

Nice-to-have

Impact Context

  • It can be challenging to identify whether functions have side effects without a graph.
  • This also affects Brillig, e.g. because Brillig calls can be considered side effects
  • Using no_predicates properly is hard, because currently you need to check all dependent functions for side effects manually.
    • no_predicates is useful for performance.
    • The list of builtins or features that are allowed with no_predicates could change.

Workaround

Yes

Workaround Description

For Noir users:
Manually ensure there are no predicates and use the no_predicates attribute.

For developing Noir:

  1. Test control flow (see chore: test blackbox binary op instructions #5484, which found [T]::pop_front() panics when inside non-executed if #5462)
  2. Fix side-effect-in-branch bugs individually

Additional Context

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@michaeljklein michaeljklein added the enhancement New feature or request label Jul 11, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: 📋 Backlog
Development

No branches or pull requests

1 participant