From 594ec91de55c4cf191d7cdc94a00bb16711cd430 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 2 Oct 2024 10:49:59 -0400 Subject: [PATCH] fix(ssa): Check if result of array set is used in value of another array set (#6197) # Description ## Problem\* Resolves ## Summary\* Fix for failure in the `test_barycentric` of the `blob` lib in `noir-protocol-circuits`. The test now passes when Brillig's `MAX_STACK_SIZE` is increased to 32k. I included an additional map to track whether an array set result was used as a value in another array set. We can not allow for inner nested array sets to be mutable across blocks as the full nested array may be used in another block. We could add additional logic to check whether the parent array set will be marked mutable, but this seems overly complex for little benefit at the moment as this fix showed a quite small size regression. ## Additional Context ## Documentation\* Check one: - [X] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [X] I have tested the changes locally. - [X] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../noirc_evaluator/src/ssa/opt/array_set.rs | 91 ++++++++++++------- 1 file changed, 57 insertions(+), 34 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs index 267a2c105f2..e7d765949b9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs @@ -2,7 +2,7 @@ use crate::ssa::{ ir::{ basic_block::BasicBlockId, dfg::DataFlowGraph, - function::Function, + function::{Function, RuntimeType}, instruction::{Instruction, InstructionId, TerminatorInstruction}, types::Type::{Array, Slice}, value::ValueId, @@ -34,14 +34,17 @@ impl Function { let mut array_to_last_use = HashMap::default(); let mut instructions_to_update = HashSet::default(); let mut arrays_from_load = HashSet::default(); + let mut inner_nested_arrays = HashMap::default(); for block in reachable_blocks.iter() { analyze_last_uses( &self.dfg, *block, + matches!(self.runtime(), RuntimeType::Brillig), &mut array_to_last_use, &mut instructions_to_update, &mut arrays_from_load, + &mut inner_nested_arrays, ); } for block in reachable_blocks { @@ -55,9 +58,11 @@ impl Function { fn analyze_last_uses( dfg: &DataFlowGraph, block_id: BasicBlockId, + is_brillig_func: bool, array_to_last_use: &mut HashMap, instructions_that_can_be_made_mutable: &mut HashSet, arrays_from_load: &mut HashSet, + inner_nested_arrays: &mut HashMap, ) { let block = &dfg[block_id]; @@ -70,12 +75,22 @@ fn analyze_last_uses( instructions_that_can_be_made_mutable.remove(&existing); } } - Instruction::ArraySet { array, .. } => { + Instruction::ArraySet { array, value, .. } => { let array = dfg.resolve(*array); if let Some(existing) = array_to_last_use.insert(array, *instruction_id) { instructions_that_can_be_made_mutable.remove(&existing); } + if is_brillig_func { + let value = dfg.resolve(*value); + + if let Some(existing) = inner_nested_arrays.get(&value) { + instructions_that_can_be_made_mutable.remove(existing); + } + let result = dfg.instruction_results(*instruction_id)[0]; + inner_nested_arrays.insert(result, *instruction_id); + } + // If the array we are setting does not come from a load we can safely mark it mutable. // If the array comes from a load we may potentially being mutating an array at a reference // that is loaded from by other values. @@ -169,29 +184,31 @@ mod tests { // from and cloned in a loop. If the array is inadvertently marked mutable, and is cloned in a previous iteration // of the loop, its clone will also be altered. // - // acir(inline) fn main f0 { + // brillig fn main f0 { // b0(): - // v2 = allocate - // store [Field 0, Field 0, Field 0, Field 0, Field 0] at v2 // v3 = allocate - // store [Field 0, Field 0, Field 0, Field 0, Field 0] at v3 + // store [[Field 0, Field 0, Field 0, Field 0, Field 0], [Field 0, Field 0, Field 0, Field 0, Field 0]] at v3 + // v4 = allocate + // store [[Field 0, Field 0, Field 0, Field 0, Field 0], [Field 0, Field 0, Field 0, Field 0, Field 0]] at v4 // jmp b1(u32 0) - // b1(v5: u32): - // v7 = lt v5, u32 5 - // jmpif v7 then: b3, else: b2 + // b1(v6: u32): + // v8 = lt v6, u32 5 + // jmpif v8 then: b3, else: b2 // b3(): - // v8 = eq v5, u32 5 - // jmpif v8 then: b4, else: b5 + // v9 = eq v6, u32 5 + // jmpif v9 then: b4, else: b5 // b4(): - // v9 = load v2 - // store v9 at v3 + // v10 = load v3 + // store v10 at v4 // jmp b5() // b5(): - // v10 = load v2 - // v12 = array_set v10, index v5, value Field 20 - // store v12 at v2 - // v14 = add v5, u32 1 - // jmp b1(v14) + // v11 = load v3 + // v13 = array_get v11, index Field 0 + // v14 = array_set v13, index v6, value Field 20 + // v15 = array_set v11, index v6, value v14 + // store v15 at v3 + // v17 = add v6, u32 1 + // jmp b1(v17) // b2(): // return // } @@ -203,13 +220,16 @@ mod tests { let zero = builder.field_constant(0u128); let array_constant = builder.array_constant(vector![zero, zero, zero, zero, zero], array_type.clone()); + let nested_array_type = Type::Array(Arc::new(vec![array_type.clone()]), 2); + let nested_array_constant = builder + .array_constant(vector![array_constant, array_constant], nested_array_type.clone()); - let v2 = builder.insert_allocate(array_type.clone()); + let v3 = builder.insert_allocate(array_type.clone()); - builder.insert_store(v2, array_constant); + builder.insert_store(v3, nested_array_constant); - let v3 = builder.insert_allocate(array_type.clone()); - builder.insert_store(v3, array_constant); + let v4 = builder.insert_allocate(array_type.clone()); + builder.insert_store(v4, nested_array_constant); let b1 = builder.insert_block(); let zero_u32 = builder.numeric_constant(0u128, Type::unsigned(32)); @@ -219,35 +239,38 @@ mod tests { builder.switch_to_block(b1); let v5 = builder.add_block_parameter(b1, Type::unsigned(32)); let five = builder.numeric_constant(5u128, Type::unsigned(32)); - let v7 = builder.insert_binary(v5, BinaryOp::Lt, five); + let v8 = builder.insert_binary(v5, BinaryOp::Lt, five); let b2 = builder.insert_block(); let b3 = builder.insert_block(); let b4 = builder.insert_block(); let b5 = builder.insert_block(); - builder.terminate_with_jmpif(v7, b3, b2); + builder.terminate_with_jmpif(v8, b3, b2); // Loop body // b3 is the if statement conditional builder.switch_to_block(b3); let two = builder.numeric_constant(5u128, Type::unsigned(32)); - let v8 = builder.insert_binary(v5, BinaryOp::Eq, two); - builder.terminate_with_jmpif(v8, b4, b5); + let v9 = builder.insert_binary(v5, BinaryOp::Eq, two); + builder.terminate_with_jmpif(v9, b4, b5); // b4 is the rest of the loop after the if statement builder.switch_to_block(b4); - let v9 = builder.insert_load(v2, array_type.clone()); - builder.insert_store(v3, v9); + let v10 = builder.insert_load(v3, nested_array_type.clone()); + builder.insert_store(v4, v10); builder.terminate_with_jmp(b5, vec![]); builder.switch_to_block(b5); - let v10 = builder.insert_load(v2, array_type.clone()); + let v11 = builder.insert_load(v3, nested_array_type.clone()); let twenty = builder.field_constant(20u128); - let v12 = builder.insert_array_set(v10, v5, twenty); - builder.insert_store(v2, v12); + let v13 = builder.insert_array_get(v11, zero, array_type.clone()); + let v14 = builder.insert_array_set(v13, v5, twenty); + let v15 = builder.insert_array_set(v11, v5, v14); + + builder.insert_store(v3, v15); let one = builder.numeric_constant(1u128, Type::unsigned(32)); - let v14 = builder.insert_binary(v5, BinaryOp::Add, one); - builder.terminate_with_jmp(b1, vec![v14]); + let v17 = builder.insert_binary(v5, BinaryOp::Add, one); + builder.terminate_with_jmp(b1, vec![v17]); builder.switch_to_block(b2); builder.terminate_with_return(vec![]); @@ -265,7 +288,7 @@ mod tests { .filter(|instruction| matches!(&main.dfg[**instruction], Instruction::ArraySet { .. })) .collect::>(); - assert_eq!(array_set_instructions.len(), 1); + assert_eq!(array_set_instructions.len(), 2); if let Instruction::ArraySet { mutable, .. } = &main.dfg[*array_set_instructions[0]] { // The single array set should not be marked mutable assert!(!mutable);