feat: use more efficient field merger code #6815
Conversation
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
TomAFrench
added
the
run-external-checks
Changes to circuit sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Peak Memory Sample
|
Compilation Sample
|
| dfg.insert_instruction_and_results(merged_field, block, None, call_stack.clone()) | ||
| .first() | ||
| } else { | ||
| let cast = Instruction::Cast(else_condition, else_type); |
There was a problem hiding this comment.
Related to this I found that when we were casting to Field we saved a lot of range constraints implied on the binary operations performed on Unsigned types. First I thought that must be unsafe as it would mask a potential overflow, but with the c*a + !c*b formula below it seems safe (or actually it has an else_condition below, but assuming that they are mutually exclusive).
I assume you're not doing the c * (a-b) + b trick because of a potential overflow, but isn't it true that mathematically this should be okay, if the cost of the extra Casts to Field would be acceptable?
There was a problem hiding this comment.
Yeah, we're not concerned about overflows here as the merged value is constrained to then_value or else_value and any underflow in the difference term will disappear (This is why we needed to cast to Fields as the integer types imply checked arithmetic)
I did this just for fields in this PR as this doesn't need the inputs to be casted to fields and back (as it's already the correct type).
|
I think we should wait for #6817 before pushing this further due to negligible changes within the test suite. |
Description
Problem*
Resolves
Summary*
This PR replaces the value merger code reverted in #6673 but solely for fields.
Additional Context
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.