-
Notifications
You must be signed in to change notification settings - Fork 219
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: 9_conditional
end to end test
#1951
Conversation
minimal circuit to reproduce:
When a == 0, we are getting a constraint failed for 1/a which should not be triggering. |
checking euclidean division, the predicate is 1 (constant) which triggers the subsequent constraints. The ssa is:
Which means that the predicate should not be a constant for division. The error can then be seen in |
New error is because we are multiplying two Expressions which should be Linear expressions, but they are not -- we will need to reduce the lhs and rhs before multiplying |
New minimal reproducable error is now:
SSA for this is:
Noting that:
Is somewhat confusing to read in the case that v16 is not enabled |
The problem; inv_var unconditionally asserts that the result has an inverse, so for the case of 1, it will always fail |
A bigger refactor will be needed to remove the idea of an optional predicate. Since Option is a compile time known concept, this implies that predicates are known at compile time which is not correct. Another problem with Option or Option is that its not quite clear as to why/if None is different to Some(Expression::zero()). |
Blocking this PR until the issue above is created for Option<Expression/AcirVar> |
crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/generated_acir.rs
Outdated
Show resolved
Hide resolved
Created #1972 |
@guipublic spoke offline, though could you give an official approval? @jfecher pinging for comments! |
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.
LGTM
Description
Resolves #1802
Problem*
Resolves
Summary*
Documentation
This PR requires documentation updates when merged.
Additional Context
PR Checklist*
cargo fmt
on default settings.