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

chore: remove some _else_condition tech debt #6522

Merged
merged 7 commits into from
Nov 19, 2024
Merged

chore: remove some _else_condition tech debt #6522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2d55b39
chore: remove some `_else_condition` tech debt
TomAFrench Nov 14, 2024
205bfe5
chore: update test outputs
TomAFrench Nov 18, 2024
1b3f07e
Merge branch 'master' into tf/clean-up-else-conditions
TomAFrench Nov 18, 2024
90f8397
Merge branch 'master' into tf/clean-up-else-conditions
TomAFrench Nov 18, 2024
97d7167
.
TomAFrench Nov 18, 2024
f473d1b
Merge branch 'master' into tf/clean-up-else-conditions
TomAFrench Nov 19, 2024
d6c0198
chore: update tests
TomAFrench Nov 19, 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
30 changes: 8 additions & 22 deletions compiler/noirc_evaluator/src/ssa/ir/instruction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,15 +273,7 @@
/// 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 @@ -356,7 +348,7 @@
// We can deduplicate these instructions if we know the predicate is also the same.
Constrain(..) | RangeCheck { .. } => deduplicate_with_predicate,

// This should never be side-effectful

Check warning on line 351 in compiler/noirc_evaluator/src/ssa/ir/instruction.rs

View workflow job for this annotation

GitHub Actions / Code 

Unknown word (effectful)
MakeArray { .. } => true,

// These can have different behavior depending on the EnableSideEffectsIf context.
Expand Down Expand Up @@ -536,14 +528,11 @@
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 @@
| 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 @@
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 @@

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading