Skip to content

Commit

Permalink
chore: remove some _else_condition tech debt (#6522)
Browse files Browse the repository at this point in the history
  • Loading branch information
@TomAFrench
TomAFrench authored Nov 19, 2024
1 parent e1510ec commit 5f730e8
Show file tree
Hide file tree
Showing 6 changed files with 57 additions and 110 deletions.
30 changes: 8 additions & 22 deletions compiler/noirc_evaluator/src/ssa/ir/instruction.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,15 +273,7 @@ pub(crate) enum Instruction {
/// else_value
/// }
/// ```
///
/// Where we save the result of !then_condition so that we have the same
/// ValueId for it each time.
IfElse {
then_condition: ValueId,
then_value: ValueId,
else_condition: ValueId,
else_value: ValueId,
},
IfElse { then_condition: ValueId, then_value: ValueId, else_value: ValueId },

/// Creates a new array or slice.
///
Expand Down Expand Up @@ -536,14 +528,11 @@ impl Instruction {
assert_message: assert_message.clone(),
}
}
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::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::MakeArray { elements, typ } => Instruction::MakeArray {
elements: elements.iter().copied().map(f).collect(),
typ: typ.clone(),
Expand Down Expand Up @@ -602,10 +591,9 @@ impl Instruction {
| Instruction::RangeCheck { value, .. } => {
f(*value);
}
Instruction::IfElse { then_condition, then_value, else_condition, else_value } => {
Instruction::IfElse { then_condition, then_value, else_value } => {
f(*then_condition);
f(*then_value);
f(*else_condition);
f(*else_value);
}
Instruction::MakeArray { elements, typ: _ } => {
Expand Down Expand Up @@ -768,7 +756,7 @@ impl Instruction {
None
}
}
Instruction::IfElse { then_condition, then_value, else_condition, else_value } => {
Instruction::IfElse { then_condition, then_value, else_value } => {
let typ = dfg.type_of_value(*then_value);

if let Some(constant) = dfg.get_numeric_constant(*then_condition) {
Expand All @@ -787,13 +775,11 @@ 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: 2 additions & 6 deletions compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -443,12 +443,8 @@ 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,
len_equals_capacity,
set_last_slice_value,
new_slice,
);
let new_slice =
value_merger.merge_values(len_not_equals_capacity, set_last_slice_value, new_slice);

SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice])
}
Expand Down
8 changes: 2 additions & 6 deletions compiler/noirc_evaluator/src/ssa/ir/printer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,15 +210,11 @@ 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_condition, else_value } => {
Instruction::IfElse { then_condition, then_value, 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 if {else_condition} then {else_value}"
)
writeln!(f, "if {then_condition} then {then_value} else {else_value}")
}
Instruction::MakeArray { elements, typ } => {
write!(f, "make_array [")?;
Expand Down
79 changes: 36 additions & 43 deletions compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,6 @@ impl<'f> Context<'f> {
let instruction = Instruction::IfElse {
then_condition: cond_context.then_branch.condition,
then_value: then_arg,
else_condition: cond_context.else_branch.as_ref().unwrap().condition,
else_value: else_arg,
};
let call_stack = cond_context.call_stack.clone();
Expand Down Expand Up @@ -667,13 +666,10 @@ impl<'f> Context<'f> {
)
.first();

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

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

else_value: previous_value,
};

Expand Down Expand Up @@ -907,13 +903,12 @@ mod test {
b0(v0: u1, v1: &mut Field):
enable_side_effects v0
v2 = load v1 -> Field
v3 = not v0
v4 = cast v0 as Field
v6 = sub Field 5, v2
v7 = mul v4, v6
v8 = add v2, v7
store v8 at v1
v9 = not v0
v3 = cast v0 as Field
v5 = sub Field 5, v2
v6 = mul v3, v5
v7 = add v2, v6
store v7 at v1
v8 = not v0
enable_side_effects u1 1
return
}
Expand Down Expand Up @@ -945,20 +940,19 @@ mod test {
b0(v0: u1, v1: &mut Field):
enable_side_effects v0
v2 = load v1 -> Field
v3 = not v0
v4 = cast v0 as Field
v6 = sub Field 5, v2
v7 = mul v4, v6
v8 = add v2, v7
store v8 at v1
v9 = not v0
enable_side_effects v9
v10 = load v1 -> Field
v11 = cast v9 as Field
v13 = sub Field 6, v10
v14 = mul v11, v13
v15 = add v10, v14
store v15 at v1
v3 = cast v0 as Field
v5 = sub Field 5, v2
v6 = mul v3, v5
v7 = add v2, v6
store v7 at v1
v8 = not v0
enable_side_effects v8
v9 = load v1 -> Field
v10 = cast v8 as Field
v12 = sub Field 6, v9
v13 = mul v10, v12
v14 = add v9, v13
store v14 at v1
enable_side_effects u1 1
return
}
Expand Down Expand Up @@ -1306,24 +1300,23 @@ mod test {
v9 = add v7, Field 1
v10 = cast v9 as u8
v11 = load v6 -> u8
v12 = not v5
v13 = cast v4 as Field
v14 = cast v11 as Field
v15 = sub v9, v14
v16 = mul v13, v15
v17 = add v14, v16
v18 = cast v17 as u8
store v18 at v6
v19 = not v5
enable_side_effects v19
v20 = load v6 -> u8
v12 = cast v4 as Field
v13 = cast v11 as Field
v14 = sub v9, v13
v15 = mul v12, v14
v16 = add v13, v15
v17 = cast v16 as u8
store v17 at v6
v18 = not v5
enable_side_effects v18
v19 = load v6 -> u8
v20 = cast v18 as Field
v21 = cast v19 as Field
v22 = cast v20 as Field
v24 = sub Field 0, v22
v25 = mul v21, v24
v26 = add v22, v25
v27 = cast v26 as u8
store v27 at v6
v23 = sub Field 0, v21
v24 = mul v20, v23
v25 = add v21, v24
v26 = cast v25 as u8
store v26 at v6
enable_side_effects u1 1
constrain v5 == u1 1
return
Expand Down
32 changes: 7 additions & 25 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 @@ -45,7 +45,7 @@ impl<'a> ValueMerger<'a> {

/// Merge two values a and b from separate basic blocks to a single value.
/// If these two values are numeric, the result will be
/// `then_condition * then_value + else_condition * else_value`.
/// `then_condition * (then_value - else_value) + else_value`.
/// Otherwise, if the values being merged are arrays, a new array will be made
/// recursively from combining each element of both input arrays.
///
Expand All @@ -54,7 +54,6 @@ 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 @@ -70,28 +69,26 @@ 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, else_condition, then_value, else_value)
self.merge_array_values(typ, then_condition, then_value, else_value)
}
typ @ Type::Slice(_) => {
self.merge_slice_values(typ, then_condition, else_condition, then_value, else_value)
self.merge_slice_values(typ, then_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 + (!c)*b`.
/// function would return the result of `if c { a } else { b }` as `c * (a-b) + 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 Down Expand Up @@ -155,7 +152,6 @@ impl<'a> ValueMerger<'a> {
&mut self,
typ: Type,
then_condition: ValueId,
else_condition: ValueId,
then_value: ValueId,
else_value: ValueId,
) -> ValueId {
Expand All @@ -170,7 +166,6 @@ 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 @@ -200,12 +195,7 @@ 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,
else_condition,
then_element,
else_element,
));
merged.push_back(self.merge_values(then_condition, then_element, else_element));
}
}

Expand All @@ -218,7 +208,6 @@ 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 @@ -276,12 +265,7 @@ 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,
else_condition,
then_element,
else_element,
));
merged.push_back(self.merge_values(then_condition, then_element, else_element));
}
}

Expand Down Expand Up @@ -330,7 +314,6 @@ 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 @@ -414,8 +397,7 @@ 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, else_condition, then_element, else_element);
let value = self.merge_values(then_condition, then_element, else_element);

array = self.insert_array_set(array, index, value, Some(condition)).first();
}
Expand Down
10 changes: 2 additions & 8 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,10 +66,9 @@ impl Context {

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

let typ = function.dfg.type_of_value(then_value);
Expand All @@ -85,12 +84,7 @@ impl Context {
call_stack,
);

let value = value_merger.merge_values(
then_condition,
else_condition,
then_value,
else_value,
);
let value = value_merger.merge_values(then_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 5f730e8

Please sign in to comment.