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 3 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
27 changes: 8 additions & 19 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 @@ -276,12 +276,7 @@ pub(crate) enum Instruction {
///
/// 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 },
}

impl Instruction {
Expand Down Expand Up @@ -523,14 +518,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),
},
}
}

Expand Down Expand Up @@ -585,10 +577,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);
}
}
Expand Down Expand Up @@ -738,7 +729,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 @@ -757,13 +748,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,12 +440,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 @@ -221,15 +221,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}")
}
}
}
Expand Down
46 changes: 20 additions & 26 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 @@ -669,13 +668,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 @@ -909,13 +905,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 @@ -947,20 +942,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
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 @@ -216,7 +206,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 @@ -274,12 +263,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 @@ -322,7 +306,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 @@ -406,8 +389,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