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.

Sign up for GitHub

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

feat: use more efficient field merger code #6815

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

feat: use more efficient field merger code #6815

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c86f962
feat: use more efficient field merger code
TomAFrench Dec 13, 2024
16ba340
.
TomAFrench Dec 14, 2024
bab687e
Merge branch 'master' into tf/explore-re-adding-value-merger-change
TomAFrench Dec 16, 2024
5570a8b
Merge branch 'master' into tf/explore-re-adding-value-merger-change
TomAFrench Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 33 additions & 12 deletions compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,18 +117,39 @@ impl<'a> ValueMerger<'a> {
let then_condition =
dfg.insert_instruction_and_results(cast, block, None, call_stack).first();

let cast = Instruction::Cast(else_condition, else_type);
let else_condition =
dfg.insert_instruction_and_results(cast, block, None, call_stack).first();

let mul = Instruction::binary(BinaryOp::Mul, then_condition, then_value);
let then_value = dfg.insert_instruction_and_results(mul, block, None, call_stack).first();

let mul = Instruction::binary(BinaryOp::Mul, else_condition, else_value);
let else_value = dfg.insert_instruction_and_results(mul, block, None, call_stack).first();

let add = Instruction::binary(BinaryOp::Add, then_value, else_value);
dfg.insert_instruction_and_results(add, block, None, call_stack).first()
if then_type == NumericType::NativeField {
// We're merging two fields so we can do a more efficient value merger as by converting
// the expression `if c { a } else { b }` as `c * (a-b) + b`.
//
// This removes a multiplication compared to the standard `c*a + (!c)*b`.

let diff = Instruction::binary(BinaryOp::Sub, then_value, else_value);
let diff_value =
dfg.insert_instruction_and_results(diff, block, None, call_stack).first();

let conditional_diff = Instruction::binary(BinaryOp::Mul, then_condition, diff_value);
let conditional_diff_value = dfg
.insert_instruction_and_results(conditional_diff, block, None, call_stack)
.first();

let merged_field =
Instruction::binary(BinaryOp::Add, else_value, conditional_diff_value);
dfg.insert_instruction_and_results(merged_field, block, None, call_stack)
.first()
} else {
let cast = Instruction::Cast(else_condition, else_type);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

let else_condition =
dfg.insert_instruction_and_results(cast, block, None, call_stack).first();

let mul = Instruction::binary(BinaryOp::Mul, then_condition, then_value);
let then_value = dfg.insert_instruction_and_results(mul, block, None, call_stack).first();

let mul = Instruction::binary(BinaryOp::Mul, else_condition, else_value);
let else_value = dfg.insert_instruction_and_results(mul, block, None, call_stack).first();

let add = Instruction::binary(BinaryOp::Add, then_value, else_value);
dfg.insert_instruction_and_results(add, block, None, call_stack).first()
}
}

/// Given an if expression that returns an array: `if c { array1 } else { array2 }`,
Expand Down
Loading