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

Jump to bottom

fix(ssa): Check if result of array set is used in value of another array set #6197

Merged
merged 7 commits into from
Oct 2, 2024
Merged

fix(ssa): Check if result of array set is used in value of another array set #6197

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
767a209
check if result of array set is used in value of another array set
vezenovm Oct 1, 2024
3827460
gate nested arrays check to brillig
vezenovm Oct 1, 2024
ef80140
reduce blow-up for non nestested arrays
vezenovm Oct 1, 2024
7cb4d55
cleanup
vezenovm Oct 1, 2024
2ae9d43
Update compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs
vezenovm Oct 1, 2024
64b5156
Update compiler/noirc_evaluator/src/ssa/opt/array_set.rs
vezenovm Oct 1, 2024
25060d3
Update compiler/noirc_evaluator/src/ssa/opt/array_set.rs
vezenovm Oct 1, 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
91 changes: 57 additions & 34 deletions compiler/noirc_evaluator/src/ssa/opt/array_set.rs
View file
Open in desktop
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>,
vezenovm marked this conversation as resolved.
Show resolved Hide resolved
) {
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
Loading