Skip to content

Commit

Permalink
fix(ssa): Check if result of array set is used in value of another ar…
Browse files Browse the repository at this point in the history 
…ray set (noir-lang#6197)

# Description

## Problem\*

Resolves <!-- Link to GitHub Issue -->

## 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.
  • Loading branch information
@vezenovm
vezenovm authored Oct 2, 2024
1 parent 054e48b commit 594ec91
Showing 1 changed file with 57 additions and 34 deletions.
91 changes: 57 additions & 34 deletions compiler/noirc_evaluator/src/ssa/opt/array_set.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand All @@ -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<ValueId, InstructionId>,
instructions_that_can_be_made_mutable: &mut HashSet<InstructionId>,
arrays_from_load: &mut HashSet<ValueId>,
inner_nested_arrays: &mut HashMap<ValueId, InstructionId>,
) {
let block = &dfg[block_id];

Expand All @@ -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.
Expand Down Expand Up @@ -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
// }
Expand All @@ -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));
Expand All @@ -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![]);
Expand All @@ -265,7 +288,7 @@ mod tests {
.filter(|instruction| matches!(&main.dfg[**instruction], Instruction::ArraySet { .. }))
.collect::<Vec<_>>();

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);
Expand Down

0 comments on commit 594ec91

Please sign in to comment.