Skip to content

Commit

Permalink
feat: revert changes to ValueMerger and Instruction::IfElse
Browse files Browse the repository at this point in the history
  • Loading branch information
@TomAFrench
TomAFrench committed Dec 2, 2024
1 parent d80a9d7 commit 21e637e
Show file tree
Hide file tree
Showing 6 changed files with 90 additions and 46 deletions.
27 changes: 19 additions & 8 deletions compiler/noirc_evaluator/src/ssa/ir/instruction.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,12 @@ pub(crate) enum Instruction {
/// else_value
/// }
/// ```
IfElse { then_condition: ValueId, then_value: ValueId, else_value: ValueId },
IfElse {
then_condition: ValueId,
then_value: ValueId,
else_condition: ValueId,
else_value: ValueId,
},

/// Creates a new array or slice.
///
Expand Down Expand Up @@ -619,11 +624,14 @@ impl Instruction {
assert_message: assert_message.clone(),
}
}
Instruction::IfElse { then_condition, then_value, else_value } => Instruction::IfElse {
then_condition: f(*then_condition),
then_value: f(*then_value),
else_value: f(*else_value),
},
Instruction::IfElse { then_condition, then_value, else_condition, else_value } => {
Instruction::IfElse {
then_condition: f(*then_condition),
then_value: f(*then_value),
else_condition: f(*else_condition),
else_value: f(*else_value),
}
}
Instruction::MakeArray { elements, typ } => Instruction::MakeArray {
elements: elements.iter().copied().map(f).collect(),
typ: typ.clone(),
Expand Down Expand Up @@ -682,9 +690,10 @@ impl Instruction {
| Instruction::RangeCheck { value, .. } => {
f(*value);
}
Instruction::IfElse { then_condition, then_value, else_value } => {
Instruction::IfElse { then_condition, then_value, else_condition, else_value } => {
f(*then_condition);
f(*then_value);
f(*else_condition);
f(*else_value);
}
Instruction::MakeArray { elements, typ: _ } => {
Expand Down Expand Up @@ -847,7 +856,7 @@ impl Instruction {
None
}
}
Instruction::IfElse { then_condition, then_value, else_value } => {
Instruction::IfElse { then_condition, then_value, else_condition, else_value } => {
let typ = dfg.type_of_value(*then_value);

if let Some(constant) = dfg.get_numeric_constant(*then_condition) {
Expand All @@ -866,11 +875,13 @@ impl Instruction {

if matches!(&typ, Type::Numeric(_)) {
let then_condition = *then_condition;
let else_condition = *else_condition;

let result = ValueMerger::merge_numeric_values(
dfg,
block,
then_condition,
else_condition,
then_value,
else_value,
);
Expand Down
8 changes: 6 additions & 2 deletions compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -465,8 +465,12 @@ fn simplify_slice_push_back(
let mut value_merger =
ValueMerger::new(dfg, block, &mut slice_sizes, unknown, None, call_stack);

let new_slice =
value_merger.merge_values(len_not_equals_capacity, set_last_slice_value, new_slice);
let new_slice = value_merger.merge_values(
len_not_equals_capacity,
len_equals_capacity,
set_last_slice_value,
new_slice,
);

SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice])
}
Expand Down
8 changes: 6 additions & 2 deletions compiler/noirc_evaluator/src/ssa/ir/printer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,11 +209,15 @@ fn display_instruction_inner(
Instruction::RangeCheck { value, max_bit_size, .. } => {
writeln!(f, "range_check {} to {} bits", show(*value), *max_bit_size,)
}
Instruction::IfElse { then_condition, then_value, else_value } => {
Instruction::IfElse { then_condition, then_value, else_condition, else_value } => {
let then_condition = show(*then_condition);
let then_value = show(*then_value);
let else_condition = show(*else_condition);
let else_value = show(*else_value);
writeln!(f, "if {then_condition} then {then_value} else {else_value}")
writeln!(
f,
"if {then_condition} then {then_value} else (if {else_condition}) {else_value}"
)
}
Instruction::MakeArray { elements, typ } => {
write!(f, "make_array [")?;
Expand Down
12 changes: 10 additions & 2 deletions compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -537,14 +537,19 @@ impl<'f> Context<'f> {
let args = vecmap(then_args.iter().zip(else_args), |(then_arg, else_arg)| {
(self.inserter.resolve(*then_arg), self.inserter.resolve(else_arg))
});

let else_condition = if let Some(branch) = cond_context.else_branch {
branch.condition
} else {
self.inserter.function.dfg.make_constant(FieldElement::zero(), Type::bool())
};
let block = self.inserter.function.entry_block();

// Cannot include this in the previous vecmap since it requires exclusive access to self
let args = vecmap(args, |(then_arg, else_arg)| {
let instruction = Instruction::IfElse {
then_condition: cond_context.then_branch.condition,
then_value: then_arg,
else_condition,
else_value: else_arg,
};
let call_stack = cond_context.call_stack.clone();
Expand Down Expand Up @@ -684,10 +689,13 @@ impl<'f> Context<'f> {
)
.first();

let else_condition = self
.insert_instruction(Instruction::Not(condition), call_stack.clone());

let instruction = Instruction::IfElse {
then_condition: condition,
then_value: value,

else_condition,
else_value: previous_value,
};

Expand Down
71 changes: 41 additions & 30 deletions compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ impl<'a> ValueMerger<'a> {
pub(crate) fn merge_values(
&mut self,
then_condition: ValueId,
else_condition: ValueId,
then_value: ValueId,
else_value: ValueId,
) -> ValueId {
Expand All @@ -69,26 +70,28 @@ impl<'a> ValueMerger<'a> {
self.dfg,
self.block,
then_condition,
else_condition,
then_value,
else_value,
),
typ @ Type::Array(_, _) => {
self.merge_array_values(typ, then_condition, then_value, else_value)
self.merge_array_values(typ, then_condition, else_condition, then_value, else_value)
}
typ @ Type::Slice(_) => {
self.merge_slice_values(typ, then_condition, then_value, else_value)
self.merge_slice_values(typ, then_condition, else_condition, then_value, else_value)
}
Type::Reference(_) => panic!("Cannot return references from an if expression"),
Type::Function => panic!("Cannot return functions from an if expression"),
}
}

/// Merge two numeric values a and b from separate basic blocks to a single value. This
/// function would return the result of `if c { a } else { b }` as `c * (a-b) + b`.
/// function would return the result of `if c { a } else { b }` as `c*a + (!c)*b`.
pub(crate) fn merge_numeric_values(
dfg: &mut DataFlowGraph,
block: BasicBlockId,
then_condition: ValueId,
else_condition: ValueId,
then_value: ValueId,
else_value: ValueId,
) -> ValueId {
Expand All @@ -111,38 +114,31 @@ impl<'a> ValueMerger<'a> {
// We must cast the bool conditions to the actual numeric type used by each value.
let then_condition = dfg
.insert_instruction_and_results(
Instruction::Cast(then_condition, Type::field()),
Instruction::Cast(then_condition, then_type),
block,
None,
call_stack.clone(),
)
.first();

let then_field = Instruction::Cast(then_value, Type::field());
let then_field_value =
dfg.insert_instruction_and_results(then_field, block, None, call_stack.clone()).first();

let else_field = Instruction::Cast(else_value, Type::field());
let else_field_value =
dfg.insert_instruction_and_results(else_field, block, None, call_stack.clone()).first();

let diff = Instruction::binary(BinaryOp::Sub, then_field_value, else_field_value);
let diff_value =
dfg.insert_instruction_and_results(diff, block, None, call_stack.clone()).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.clone())
let else_condition = dfg
.insert_instruction_and_results(
Instruction::Cast(else_condition, else_type),
block,
None,
call_stack.clone(),
)
.first();

let merged_field =
Instruction::binary(BinaryOp::Add, else_field_value, conditional_diff_value);
let merged_field_value = dfg
.insert_instruction_and_results(merged_field, block, None, call_stack.clone())
.first();
let mul = Instruction::binary(BinaryOp::Mul, then_condition, then_value);
let then_value =
dfg.insert_instruction_and_results(mul, block, None, call_stack.clone()).first();

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

let merged = Instruction::Cast(merged_field_value, then_type);
dfg.insert_instruction_and_results(merged, 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 All @@ -152,6 +148,7 @@ impl<'a> ValueMerger<'a> {
&mut self,
typ: Type,
then_condition: ValueId,
else_condition: ValueId,
then_value: ValueId,
else_value: ValueId,
) -> ValueId {
Expand All @@ -166,6 +163,7 @@ impl<'a> ValueMerger<'a> {

if let Some(result) = self.try_merge_only_changed_indices(
then_condition,
else_condition,
then_value,
else_value,
actual_length,
Expand Down Expand Up @@ -195,7 +193,12 @@ impl<'a> ValueMerger<'a> {
let then_element = get_element(then_value, typevars.clone());
let else_element = get_element(else_value, typevars);

merged.push_back(self.merge_values(then_condition, then_element, else_element));
merged.push_back(self.merge_values(
then_condition,
else_condition,
then_element,
else_element,
));
}
}

Expand All @@ -208,6 +211,7 @@ impl<'a> ValueMerger<'a> {
&mut self,
typ: Type,
then_condition: ValueId,
else_condition: ValueId,
then_value_id: ValueId,
else_value_id: ValueId,
) -> ValueId {
Expand Down Expand Up @@ -265,7 +269,12 @@ impl<'a> ValueMerger<'a> {
let else_element =
get_element(else_value_id, typevars, else_len * element_types.len());

merged.push_back(self.merge_values(then_condition, then_element, else_element));
merged.push_back(self.merge_values(
then_condition,
else_condition,
then_element,
else_element,
));
}
}

Expand Down Expand Up @@ -314,6 +323,7 @@ impl<'a> ValueMerger<'a> {
fn try_merge_only_changed_indices(
&mut self,
then_condition: ValueId,
else_condition: ValueId,
then_value: ValueId,
else_value: ValueId,
array_length: usize,
Expand Down Expand Up @@ -397,7 +407,8 @@ impl<'a> ValueMerger<'a> {
let then_element = get_element(then_value, typevars.clone());
let else_element = get_element(else_value, typevars);

let value = self.merge_values(then_condition, then_element, else_element);
let value =
self.merge_values(then_condition, else_condition, then_element, else_element);

array = self.insert_array_set(array, index, value, Some(condition)).first();
}
Expand Down
10 changes: 8 additions & 2 deletions compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,9 @@ impl Context {

for instruction in instructions {
match &function.dfg[instruction] {
Instruction::IfElse { then_condition, then_value, else_value } => {
Instruction::IfElse { then_condition, then_value, else_condition, else_value } => {
let then_condition = *then_condition;
let else_condition = *else_condition;
let then_value = *then_value;
let else_value = *else_value;

Expand All @@ -84,7 +85,12 @@ impl Context {
call_stack,
);

let value = value_merger.merge_values(then_condition, then_value, else_value);
let value = value_merger.merge_values(
then_condition,
else_condition,
then_value,
else_value,
);

let _typ = function.dfg.type_of_value(value);
let results = function.dfg.instruction_results(instruction);
Expand Down

0 comments on commit 21e637e

Please sign in to comment.