-
Notifications
You must be signed in to change notification settings - Fork 200
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
chore(ssa refactor): Add code to handle less than comparison #1433
Conversation
- This method needs to know the bit size, so we cache this information whenever we do a range constraint. We should ideally also cache it for constants too since we can figure out their bit-sizes easily
This was failing before because the result was not being cached when we did a cast operation. I incorrectly assumed that the value_id of the input to cast will equal the value_id of the output to cast |
crates/nargo_cli/tests/test_data_ssa_refactor/simple_comparison/src/main.nr
Show resolved
Hide resolved
crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs
Show resolved
Hide resolved
crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs
Show resolved
Hide resolved
crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs
Show resolved
Hide resolved
crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/generated_acir.rs
Show resolved
Hide resolved
This is now blocked by #1435 since we are now producing circuits which have a width greater than three |
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.
Any particular reason for the merkle test changes in this PR?
crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs
Show resolved
Hide resolved
I think this might be because of a rebase, we just merged merkle changes into master |
346a216
to
7d8753b
Compare
Can we merge master with this PR so the extraneous diffs get removed? |
We are merging into the range-constraints PR so I think we need to update it 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'm alright with merging this into another PR. We can fix up the other PR afterward so the diff is cleaner.
Diff should be clean now! |
Description
This adds code to handle the less than operation, to handle more than, we just need to handle the NOT operation which we can put in another PR.
Problem*
Resolves
Summary*
This PR sets out to
Example
Before:
After:
Documentation
This PR requires documentation updates when merged.
Additional Context
PR Checklist*
cargo fmt
on default settings.