From 8b2ac2a14acc015f13a56cfac9e5b8b10275d425 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 19 Jul 2024 17:33:25 -0300 Subject: [PATCH 01/15] Push ArrayGet instructions backwards through IfElse instructions to avoid expensive array merges But with bad Rust --- compiler/noirc_evaluator/src/ssa.rs | 1 + .../noirc_evaluator/src/ssa/opt/array_get.rs | 126 ++++++++++++++++++ compiler/noirc_evaluator/src/ssa/opt/mod.rs | 1 + 3 files changed, 128 insertions(+) create mode 100644 compiler/noirc_evaluator/src/ssa/opt/array_get.rs diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 81327cec013..5adc20d3bd9 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -84,6 +84,7 @@ pub(crate) fn optimize_into_acir( // This pass must come immediately following `mem2reg` as the succeeding passes // may create an SSA which inlining fails to handle. .run_pass(Ssa::inline_functions_with_no_predicates, "After Inlining:") + .run_pass(Ssa::array_get_optimization, "After Array Get Optimizations:") .run_pass(Ssa::remove_if_else, "After Remove IfElse:") .run_pass(Ssa::fold_constants, "After Constant Folding:") .run_pass(Ssa::remove_enable_side_effects, "After EnableSideEffects removal:") diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_get.rs b/compiler/noirc_evaluator/src/ssa/opt/array_get.rs new file mode 100644 index 00000000000..58e24c3f670 --- /dev/null +++ b/compiler/noirc_evaluator/src/ssa/opt/array_get.rs @@ -0,0 +1,126 @@ +use std::rc::Rc; + +use crate::ssa::{ + ir::{ + dfg::CallStack, function::Function, instruction::Instruction, map::Id, types::Type, + value::ValueId, + }, + ssa_gen::Ssa, +}; +use fxhash::FxHashMap as HashMap; + +impl Ssa { + /// Map arrays with the last instruction that uses it + /// For this we simply process all the instructions in execution order + /// and update the map whenever there is a match + #[tracing::instrument(level = "trace", skip(self))] + pub(crate) fn array_get_optimization(mut self) -> Self { + for function in self.functions.values_mut() { + // This should match the check in flatten_cfg + if let crate::ssa::ir::function::RuntimeType::Brillig = function.runtime() { + continue; + } + + Context::default().optimize_array_get(function); + } + + self + } +} + +#[derive(Default)] +struct Context { + result_if_else: HashMap>, +} + +impl Context { + fn optimize_array_get(&mut self, function: &mut Function) { + let block = function.entry_block(); + let instructions = function.dfg[block].take_instructions(); + + for instruction_id in instructions { + let instruction = function.dfg[instruction_id].clone(); + + match &instruction { + Instruction::IfElse { then_value, .. } => { + let then_value = *then_value; + + // Only apply this optimization to IfElse where values are arrays + let Type::Array(..) = function.dfg.type_of_value(then_value) else { + continue; + }; + + let results = function.dfg.instruction_results(instruction_id); + let result = results[0]; + self.result_if_else.insert(result, instruction_id); + + function.dfg[block].instructions_mut().push(instruction_id); + } + Instruction::ArrayGet { array, index } => { + if let Some(if_else) = self.result_if_else.get(array) { + if let Instruction::IfElse { + then_condition, + then_value, + else_condition, + else_value, + .. + } = &function.dfg[*if_else] + { + let then_condition = *then_condition; + let then_value = *then_value; + let else_condition = *else_condition; + let else_value = *else_value; + + let then_value_type = function.dfg.type_of_value(then_value); + + let Type::Array(element_type, _) = then_value_type else { + panic!("ice: expected array type, got {:?}", then_value_type); + }; + let element_type: &Vec = &element_type; + + let then_result = function.dfg.insert_instruction_and_results( + Instruction::ArrayGet { array: then_value, index: *index }, + block, + Some(element_type.clone()), + CallStack::new(), // TODO: check callstack + ); + let then_result = then_result.first(); + + let else_result = function.dfg.insert_instruction_and_results( + Instruction::ArrayGet { array: else_value, index: *index }, + block, + Some(element_type.clone()), + CallStack::new(), // TODO: check callstack + ); + let else_result = else_result.first(); + + let new_result = function.dfg.insert_instruction_and_results( + Instruction::IfElse { + then_condition: then_condition, + then_value: then_result, + else_condition: else_condition, + else_value: else_result, + }, + block, + None, // TODO: are these needed? + CallStack::new(), // TODO: check callstack + ); + let new_result = new_result.first(); + + let results = function.dfg.instruction_results(instruction_id); + let result = results[0]; + function.dfg.set_value_from_id(result, new_result); + + continue; + } + } + + function.dfg[block].instructions_mut().push(instruction_id); + } + _ => { + function.dfg[block].instructions_mut().push(instruction_id); + } + } + } + } +} diff --git a/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 4e5fa262696..8f2851cf8ab 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -3,6 +3,7 @@ //! Each pass is generally expected to mutate the SSA IR into a gradually //! simpler form until the IR only has a single function remaining with 1 block within it. //! Generally, these passes are also expected to minimize the final amount of instructions. +mod array_get; mod array_set; mod as_slice_length; mod assert_constant; From 9a912ca79a392c23cbc31846a917d20b20867e51 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 19 Jul 2024 17:40:08 -0300 Subject: [PATCH 02/15] Add some comments --- .../noirc_evaluator/src/ssa/opt/array_get.rs | 39 ++++++++++++++++--- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_get.rs b/compiler/noirc_evaluator/src/ssa/opt/array_get.rs index 58e24c3f670..61fc1e7f5df 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_get.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_get.rs @@ -1,5 +1,3 @@ -use std::rc::Rc; - use crate::ssa::{ ir::{ dfg::CallStack, function::Function, instruction::Instruction, map::Id, types::Type, @@ -10,9 +8,19 @@ use crate::ssa::{ use fxhash::FxHashMap as HashMap; impl Ssa { - /// Map arrays with the last instruction that uses it - /// For this we simply process all the instructions in execution order - /// and update the map whenever there is a match + // Given an original IfElse instruction is this: + // + // v10 = if v0 then v2 else if v1 then v3 + // + // and a later ArrayGet instruction is this: + // + // v11 = array_get v4, index v4 + // + // we optimize it to this: + // + // v12 = array_get v2, index v4 + // v13 = array_get v3, index v4 + // v14 = if v0 then v12 else if v1 then v13 #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn array_get_optimization(mut self) -> Self { for function in self.functions.values_mut() { @@ -30,6 +38,8 @@ impl Ssa { #[derive(Default)] struct Context { + // Given an IfElse instruction, here we map its result to that instruction. + // We only capture such values if the IfElse values are arrays. result_if_else: HashMap>, } @@ -57,6 +67,7 @@ impl Context { function.dfg[block].instructions_mut().push(instruction_id); } Instruction::ArrayGet { array, index } => { + // If this array get is for an array that is the result of a previous IfElse... if let Some(if_else) = self.result_if_else.get(array) { if let Instruction::IfElse { then_condition, @@ -78,6 +89,17 @@ impl Context { }; let element_type: &Vec = &element_type; + // Given the original IfElse instruction is this: + // + // v10 = if v0 then v2 else if v1 then v3 + // + // and the ArrayGet instruction is this: + // + // v11 = array_get v4, index v4 + + // First create an instruction like this, for the then branch: + // + // v12 = array_get v2, index v4 let then_result = function.dfg.insert_instruction_and_results( Instruction::ArrayGet { array: then_value, index: *index }, block, @@ -86,6 +108,9 @@ impl Context { ); let then_result = then_result.first(); + // Then create an instruction like this, for the else branch: + // + // v13 = array_get v3, index v4 let else_result = function.dfg.insert_instruction_and_results( Instruction::ArrayGet { array: else_value, index: *index }, block, @@ -94,6 +119,9 @@ impl Context { ); let else_result = else_result.first(); + // Finally create an IfElse instruction like this: + // + // v14 = if v0 then v12 else if v1 then v13 let new_result = function.dfg.insert_instruction_and_results( Instruction::IfElse { then_condition: then_condition, @@ -107,6 +135,7 @@ impl Context { ); let new_result = new_result.first(); + // And replace the original instruction's value with this final value let results = function.dfg.instruction_results(instruction_id); let result = results[0]; function.dfg.set_value_from_id(result, new_result); From b4885a5efb07ba9f55d3e94a89a31af3bff3c535 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 19 Jul 2024 17:41:26 -0300 Subject: [PATCH 03/15] Extract dfg variable --- .../noirc_evaluator/src/ssa/opt/array_get.rs | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_get.rs b/compiler/noirc_evaluator/src/ssa/opt/array_get.rs index 61fc1e7f5df..c2fcd93ac6e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_get.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_get.rs @@ -46,25 +46,26 @@ struct Context { impl Context { fn optimize_array_get(&mut self, function: &mut Function) { let block = function.entry_block(); - let instructions = function.dfg[block].take_instructions(); + let dfg = &mut function.dfg; + let instructions = dfg[block].take_instructions(); for instruction_id in instructions { - let instruction = function.dfg[instruction_id].clone(); + let instruction = dfg[instruction_id].clone(); match &instruction { Instruction::IfElse { then_value, .. } => { let then_value = *then_value; // Only apply this optimization to IfElse where values are arrays - let Type::Array(..) = function.dfg.type_of_value(then_value) else { + let Type::Array(..) = dfg.type_of_value(then_value) else { continue; }; - let results = function.dfg.instruction_results(instruction_id); + let results = dfg.instruction_results(instruction_id); let result = results[0]; self.result_if_else.insert(result, instruction_id); - function.dfg[block].instructions_mut().push(instruction_id); + dfg[block].instructions_mut().push(instruction_id); } Instruction::ArrayGet { array, index } => { // If this array get is for an array that is the result of a previous IfElse... @@ -75,14 +76,14 @@ impl Context { else_condition, else_value, .. - } = &function.dfg[*if_else] + } = &dfg[*if_else] { let then_condition = *then_condition; let then_value = *then_value; let else_condition = *else_condition; let else_value = *else_value; - let then_value_type = function.dfg.type_of_value(then_value); + let then_value_type = dfg.type_of_value(then_value); let Type::Array(element_type, _) = then_value_type else { panic!("ice: expected array type, got {:?}", then_value_type); @@ -100,7 +101,7 @@ impl Context { // First create an instruction like this, for the then branch: // // v12 = array_get v2, index v4 - let then_result = function.dfg.insert_instruction_and_results( + let then_result = dfg.insert_instruction_and_results( Instruction::ArrayGet { array: then_value, index: *index }, block, Some(element_type.clone()), @@ -111,7 +112,7 @@ impl Context { // Then create an instruction like this, for the else branch: // // v13 = array_get v3, index v4 - let else_result = function.dfg.insert_instruction_and_results( + let else_result = dfg.insert_instruction_and_results( Instruction::ArrayGet { array: else_value, index: *index }, block, Some(element_type.clone()), @@ -122,7 +123,7 @@ impl Context { // Finally create an IfElse instruction like this: // // v14 = if v0 then v12 else if v1 then v13 - let new_result = function.dfg.insert_instruction_and_results( + let new_result = dfg.insert_instruction_and_results( Instruction::IfElse { then_condition: then_condition, then_value: then_result, @@ -136,18 +137,18 @@ impl Context { let new_result = new_result.first(); // And replace the original instruction's value with this final value - let results = function.dfg.instruction_results(instruction_id); + let results = dfg.instruction_results(instruction_id); let result = results[0]; - function.dfg.set_value_from_id(result, new_result); + dfg.set_value_from_id(result, new_result); continue; } } - function.dfg[block].instructions_mut().push(instruction_id); + dfg[block].instructions_mut().push(instruction_id); } _ => { - function.dfg[block].instructions_mut().push(instruction_id); + dfg[block].instructions_mut().push(instruction_id); } } } From 494b2b872a664bc17d670b9ffd5401e99f6d411c Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 19 Jul 2024 17:57:15 -0300 Subject: [PATCH 04/15] Small comment adjustment --- compiler/noirc_evaluator/src/ssa/opt/array_get.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_get.rs b/compiler/noirc_evaluator/src/ssa/opt/array_get.rs index c2fcd93ac6e..51d71f114ce 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_get.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_get.rs @@ -16,7 +16,7 @@ impl Ssa { // // v11 = array_get v4, index v4 // - // we optimize it to this: + // we optimize the latter to this: // // v12 = array_get v2, index v4 // v13 = array_get v3, index v4 From 50ca2ef7e10eb5a873b77554f5a6be64d4f3eb0b Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 22 Jul 2024 08:29:02 -0300 Subject: [PATCH 05/15] Remove extra check --- compiler/noirc_evaluator/src/ssa/opt/array_get.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_get.rs b/compiler/noirc_evaluator/src/ssa/opt/array_get.rs index 51d71f114ce..4c714ab4416 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_get.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_get.rs @@ -24,11 +24,6 @@ impl Ssa { #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn array_get_optimization(mut self) -> Self { for function in self.functions.values_mut() { - // This should match the check in flatten_cfg - if let crate::ssa::ir::function::RuntimeType::Brillig = function.runtime() { - continue; - } - Context::default().optimize_array_get(function); } From 539c5f9b3d8e7084b712eb1aa3793b270f38f6d5 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 22 Jul 2024 08:31:13 -0300 Subject: [PATCH 06/15] Fill out callstacks --- compiler/noirc_evaluator/src/ssa/opt/array_get.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_get.rs b/compiler/noirc_evaluator/src/ssa/opt/array_get.rs index 4c714ab4416..f1fdd2ba266 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_get.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_get.rs @@ -1,8 +1,5 @@ use crate::ssa::{ - ir::{ - dfg::CallStack, function::Function, instruction::Instruction, map::Id, types::Type, - value::ValueId, - }, + ir::{function::Function, instruction::Instruction, map::Id, types::Type, value::ValueId}, ssa_gen::Ssa, }; use fxhash::FxHashMap as HashMap; @@ -85,6 +82,8 @@ impl Context { }; let element_type: &Vec = &element_type; + let call_stack = dfg.get_call_stack(instruction_id); + // Given the original IfElse instruction is this: // // v10 = if v0 then v2 else if v1 then v3 @@ -100,7 +99,7 @@ impl Context { Instruction::ArrayGet { array: then_value, index: *index }, block, Some(element_type.clone()), - CallStack::new(), // TODO: check callstack + call_stack.clone(), ); let then_result = then_result.first(); @@ -111,7 +110,7 @@ impl Context { Instruction::ArrayGet { array: else_value, index: *index }, block, Some(element_type.clone()), - CallStack::new(), // TODO: check callstack + call_stack.clone(), ); let else_result = else_result.first(); @@ -126,8 +125,8 @@ impl Context { else_value: else_result, }, block, - None, // TODO: are these needed? - CallStack::new(), // TODO: check callstack + None, + call_stack, ); let new_result = new_result.first(); From c8df51fa971e8aeee6550945aee3e2dd952348b6 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 22 Jul 2024 08:36:16 -0300 Subject: [PATCH 07/15] Rename optimization --- compiler/noirc_evaluator/src/ssa.rs | 5 ++++- .../opt/{array_get.rs => array_get_from_if_else_result.rs} | 6 +++--- compiler/noirc_evaluator/src/ssa/opt/mod.rs | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) rename compiler/noirc_evaluator/src/ssa/opt/{array_get.rs => array_get_from_if_else_result.rs} (96%) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 5adc20d3bd9..2cfa86693e5 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -84,7 +84,10 @@ pub(crate) fn optimize_into_acir( // This pass must come immediately following `mem2reg` as the succeeding passes // may create an SSA which inlining fails to handle. .run_pass(Ssa::inline_functions_with_no_predicates, "After Inlining:") - .run_pass(Ssa::array_get_optimization, "After Array Get Optimizations:") + .run_pass( + Ssa::array_get_from_if_else_result_optimization, + "After Array Get From If Else Result Optimizations:", + ) .run_pass(Ssa::remove_if_else, "After Remove IfElse:") .run_pass(Ssa::fold_constants, "After Constant Folding:") .run_pass(Ssa::remove_enable_side_effects, "After EnableSideEffects removal:") diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_get.rs b/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs similarity index 96% rename from compiler/noirc_evaluator/src/ssa/opt/array_get.rs rename to compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs index f1fdd2ba266..b90eb003ccf 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_get.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs @@ -19,9 +19,9 @@ impl Ssa { // v13 = array_get v3, index v4 // v14 = if v0 then v12 else if v1 then v13 #[tracing::instrument(level = "trace", skip(self))] - pub(crate) fn array_get_optimization(mut self) -> Self { + pub(crate) fn array_get_from_if_else_result_optimization(mut self) -> Self { for function in self.functions.values_mut() { - Context::default().optimize_array_get(function); + Context::default().optimize_array_get_from_if_else_result(function); } self @@ -36,7 +36,7 @@ struct Context { } impl Context { - fn optimize_array_get(&mut self, function: &mut Function) { + fn optimize_array_get_from_if_else_result(&mut self, function: &mut Function) { let block = function.entry_block(); let dfg = &mut function.dfg; let instructions = dfg[block].take_instructions(); diff --git a/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 8f2851cf8ab..12d54b586b2 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -3,7 +3,7 @@ //! Each pass is generally expected to mutate the SSA IR into a gradually //! simpler form until the IR only has a single function remaining with 1 block within it. //! Generally, these passes are also expected to minimize the final amount of instructions. -mod array_get; +mod array_get_from_if_else_result; mod array_set; mod as_slice_length; mod assert_constant; From 660b428caacaa3dffe6d9074f1f4b664230e75de Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 22 Jul 2024 10:03:15 -0300 Subject: [PATCH 08/15] Add a test --- .../ssa/opt/array_get_from_if_else_result.rs | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs b/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs index b90eb003ccf..bc0f217834a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs @@ -148,3 +148,115 @@ impl Context { } } } + +#[cfg(test)] +mod test { + use std::rc::Rc; + + use crate::ssa::{ + function_builder::FunctionBuilder, + ir::{ + instruction::{Binary, Instruction}, + map::Id, + types::Type, + }, + }; + + #[test] + fn check_array_get_from_if_else_result_optimization() { + // acir(inline) fn main f0 { + // b0(v0: [Field; 3], v1: [Field; 3], v2: u1, v3: u32): + // v4 = not v2 + // v5 = if v2 then v0 else if v4 then v1 + // v6 = array_get v5, index v3 + // (no terminator instruction) + // } + + let main_id = Id::test_new(0); + let mut builder = FunctionBuilder::new("main".into(), main_id); + let v0 = builder.add_parameter(Type::Array(Rc::new(vec![Type::field()]), 3)); + let v1 = builder.add_parameter(Type::Array(Rc::new(vec![Type::field()]), 3)); + let v2 = builder.add_parameter(Type::bool()); + let v3 = builder.add_parameter(Type::unsigned(32)); + + let v4 = builder.insert_not(v2); + let v5 = builder + .insert_instruction( + Instruction::IfElse { + then_condition: v2, + then_value: v0, + else_condition: v4, + else_value: v1, + }, + None, + ) + .first(); + builder.insert_array_get(v5, v3, Type::field()); + + let ssa = builder.finish(); + println!("{ssa}"); + + // Expected output: + // acir(inline) fn main f0 { + // b0(v0: [Field; 3], v1: [Field; 3], v2: u1, v3: u32): + // v4 = not v2 + // v5 = if v2 then v0 else if v4 then v1 + // v7 = array_get v0, index v3 + // v8 = array_get v1, index v3 + // v9 = cast v2 as Field + // v10 = cast v4 as Field + // v11 = mul v9, v7 + // v12 = mul v10, v8 + // v13 = add v11, v12 + // (no terminator instruction) + // } + let ssa = ssa.array_get_from_if_else_result_optimization(); + println!("{ssa}"); + + let main = ssa.main(); + let instructions = main.dfg[main.entry_block()].instructions(); + + // Let's check only instructions v7..=v13 + let v7 = &main.dfg[instructions[2]]; + assert_eq!(v7, &Instruction::ArrayGet { array: v0, index: v3 }); + + let v8 = &main.dfg[instructions[3]]; + assert_eq!(v8, &Instruction::ArrayGet { array: v1, index: v3 }); + + let v9 = &main.dfg[instructions[4]]; + assert_eq!(v9, &Instruction::Cast(v2, Type::field())); + + let v10 = &main.dfg[instructions[5]]; + assert_eq!(v10, &Instruction::Cast(v4, Type::field())); + + let v11 = &main.dfg[instructions[6]]; + assert_eq!( + v11, + &Instruction::Binary(Binary { + lhs: main.dfg.instruction_results(instructions[4])[0], // v9 + rhs: main.dfg.instruction_results(instructions[2])[0], // v7 + operator: crate::ssa::ir::instruction::BinaryOp::Mul + }) + ); + + let v12 = &main.dfg[instructions[7]]; + assert_eq!( + v12, + &Instruction::Binary(Binary { + lhs: main.dfg.instruction_results(instructions[5])[0], // v10 + rhs: main.dfg.instruction_results(instructions[3])[0], // v8 + operator: crate::ssa::ir::instruction::BinaryOp::Mul + }) + ); + + let v13 = &main.dfg[instructions[8]]; + assert_eq!( + v13, + &Instruction::Binary(Binary { + lhs: main.dfg.instruction_results(instructions[6])[0], // v11 + rhs: main.dfg.instruction_results(instructions[7])[0], // v12 + operator: crate::ssa::ir::instruction::BinaryOp::Add + }) + ); + } +} From 7e8b0fb534384be6722cf239cf22e721d7cb79c8 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 22 Jul 2024 10:22:22 -0300 Subject: [PATCH 09/15] No need to keep track of previous IfElse instructions --- .../ssa/opt/array_get_from_if_else_result.rs | 213 ++++++++---------- 1 file changed, 93 insertions(+), 120 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs b/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs index bc0f217834a..466b95210fd 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs @@ -1,8 +1,7 @@ use crate::ssa::{ - ir::{function::Function, instruction::Instruction, map::Id, types::Type, value::ValueId}, + ir::{function::Function, instruction::Instruction, types::Type, value::Value}, ssa_gen::Ssa, }; -use fxhash::FxHashMap as HashMap; impl Ssa { // Given an original IfElse instruction is this: @@ -21,131 +20,105 @@ impl Ssa { #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn array_get_from_if_else_result_optimization(mut self) -> Self { for function in self.functions.values_mut() { - Context::default().optimize_array_get_from_if_else_result(function); + optimize_array_get_from_if_else_result(function); } self } } -#[derive(Default)] -struct Context { - // Given an IfElse instruction, here we map its result to that instruction. - // We only capture such values if the IfElse values are arrays. - result_if_else: HashMap>, -} - -impl Context { - fn optimize_array_get_from_if_else_result(&mut self, function: &mut Function) { - let block = function.entry_block(); - let dfg = &mut function.dfg; - let instructions = dfg[block].take_instructions(); - - for instruction_id in instructions { - let instruction = dfg[instruction_id].clone(); - - match &instruction { - Instruction::IfElse { then_value, .. } => { - let then_value = *then_value; - - // Only apply this optimization to IfElse where values are arrays - let Type::Array(..) = dfg.type_of_value(then_value) else { - continue; - }; - - let results = dfg.instruction_results(instruction_id); - let result = results[0]; - self.result_if_else.insert(result, instruction_id); - - dfg[block].instructions_mut().push(instruction_id); - } - Instruction::ArrayGet { array, index } => { - // If this array get is for an array that is the result of a previous IfElse... - if let Some(if_else) = self.result_if_else.get(array) { - if let Instruction::IfElse { - then_condition, - then_value, - else_condition, - else_value, - .. - } = &dfg[*if_else] - { - let then_condition = *then_condition; - let then_value = *then_value; - let else_condition = *else_condition; - let else_value = *else_value; - - let then_value_type = dfg.type_of_value(then_value); - - let Type::Array(element_type, _) = then_value_type else { - panic!("ice: expected array type, got {:?}", then_value_type); - }; - let element_type: &Vec = &element_type; - - let call_stack = dfg.get_call_stack(instruction_id); - - // Given the original IfElse instruction is this: - // - // v10 = if v0 then v2 else if v1 then v3 - // - // and the ArrayGet instruction is this: - // - // v11 = array_get v4, index v4 - - // First create an instruction like this, for the then branch: - // - // v12 = array_get v2, index v4 - let then_result = dfg.insert_instruction_and_results( - Instruction::ArrayGet { array: then_value, index: *index }, - block, - Some(element_type.clone()), - call_stack.clone(), - ); - let then_result = then_result.first(); - - // Then create an instruction like this, for the else branch: - // - // v13 = array_get v3, index v4 - let else_result = dfg.insert_instruction_and_results( - Instruction::ArrayGet { array: else_value, index: *index }, - block, - Some(element_type.clone()), - call_stack.clone(), - ); - let else_result = else_result.first(); - - // Finally create an IfElse instruction like this: - // - // v14 = if v0 then v12 else if v1 then v13 - let new_result = dfg.insert_instruction_and_results( - Instruction::IfElse { - then_condition: then_condition, - then_value: then_result, - else_condition: else_condition, - else_value: else_result, - }, - block, - None, - call_stack, - ); - let new_result = new_result.first(); - - // And replace the original instruction's value with this final value - let results = dfg.instruction_results(instruction_id); - let result = results[0]; - dfg.set_value_from_id(result, new_result); - - continue; - } - } +fn optimize_array_get_from_if_else_result(function: &mut Function) { + let block_id = function.entry_block(); + let dfg = &mut function.dfg; + let instructions = dfg[block_id].take_instructions(); + + for instruction_id in instructions { + // Only apply this optimization to ArrayGet + let Instruction::ArrayGet { array, index } = &dfg[instruction_id].clone() else { + dfg[block_id].instructions_mut().push(instruction_id); + continue; + }; + + // Only if getting an array from a previous instruction + let Value::Instruction { instruction, .. } = &dfg[dfg.resolve(*array)] else { + dfg[block_id].instructions_mut().push(instruction_id); + continue; + }; + + // Only if that previous instruction is an IfElse + let Instruction::IfElse { then_condition, then_value, else_condition, else_value } = + &dfg[*instruction] + else { + dfg[block_id].instructions_mut().push(instruction_id); + continue; + }; + + let then_condition = *then_condition; + let then_value = *then_value; + let else_condition = *else_condition; + let else_value = *else_value; + + let then_value_type = dfg.type_of_value(then_value); + + // Only if the IfElse instruction has an array type + let Type::Array(element_type, _) = then_value_type else { + dfg[block_id].instructions_mut().push(instruction_id); + continue; + }; + + let element_type: &Vec = &element_type; + let call_stack = dfg.get_call_stack(instruction_id); + + // Given the original IfElse instruction is this: + // + // v10 = if v0 then v2 else if v1 then v3 + // + // and the ArrayGet instruction is this: + // + // v11 = array_get v4, index v4 + + // First create an instruction like this, for the then branch: + // + // v12 = array_get v2, index v4 + let then_result = dfg.insert_instruction_and_results( + Instruction::ArrayGet { array: then_value, index: *index }, + block_id, + Some(element_type.clone()), + call_stack.clone(), + ); + let then_result = then_result.first(); + + // Then create an instruction like this, for the else branch: + // + // v13 = array_get v3, index v4 + let else_result = dfg.insert_instruction_and_results( + Instruction::ArrayGet { array: else_value, index: *index }, + block_id, + Some(element_type.clone()), + call_stack.clone(), + ); + let else_result = else_result.first(); + + // Finally create an IfElse instruction like this: + // + // v14 = if v0 then v12 else if v1 then v13 + let new_result = dfg.insert_instruction_and_results( + Instruction::IfElse { + then_condition: then_condition, + then_value: then_result, + else_condition: else_condition, + else_value: else_result, + }, + block_id, + None, + call_stack, + ); + let new_result = new_result.first(); - dfg[block].instructions_mut().push(instruction_id); - } - _ => { - dfg[block].instructions_mut().push(instruction_id); - } - } - } + // And replace the original instruction's value with this final value + let results = dfg.instruction_results(instruction_id); + let result = results[0]; + dfg.set_value_from_id(result, new_result); } } From f7f8763dcdb89f2b93d6bfe4a028d0d4c284ad4c Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 22 Jul 2024 10:30:46 -0300 Subject: [PATCH 10/15] clippy --- .../ssa/opt/array_get_from_if_else_result.rs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs b/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs index 466b95210fd..33340ae2601 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs @@ -28,20 +28,20 @@ impl Ssa { } fn optimize_array_get_from_if_else_result(function: &mut Function) { - let block_id = function.entry_block(); + let block = function.entry_block(); let dfg = &mut function.dfg; - let instructions = dfg[block_id].take_instructions(); + let instructions = dfg[block].take_instructions(); for instruction_id in instructions { // Only apply this optimization to ArrayGet let Instruction::ArrayGet { array, index } = &dfg[instruction_id].clone() else { - dfg[block_id].instructions_mut().push(instruction_id); + dfg[block].instructions_mut().push(instruction_id); continue; }; // Only if getting an array from a previous instruction let Value::Instruction { instruction, .. } = &dfg[dfg.resolve(*array)] else { - dfg[block_id].instructions_mut().push(instruction_id); + dfg[block].instructions_mut().push(instruction_id); continue; }; @@ -49,7 +49,7 @@ fn optimize_array_get_from_if_else_result(function: &mut Function) { let Instruction::IfElse { then_condition, then_value, else_condition, else_value } = &dfg[*instruction] else { - dfg[block_id].instructions_mut().push(instruction_id); + dfg[block].instructions_mut().push(instruction_id); continue; }; @@ -62,7 +62,7 @@ fn optimize_array_get_from_if_else_result(function: &mut Function) { // Only if the IfElse instruction has an array type let Type::Array(element_type, _) = then_value_type else { - dfg[block_id].instructions_mut().push(instruction_id); + dfg[block].instructions_mut().push(instruction_id); continue; }; @@ -82,7 +82,7 @@ fn optimize_array_get_from_if_else_result(function: &mut Function) { // v12 = array_get v2, index v4 let then_result = dfg.insert_instruction_and_results( Instruction::ArrayGet { array: then_value, index: *index }, - block_id, + block, Some(element_type.clone()), call_stack.clone(), ); @@ -93,7 +93,7 @@ fn optimize_array_get_from_if_else_result(function: &mut Function) { // v13 = array_get v3, index v4 let else_result = dfg.insert_instruction_and_results( Instruction::ArrayGet { array: else_value, index: *index }, - block_id, + block, Some(element_type.clone()), call_stack.clone(), ); @@ -104,12 +104,12 @@ fn optimize_array_get_from_if_else_result(function: &mut Function) { // v14 = if v0 then v12 else if v1 then v13 let new_result = dfg.insert_instruction_and_results( Instruction::IfElse { - then_condition: then_condition, + then_condition, then_value: then_result, - else_condition: else_condition, + else_condition, else_value: else_result, }, - block_id, + block, None, call_stack, ); From c81bb945c2d90629cbf3670cf625eb262448b2db Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 22 Jul 2024 11:21:05 -0300 Subject: [PATCH 11/15] Don't apply optimization to array of composite types --- .../src/ssa/opt/array_get_from_if_else_result.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs b/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs index 33340ae2601..f2b2b91752f 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs @@ -61,12 +61,19 @@ fn optimize_array_get_from_if_else_result(function: &mut Function) { let then_value_type = dfg.type_of_value(then_value); // Only if the IfElse instruction has an array type - let Type::Array(element_type, _) = then_value_type else { + let Type::Array(element_types, _) = then_value_type else { dfg[block].instructions_mut().push(instruction_id); continue; }; - let element_type: &Vec = &element_type; + let element_types: &Vec = &element_types; + + // Only if the array isn't of a tuple type (or a composite type) + if element_types.len() != 1 { + dfg[block].instructions_mut().push(instruction_id); + continue; + } + let call_stack = dfg.get_call_stack(instruction_id); // Given the original IfElse instruction is this: @@ -83,7 +90,7 @@ fn optimize_array_get_from_if_else_result(function: &mut Function) { let then_result = dfg.insert_instruction_and_results( Instruction::ArrayGet { array: then_value, index: *index }, block, - Some(element_type.clone()), + Some(element_types.clone()), call_stack.clone(), ); let then_result = then_result.first(); @@ -94,7 +101,7 @@ fn optimize_array_get_from_if_else_result(function: &mut Function) { let else_result = dfg.insert_instruction_and_results( Instruction::ArrayGet { array: else_value, index: *index }, block, - Some(element_type.clone()), + Some(element_types.clone()), call_stack.clone(), ); let else_result = else_result.first(); From 7319ddbc5c9a5527aa84fe09e428845e3f6c2dc6 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 22 Jul 2024 11:32:23 -0300 Subject: [PATCH 12/15] Don't optimize if index is constant --- .../src/ssa/opt/array_get_from_if_else_result.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs b/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs index f2b2b91752f..6ec56d3fcbe 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs @@ -39,6 +39,12 @@ fn optimize_array_get_from_if_else_result(function: &mut Function) { continue; }; + // Don't optimize if the index is a constant (this is optimized later on in a different way) + if let Value::NumericConstant { .. } = &dfg[dfg.resolve(*index)] { + dfg[block].instructions_mut().push(instruction_id); + continue; + } + // Only if getting an array from a previous instruction let Value::Instruction { instruction, .. } = &dfg[dfg.resolve(*array)] else { dfg[block].instructions_mut().push(instruction_id); From 30b79de79822d27fe4aac04e54b39202f17833d5 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 23 Aug 2024 13:37:54 +0000 Subject: [PATCH 13/15] chore: add regression test showing large array merges --- .../execution_success/regression_5501/Nargo.toml | 7 +++++++ .../execution_success/regression_5501/Prover.toml | 4 ++++ .../execution_success/regression_5501/src/main.nr | 8 ++++++++ 3 files changed, 19 insertions(+) create mode 100644 test_programs/execution_success/regression_5501/Nargo.toml create mode 100644 test_programs/execution_success/regression_5501/Prover.toml create mode 100644 test_programs/execution_success/regression_5501/src/main.nr diff --git a/test_programs/execution_success/regression_5501/Nargo.toml b/test_programs/execution_success/regression_5501/Nargo.toml new file mode 100644 index 00000000000..46b7d4e6306 --- /dev/null +++ b/test_programs/execution_success/regression_5501/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_5501" +type = "bin" +authors = [""] +compiler_version = ">=0.33.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_success/regression_5501/Prover.toml b/test_programs/execution_success/regression_5501/Prover.toml new file mode 100644 index 00000000000..ace50af2637 --- /dev/null +++ b/test_programs/execution_success/regression_5501/Prover.toml @@ -0,0 +1,4 @@ +array_a = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0] +array_b = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0] +is_b = false +whatever_index = 0 diff --git a/test_programs/execution_success/regression_5501/src/main.nr b/test_programs/execution_success/regression_5501/src/main.nr new file mode 100644 index 00000000000..0a69b7ac021 --- /dev/null +++ b/test_programs/execution_success/regression_5501/src/main.nr @@ -0,0 +1,8 @@ +fn main(array_a: [Field; 100], array_b: [Field; 100], is_b: bool, whatever_index: u32) { + let mut array = array_a; + if is_b { + array = array_b; + } + + assert(array[whatever_index] == 0); +} \ No newline at end of file From 1ea506fe42540d80b1e9346f1470be4d54dba894 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 23 Aug 2024 14:18:52 +0000 Subject: [PATCH 14/15] . --- test_programs/execution_success/regression_5501/src/main.nr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test_programs/execution_success/regression_5501/src/main.nr b/test_programs/execution_success/regression_5501/src/main.nr index 0a69b7ac021..89be615cda3 100644 --- a/test_programs/execution_success/regression_5501/src/main.nr +++ b/test_programs/execution_success/regression_5501/src/main.nr @@ -2,7 +2,7 @@ fn main(array_a: [Field; 100], array_b: [Field; 100], is_b: bool, whatever_index let mut array = array_a; if is_b { array = array_b; - } + } assert(array[whatever_index] == 0); -} \ No newline at end of file +} From 70368e2fd46862573e7b5ca3d558efde3ae6d20c Mon Sep 17 00:00:00 2001 From: Tom French Date: Wed, 8 Jan 2025 00:00:48 +0000 Subject: [PATCH 15/15] . --- .../ssa/opt/array_get_from_if_else_result.rs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs b/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs index 6ec56d3fcbe..7311bf5b143 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs @@ -80,7 +80,7 @@ fn optimize_array_get_from_if_else_result(function: &mut Function) { continue; } - let call_stack = dfg.get_call_stack(instruction_id); + let call_stack_id = dfg.get_instruction_call_stack_id(instruction_id); // Given the original IfElse instruction is this: // @@ -97,7 +97,7 @@ fn optimize_array_get_from_if_else_result(function: &mut Function) { Instruction::ArrayGet { array: then_value, index: *index }, block, Some(element_types.clone()), - call_stack.clone(), + call_stack_id, ); let then_result = then_result.first(); @@ -108,7 +108,7 @@ fn optimize_array_get_from_if_else_result(function: &mut Function) { Instruction::ArrayGet { array: else_value, index: *index }, block, Some(element_types.clone()), - call_stack.clone(), + call_stack_id, ); let else_result = else_result.first(); @@ -124,7 +124,7 @@ fn optimize_array_get_from_if_else_result(function: &mut Function) { }, block, None, - call_stack, + call_stack_id, ); let new_result = new_result.first(); @@ -137,14 +137,14 @@ fn optimize_array_get_from_if_else_result(function: &mut Function) { #[cfg(test)] mod test { - use std::rc::Rc; + use std::sync::Arc; use crate::ssa::{ function_builder::FunctionBuilder, ir::{ instruction::{Binary, Instruction}, map::Id, - types::Type, + types::{NumericType, Type}, }, }; @@ -160,8 +160,8 @@ mod test { let main_id = Id::test_new(0); let mut builder = FunctionBuilder::new("main".into(), main_id); - let v0 = builder.add_parameter(Type::Array(Rc::new(vec![Type::field()]), 3)); - let v1 = builder.add_parameter(Type::Array(Rc::new(vec![Type::field()]), 3)); + let v0 = builder.add_parameter(Type::Array(Arc::new(vec![Type::field()]), 3)); + let v1 = builder.add_parameter(Type::Array(Arc::new(vec![Type::field()]), 3)); let v2 = builder.add_parameter(Type::bool()); let v3 = builder.add_parameter(Type::unsigned(32)); @@ -210,10 +210,10 @@ mod test { assert_eq!(v8, &Instruction::ArrayGet { array: v1, index: v3 }); let v9 = &main.dfg[instructions[4]]; - assert_eq!(v9, &Instruction::Cast(v2, Type::field())); + assert_eq!(v9, &Instruction::Cast(v2, NumericType::NativeField)); let v10 = &main.dfg[instructions[5]]; - assert_eq!(v10, &Instruction::Cast(v4, Type::field())); + assert_eq!(v10, &Instruction::Cast(v4, NumericType::NativeField)); let v11 = &main.dfg[instructions[6]]; assert_eq!(