From 2d55b39194ae09fa14994de026ae74345531fdd3 Mon Sep 17 00:00:00 2001 From: Tom French Date: Thu, 14 Nov 2024 21:05:21 +0000 Subject: [PATCH 1/4] chore: remove some `_else_condition` tech debt --- .../noirc_evaluator/src/ssa/ir/instruction.rs | 27 +++++----------- .../src/ssa/ir/instruction/call.rs | 8 ++--- .../noirc_evaluator/src/ssa/ir/printer.rs | 8 ++--- .../src/ssa/opt/flatten_cfg.rs | 6 +--- .../src/ssa/opt/flatten_cfg/value_merger.rs | 32 ++++--------------- .../src/ssa/opt/remove_if_else.rs | 10 ++---- 6 files changed, 22 insertions(+), 69 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 254a0afe88b..a4c1e41dfd6 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -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 { @@ -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), + }, } } @@ -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); } } @@ -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) { @@ -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, ); diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 9dbd2c56993..476ec0d0f52 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -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]) } diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index 21de85dbecb..eb55746d852 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -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}") } } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 54c21a68ea2..e5258d19a96 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -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(); @@ -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, }; diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs index 799378b1678..84baaac7778 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs @@ -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. /// @@ -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 { @@ -70,15 +69,14 @@ 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"), @@ -86,12 +84,11 @@ impl<'a> ValueMerger<'a> { } /// 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 { @@ -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 { @@ -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, @@ -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)); } } @@ -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 { @@ -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)); } } @@ -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, @@ -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(); } diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs index c387e0b6234..8076bc3cc99 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -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); @@ -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); From 205bfe5804aa43efa75656c9925a558e87f670e3 Mon Sep 17 00:00:00 2001 From: Tom French Date: Mon, 18 Nov 2024 12:42:42 +0000 Subject: [PATCH 2/4] chore: update test outputs --- .../src/ssa/opt/flatten_cfg.rs | 40 +++++++++---------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index e5258d19a96..4fcd5ba8c02 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -907,13 +907,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 } @@ -945,20 +944,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 } From 97d7167167b99e58fb36b6000519bbfc8f99fcce Mon Sep 17 00:00:00 2001 From: Tom French Date: Mon, 18 Nov 2024 16:42:23 +0000 Subject: [PATCH 3/4] . --- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index df62502a93f..081c1f3afdf 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -273,9 +273,6 @@ 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_value: ValueId }, /// Creates a new array or slice. From d6c019802b2d58ab8c2d19601427dc140b4503c0 Mon Sep 17 00:00:00 2001 From: Tom French Date: Tue, 19 Nov 2024 13:23:12 +0000 Subject: [PATCH 4/4] chore: update tests --- .../src/ssa/opt/flatten_cfg.rs | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 37692a31cf4..61a93aee58d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -1300,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