From af686e3652e1e537e894a04fccdf0af1c8e1c43b Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 6 Aug 2024 17:12:41 -0300 Subject: [PATCH 01/29] Replace unused ArrayGet/Set with constrain if possibly out of bounds --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 149 +++++++++++++++++++- 1 file changed, 144 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 24519d530ee..6e7ce05199c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -7,11 +7,12 @@ use crate::ssa::{ basic_block::{BasicBlock, BasicBlockId}, dfg::DataFlowGraph, function::Function, - instruction::{Instruction, InstructionId, Intrinsic}, + instruction::{Binary, BinaryOp, Instruction, InstructionId, Intrinsic}, post_order::PostOrder, + types::Type, value::{Value, ValueId}, }, - ssa_gen::Ssa, + ssa_gen::{Ssa, SSA_WORD_SIZE}, }; impl Ssa { @@ -79,12 +80,52 @@ impl Context { let block = &function.dfg[block_id]; self.mark_terminator_values_as_used(function, block); - for instruction_id in block.instructions().iter().rev() { + let instructions_len = block.instructions().len(); + + // Indexes of instructions that might be out of bounds. + // We'll remove those, but before that we'll insert bounds checks for them. + let mut possible_index_out_of_bounds_indexes = Vec::new(); + + for (instruction_index, instruction_id) in block.instructions().iter().rev().enumerate() { + let instruction = &function.dfg[*instruction_id]; + if self.is_unused(*instruction_id, function) { self.instructions_to_remove.insert(*instruction_id); - } else { - let instruction = &function.dfg[*instruction_id]; + // Check if the removed instruction could possibly result in an index out of bounds + use Instruction::*; + match instruction { + ArrayGet { array, index } | ArraySet { array, index, .. } => { + let might_be_out_of_bounds = if let Some(array_length) = + function.dfg.try_get_array_length(*array) + { + if let Some(known_index) = function.dfg.get_numeric_constant(*index) { + // If the index is known at compile-time, we can only remove it if it's not out of bounds. + // If it's out of bounds we'd like that to keep failing at runtime. + known_index >= array_length.into() + } else { + // If the index is not known at compile-time we can't remove this instruction as this + // might be an index out of bounds. + true + } + } else { + if let ArrayGet { .. } = instruction { + // array_get on a slice always does an index in bounds check, + // so we can remove this instruction if it's unused + false + } else { + // The same check isn't done on array_set, though + true + } + }; + if might_be_out_of_bounds { + possible_index_out_of_bounds_indexes + .push(instructions_len - instruction_index - 1); + } + } + _ => (), + } + } else { use Instruction::*; if matches!(instruction, IncrementRc { .. } | DecrementRc { .. }) { self.rc_instructions.push((*instruction_id, block_id)); @@ -96,6 +137,104 @@ impl Context { } } + if !possible_index_out_of_bounds_indexes.is_empty() { + let mut next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); + + let instructions = function.dfg[block_id].take_instructions(); + for (index, instruction_id) in instructions.iter().enumerate() { + let instruction_id = *instruction_id; + + if let Some(out_of_bounds_index) = next_out_of_bounds_index { + if index == out_of_bounds_index { + // Need to add a constrain + let instruction = &function.dfg[instruction_id]; + let call_stack = function.dfg.get_call_stack(instruction_id); + + match instruction { + Instruction::ArrayGet { array, index } + | Instruction::ArraySet { array, index, .. } => { + if let Some(array_length) = + function.dfg.try_get_array_length(*array) + { + if let Some(known_index) = + function.dfg.get_numeric_constant(*index) + { + // If we are here it means the index is known but out of bounds. That's always an error! + let false_const = + function.dfg.make_constant(false.into(), Type::bool()); + let true_const = + function.dfg.make_constant(true.into(), Type::bool()); + + function.dfg.insert_instruction_and_results( + Instruction::Constrain( + false_const, + true_const, + Some("Index out of bounds".to_owned().into()), + ), + block_id, + None, + call_stack, + ); + } else { + // If we are here it means the index is dynamic, so let's add a check that it's less than length + let index = function + .dfg + .insert_instruction_and_results( + Instruction::Cast( + *index, + Type::unsigned(SSA_WORD_SIZE), + ), + block_id, + None, + call_stack.clone(), + ) + .first(); + let array_length = function.dfg.make_constant( + (array_length as u128).into(), + Type::unsigned(SSA_WORD_SIZE), + ); + let is_index_out_of_bounds = function + .dfg + .insert_instruction_and_results( + Instruction::Binary(Binary { + operator: BinaryOp::Lt, + lhs: index, + rhs: array_length, + }), + block_id, + None, + call_stack.clone(), + ) + .first(); + let true_const = + function.dfg.make_constant(true.into(), Type::bool()); + + function.dfg.insert_instruction_and_results( + Instruction::Constrain( + is_index_out_of_bounds, + true_const, + Some("Index out of bounds".to_owned().into()), + ), + block_id, + None, + call_stack, + ); + } + } else { + // TODO: this is tricky because we don't know the slice length... 🤔 + } + } + _ => panic!("Expected an ArrayGet or ArraySet instruction here"), + } + + next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); + } + } + + function.dfg[block_id].instructions_mut().push(instruction_id); + } + } + function.dfg[block_id] .instructions_mut() .retain(|instruction| !self.instructions_to_remove.contains(instruction)); From 6f21340f644043506d9f060c77e83720447d37de Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 6 Aug 2024 17:19:32 -0300 Subject: [PATCH 02/29] Add some test programs that should fail --- .../array_get_known_index_out_of_bounds/Nargo.toml | 7 +++++++ .../array_get_known_index_out_of_bounds/Prover.toml | 0 .../array_get_known_index_out_of_bounds/src/main.nr | 4 ++++ .../array_get_unknown_index_out_of_bounds/Nargo.toml | 7 +++++++ .../array_get_unknown_index_out_of_bounds/Prover.toml | 1 + .../array_get_unknown_index_out_of_bounds/src/main.nr | 4 ++++ .../array_set_known_index_out_of_bounds/Nargo.toml | 7 +++++++ .../array_set_known_index_out_of_bounds/Prover.toml | 0 .../array_set_known_index_out_of_bounds/src/main.nr | 4 ++++ .../array_set_unknown_index_out_of_bounds/Nargo.toml | 7 +++++++ .../array_set_unknown_index_out_of_bounds/Prover.toml | 1 + .../array_set_unknown_index_out_of_bounds/src/main.nr | 4 ++++ .../slice_get_known_index_out_of_bounds/Nargo.toml | 7 +++++++ .../slice_get_known_index_out_of_bounds/Prover.toml | 0 .../slice_get_known_index_out_of_bounds/src/main.nr | 4 ++++ .../slice_get_unknown_index_out_of_bounds/Nargo.toml | 7 +++++++ .../slice_get_unknown_index_out_of_bounds/Prover.toml | 1 + .../slice_get_unknown_index_out_of_bounds/src/main.nr | 4 ++++ .../slice_set_known_index_out_of_bounds/Nargo.toml | 7 +++++++ .../slice_set_known_index_out_of_bounds/Prover.toml | 0 .../slice_set_known_index_out_of_bounds/src/main.nr | 4 ++++ .../slice_set_unknown_index_out_of_bounds/Nargo.toml | 7 +++++++ .../slice_set_unknown_index_out_of_bounds/Prover.toml | 1 + .../slice_set_unknown_index_out_of_bounds/src/main.nr | 4 ++++ 24 files changed, 92 insertions(+) create mode 100644 test_programs/execution_failure/array_get_known_index_out_of_bounds/Nargo.toml create mode 100644 test_programs/execution_failure/array_get_known_index_out_of_bounds/Prover.toml create mode 100644 test_programs/execution_failure/array_get_known_index_out_of_bounds/src/main.nr create mode 100644 test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Nargo.toml create mode 100644 test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Prover.toml create mode 100644 test_programs/execution_failure/array_get_unknown_index_out_of_bounds/src/main.nr create mode 100644 test_programs/execution_failure/array_set_known_index_out_of_bounds/Nargo.toml create mode 100644 test_programs/execution_failure/array_set_known_index_out_of_bounds/Prover.toml create mode 100644 test_programs/execution_failure/array_set_known_index_out_of_bounds/src/main.nr create mode 100644 test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Nargo.toml create mode 100644 test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Prover.toml create mode 100644 test_programs/execution_failure/array_set_unknown_index_out_of_bounds/src/main.nr create mode 100644 test_programs/execution_failure/slice_get_known_index_out_of_bounds/Nargo.toml create mode 100644 test_programs/execution_failure/slice_get_known_index_out_of_bounds/Prover.toml create mode 100644 test_programs/execution_failure/slice_get_known_index_out_of_bounds/src/main.nr create mode 100644 test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Nargo.toml create mode 100644 test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Prover.toml create mode 100644 test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/src/main.nr create mode 100644 test_programs/execution_failure/slice_set_known_index_out_of_bounds/Nargo.toml create mode 100644 test_programs/execution_failure/slice_set_known_index_out_of_bounds/Prover.toml create mode 100644 test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr create mode 100644 test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Nargo.toml create mode 100644 test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Prover.toml create mode 100644 test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr diff --git a/test_programs/execution_failure/array_get_known_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/array_get_known_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..98b59477906 --- /dev/null +++ b/test_programs/execution_failure/array_get_known_index_out_of_bounds/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "array_get_known_index_out_of_bounds" +type = "bin" +authors = [""] +compiler_version = ">=0.31.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_failure/array_get_known_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/array_get_known_index_out_of_bounds/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test_programs/execution_failure/array_get_known_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/array_get_known_index_out_of_bounds/src/main.nr new file mode 100644 index 00000000000..bdc645ec483 --- /dev/null +++ b/test_programs/execution_failure/array_get_known_index_out_of_bounds/src/main.nr @@ -0,0 +1,4 @@ +fn main() { + let array = [1, 2, 3]; + let _ = array[10]; // Index out of bounds +} diff --git a/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..986e9e8e2e6 --- /dev/null +++ b/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "array_get_unknown_index_out_of_bounds" +type = "bin" +authors = [""] +compiler_version = ">=0.31.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Prover.toml new file mode 100644 index 00000000000..1ec81884d61 --- /dev/null +++ b/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Prover.toml @@ -0,0 +1 @@ +x = "10" \ No newline at end of file diff --git a/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/src/main.nr new file mode 100644 index 00000000000..15c2d1f1f23 --- /dev/null +++ b/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/src/main.nr @@ -0,0 +1,4 @@ +fn main(x: Field) { + let array = [1, 2, 3]; + let _ = array[x]; // Index out of bounds +} diff --git a/test_programs/execution_failure/array_set_known_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/array_set_known_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..f6beafc4e90 --- /dev/null +++ b/test_programs/execution_failure/array_set_known_index_out_of_bounds/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "array_set_known_index_out_of_bounds" +type = "bin" +authors = [""] +compiler_version = ">=0.31.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_failure/array_set_known_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/array_set_known_index_out_of_bounds/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test_programs/execution_failure/array_set_known_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/array_set_known_index_out_of_bounds/src/main.nr new file mode 100644 index 00000000000..9c447aee08f --- /dev/null +++ b/test_programs/execution_failure/array_set_known_index_out_of_bounds/src/main.nr @@ -0,0 +1,4 @@ +fn main() { + let mut array = [1, 2, 3]; + array[10] = 1; // Index out of bounds +} diff --git a/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..8d3b47b2b86 --- /dev/null +++ b/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "array_set_unknown_index_out_of_bounds" +type = "bin" +authors = [""] +compiler_version = ">=0.31.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Prover.toml new file mode 100644 index 00000000000..1ec81884d61 --- /dev/null +++ b/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Prover.toml @@ -0,0 +1 @@ +x = "10" \ No newline at end of file diff --git a/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/src/main.nr new file mode 100644 index 00000000000..dbde898f7a9 --- /dev/null +++ b/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/src/main.nr @@ -0,0 +1,4 @@ +fn main(x: Field) { + let mut array = [1, 2, 3]; + array[x] = 1; // Index out of bounds +} diff --git a/test_programs/execution_failure/slice_get_known_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/slice_get_known_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..7520ed9a9c4 --- /dev/null +++ b/test_programs/execution_failure/slice_get_known_index_out_of_bounds/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "slice_get_known_index_out_of_bounds" +type = "bin" +authors = [""] +compiler_version = ">=0.31.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_failure/slice_get_known_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/slice_get_known_index_out_of_bounds/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test_programs/execution_failure/slice_get_known_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/slice_get_known_index_out_of_bounds/src/main.nr new file mode 100644 index 00000000000..59e57664bbe --- /dev/null +++ b/test_programs/execution_failure/slice_get_known_index_out_of_bounds/src/main.nr @@ -0,0 +1,4 @@ +fn main() { + let slice = &[1, 2, 3]; + let _ = slice[10]; // Index out of bounds +} diff --git a/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..ddda52adeaf --- /dev/null +++ b/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "slice_get_unknown_index_out_of_bounds" +type = "bin" +authors = [""] +compiler_version = ">=0.31.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Prover.toml new file mode 100644 index 00000000000..1ec81884d61 --- /dev/null +++ b/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Prover.toml @@ -0,0 +1 @@ +x = "10" \ No newline at end of file diff --git a/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/src/main.nr new file mode 100644 index 00000000000..5a62e0e9843 --- /dev/null +++ b/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/src/main.nr @@ -0,0 +1,4 @@ +fn main(x: Field) { + let slice = &[1, 2, 3]; + let _ = slice[x]; // Index out of bounds +} diff --git a/test_programs/execution_failure/slice_set_known_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/slice_set_known_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..ea535921e99 --- /dev/null +++ b/test_programs/execution_failure/slice_set_known_index_out_of_bounds/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "slice_set_known_index_out_of_bounds" +type = "bin" +authors = [""] +compiler_version = ">=0.31.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_failure/slice_set_known_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/slice_set_known_index_out_of_bounds/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr new file mode 100644 index 00000000000..31d06cc200d --- /dev/null +++ b/test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr @@ -0,0 +1,4 @@ +fn main() { + let mut slice = &[1, 2, 3]; + slice[10] = 1; // Index out of bounds +} diff --git a/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..597f7be81ba --- /dev/null +++ b/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "slice_set_unknown_index_out_of_bounds" +type = "bin" +authors = [""] +compiler_version = ">=0.31.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Prover.toml new file mode 100644 index 00000000000..1ec81884d61 --- /dev/null +++ b/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Prover.toml @@ -0,0 +1 @@ +x = "10" \ No newline at end of file diff --git a/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr new file mode 100644 index 00000000000..33d30a4b91a --- /dev/null +++ b/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr @@ -0,0 +1,4 @@ +fn main(x: Field) { + let mut slice = &[1, 2, 3]; + slice[x] = 1; // Index out of bounds +} From 6622719d259e1c7aaa74b43beb92fad8aca4f583 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 6 Aug 2024 17:24:58 -0300 Subject: [PATCH 03/29] clippy --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 6e7ce05199c..28be5e7ac65 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -108,15 +108,13 @@ impl Context { // might be an index out of bounds. true } + } else if let ArrayGet { .. } = instruction { + // array_get on a slice always does an index in bounds check, + // so we can remove this instruction if it's unused + false } else { - if let ArrayGet { .. } = instruction { - // array_get on a slice always does an index in bounds check, - // so we can remove this instruction if it's unused - false - } else { - // The same check isn't done on array_set, though - true - } + // The same check isn't done on array_set, though + true }; if might_be_out_of_bounds { possible_index_out_of_bounds_indexes @@ -156,9 +154,7 @@ impl Context { if let Some(array_length) = function.dfg.try_get_array_length(*array) { - if let Some(known_index) = - function.dfg.get_numeric_constant(*index) - { + if let Some(_) = function.dfg.get_numeric_constant(*index) { // If we are here it means the index is known but out of bounds. That's always an error! let false_const = function.dfg.make_constant(false.into(), Type::bool()); From 71b33e5f42ea57a474cc4cb94a549c110ec2c2bc Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 6 Aug 2024 17:26:49 -0300 Subject: [PATCH 04/29] clippy again --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 28be5e7ac65..4d689858cfc 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -154,7 +154,7 @@ impl Context { if let Some(array_length) = function.dfg.try_get_array_length(*array) { - if let Some(_) = function.dfg.get_numeric_constant(*index) { + if function.dfg.get_numeric_constant(*index).is_some() { // If we are here it means the index is known but out of bounds. That's always an error! let false_const = function.dfg.make_constant(false.into(), Type::bool()); From edf69418603022f64235a220bb9f6ee4eaf5ef76 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 6 Aug 2024 17:27:38 -0300 Subject: [PATCH 05/29] Comment failing tests, for now --- .../slice_set_known_index_out_of_bounds/src/main.nr | 5 +++-- .../slice_set_unknown_index_out_of_bounds/src/main.nr | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr index 31d06cc200d..f60b6a21745 100644 --- a/test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr +++ b/test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr @@ -1,4 +1,5 @@ fn main() { - let mut slice = &[1, 2, 3]; - slice[10] = 1; // Index out of bounds + // TODO: uncomment this + // let mut slice = &[1, 2, 3]; + // slice[10] = 1; // Index out of bounds } diff --git a/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr index 33d30a4b91a..53ca9d72121 100644 --- a/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr +++ b/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr @@ -1,4 +1,5 @@ fn main(x: Field) { - let mut slice = &[1, 2, 3]; - slice[x] = 1; // Index out of bounds + // TODO: uncomment this + // let mut slice = &[1, 2, 3]; + // slice[x] = 1; // Index out of bounds } From 3fed72a6c975b78e4b5eb544e4571b23a7be789b Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 6 Aug 2024 17:47:14 -0300 Subject: [PATCH 06/29] Leave for later the case where index might be simplified --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 84 +++++++++++---------- 1 file changed, 45 insertions(+), 39 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 4d689858cfc..31a26c96704 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -5,7 +5,7 @@ use std::collections::HashSet; use crate::ssa::{ ir::{ basic_block::{BasicBlock, BasicBlockId}, - dfg::DataFlowGraph, + dfg::{DataFlowGraph, InsertInstructionResult}, function::Function, instruction::{Binary, BinaryOp, Instruction, InstructionId, Intrinsic}, post_order::PostOrder, @@ -173,48 +173,54 @@ impl Context { ); } else { // If we are here it means the index is dynamic, so let's add a check that it's less than length - let index = function - .dfg - .insert_instruction_and_results( - Instruction::Cast( - *index, - Type::unsigned(SSA_WORD_SIZE), - ), - block_id, - None, - call_stack.clone(), - ) - .first(); - let array_length = function.dfg.make_constant( - (array_length as u128).into(), - Type::unsigned(SSA_WORD_SIZE), - ); - let is_index_out_of_bounds = function - .dfg - .insert_instruction_and_results( - Instruction::Binary(Binary { - operator: BinaryOp::Lt, - lhs: index, - rhs: array_length, - }), - block_id, - None, - call_stack.clone(), - ) - .first(); - let true_const = - function.dfg.make_constant(true.into(), Type::bool()); - - function.dfg.insert_instruction_and_results( - Instruction::Constrain( - is_index_out_of_bounds, - true_const, - Some("Index out of bounds".to_owned().into()), + let index = function.dfg.insert_instruction_and_results( + Instruction::Cast( + *index, + Type::unsigned(SSA_WORD_SIZE), ), block_id, None, - call_stack, + call_stack.clone(), ); + if let InsertInstructionResult::Results(_, results) = index + { + assert_eq!(results.len(), 1); + let index = results[0]; + + let array_length = function.dfg.make_constant( + (array_length as u128).into(), + Type::unsigned(SSA_WORD_SIZE), + ); + let is_index_out_of_bounds = function + .dfg + .insert_instruction_and_results( + Instruction::Binary(Binary { + operator: BinaryOp::Lt, + lhs: index, + rhs: array_length, + }), + block_id, + None, + call_stack.clone(), + ) + .first(); + let true_const = function + .dfg + .make_constant(true.into(), Type::bool()); + + function.dfg.insert_instruction_and_results( + Instruction::Constrain( + is_index_out_of_bounds, + true_const, + Some("Index out of bounds".to_owned().into()), + ), + block_id, + None, + call_stack, + ); + } else { + // TODO: handle the case where the index might have been simplified + } } } else { // TODO: this is tricky because we don't know the slice length... 🤔 From 49c80ad573b180df3c6bad9436441405d76614aa Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 6 Aug 2024 18:03:16 -0300 Subject: [PATCH 07/29] Make it in two passes --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 304 ++++++++++++-------- 1 file changed, 183 insertions(+), 121 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 31a26c96704..30de318b574 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -2,10 +2,12 @@ //! which the results are unused. use std::collections::HashSet; +use noirc_frontend::hir_def::function; + use crate::ssa::{ ir::{ basic_block::{BasicBlock, BasicBlockId}, - dfg::{DataFlowGraph, InsertInstructionResult}, + dfg::DataFlowGraph, function::Function, instruction::{Binary, BinaryOp, Instruction, InstructionId, Intrinsic}, post_order::PostOrder, @@ -77,6 +79,20 @@ impl Context { function: &mut Function, block_id: BasicBlockId, ) { + self.remove_unused_instructions_in_block_impl(function, block_id, true) + } + + fn remove_unused_instructions_in_block_impl( + &mut self, + function: &mut Function, + block_id: BasicBlockId, + analyze_index_out_of_bounds: bool, + ) { + // Clear state because this might have been called recursively + self.used_values.clear(); + self.instructions_to_remove.clear(); + self.rc_instructions.clear(); + let block = &function.dfg[block_id]; self.mark_terminator_values_as_used(function, block); @@ -92,36 +108,11 @@ impl Context { if self.is_unused(*instruction_id, function) { self.instructions_to_remove.insert(*instruction_id); - // Check if the removed instruction could possibly result in an index out of bounds - use Instruction::*; - match instruction { - ArrayGet { array, index } | ArraySet { array, index, .. } => { - let might_be_out_of_bounds = if let Some(array_length) = - function.dfg.try_get_array_length(*array) - { - if let Some(known_index) = function.dfg.get_numeric_constant(*index) { - // If the index is known at compile-time, we can only remove it if it's not out of bounds. - // If it's out of bounds we'd like that to keep failing at runtime. - known_index >= array_length.into() - } else { - // If the index is not known at compile-time we can't remove this instruction as this - // might be an index out of bounds. - true - } - } else if let ArrayGet { .. } = instruction { - // array_get on a slice always does an index in bounds check, - // so we can remove this instruction if it's unused - false - } else { - // The same check isn't done on array_set, though - true - }; - if might_be_out_of_bounds { - possible_index_out_of_bounds_indexes - .push(instructions_len - instruction_index - 1); - } - } - _ => (), + if analyze_index_out_of_bounds + && instruction_might_result_in_out_of_bounds(function, instruction) + { + possible_index_out_of_bounds_indexes + .push(instructions_len - instruction_index - 1); } } else { use Instruction::*; @@ -135,45 +126,99 @@ impl Context { } } + // If there are some instructions that might result trigger an out of bounds error, + // first add constrain checks. Then run the DIE pass again, which will remove those + // but leave the constrains (any any value needed by those constrains) if !possible_index_out_of_bounds_indexes.is_empty() { - let mut next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); - - let instructions = function.dfg[block_id].take_instructions(); - for (index, instruction_id) in instructions.iter().enumerate() { - let instruction_id = *instruction_id; - - if let Some(out_of_bounds_index) = next_out_of_bounds_index { - if index == out_of_bounds_index { - // Need to add a constrain - let instruction = &function.dfg[instruction_id]; - let call_stack = function.dfg.get_call_stack(instruction_id); - - match instruction { - Instruction::ArrayGet { array, index } - | Instruction::ArraySet { array, index, .. } => { - if let Some(array_length) = - function.dfg.try_get_array_length(*array) - { - if function.dfg.get_numeric_constant(*index).is_some() { - // If we are here it means the index is known but out of bounds. That's always an error! - let false_const = - function.dfg.make_constant(false.into(), Type::bool()); - let true_const = - function.dfg.make_constant(true.into(), Type::bool()); - - function.dfg.insert_instruction_and_results( - Instruction::Constrain( - false_const, - true_const, - Some("Index out of bounds".to_owned().into()), - ), - block_id, - None, - call_stack, - ); - } else { - // If we are here it means the index is dynamic, so let's add a check that it's less than length - let index = function.dfg.insert_instruction_and_results( + self.insert_out_of_bounds_checks( + function, + block_id, + &mut possible_index_out_of_bounds_indexes, + ); + + self.remove_unused_instructions_in_block_impl(function, block_id, false); + return; + } + + function.dfg[block_id] + .instructions_mut() + .retain(|instruction| !self.instructions_to_remove.contains(instruction)); + } + + fn instruction_might_result_in_out_of_bounds( + function: Function, + instruction: &Instruction, + ) -> bool { + use Instruction::*; + match instruction { + ArrayGet { array, index } | ArraySet { array, index, .. } => { + if let Some(array_length) = function.dfg.try_get_array_length(*array) { + if let Some(known_index) = function.dfg.get_numeric_constant(*index) { + // If the index is known at compile-time, we can only remove it if it's not out of bounds. + // If it's out of bounds we'd like that to keep failing at runtime. + known_index >= array_length.into() + } else { + // If the index is not known at compile-time we can't remove this instruction as this + // might be an index out of bounds. + true + } + } else if let ArrayGet { .. } = instruction { + // array_get on a slice always does an index in bounds check, + // so we can remove this instruction if it's unused + false + } else { + // The same check isn't done on array_set, though + true + } + } + _ => false, + } + } + + fn insert_out_of_bounds_checks( + &mut self, + function: &mut Function, + block_id: BasicBlockId, + possible_index_out_of_bounds_indexes: &mut Vec, + ) { + let mut next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); + + let instructions = function.dfg[block_id].take_instructions(); + for (index, instruction_id) in instructions.iter().enumerate() { + let instruction_id = *instruction_id; + + if let Some(out_of_bounds_index) = next_out_of_bounds_index { + if index == out_of_bounds_index { + // Need to add a constrain + let instruction = &function.dfg[instruction_id]; + let call_stack = function.dfg.get_call_stack(instruction_id); + + match instruction { + Instruction::ArrayGet { array, index } + | Instruction::ArraySet { array, index, .. } => { + if let Some(array_length) = function.dfg.try_get_array_length(*array) { + if function.dfg.get_numeric_constant(*index).is_some() { + // If we are here it means the index is known but out of bounds. That's always an error! + let false_const = + function.dfg.make_constant(false.into(), Type::bool()); + let true_const = + function.dfg.make_constant(true.into(), Type::bool()); + + function.dfg.insert_instruction_and_results( + Instruction::Constrain( + false_const, + true_const, + Some("Index out of bounds".to_owned().into()), + ), + block_id, + None, + call_stack, + ); + } else { + // If we are here it means the index is dynamic, so let's add a check that it's less than length + let index = function + .dfg + .insert_instruction_and_results( Instruction::Cast( *index, Type::unsigned(SSA_WORD_SIZE), @@ -181,65 +226,52 @@ impl Context { block_id, None, call_stack.clone(), - ); - if let InsertInstructionResult::Results(_, results) = index - { - assert_eq!(results.len(), 1); - let index = results[0]; - - let array_length = function.dfg.make_constant( - (array_length as u128).into(), - Type::unsigned(SSA_WORD_SIZE), - ); - let is_index_out_of_bounds = function - .dfg - .insert_instruction_and_results( - Instruction::Binary(Binary { - operator: BinaryOp::Lt, - lhs: index, - rhs: array_length, - }), - block_id, - None, - call_stack.clone(), - ) - .first(); - let true_const = function - .dfg - .make_constant(true.into(), Type::bool()); - - function.dfg.insert_instruction_and_results( - Instruction::Constrain( - is_index_out_of_bounds, - true_const, - Some("Index out of bounds".to_owned().into()), - ), - block_id, - None, - call_stack, - ); - } else { - // TODO: handle the case where the index might have been simplified - } - } - } else { - // TODO: this is tricky because we don't know the slice length... 🤔 + ) + .first(); + let array_length = function.dfg.make_constant( + (array_length as u128).into(), + Type::unsigned(SSA_WORD_SIZE), + ); + let is_index_out_of_bounds = function + .dfg + .insert_instruction_and_results( + Instruction::Binary(Binary { + operator: BinaryOp::Lt, + lhs: index, + rhs: array_length, + }), + block_id, + None, + call_stack.clone(), + ) + .first(); + let true_const = + function.dfg.make_constant(true.into(), Type::bool()); + + function.dfg.insert_instruction_and_results( + Instruction::Constrain( + is_index_out_of_bounds, + true_const, + Some("Index out of bounds".to_owned().into()), + ), + block_id, + None, + call_stack, + ); } + } else { + // TODO: this is tricky because we don't know the slice length... 🤔 } - _ => panic!("Expected an ArrayGet or ArraySet instruction here"), } - - next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); + _ => panic!("Expected an ArrayGet or ArraySet instruction here"), } - } - function.dfg[block_id].instructions_mut().push(instruction_id); + next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); + } } - } - function.dfg[block_id] - .instructions_mut() - .retain(|instruction| !self.instructions_to_remove.contains(instruction)); + function.dfg[block_id].instructions_mut().push(instruction_id); + } } /// Returns true if an instruction can be removed. @@ -309,6 +341,36 @@ impl Context { } } +fn instruction_might_result_in_out_of_bounds( + function: &Function, + instruction: &Instruction, +) -> bool { + use Instruction::*; + match instruction { + ArrayGet { array, index } | ArraySet { array, index, .. } => { + if let Some(array_length) = function.dfg.try_get_array_length(*array) { + if let Some(known_index) = function.dfg.get_numeric_constant(*index) { + // If the index is known at compile-time, we can only remove it if it's not out of bounds. + // If it's out of bounds we'd like that to keep failing at runtime. + known_index >= array_length.into() + } else { + // If the index is not known at compile-time we can't remove this instruction as this + // might be an index out of bounds. + true + } + } else if let ArrayGet { .. } = instruction { + // array_get on a slice always does an index in bounds check, + // so we can remove this instruction if it's unused + false + } else { + // The same check isn't done on array_set, though + true + } + } + _ => false, + } +} + #[cfg(test)] mod test { use crate::ssa::{ From 165e183c9bf9cdf807f204fc0f71a35b0b9825fa Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 6 Aug 2024 18:05:02 -0300 Subject: [PATCH 08/29] Fix typo --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 30de318b574..d953be999df 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -126,7 +126,7 @@ impl Context { } } - // If there are some instructions that might result trigger an out of bounds error, + // If there are some instructions that might trigger an out of bounds error, // first add constrain checks. Then run the DIE pass again, which will remove those // but leave the constrains (any any value needed by those constrains) if !possible_index_out_of_bounds_indexes.is_empty() { From 4bcecb1f1344b8791b6d20c7c6b8158c4a2700ca Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 6 Aug 2024 18:08:37 -0300 Subject: [PATCH 09/29] clippy --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index d953be999df..ce0f29be258 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -2,8 +2,6 @@ //! which the results are unused. use std::collections::HashSet; -use noirc_frontend::hir_def::function; - use crate::ssa::{ ir::{ basic_block::{BasicBlock, BasicBlockId}, @@ -79,7 +77,7 @@ impl Context { function: &mut Function, block_id: BasicBlockId, ) { - self.remove_unused_instructions_in_block_impl(function, block_id, true) + self.remove_unused_instructions_in_block_impl(function, block_id, true); } fn remove_unused_instructions_in_block_impl( From 0a6bd4536b5e798fb08bd0b4a344d4526a677058 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 7 Aug 2024 07:15:24 -0300 Subject: [PATCH 10/29] Do it right --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 54 ++++++++++++--------- 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index ce0f29be258..83adf7e55b5 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -21,7 +21,7 @@ impl Ssa { #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn dead_instruction_elimination(mut self) -> Ssa { for function in self.functions.values_mut() { - dead_instruction_elimination(function); + dead_instruction_elimination(function, true); } self } @@ -33,16 +33,29 @@ impl Ssa { /// instructions that reference results from an instruction in another block are evaluated first. /// If we did not iterate blocks in this order we could not safely say whether or not the results /// of its instructions are needed elsewhere. -fn dead_instruction_elimination(function: &mut Function) { +fn dead_instruction_elimination(function: &mut Function, insert_out_of_bounds_checks: bool) { let mut context = Context::default(); for call_data in &function.dfg.data_bus.call_data { context.mark_used_instruction_results(&function.dfg, call_data.array_id); } - let blocks = PostOrder::with_function(function); + let mut inserted_out_of_bounds_checks = false; + let blocks = PostOrder::with_function(function); for block in blocks.as_slice() { - context.remove_unused_instructions_in_block(function, *block); + inserted_out_of_bounds_checks |= context.remove_unused_instructions_in_block( + function, + *block, + insert_out_of_bounds_checks, + ); + } + + // If we inserted out of bounds check, let's run the pass again with those new + // instructions (we don't want to remove those checks, or instructions that are + // dependencies of those checks) + if inserted_out_of_bounds_checks { + dead_instruction_elimination(function, false); + return; } context.remove_rc_instructions(&mut function.dfg); @@ -72,25 +85,18 @@ impl Context { /// values set. This allows DIE to identify whole chains of unused instructions. (If the /// values referenced by an unused instruction were considered to be used, only the head of /// such chains would be removed.) + /// + /// If `insert_out_of_bounds_checks` is true and there are unused ArrayGet/ArraySet that + /// might be out of bounds, this method will insert out of bounds checks instead of + /// removing unused instructions and return `true`. The idea then is to later call this + /// function again with `insert_out_of_bounds_checks` set to false to effectively remove + /// unused instructions but leave the out of bounds checks. fn remove_unused_instructions_in_block( &mut self, function: &mut Function, block_id: BasicBlockId, - ) { - self.remove_unused_instructions_in_block_impl(function, block_id, true); - } - - fn remove_unused_instructions_in_block_impl( - &mut self, - function: &mut Function, - block_id: BasicBlockId, - analyze_index_out_of_bounds: bool, - ) { - // Clear state because this might have been called recursively - self.used_values.clear(); - self.instructions_to_remove.clear(); - self.rc_instructions.clear(); - + insert_out_of_bounds_checks: bool, + ) -> bool { let block = &function.dfg[block_id]; self.mark_terminator_values_as_used(function, block); @@ -106,7 +112,7 @@ impl Context { if self.is_unused(*instruction_id, function) { self.instructions_to_remove.insert(*instruction_id); - if analyze_index_out_of_bounds + if insert_out_of_bounds_checks && instruction_might_result_in_out_of_bounds(function, instruction) { possible_index_out_of_bounds_indexes @@ -133,14 +139,14 @@ impl Context { block_id, &mut possible_index_out_of_bounds_indexes, ); - - self.remove_unused_instructions_in_block_impl(function, block_id, false); - return; + return true; } function.dfg[block_id] .instructions_mut() .retain(|instruction| !self.instructions_to_remove.contains(instruction)); + + false } fn instruction_might_result_in_out_of_bounds( @@ -162,7 +168,7 @@ impl Context { } } else if let ArrayGet { .. } = instruction { // array_get on a slice always does an index in bounds check, - // so we can remove this instruction if it's unused + // so no need to do it again false } else { // The same check isn't done on array_set, though From f9fc76ec35b31c380b99043ad3a2cbcc8376fc1d Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 7 Aug 2024 07:16:36 -0300 Subject: [PATCH 11/29] Better comments --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 83adf7e55b5..a82dec79ac7 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -158,12 +158,9 @@ impl Context { ArrayGet { array, index } | ArraySet { array, index, .. } => { if let Some(array_length) = function.dfg.try_get_array_length(*array) { if let Some(known_index) = function.dfg.get_numeric_constant(*index) { - // If the index is known at compile-time, we can only remove it if it's not out of bounds. - // If it's out of bounds we'd like that to keep failing at runtime. known_index >= array_length.into() } else { - // If the index is not known at compile-time we can't remove this instruction as this - // might be an index out of bounds. + // The index is not know so it might be out of bounds true } } else if let ArrayGet { .. } = instruction { From 0e9494ce295526fa4d66aca57a843894bc0c1434 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 7 Aug 2024 09:31:02 -0300 Subject: [PATCH 12/29] Consider flattened array length --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 46 ++++++--------------- 1 file changed, 12 insertions(+), 34 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index a82dec79ac7..b74fa9f27b7 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -149,33 +149,6 @@ impl Context { false } - fn instruction_might_result_in_out_of_bounds( - function: Function, - instruction: &Instruction, - ) -> bool { - use Instruction::*; - match instruction { - ArrayGet { array, index } | ArraySet { array, index, .. } => { - if let Some(array_length) = function.dfg.try_get_array_length(*array) { - if let Some(known_index) = function.dfg.get_numeric_constant(*index) { - known_index >= array_length.into() - } else { - // The index is not know so it might be out of bounds - true - } - } else if let ArrayGet { .. } = instruction { - // array_get on a slice always does an index in bounds check, - // so no need to do it again - false - } else { - // The same check isn't done on array_set, though - true - } - } - _ => false, - } - } - fn insert_out_of_bounds_checks( &mut self, function: &mut Function, @@ -197,7 +170,7 @@ impl Context { match instruction { Instruction::ArrayGet { array, index } | Instruction::ArraySet { array, index, .. } => { - if let Some(array_length) = function.dfg.try_get_array_length(*array) { + if function.dfg.try_get_array_length(*array).is_some() { if function.dfg.get_numeric_constant(*index).is_some() { // If we are here it means the index is known but out of bounds. That's always an error! let false_const = @@ -216,6 +189,10 @@ impl Context { call_stack, ); } else { + // `index` will be relative to the flattened array length, so we need to take that into account + let array_length = + function.dfg.type_of_value(*array).flattened_size(); + // If we are here it means the index is dynamic, so let's add a check that it's less than length let index = function .dfg @@ -229,6 +206,7 @@ impl Context { call_stack.clone(), ) .first(); + let array_length = function.dfg.make_constant( (array_length as u128).into(), Type::unsigned(SSA_WORD_SIZE), @@ -349,19 +327,19 @@ fn instruction_might_result_in_out_of_bounds( use Instruction::*; match instruction { ArrayGet { array, index } | ArraySet { array, index, .. } => { - if let Some(array_length) = function.dfg.try_get_array_length(*array) { + if function.dfg.try_get_array_length(*array).is_some() { if let Some(known_index) = function.dfg.get_numeric_constant(*index) { - // If the index is known at compile-time, we can only remove it if it's not out of bounds. - // If it's out of bounds we'd like that to keep failing at runtime. + // `index` will be relative to the flattened array length, so we need to take that into account + let typ = function.dfg.type_of_value(*array); + let array_length = typ.flattened_size(); known_index >= array_length.into() } else { - // If the index is not known at compile-time we can't remove this instruction as this - // might be an index out of bounds. + // A dynamic index might always be out of bounds true } } else if let ArrayGet { .. } = instruction { // array_get on a slice always does an index in bounds check, - // so we can remove this instruction if it's unused + // so no need to do it again false } else { // The same check isn't done on array_set, though From d37876881eb050eeb04f0ec7f979716485b2e91d Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 7 Aug 2024 10:01:04 -0300 Subject: [PATCH 13/29] Fix a couple of tests --- .../slice_set_known_index_out_of_bounds/src/main.nr | 1 + .../slice_set_unknown_index_out_of_bounds/src/main.nr | 1 + 2 files changed, 2 insertions(+) diff --git a/test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr index f60b6a21745..b704dbb0c79 100644 --- a/test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr +++ b/test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr @@ -2,4 +2,5 @@ fn main() { // TODO: uncomment this // let mut slice = &[1, 2, 3]; // slice[10] = 1; // Index out of bounds + assert(false); } diff --git a/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr index 53ca9d72121..9f344e362f6 100644 --- a/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr +++ b/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr @@ -2,4 +2,5 @@ fn main(x: Field) { // TODO: uncomment this // let mut slice = &[1, 2, 3]; // slice[x] = 1; // Index out of bounds + assert(false); } From ffe067a12c14269913bfdf9bc874bc4232f5080a Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 7 Aug 2024 11:11:55 -0300 Subject: [PATCH 14/29] Keep track of side effects --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 227 ++++++++++++-------- 1 file changed, 142 insertions(+), 85 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index b74fa9f27b7..edc919a5d01 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -2,6 +2,9 @@ //! which the results are unused. use std::collections::HashSet; +use im::Vector; +use noirc_errors::Location; + use crate::ssa::{ ir::{ basic_block::{BasicBlock, BasicBlockId}, @@ -155,101 +158,110 @@ impl Context { block_id: BasicBlockId, possible_index_out_of_bounds_indexes: &mut Vec, ) { + // Keep track of the current side effects condition + let mut side_effects_condition = None; + + // Keep track of the next index we need to handle let mut next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); let instructions = function.dfg[block_id].take_instructions(); for (index, instruction_id) in instructions.iter().enumerate() { let instruction_id = *instruction_id; + let instruction = &function.dfg[instruction_id]; - if let Some(out_of_bounds_index) = next_out_of_bounds_index { - if index == out_of_bounds_index { - // Need to add a constrain - let instruction = &function.dfg[instruction_id]; - let call_stack = function.dfg.get_call_stack(instruction_id); - - match instruction { - Instruction::ArrayGet { array, index } - | Instruction::ArraySet { array, index, .. } => { - if function.dfg.try_get_array_length(*array).is_some() { - if function.dfg.get_numeric_constant(*index).is_some() { - // If we are here it means the index is known but out of bounds. That's always an error! - let false_const = - function.dfg.make_constant(false.into(), Type::bool()); - let true_const = - function.dfg.make_constant(true.into(), Type::bool()); - - function.dfg.insert_instruction_and_results( - Instruction::Constrain( - false_const, - true_const, - Some("Index out of bounds".to_owned().into()), - ), - block_id, - None, - call_stack, - ); - } else { - // `index` will be relative to the flattened array length, so we need to take that into account - let array_length = - function.dfg.type_of_value(*array).flattened_size(); - - // If we are here it means the index is dynamic, so let's add a check that it's less than length - let index = function - .dfg - .insert_instruction_and_results( - Instruction::Cast( - *index, - Type::unsigned(SSA_WORD_SIZE), - ), - block_id, - None, - call_stack.clone(), - ) - .first(); - - let array_length = function.dfg.make_constant( - (array_length as u128).into(), - Type::unsigned(SSA_WORD_SIZE), - ); - let is_index_out_of_bounds = function - .dfg - .insert_instruction_and_results( - Instruction::Binary(Binary { - operator: BinaryOp::Lt, - lhs: index, - rhs: array_length, - }), - block_id, - None, - call_stack.clone(), - ) - .first(); - let true_const = - function.dfg.make_constant(true.into(), Type::bool()); - - function.dfg.insert_instruction_and_results( - Instruction::Constrain( - is_index_out_of_bounds, - true_const, - Some("Index out of bounds".to_owned().into()), - ), - block_id, - None, - call_stack, - ); - } - } else { - // TODO: this is tricky because we don't know the slice length... 🤔 - } - } - _ => panic!("Expected an ArrayGet or ArraySet instruction here"), - } + if let Instruction::EnableSideEffects { condition } = instruction { + side_effects_condition = Some(*condition); + + // We still need to keep the EnableSideEffects instruction + function.dfg[block_id].instructions_mut().push(instruction_id); + continue; + }; + + let Some(out_of_bounds_index) = next_out_of_bounds_index else { + // No more out of bounds instructions to insert, just push the current instruction + function.dfg[block_id].instructions_mut().push(instruction_id); + continue; + }; - next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); + if index != out_of_bounds_index { + // This instruction is not out of bounds: let's just push it + function.dfg[block_id].instructions_mut().push(instruction_id); + continue; + } + + // This is an instruction that might be out of bounds: let's add a constrain. + let call_stack = function.dfg.get_call_stack(instruction_id); + + match instruction { + Instruction::ArrayGet { array, index } + | Instruction::ArraySet { array, index, .. } => { + if function.dfg.try_get_array_length(*array).is_some() { + let (lhs, rhs) = if function.dfg.get_numeric_constant(*index).is_some() { + // If we are here it means the index is known but out of bounds. That's always an error! + let false_const = + function.dfg.make_constant(false.into(), Type::bool()); + let true_const = function.dfg.make_constant(true.into(), Type::bool()); + (false_const, true_const) + } else { + // `index` will be relative to the flattened array length, so we need to take that into account + let array_length = function.dfg.type_of_value(*array).flattened_size(); + + // If we are here it means the index is dynamic, so let's add a check that it's less than length + let index = function + .dfg + .insert_instruction_and_results( + Instruction::Cast(*index, Type::unsigned(SSA_WORD_SIZE)), + block_id, + None, + call_stack.clone(), + ) + .first(); + + let array_length = function.dfg.make_constant( + (array_length as u128).into(), + Type::unsigned(SSA_WORD_SIZE), + ); + let is_index_out_of_bounds = function + .dfg + .insert_instruction_and_results( + Instruction::Binary(Binary { + operator: BinaryOp::Lt, + lhs: index, + rhs: array_length, + }), + block_id, + None, + call_stack.clone(), + ) + .first(); + let true_const = function.dfg.make_constant(true.into(), Type::bool()); + (is_index_out_of_bounds, true_const) + }; + + let (lhs, rhs) = apply_side_effects( + side_effects_condition, + lhs, + rhs, + function, + block_id, + call_stack.clone(), + ); + + let message = Some("Index out of bounds".to_owned().into()); + function.dfg.insert_instruction_and_results( + Instruction::Constrain(lhs, rhs, message), + block_id, + None, + call_stack, + ); + } else { + // TODO: this is tricky because we don't know the slice length... 🤔 + } } + _ => panic!("Expected an ArrayGet or ArraySet instruction here"), } - function.dfg[block_id].instructions_mut().push(instruction_id); + next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); } } @@ -350,6 +362,51 @@ fn instruction_might_result_in_out_of_bounds( } } +fn apply_side_effects( + side_effects_condition: Option, + lhs: ValueId, + rhs: ValueId, + function: &mut Function, + block_id: BasicBlockId, + call_stack: Vector, +) -> (ValueId, ValueId) { + // See if there's an active "enable side effects" condition + let Some(condition) = side_effects_condition else { + return (lhs, rhs); + }; + + // Condition needs to be cast to argument type in order to multiply them together. + // In our case, lhs is always a boolean. + let casted_condition = function + .dfg + .insert_instruction_and_results( + Instruction::Cast(condition, Type::bool()), + block_id, + None, + call_stack.clone(), + ) + .first(); + let lhs = function + .dfg + .insert_instruction_and_results( + Instruction::binary(BinaryOp::Mul, lhs, casted_condition), + block_id, + None, + call_stack.clone(), + ) + .first(); + let rhs = function + .dfg + .insert_instruction_and_results( + Instruction::binary(BinaryOp::Mul, rhs, casted_condition), + block_id, + None, + call_stack, + ) + .first(); + (lhs, rhs) +} + #[cfg(test)] mod test { use crate::ssa::{ From ad812f389faca067d2fddbc9e41ca915177274d9 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 7 Aug 2024 20:14:08 -0300 Subject: [PATCH 15/29] Only deal with array instructions, not slice --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 126 +++++++++----------- 1 file changed, 59 insertions(+), 67 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index edc919a5d01..fe2d0ffc4ae 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -195,68 +195,63 @@ impl Context { match instruction { Instruction::ArrayGet { array, index } | Instruction::ArraySet { array, index, .. } => { - if function.dfg.try_get_array_length(*array).is_some() { - let (lhs, rhs) = if function.dfg.get_numeric_constant(*index).is_some() { - // If we are here it means the index is known but out of bounds. That's always an error! - let false_const = - function.dfg.make_constant(false.into(), Type::bool()); - let true_const = function.dfg.make_constant(true.into(), Type::bool()); - (false_const, true_const) - } else { - // `index` will be relative to the flattened array length, so we need to take that into account - let array_length = function.dfg.type_of_value(*array).flattened_size(); - - // If we are here it means the index is dynamic, so let's add a check that it's less than length - let index = function - .dfg - .insert_instruction_and_results( - Instruction::Cast(*index, Type::unsigned(SSA_WORD_SIZE)), - block_id, - None, - call_stack.clone(), - ) - .first(); - - let array_length = function.dfg.make_constant( - (array_length as u128).into(), - Type::unsigned(SSA_WORD_SIZE), - ); - let is_index_out_of_bounds = function - .dfg - .insert_instruction_and_results( - Instruction::Binary(Binary { - operator: BinaryOp::Lt, - lhs: index, - rhs: array_length, - }), - block_id, - None, - call_stack.clone(), - ) - .first(); - let true_const = function.dfg.make_constant(true.into(), Type::bool()); - (is_index_out_of_bounds, true_const) - }; - - let (lhs, rhs) = apply_side_effects( - side_effects_condition, - lhs, - rhs, - function, - block_id, - call_stack.clone(), - ); - - let message = Some("Index out of bounds".to_owned().into()); - function.dfg.insert_instruction_and_results( - Instruction::Constrain(lhs, rhs, message), - block_id, - None, - call_stack, - ); + let (lhs, rhs) = if function.dfg.get_numeric_constant(*index).is_some() { + // If we are here it means the index is known but out of bounds. That's always an error! + let false_const = function.dfg.make_constant(false.into(), Type::bool()); + let true_const = function.dfg.make_constant(true.into(), Type::bool()); + (false_const, true_const) } else { - // TODO: this is tricky because we don't know the slice length... 🤔 - } + // `index` will be relative to the flattened array length, so we need to take that into account + let array_length = function.dfg.type_of_value(*array).flattened_size(); + + // If we are here it means the index is dynamic, so let's add a check that it's less than length + let index = function + .dfg + .insert_instruction_and_results( + Instruction::Cast(*index, Type::unsigned(SSA_WORD_SIZE)), + block_id, + None, + call_stack.clone(), + ) + .first(); + + let array_length = function.dfg.make_constant( + (array_length as u128).into(), + Type::unsigned(SSA_WORD_SIZE), + ); + let is_index_out_of_bounds = function + .dfg + .insert_instruction_and_results( + Instruction::Binary(Binary { + operator: BinaryOp::Lt, + lhs: index, + rhs: array_length, + }), + block_id, + None, + call_stack.clone(), + ) + .first(); + let true_const = function.dfg.make_constant(true.into(), Type::bool()); + (is_index_out_of_bounds, true_const) + }; + + let (lhs, rhs) = apply_side_effects( + side_effects_condition, + lhs, + rhs, + function, + block_id, + call_stack.clone(), + ); + + let message = Some("Index out of bounds".to_owned().into()); + function.dfg.insert_instruction_and_results( + Instruction::Constrain(lhs, rhs, message), + block_id, + None, + call_stack, + ); } _ => panic!("Expected an ArrayGet or ArraySet instruction here"), } @@ -349,13 +344,10 @@ fn instruction_might_result_in_out_of_bounds( // A dynamic index might always be out of bounds true } - } else if let ArrayGet { .. } = instruction { - // array_get on a slice always does an index in bounds check, - // so no need to do it again - false } else { - // The same check isn't done on array_set, though - true + // Slice operations might be out of bounds, but there's no way we + // can insert a check because we don't know a slice's length + false } } _ => false, From 61f2f170170abe236c8f0b395acb451899c867e5 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 7 Aug 2024 20:16:54 -0300 Subject: [PATCH 16/29] Less nesting --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 115 ++++++++++---------- 1 file changed, 57 insertions(+), 58 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index fe2d0ffc4ae..d0cf45ef3dd 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -192,69 +192,68 @@ impl Context { // This is an instruction that might be out of bounds: let's add a constrain. let call_stack = function.dfg.get_call_stack(instruction_id); - match instruction { + let (array, index) = match instruction { Instruction::ArrayGet { array, index } - | Instruction::ArraySet { array, index, .. } => { - let (lhs, rhs) = if function.dfg.get_numeric_constant(*index).is_some() { - // If we are here it means the index is known but out of bounds. That's always an error! - let false_const = function.dfg.make_constant(false.into(), Type::bool()); - let true_const = function.dfg.make_constant(true.into(), Type::bool()); - (false_const, true_const) - } else { - // `index` will be relative to the flattened array length, so we need to take that into account - let array_length = function.dfg.type_of_value(*array).flattened_size(); - - // If we are here it means the index is dynamic, so let's add a check that it's less than length - let index = function - .dfg - .insert_instruction_and_results( - Instruction::Cast(*index, Type::unsigned(SSA_WORD_SIZE)), - block_id, - None, - call_stack.clone(), - ) - .first(); - - let array_length = function.dfg.make_constant( - (array_length as u128).into(), - Type::unsigned(SSA_WORD_SIZE), - ); - let is_index_out_of_bounds = function - .dfg - .insert_instruction_and_results( - Instruction::Binary(Binary { - operator: BinaryOp::Lt, - lhs: index, - rhs: array_length, - }), - block_id, - None, - call_stack.clone(), - ) - .first(); - let true_const = function.dfg.make_constant(true.into(), Type::bool()); - (is_index_out_of_bounds, true_const) - }; - - let (lhs, rhs) = apply_side_effects( - side_effects_condition, - lhs, - rhs, - function, + | Instruction::ArraySet { array, index, .. } => (array, index), + _ => panic!("Expected an ArrayGet or ArraySet instruction here"), + }; + + let (lhs, rhs) = if function.dfg.get_numeric_constant(*index).is_some() { + // If we are here it means the index is known but out of bounds. That's always an error! + let false_const = function.dfg.make_constant(false.into(), Type::bool()); + let true_const = function.dfg.make_constant(true.into(), Type::bool()); + (false_const, true_const) + } else { + // `index` will be relative to the flattened array length, so we need to take that into account + let array_length = function.dfg.type_of_value(*array).flattened_size(); + + // If we are here it means the index is dynamic, so let's add a check that it's less than length + let index = function + .dfg + .insert_instruction_and_results( + Instruction::Cast(*index, Type::unsigned(SSA_WORD_SIZE)), block_id, + None, call_stack.clone(), - ); - - let message = Some("Index out of bounds".to_owned().into()); - function.dfg.insert_instruction_and_results( - Instruction::Constrain(lhs, rhs, message), + ) + .first(); + + let array_length = function + .dfg + .make_constant((array_length as u128).into(), Type::unsigned(SSA_WORD_SIZE)); + let is_index_out_of_bounds = function + .dfg + .insert_instruction_and_results( + Instruction::Binary(Binary { + operator: BinaryOp::Lt, + lhs: index, + rhs: array_length, + }), block_id, None, - call_stack, - ); - } - _ => panic!("Expected an ArrayGet or ArraySet instruction here"), - } + call_stack.clone(), + ) + .first(); + let true_const = function.dfg.make_constant(true.into(), Type::bool()); + (is_index_out_of_bounds, true_const) + }; + + let (lhs, rhs) = apply_side_effects( + side_effects_condition, + lhs, + rhs, + function, + block_id, + call_stack.clone(), + ); + + let message = Some("Index out of bounds".to_owned().into()); + function.dfg.insert_instruction_and_results( + Instruction::Constrain(lhs, rhs, message), + block_id, + None, + call_stack, + ); next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); } From 7bdc2279046e9a099191536393dc768a388663f2 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 7 Aug 2024 20:55:55 -0300 Subject: [PATCH 17/29] Handle groups of array_get --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 74 +++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index d0cf45ef3dd..5942af0124a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -177,6 +177,80 @@ impl Context { continue; }; + // If it's an ArrayGet we'll deal with groups of it in case the array type is a composite type. + if let Instruction::ArrayGet { array, .. } = instruction { + if let Some(array_length) = function.dfg.try_get_array_length(*array) { + let flattened_size = function.dfg.type_of_value(*array).flattened_size(); + let element_size = flattened_size / array_length; + if element_size > 1 { + // It's a composite type. + // When doing ArrayGet on a composite type, this **always** results in instructions like these + // (assuming element_size == 3): + // + // 1. v27 = array_get v1, index v26 + // 2. v28 = add v26, u32 1 + // 3. v29 = array_get v1, index v28 + // 4. v30 = add v26, u32 2 + // 5. v31 = array_get v1, index v30 + // + // That means that after this instructions, (element_size - 1) instructions will be + // part of this composite array get, and they'll be two instructions apart. + // + // Now three things can happen: + // a) none of the array_get instructions are unused: in this case they won't be in + // `possible_index_out_of_bounds_indexes` and they won't be removed, nothing to do here + // b) all of the array_get instructions are unused: in this case we can replace **all** + // of them with just one constrain: no need to do one per array_get + // c) some of the array_get instructions are unused, but not all: in this case + // we don't need to insert any constrain, because on a later stage array bound checks + // will be performed anyway. We'll let DIE remove the unused ones, without replacing + // them with bounds checks, and leave the used ones. + // + // To check in which scenario we are we can get from `possible_index_out_of_bounds_indexes` + // (starting from `next_out_of_bounds_index`) while we are in the group ranges + // (1..=5 in the example above) + + if let Some(out_of_bounds_index) = next_out_of_bounds_index { + if index == out_of_bounds_index { + // What's the last instruction that's part of the group? (5 in the example above) + let last_instruction_index = index + 2 * (element_size - 1); + // How many unused instructions are in this group? + let mut unused_count = 1; + loop { + next_out_of_bounds_index = + possible_index_out_of_bounds_indexes.pop(); + if let Some(out_of_bounds_index) = next_out_of_bounds_index { + if out_of_bounds_index <= last_instruction_index { + unused_count += 1; + if unused_count == element_size { + // We are in case b): we need to insert just one constrain. + // Since we popped all of the group indexes, and given that we + // are analyzing the first instruction in the group, we can + // set `next_out_of_bounds_index` to the current index: + // then a check will be inserted, and no other checkk will be + // inserted for the rest of the group. + next_out_of_bounds_index = Some(index); + break; + } else { + continue; + } + } + } + + // We are in case c): some of the instructions are unused. + // We don't need to insert any checks, and given that we already popped + // all of the indexes in the group, there's nothing else to do here. + break; + } + } + } + + // else (for any of the if's above): the next index is not the one for the current instructions, + // so we are in case a), and nothing needs to be done here. + } + } + } + let Some(out_of_bounds_index) = next_out_of_bounds_index else { // No more out of bounds instructions to insert, just push the current instruction function.dfg[block_id].instructions_mut().push(instruction_id); From 03617003dc277287c4cf5a32932102b035d717b4 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 8 Aug 2024 08:03:45 -0300 Subject: [PATCH 18/29] Better names for test programs --- .../slice_get_known_index_out_of_bounds/Nargo.toml | 7 ------- .../slice_get_unknown_index_out_of_bounds/Nargo.toml | 7 ------- .../Nargo.toml | 2 +- .../Prover.toml | 0 .../src/main.nr | 0 .../Nargo.toml | 2 +- .../Prover.toml | 0 .../src/main.nr | 0 .../Nargo.toml | 2 +- .../Prover.toml | 0 .../src/main.nr | 0 .../Nargo.toml | 7 +++++++ .../Prover.toml | 0 .../src/main.nr | 0 .../Nargo.toml | 2 +- .../Prover.toml | 0 .../src/main.nr | 0 .../Nargo.toml | 7 +++++++ .../Prover.toml | 0 .../src/main.nr | 0 20 files changed, 18 insertions(+), 18 deletions(-) delete mode 100644 test_programs/execution_failure/slice_get_known_index_out_of_bounds/Nargo.toml delete mode 100644 test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Nargo.toml rename test_programs/execution_failure/{array_get_known_index_out_of_bounds => unused_array_get_known_index_out_of_bounds}/Nargo.toml (61%) rename test_programs/execution_failure/{array_get_known_index_out_of_bounds => unused_array_get_known_index_out_of_bounds}/Prover.toml (100%) rename test_programs/execution_failure/{array_get_known_index_out_of_bounds => unused_array_get_known_index_out_of_bounds}/src/main.nr (100%) rename test_programs/execution_failure/{array_set_unknown_index_out_of_bounds => unused_array_get_unknown_index_out_of_bounds}/Nargo.toml (60%) rename test_programs/execution_failure/{array_get_unknown_index_out_of_bounds => unused_array_get_unknown_index_out_of_bounds}/Prover.toml (100%) rename test_programs/execution_failure/{array_get_unknown_index_out_of_bounds => unused_array_get_unknown_index_out_of_bounds}/src/main.nr (100%) rename test_programs/execution_failure/{array_get_unknown_index_out_of_bounds => unused_array_set_known_index_out_of_bounds}/Nargo.toml (61%) rename test_programs/execution_failure/{array_set_known_index_out_of_bounds => unused_array_set_known_index_out_of_bounds}/Prover.toml (100%) rename test_programs/execution_failure/{array_set_known_index_out_of_bounds => unused_array_set_known_index_out_of_bounds}/src/main.nr (100%) create mode 100644 test_programs/execution_failure/unused_array_set_unknown_index_out_of_bounds/Nargo.toml rename test_programs/execution_failure/{array_set_unknown_index_out_of_bounds => unused_array_set_unknown_index_out_of_bounds}/Prover.toml (100%) rename test_programs/execution_failure/{array_set_unknown_index_out_of_bounds => unused_array_set_unknown_index_out_of_bounds}/src/main.nr (100%) rename test_programs/execution_failure/{array_set_known_index_out_of_bounds => unused_slice_get_known_index_out_of_bounds}/Nargo.toml (61%) rename test_programs/execution_failure/{slice_get_known_index_out_of_bounds => unused_slice_get_known_index_out_of_bounds}/Prover.toml (100%) rename test_programs/execution_failure/{slice_get_known_index_out_of_bounds => unused_slice_get_known_index_out_of_bounds}/src/main.nr (100%) create mode 100644 test_programs/execution_failure/unused_slice_get_unknown_index_out_of_bounds/Nargo.toml rename test_programs/execution_failure/{slice_get_unknown_index_out_of_bounds => unused_slice_get_unknown_index_out_of_bounds}/Prover.toml (100%) rename test_programs/execution_failure/{slice_get_unknown_index_out_of_bounds => unused_slice_get_unknown_index_out_of_bounds}/src/main.nr (100%) diff --git a/test_programs/execution_failure/slice_get_known_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/slice_get_known_index_out_of_bounds/Nargo.toml deleted file mode 100644 index 7520ed9a9c4..00000000000 --- a/test_programs/execution_failure/slice_get_known_index_out_of_bounds/Nargo.toml +++ /dev/null @@ -1,7 +0,0 @@ -[package] -name = "slice_get_known_index_out_of_bounds" -type = "bin" -authors = [""] -compiler_version = ">=0.31.0" - -[dependencies] \ No newline at end of file diff --git a/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Nargo.toml deleted file mode 100644 index ddda52adeaf..00000000000 --- a/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Nargo.toml +++ /dev/null @@ -1,7 +0,0 @@ -[package] -name = "slice_get_unknown_index_out_of_bounds" -type = "bin" -authors = [""] -compiler_version = ">=0.31.0" - -[dependencies] \ No newline at end of file diff --git a/test_programs/execution_failure/array_get_known_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/unused_array_get_known_index_out_of_bounds/Nargo.toml similarity index 61% rename from test_programs/execution_failure/array_get_known_index_out_of_bounds/Nargo.toml rename to test_programs/execution_failure/unused_array_get_known_index_out_of_bounds/Nargo.toml index 98b59477906..4123215e2b6 100644 --- a/test_programs/execution_failure/array_get_known_index_out_of_bounds/Nargo.toml +++ b/test_programs/execution_failure/unused_array_get_known_index_out_of_bounds/Nargo.toml @@ -1,5 +1,5 @@ [package] -name = "array_get_known_index_out_of_bounds" +name = "unused_array_get_known_index_out_of_bounds" type = "bin" authors = [""] compiler_version = ">=0.31.0" diff --git a/test_programs/execution_failure/array_get_known_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/unused_array_get_known_index_out_of_bounds/Prover.toml similarity index 100% rename from test_programs/execution_failure/array_get_known_index_out_of_bounds/Prover.toml rename to test_programs/execution_failure/unused_array_get_known_index_out_of_bounds/Prover.toml diff --git a/test_programs/execution_failure/array_get_known_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/unused_array_get_known_index_out_of_bounds/src/main.nr similarity index 100% rename from test_programs/execution_failure/array_get_known_index_out_of_bounds/src/main.nr rename to test_programs/execution_failure/unused_array_get_known_index_out_of_bounds/src/main.nr diff --git a/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/unused_array_get_unknown_index_out_of_bounds/Nargo.toml similarity index 60% rename from test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Nargo.toml rename to test_programs/execution_failure/unused_array_get_unknown_index_out_of_bounds/Nargo.toml index 8d3b47b2b86..04d9146b881 100644 --- a/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Nargo.toml +++ b/test_programs/execution_failure/unused_array_get_unknown_index_out_of_bounds/Nargo.toml @@ -1,5 +1,5 @@ [package] -name = "array_set_unknown_index_out_of_bounds" +name = "unused_array_get_unknown_index_out_of_bounds" type = "bin" authors = [""] compiler_version = ">=0.31.0" diff --git a/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/unused_array_get_unknown_index_out_of_bounds/Prover.toml similarity index 100% rename from test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Prover.toml rename to test_programs/execution_failure/unused_array_get_unknown_index_out_of_bounds/Prover.toml diff --git a/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/unused_array_get_unknown_index_out_of_bounds/src/main.nr similarity index 100% rename from test_programs/execution_failure/array_get_unknown_index_out_of_bounds/src/main.nr rename to test_programs/execution_failure/unused_array_get_unknown_index_out_of_bounds/src/main.nr diff --git a/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/unused_array_set_known_index_out_of_bounds/Nargo.toml similarity index 61% rename from test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Nargo.toml rename to test_programs/execution_failure/unused_array_set_known_index_out_of_bounds/Nargo.toml index 986e9e8e2e6..b8fe7e955a1 100644 --- a/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Nargo.toml +++ b/test_programs/execution_failure/unused_array_set_known_index_out_of_bounds/Nargo.toml @@ -1,5 +1,5 @@ [package] -name = "array_get_unknown_index_out_of_bounds" +name = "unused_array_set_known_index_out_of_bounds" type = "bin" authors = [""] compiler_version = ">=0.31.0" diff --git a/test_programs/execution_failure/array_set_known_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/unused_array_set_known_index_out_of_bounds/Prover.toml similarity index 100% rename from test_programs/execution_failure/array_set_known_index_out_of_bounds/Prover.toml rename to test_programs/execution_failure/unused_array_set_known_index_out_of_bounds/Prover.toml diff --git a/test_programs/execution_failure/array_set_known_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/unused_array_set_known_index_out_of_bounds/src/main.nr similarity index 100% rename from test_programs/execution_failure/array_set_known_index_out_of_bounds/src/main.nr rename to test_programs/execution_failure/unused_array_set_known_index_out_of_bounds/src/main.nr diff --git a/test_programs/execution_failure/unused_array_set_unknown_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/unused_array_set_unknown_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..ccc00956e80 --- /dev/null +++ b/test_programs/execution_failure/unused_array_set_unknown_index_out_of_bounds/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "unused_array_set_unknown_index_out_of_bounds" +type = "bin" +authors = [""] +compiler_version = ">=0.31.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/unused_array_set_unknown_index_out_of_bounds/Prover.toml similarity index 100% rename from test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Prover.toml rename to test_programs/execution_failure/unused_array_set_unknown_index_out_of_bounds/Prover.toml diff --git a/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/unused_array_set_unknown_index_out_of_bounds/src/main.nr similarity index 100% rename from test_programs/execution_failure/array_set_unknown_index_out_of_bounds/src/main.nr rename to test_programs/execution_failure/unused_array_set_unknown_index_out_of_bounds/src/main.nr diff --git a/test_programs/execution_failure/array_set_known_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/unused_slice_get_known_index_out_of_bounds/Nargo.toml similarity index 61% rename from test_programs/execution_failure/array_set_known_index_out_of_bounds/Nargo.toml rename to test_programs/execution_failure/unused_slice_get_known_index_out_of_bounds/Nargo.toml index f6beafc4e90..f2acfa4d4cf 100644 --- a/test_programs/execution_failure/array_set_known_index_out_of_bounds/Nargo.toml +++ b/test_programs/execution_failure/unused_slice_get_known_index_out_of_bounds/Nargo.toml @@ -1,5 +1,5 @@ [package] -name = "array_set_known_index_out_of_bounds" +name = "unused_slice_get_known_index_out_of_bounds" type = "bin" authors = [""] compiler_version = ">=0.31.0" diff --git a/test_programs/execution_failure/slice_get_known_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/unused_slice_get_known_index_out_of_bounds/Prover.toml similarity index 100% rename from test_programs/execution_failure/slice_get_known_index_out_of_bounds/Prover.toml rename to test_programs/execution_failure/unused_slice_get_known_index_out_of_bounds/Prover.toml diff --git a/test_programs/execution_failure/slice_get_known_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/unused_slice_get_known_index_out_of_bounds/src/main.nr similarity index 100% rename from test_programs/execution_failure/slice_get_known_index_out_of_bounds/src/main.nr rename to test_programs/execution_failure/unused_slice_get_known_index_out_of_bounds/src/main.nr diff --git a/test_programs/execution_failure/unused_slice_get_unknown_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/unused_slice_get_unknown_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..3c8ae8fe07a --- /dev/null +++ b/test_programs/execution_failure/unused_slice_get_unknown_index_out_of_bounds/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "unused_slice_get_unknown_index_out_of_bounds" +type = "bin" +authors = [""] +compiler_version = ">=0.31.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/unused_slice_get_unknown_index_out_of_bounds/Prover.toml similarity index 100% rename from test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Prover.toml rename to test_programs/execution_failure/unused_slice_get_unknown_index_out_of_bounds/Prover.toml diff --git a/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/unused_slice_get_unknown_index_out_of_bounds/src/main.nr similarity index 100% rename from test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/src/main.nr rename to test_programs/execution_failure/unused_slice_get_unknown_index_out_of_bounds/src/main.nr From 8bae45934cfc420566c6b8c3f1c00b474e5d34ac Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 8 Aug 2024 08:15:10 -0300 Subject: [PATCH 19/29] Extract `handle_array_get_group` --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 165 +++++++++++--------- 1 file changed, 95 insertions(+), 70 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 5942af0124a..34cbe4d2d5c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -179,76 +179,13 @@ impl Context { // If it's an ArrayGet we'll deal with groups of it in case the array type is a composite type. if let Instruction::ArrayGet { array, .. } = instruction { - if let Some(array_length) = function.dfg.try_get_array_length(*array) { - let flattened_size = function.dfg.type_of_value(*array).flattened_size(); - let element_size = flattened_size / array_length; - if element_size > 1 { - // It's a composite type. - // When doing ArrayGet on a composite type, this **always** results in instructions like these - // (assuming element_size == 3): - // - // 1. v27 = array_get v1, index v26 - // 2. v28 = add v26, u32 1 - // 3. v29 = array_get v1, index v28 - // 4. v30 = add v26, u32 2 - // 5. v31 = array_get v1, index v30 - // - // That means that after this instructions, (element_size - 1) instructions will be - // part of this composite array get, and they'll be two instructions apart. - // - // Now three things can happen: - // a) none of the array_get instructions are unused: in this case they won't be in - // `possible_index_out_of_bounds_indexes` and they won't be removed, nothing to do here - // b) all of the array_get instructions are unused: in this case we can replace **all** - // of them with just one constrain: no need to do one per array_get - // c) some of the array_get instructions are unused, but not all: in this case - // we don't need to insert any constrain, because on a later stage array bound checks - // will be performed anyway. We'll let DIE remove the unused ones, without replacing - // them with bounds checks, and leave the used ones. - // - // To check in which scenario we are we can get from `possible_index_out_of_bounds_indexes` - // (starting from `next_out_of_bounds_index`) while we are in the group ranges - // (1..=5 in the example above) - - if let Some(out_of_bounds_index) = next_out_of_bounds_index { - if index == out_of_bounds_index { - // What's the last instruction that's part of the group? (5 in the example above) - let last_instruction_index = index + 2 * (element_size - 1); - // How many unused instructions are in this group? - let mut unused_count = 1; - loop { - next_out_of_bounds_index = - possible_index_out_of_bounds_indexes.pop(); - if let Some(out_of_bounds_index) = next_out_of_bounds_index { - if out_of_bounds_index <= last_instruction_index { - unused_count += 1; - if unused_count == element_size { - // We are in case b): we need to insert just one constrain. - // Since we popped all of the group indexes, and given that we - // are analyzing the first instruction in the group, we can - // set `next_out_of_bounds_index` to the current index: - // then a check will be inserted, and no other checkk will be - // inserted for the rest of the group. - next_out_of_bounds_index = Some(index); - break; - } else { - continue; - } - } - } - - // We are in case c): some of the instructions are unused. - // We don't need to insert any checks, and given that we already popped - // all of the indexes in the group, there's nothing else to do here. - break; - } - } - } - - // else (for any of the if's above): the next index is not the one for the current instructions, - // so we are in case a), and nothing needs to be done here. - } - } + handle_array_get_group( + function, + array, + index, + &mut next_out_of_bounds_index, + possible_index_out_of_bounds_indexes, + ); } let Some(out_of_bounds_index) = next_out_of_bounds_index else { @@ -427,6 +364,94 @@ fn instruction_might_result_in_out_of_bounds( } } +fn handle_array_get_group( + function: &Function, + array: &ValueId, + index: usize, + next_out_of_bounds_index: &mut Option, + possible_index_out_of_bounds_indexes: &mut Vec, +) { + let Some(array_length) = function.dfg.try_get_array_length(*array) else { + // Nothing to do for slices + return; + }; + + let flattened_size = function.dfg.type_of_value(*array).flattened_size(); + let element_size = flattened_size / array_length; + if element_size <= 1 { + // Not a composite type + return; + }; + + // It's a composite type. + // When doing ArrayGet on a composite type, this **always** results in instructions like these + // (assuming element_size == 3): + // + // 1. v27 = array_get v1, index v26 + // 2. v28 = add v26, u32 1 + // 3. v29 = array_get v1, index v28 + // 4. v30 = add v26, u32 2 + // 5. v31 = array_get v1, index v30 + // + // That means that after this instructions, (element_size - 1) instructions will be + // part of this composite array get, and they'll be two instructions apart. + // + // Now three things can happen: + // a) none of the array_get instructions are unused: in this case they won't be in + // `possible_index_out_of_bounds_indexes` and they won't be removed, nothing to do here + // b) all of the array_get instructions are unused: in this case we can replace **all** + // of them with just one constrain: no need to do one per array_get + // c) some of the array_get instructions are unused, but not all: in this case + // we don't need to insert any constrain, because on a later stage array bound checks + // will be performed anyway. We'll let DIE remove the unused ones, without replacing + // them with bounds checks, and leave the used ones. + // + // To check in which scenario we are we can get from `possible_index_out_of_bounds_indexes` + // (starting from `next_out_of_bounds_index`) while we are in the group ranges + // (1..=5 in the example above) + + let Some(out_of_bounds_index) = *next_out_of_bounds_index else { + // No next unused instruction, so this is case a) and nothing needs to be done here + return; + }; + + if index != out_of_bounds_index { + // The next index is not the one for the current instructions, + // so we are in case a), and nothing needs to be done here + return; + } + + // What's the last instruction that's part of the group? (5 in the example above) + let last_instruction_index = index + 2 * (element_size - 1); + // How many unused instructions are in this group? + let mut unused_count = 1; + loop { + *next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); + if let Some(out_of_bounds_index) = *next_out_of_bounds_index { + if out_of_bounds_index <= last_instruction_index { + unused_count += 1; + if unused_count == element_size { + // We are in case b): we need to insert just one constrain. + // Since we popped all of the group indexes, and given that we + // are analyzing the first instruction in the group, we can + // set `next_out_of_bounds_index` to the current index: + // then a check will be inserted, and no other checkk will be + // inserted for the rest of the group. + *next_out_of_bounds_index = Some(index); + break; + } else { + continue; + } + } + } + + // We are in case c): some of the instructions are unused. + // We don't need to insert any checks, and given that we already popped + // all of the indexes in the group, there's nothing else to do here. + break; + } +} + fn apply_side_effects( side_effects_condition: Option, lhs: ValueId, From a0d18d18c32ac2a158027f404b140e66b62362f0 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 8 Aug 2024 08:20:13 -0300 Subject: [PATCH 20/29] Continue with DIE if no checks were inserted --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 34cbe4d2d5c..a2e0cc63a99 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -137,12 +137,15 @@ impl Context { // first add constrain checks. Then run the DIE pass again, which will remove those // but leave the constrains (any any value needed by those constrains) if !possible_index_out_of_bounds_indexes.is_empty() { - self.insert_out_of_bounds_checks( + let inserted_check = self.replace_array_instructions_with_out_of_bounds_checks( function, block_id, &mut possible_index_out_of_bounds_indexes, ); - return true; + // There's a slight chance we didn't insert any checks, so we could proceed with DIE. + if inserted_check { + return true; + } } function.dfg[block_id] @@ -152,12 +155,19 @@ impl Context { false } - fn insert_out_of_bounds_checks( + /// Replaces unused ArrayGet/ArraySet instructions with out of bounds checks. + /// Returns `true` if at least one check was inserted. + /// Becuase some ArrayGet might happen in groups (for composite types), if just + /// some of the instructions in a group are used but not all of them, no check + /// is inserted, so this method might return `false`. + fn replace_array_instructions_with_out_of_bounds_checks( &mut self, function: &mut Function, block_id: BasicBlockId, possible_index_out_of_bounds_indexes: &mut Vec, - ) { + ) -> bool { + let mut inserted_check = false; + // Keep track of the current side effects condition let mut side_effects_condition = None; @@ -265,9 +275,12 @@ impl Context { None, call_stack, ); + inserted_check = true; next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); } + + inserted_check } /// Returns true if an instruction can be removed. From 8969fb3d51c111767c32b8bc6d2f3a7a5b003212 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 8 Aug 2024 08:26:03 -0300 Subject: [PATCH 21/29] Small adjustments --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index a2e0cc63a99..2499fb23668 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -187,7 +187,8 @@ impl Context { continue; }; - // If it's an ArrayGet we'll deal with groups of it in case the array type is a composite type. + // If it's an ArrayGet we'll deal with groups of it in case the array type is a composite type, + // and adjust `next_out_of_bounds_index` and `possible_index_out_of_bounds_indexes` accordingly if let Instruction::ArrayGet { array, .. } = instruction { handle_array_get_group( function, @@ -211,14 +212,14 @@ impl Context { } // This is an instruction that might be out of bounds: let's add a constrain. - let call_stack = function.dfg.get_call_stack(instruction_id); - let (array, index) = match instruction { Instruction::ArrayGet { array, index } | Instruction::ArraySet { array, index, .. } => (array, index), _ => panic!("Expected an ArrayGet or ArraySet instruction here"), }; + let call_stack = function.dfg.get_call_stack(instruction_id); + let (lhs, rhs) = if function.dfg.get_numeric_constant(*index).is_some() { // If we are here it means the index is known but out of bounds. That's always an error! let false_const = function.dfg.make_constant(false.into(), Type::bool()); From 55e26b170480f791e184ab131b25907b69bfb398 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 8 Aug 2024 08:27:22 -0300 Subject: [PATCH 22/29] Move function down --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 132 ++++++++++---------- 1 file changed, 66 insertions(+), 66 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 2499fb23668..e3f14bb6bea 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -155,6 +155,72 @@ impl Context { false } + /// Returns true if an instruction can be removed. + /// + /// An instruction can be removed as long as it has no side-effects, and none of its result + /// values have been referenced. + fn is_unused(&self, instruction_id: InstructionId, function: &Function) -> bool { + let instruction = &function.dfg[instruction_id]; + + if instruction.can_eliminate_if_unused(&function.dfg) { + let results = function.dfg.instruction_results(instruction_id); + results.iter().all(|result| !self.used_values.contains(result)) + } else if let Instruction::Call { func, arguments } = instruction { + // TODO: make this more general for instructions which don't have results but have side effects "sometimes" like `Intrinsic::AsWitness` + let as_witness_id = function.dfg.get_intrinsic(Intrinsic::AsWitness); + as_witness_id == Some(func) && !self.used_values.contains(&arguments[0]) + } else { + // If the instruction has side effects we should never remove it. + false + } + } + + /// Adds values referenced by the terminator to the set of used values. + fn mark_terminator_values_as_used(&mut self, function: &Function, block: &BasicBlock) { + block.unwrap_terminator().for_each_value(|value| { + self.mark_used_instruction_results(&function.dfg, value); + }); + } + + /// Inspects a value recursively (as it could be an array) and marks all comprised instruction + /// results as used. + fn mark_used_instruction_results(&mut self, dfg: &DataFlowGraph, value_id: ValueId) { + let value_id = dfg.resolve(value_id); + match &dfg[value_id] { + Value::Instruction { .. } => { + self.used_values.insert(value_id); + } + Value::Array { array, .. } => { + for elem in array { + self.mark_used_instruction_results(dfg, *elem); + } + } + Value::Param { .. } => { + self.used_values.insert(value_id); + } + _ => { + // Does not comprise of any instruction results + } + } + } + + fn remove_rc_instructions(self, dfg: &mut DataFlowGraph) { + for (rc, block) in self.rc_instructions { + let value = match &dfg[rc] { + Instruction::IncrementRc { value } => *value, + Instruction::DecrementRc { value } => *value, + other => { + unreachable!("Expected IncrementRc or DecrementRc instruction, found {other:?}") + } + }; + + // This could be more efficient if we have to remove multiple instructions in a single block + if !self.used_values.contains(&value) { + dfg[block].instructions_mut().retain(|instruction| *instruction != rc); + } + } + } + /// Replaces unused ArrayGet/ArraySet instructions with out of bounds checks. /// Returns `true` if at least one check was inserted. /// Becuase some ArrayGet might happen in groups (for composite types), if just @@ -283,72 +349,6 @@ impl Context { inserted_check } - - /// Returns true if an instruction can be removed. - /// - /// An instruction can be removed as long as it has no side-effects, and none of its result - /// values have been referenced. - fn is_unused(&self, instruction_id: InstructionId, function: &Function) -> bool { - let instruction = &function.dfg[instruction_id]; - - if instruction.can_eliminate_if_unused(&function.dfg) { - let results = function.dfg.instruction_results(instruction_id); - results.iter().all(|result| !self.used_values.contains(result)) - } else if let Instruction::Call { func, arguments } = instruction { - // TODO: make this more general for instructions which don't have results but have side effects "sometimes" like `Intrinsic::AsWitness` - let as_witness_id = function.dfg.get_intrinsic(Intrinsic::AsWitness); - as_witness_id == Some(func) && !self.used_values.contains(&arguments[0]) - } else { - // If the instruction has side effects we should never remove it. - false - } - } - - /// Adds values referenced by the terminator to the set of used values. - fn mark_terminator_values_as_used(&mut self, function: &Function, block: &BasicBlock) { - block.unwrap_terminator().for_each_value(|value| { - self.mark_used_instruction_results(&function.dfg, value); - }); - } - - /// Inspects a value recursively (as it could be an array) and marks all comprised instruction - /// results as used. - fn mark_used_instruction_results(&mut self, dfg: &DataFlowGraph, value_id: ValueId) { - let value_id = dfg.resolve(value_id); - match &dfg[value_id] { - Value::Instruction { .. } => { - self.used_values.insert(value_id); - } - Value::Array { array, .. } => { - for elem in array { - self.mark_used_instruction_results(dfg, *elem); - } - } - Value::Param { .. } => { - self.used_values.insert(value_id); - } - _ => { - // Does not comprise of any instruction results - } - } - } - - fn remove_rc_instructions(self, dfg: &mut DataFlowGraph) { - for (rc, block) in self.rc_instructions { - let value = match &dfg[rc] { - Instruction::IncrementRc { value } => *value, - Instruction::DecrementRc { value } => *value, - other => { - unreachable!("Expected IncrementRc or DecrementRc instruction, found {other:?}") - } - }; - - // This could be more efficient if we have to remove multiple instructions in a single block - if !self.used_values.contains(&value) { - dfg[block].instructions_mut().retain(|instruction| *instruction != rc); - } - } - } } fn instruction_might_result_in_out_of_bounds( From 0eac5c4004311462f3e341c587ff2999a0466b62 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 8 Aug 2024 08:29:45 -0300 Subject: [PATCH 23/29] Comment function --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index e3f14bb6bea..23c328c8cef 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -466,6 +466,8 @@ fn handle_array_get_group( } } +// Given `lhs` and `rhs` values, if there's a side effects condition this will +// return (`lhs * condition`, `rhs * condition`), otherwise just (`lhs`, `rhs`) fn apply_side_effects( side_effects_condition: Option, lhs: ValueId, From f099ec82c1ddb6218cdf74b5efefc86fdf34e6cf Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 8 Aug 2024 08:38:23 -0300 Subject: [PATCH 24/29] typos --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 23c328c8cef..fd00dc83b44 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -223,7 +223,7 @@ impl Context { /// Replaces unused ArrayGet/ArraySet instructions with out of bounds checks. /// Returns `true` if at least one check was inserted. - /// Becuase some ArrayGet might happen in groups (for composite types), if just + /// Because some ArrayGet might happen in groups (for composite types), if just /// some of the instructions in a group are used but not all of them, no check /// is inserted, so this method might return `false`. fn replace_array_instructions_with_out_of_bounds_checks( @@ -449,7 +449,7 @@ fn handle_array_get_group( // Since we popped all of the group indexes, and given that we // are analyzing the first instruction in the group, we can // set `next_out_of_bounds_index` to the current index: - // then a check will be inserted, and no other checkk will be + // then a check will be inserted, and no other check will be // inserted for the rest of the group. *next_out_of_bounds_index = Some(index); break; From aa123a8ec6879075c18c12920906783235c068a1 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 8 Aug 2024 08:38:27 -0300 Subject: [PATCH 25/29] Remove unused test programs --- .../slice_set_known_index_out_of_bounds/Nargo.toml | 7 ------- .../slice_set_known_index_out_of_bounds/Prover.toml | 0 .../slice_set_known_index_out_of_bounds/src/main.nr | 6 ------ .../slice_set_unknown_index_out_of_bounds/Nargo.toml | 7 ------- .../slice_set_unknown_index_out_of_bounds/Prover.toml | 1 - .../slice_set_unknown_index_out_of_bounds/src/main.nr | 6 ------ 6 files changed, 27 deletions(-) delete mode 100644 test_programs/execution_failure/slice_set_known_index_out_of_bounds/Nargo.toml delete mode 100644 test_programs/execution_failure/slice_set_known_index_out_of_bounds/Prover.toml delete mode 100644 test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr delete mode 100644 test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Nargo.toml delete mode 100644 test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Prover.toml delete mode 100644 test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr diff --git a/test_programs/execution_failure/slice_set_known_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/slice_set_known_index_out_of_bounds/Nargo.toml deleted file mode 100644 index ea535921e99..00000000000 --- a/test_programs/execution_failure/slice_set_known_index_out_of_bounds/Nargo.toml +++ /dev/null @@ -1,7 +0,0 @@ -[package] -name = "slice_set_known_index_out_of_bounds" -type = "bin" -authors = [""] -compiler_version = ">=0.31.0" - -[dependencies] \ No newline at end of file diff --git a/test_programs/execution_failure/slice_set_known_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/slice_set_known_index_out_of_bounds/Prover.toml deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr deleted file mode 100644 index b704dbb0c79..00000000000 --- a/test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr +++ /dev/null @@ -1,6 +0,0 @@ -fn main() { - // TODO: uncomment this - // let mut slice = &[1, 2, 3]; - // slice[10] = 1; // Index out of bounds - assert(false); -} diff --git a/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Nargo.toml deleted file mode 100644 index 597f7be81ba..00000000000 --- a/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Nargo.toml +++ /dev/null @@ -1,7 +0,0 @@ -[package] -name = "slice_set_unknown_index_out_of_bounds" -type = "bin" -authors = [""] -compiler_version = ">=0.31.0" - -[dependencies] \ No newline at end of file diff --git a/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Prover.toml deleted file mode 100644 index 1ec81884d61..00000000000 --- a/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Prover.toml +++ /dev/null @@ -1 +0,0 @@ -x = "10" \ No newline at end of file diff --git a/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr deleted file mode 100644 index 9f344e362f6..00000000000 --- a/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr +++ /dev/null @@ -1,6 +0,0 @@ -fn main(x: Field) { - // TODO: uncomment this - // let mut slice = &[1, 2, 3]; - // slice[x] = 1; // Index out of bounds - assert(false); -} From 4b9538d45eb3c6ae138edb300508b3a3233b09d7 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 8 Aug 2024 10:09:31 -0300 Subject: [PATCH 26/29] Use `Instruction::binary` --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index fd00dc83b44..f488ef390e4 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -10,7 +10,7 @@ use crate::ssa::{ basic_block::{BasicBlock, BasicBlockId}, dfg::DataFlowGraph, function::Function, - instruction::{Binary, BinaryOp, Instruction, InstructionId, Intrinsic}, + instruction::{BinaryOp, Instruction, InstructionId, Intrinsic}, post_order::PostOrder, types::Type, value::{Value, ValueId}, @@ -312,11 +312,7 @@ impl Context { let is_index_out_of_bounds = function .dfg .insert_instruction_and_results( - Instruction::Binary(Binary { - operator: BinaryOp::Lt, - lhs: index, - rhs: array_length, - }), + Instruction::binary(BinaryOp::Lt, index, array_length), block_id, None, call_stack.clone(), From ef07ccbe0275451db7c23df121cbac528eaaa4b3 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 9 Aug 2024 11:21:07 -0300 Subject: [PATCH 27/29] Extract `replace_unused_array_instructions_with_bounds_checks` optimization --- compiler/noirc_evaluator/src/ssa.rs | 5 + compiler/noirc_evaluator/src/ssa/opt/die.rs | 423 +----------------- compiler/noirc_evaluator/src/ssa/opt/mod.rs | 1 + ...d_array_instructions_with_bounds_checks.rs | 374 ++++++++++++++++ compiler/noirc_evaluator/src/ssa/unused.rs | 70 +++ 5 files changed, 467 insertions(+), 406 deletions(-) create mode 100644 compiler/noirc_evaluator/src/ssa/opt/replace_unused_array_instructions_with_bounds_checks.rs create mode 100644 compiler/noirc_evaluator/src/ssa/unused.rs diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 032e3d07d7b..66144a8f974 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -46,6 +46,7 @@ pub(super) mod function_builder; pub mod ir; mod opt; pub mod ssa_gen; +mod unused; pub struct SsaEvaluatorOptions { /// Emit debug information for the intermediate SSA IR @@ -113,6 +114,10 @@ pub(crate) fn optimize_into_acir( .run_pass(Ssa::fold_constants, "After Constant Folding:") .run_pass(Ssa::remove_enable_side_effects, "After EnableSideEffects removal:") .run_pass(Ssa::fold_constants_using_constraints, "After Constraint Folding:") + .run_pass( + Ssa::replace_unused_array_instructions_with_bounds_checks, + "After replacing unused array instructions with bounds checks:", + ) .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") .run_pass(Ssa::array_set_optimization, "After Array Set Optimizations:") .finish(); diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index f488ef390e4..c52b3897434 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -2,20 +2,17 @@ //! which the results are unused. use std::collections::HashSet; -use im::Vector; -use noirc_errors::Location; - use crate::ssa::{ ir::{ - basic_block::{BasicBlock, BasicBlockId}, + basic_block::BasicBlockId, dfg::DataFlowGraph, function::Function, - instruction::{BinaryOp, Instruction, InstructionId, Intrinsic}, + instruction::{Instruction, InstructionId}, post_order::PostOrder, - types::Type, - value::{Value, ValueId}, + value::ValueId, }, - ssa_gen::{Ssa, SSA_WORD_SIZE}, + ssa_gen::Ssa, + unused::{is_unused, mark_terminator_values_as_used, mark_used_instruction_results}, }; impl Ssa { @@ -24,7 +21,7 @@ impl Ssa { #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn dead_instruction_elimination(mut self) -> Ssa { for function in self.functions.values_mut() { - dead_instruction_elimination(function, true); + dead_instruction_elimination(function); } self } @@ -36,29 +33,16 @@ impl Ssa { /// instructions that reference results from an instruction in another block are evaluated first. /// If we did not iterate blocks in this order we could not safely say whether or not the results /// of its instructions are needed elsewhere. -fn dead_instruction_elimination(function: &mut Function, insert_out_of_bounds_checks: bool) { +fn dead_instruction_elimination(function: &mut Function) { let mut context = Context::default(); for call_data in &function.dfg.data_bus.call_data { - context.mark_used_instruction_results(&function.dfg, call_data.array_id); + mark_used_instruction_results(&mut context.used_values, &function.dfg, call_data.array_id); } - let mut inserted_out_of_bounds_checks = false; - let blocks = PostOrder::with_function(function); - for block in blocks.as_slice() { - inserted_out_of_bounds_checks |= context.remove_unused_instructions_in_block( - function, - *block, - insert_out_of_bounds_checks, - ); - } - // If we inserted out of bounds check, let's run the pass again with those new - // instructions (we don't want to remove those checks, or instructions that are - // dependencies of those checks) - if inserted_out_of_bounds_checks { - dead_instruction_elimination(function, false); - return; + for block in blocks.as_slice() { + context.remove_unused_instructions_in_block(function, *block); } context.remove_rc_instructions(&mut function.dfg); @@ -88,120 +72,34 @@ impl Context { /// values set. This allows DIE to identify whole chains of unused instructions. (If the /// values referenced by an unused instruction were considered to be used, only the head of /// such chains would be removed.) - /// - /// If `insert_out_of_bounds_checks` is true and there are unused ArrayGet/ArraySet that - /// might be out of bounds, this method will insert out of bounds checks instead of - /// removing unused instructions and return `true`. The idea then is to later call this - /// function again with `insert_out_of_bounds_checks` set to false to effectively remove - /// unused instructions but leave the out of bounds checks. fn remove_unused_instructions_in_block( &mut self, function: &mut Function, block_id: BasicBlockId, - insert_out_of_bounds_checks: bool, - ) -> bool { + ) { let block = &function.dfg[block_id]; - self.mark_terminator_values_as_used(function, block); + mark_terminator_values_as_used(&mut self.used_values, function, block); - let instructions_len = block.instructions().len(); - - // Indexes of instructions that might be out of bounds. - // We'll remove those, but before that we'll insert bounds checks for them. - let mut possible_index_out_of_bounds_indexes = Vec::new(); - - for (instruction_index, instruction_id) in block.instructions().iter().rev().enumerate() { - let instruction = &function.dfg[*instruction_id]; - - if self.is_unused(*instruction_id, function) { + for instruction_id in block.instructions().iter().rev() { + if is_unused(&self.used_values, *instruction_id, function) { self.instructions_to_remove.insert(*instruction_id); - - if insert_out_of_bounds_checks - && instruction_might_result_in_out_of_bounds(function, instruction) - { - possible_index_out_of_bounds_indexes - .push(instructions_len - instruction_index - 1); - } } else { + let instruction = &function.dfg[*instruction_id]; + use Instruction::*; if matches!(instruction, IncrementRc { .. } | DecrementRc { .. }) { self.rc_instructions.push((*instruction_id, block_id)); } else { instruction.for_each_value(|value| { - self.mark_used_instruction_results(&function.dfg, value); + mark_used_instruction_results(&mut self.used_values, &function.dfg, value); }); } } } - // If there are some instructions that might trigger an out of bounds error, - // first add constrain checks. Then run the DIE pass again, which will remove those - // but leave the constrains (any any value needed by those constrains) - if !possible_index_out_of_bounds_indexes.is_empty() { - let inserted_check = self.replace_array_instructions_with_out_of_bounds_checks( - function, - block_id, - &mut possible_index_out_of_bounds_indexes, - ); - // There's a slight chance we didn't insert any checks, so we could proceed with DIE. - if inserted_check { - return true; - } - } - function.dfg[block_id] .instructions_mut() .retain(|instruction| !self.instructions_to_remove.contains(instruction)); - - false - } - - /// Returns true if an instruction can be removed. - /// - /// An instruction can be removed as long as it has no side-effects, and none of its result - /// values have been referenced. - fn is_unused(&self, instruction_id: InstructionId, function: &Function) -> bool { - let instruction = &function.dfg[instruction_id]; - - if instruction.can_eliminate_if_unused(&function.dfg) { - let results = function.dfg.instruction_results(instruction_id); - results.iter().all(|result| !self.used_values.contains(result)) - } else if let Instruction::Call { func, arguments } = instruction { - // TODO: make this more general for instructions which don't have results but have side effects "sometimes" like `Intrinsic::AsWitness` - let as_witness_id = function.dfg.get_intrinsic(Intrinsic::AsWitness); - as_witness_id == Some(func) && !self.used_values.contains(&arguments[0]) - } else { - // If the instruction has side effects we should never remove it. - false - } - } - - /// Adds values referenced by the terminator to the set of used values. - fn mark_terminator_values_as_used(&mut self, function: &Function, block: &BasicBlock) { - block.unwrap_terminator().for_each_value(|value| { - self.mark_used_instruction_results(&function.dfg, value); - }); - } - - /// Inspects a value recursively (as it could be an array) and marks all comprised instruction - /// results as used. - fn mark_used_instruction_results(&mut self, dfg: &DataFlowGraph, value_id: ValueId) { - let value_id = dfg.resolve(value_id); - match &dfg[value_id] { - Value::Instruction { .. } => { - self.used_values.insert(value_id); - } - Value::Array { array, .. } => { - for elem in array { - self.mark_used_instruction_results(dfg, *elem); - } - } - Value::Param { .. } => { - self.used_values.insert(value_id); - } - _ => { - // Does not comprise of any instruction results - } - } } fn remove_rc_instructions(self, dfg: &mut DataFlowGraph) { @@ -220,293 +118,6 @@ impl Context { } } } - - /// Replaces unused ArrayGet/ArraySet instructions with out of bounds checks. - /// Returns `true` if at least one check was inserted. - /// Because some ArrayGet might happen in groups (for composite types), if just - /// some of the instructions in a group are used but not all of them, no check - /// is inserted, so this method might return `false`. - fn replace_array_instructions_with_out_of_bounds_checks( - &mut self, - function: &mut Function, - block_id: BasicBlockId, - possible_index_out_of_bounds_indexes: &mut Vec, - ) -> bool { - let mut inserted_check = false; - - // Keep track of the current side effects condition - let mut side_effects_condition = None; - - // Keep track of the next index we need to handle - let mut next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); - - let instructions = function.dfg[block_id].take_instructions(); - for (index, instruction_id) in instructions.iter().enumerate() { - let instruction_id = *instruction_id; - let instruction = &function.dfg[instruction_id]; - - if let Instruction::EnableSideEffects { condition } = instruction { - side_effects_condition = Some(*condition); - - // We still need to keep the EnableSideEffects instruction - function.dfg[block_id].instructions_mut().push(instruction_id); - continue; - }; - - // If it's an ArrayGet we'll deal with groups of it in case the array type is a composite type, - // and adjust `next_out_of_bounds_index` and `possible_index_out_of_bounds_indexes` accordingly - if let Instruction::ArrayGet { array, .. } = instruction { - handle_array_get_group( - function, - array, - index, - &mut next_out_of_bounds_index, - possible_index_out_of_bounds_indexes, - ); - } - - let Some(out_of_bounds_index) = next_out_of_bounds_index else { - // No more out of bounds instructions to insert, just push the current instruction - function.dfg[block_id].instructions_mut().push(instruction_id); - continue; - }; - - if index != out_of_bounds_index { - // This instruction is not out of bounds: let's just push it - function.dfg[block_id].instructions_mut().push(instruction_id); - continue; - } - - // This is an instruction that might be out of bounds: let's add a constrain. - let (array, index) = match instruction { - Instruction::ArrayGet { array, index } - | Instruction::ArraySet { array, index, .. } => (array, index), - _ => panic!("Expected an ArrayGet or ArraySet instruction here"), - }; - - let call_stack = function.dfg.get_call_stack(instruction_id); - - let (lhs, rhs) = if function.dfg.get_numeric_constant(*index).is_some() { - // If we are here it means the index is known but out of bounds. That's always an error! - let false_const = function.dfg.make_constant(false.into(), Type::bool()); - let true_const = function.dfg.make_constant(true.into(), Type::bool()); - (false_const, true_const) - } else { - // `index` will be relative to the flattened array length, so we need to take that into account - let array_length = function.dfg.type_of_value(*array).flattened_size(); - - // If we are here it means the index is dynamic, so let's add a check that it's less than length - let index = function - .dfg - .insert_instruction_and_results( - Instruction::Cast(*index, Type::unsigned(SSA_WORD_SIZE)), - block_id, - None, - call_stack.clone(), - ) - .first(); - - let array_length = function - .dfg - .make_constant((array_length as u128).into(), Type::unsigned(SSA_WORD_SIZE)); - let is_index_out_of_bounds = function - .dfg - .insert_instruction_and_results( - Instruction::binary(BinaryOp::Lt, index, array_length), - block_id, - None, - call_stack.clone(), - ) - .first(); - let true_const = function.dfg.make_constant(true.into(), Type::bool()); - (is_index_out_of_bounds, true_const) - }; - - let (lhs, rhs) = apply_side_effects( - side_effects_condition, - lhs, - rhs, - function, - block_id, - call_stack.clone(), - ); - - let message = Some("Index out of bounds".to_owned().into()); - function.dfg.insert_instruction_and_results( - Instruction::Constrain(lhs, rhs, message), - block_id, - None, - call_stack, - ); - inserted_check = true; - - next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); - } - - inserted_check - } -} - -fn instruction_might_result_in_out_of_bounds( - function: &Function, - instruction: &Instruction, -) -> bool { - use Instruction::*; - match instruction { - ArrayGet { array, index } | ArraySet { array, index, .. } => { - if function.dfg.try_get_array_length(*array).is_some() { - if let Some(known_index) = function.dfg.get_numeric_constant(*index) { - // `index` will be relative to the flattened array length, so we need to take that into account - let typ = function.dfg.type_of_value(*array); - let array_length = typ.flattened_size(); - known_index >= array_length.into() - } else { - // A dynamic index might always be out of bounds - true - } - } else { - // Slice operations might be out of bounds, but there's no way we - // can insert a check because we don't know a slice's length - false - } - } - _ => false, - } -} - -fn handle_array_get_group( - function: &Function, - array: &ValueId, - index: usize, - next_out_of_bounds_index: &mut Option, - possible_index_out_of_bounds_indexes: &mut Vec, -) { - let Some(array_length) = function.dfg.try_get_array_length(*array) else { - // Nothing to do for slices - return; - }; - - let flattened_size = function.dfg.type_of_value(*array).flattened_size(); - let element_size = flattened_size / array_length; - if element_size <= 1 { - // Not a composite type - return; - }; - - // It's a composite type. - // When doing ArrayGet on a composite type, this **always** results in instructions like these - // (assuming element_size == 3): - // - // 1. v27 = array_get v1, index v26 - // 2. v28 = add v26, u32 1 - // 3. v29 = array_get v1, index v28 - // 4. v30 = add v26, u32 2 - // 5. v31 = array_get v1, index v30 - // - // That means that after this instructions, (element_size - 1) instructions will be - // part of this composite array get, and they'll be two instructions apart. - // - // Now three things can happen: - // a) none of the array_get instructions are unused: in this case they won't be in - // `possible_index_out_of_bounds_indexes` and they won't be removed, nothing to do here - // b) all of the array_get instructions are unused: in this case we can replace **all** - // of them with just one constrain: no need to do one per array_get - // c) some of the array_get instructions are unused, but not all: in this case - // we don't need to insert any constrain, because on a later stage array bound checks - // will be performed anyway. We'll let DIE remove the unused ones, without replacing - // them with bounds checks, and leave the used ones. - // - // To check in which scenario we are we can get from `possible_index_out_of_bounds_indexes` - // (starting from `next_out_of_bounds_index`) while we are in the group ranges - // (1..=5 in the example above) - - let Some(out_of_bounds_index) = *next_out_of_bounds_index else { - // No next unused instruction, so this is case a) and nothing needs to be done here - return; - }; - - if index != out_of_bounds_index { - // The next index is not the one for the current instructions, - // so we are in case a), and nothing needs to be done here - return; - } - - // What's the last instruction that's part of the group? (5 in the example above) - let last_instruction_index = index + 2 * (element_size - 1); - // How many unused instructions are in this group? - let mut unused_count = 1; - loop { - *next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); - if let Some(out_of_bounds_index) = *next_out_of_bounds_index { - if out_of_bounds_index <= last_instruction_index { - unused_count += 1; - if unused_count == element_size { - // We are in case b): we need to insert just one constrain. - // Since we popped all of the group indexes, and given that we - // are analyzing the first instruction in the group, we can - // set `next_out_of_bounds_index` to the current index: - // then a check will be inserted, and no other check will be - // inserted for the rest of the group. - *next_out_of_bounds_index = Some(index); - break; - } else { - continue; - } - } - } - - // We are in case c): some of the instructions are unused. - // We don't need to insert any checks, and given that we already popped - // all of the indexes in the group, there's nothing else to do here. - break; - } -} - -// Given `lhs` and `rhs` values, if there's a side effects condition this will -// return (`lhs * condition`, `rhs * condition`), otherwise just (`lhs`, `rhs`) -fn apply_side_effects( - side_effects_condition: Option, - lhs: ValueId, - rhs: ValueId, - function: &mut Function, - block_id: BasicBlockId, - call_stack: Vector, -) -> (ValueId, ValueId) { - // See if there's an active "enable side effects" condition - let Some(condition) = side_effects_condition else { - return (lhs, rhs); - }; - - // Condition needs to be cast to argument type in order to multiply them together. - // In our case, lhs is always a boolean. - let casted_condition = function - .dfg - .insert_instruction_and_results( - Instruction::Cast(condition, Type::bool()), - block_id, - None, - call_stack.clone(), - ) - .first(); - let lhs = function - .dfg - .insert_instruction_and_results( - Instruction::binary(BinaryOp::Mul, lhs, casted_condition), - block_id, - None, - call_stack.clone(), - ) - .first(); - let rhs = function - .dfg - .insert_instruction_and_results( - Instruction::binary(BinaryOp::Mul, rhs, casted_condition), - block_id, - None, - call_stack, - ) - .first(); - (lhs, rhs) } #[cfg(test)] diff --git a/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 4e5fa262696..48673e317bf 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -17,6 +17,7 @@ mod rc; mod remove_bit_shifts; mod remove_enable_side_effects; mod remove_if_else; +mod replace_unused_array_instructions_with_bounds_checks; mod resolve_is_unconstrained; mod runtime_separation; mod simplify_cfg; diff --git a/compiler/noirc_evaluator/src/ssa/opt/replace_unused_array_instructions_with_bounds_checks.rs b/compiler/noirc_evaluator/src/ssa/opt/replace_unused_array_instructions_with_bounds_checks.rs new file mode 100644 index 00000000000..e62b8f5af44 --- /dev/null +++ b/compiler/noirc_evaluator/src/ssa/opt/replace_unused_array_instructions_with_bounds_checks.rs @@ -0,0 +1,374 @@ +//! This optimization initially does what DIE (die.rs) does: compute unused instructions. +//! Then, it will try to replace any unused ArrayGet/ArraySet instructions with out of bounds +//! checks (or remove them if they are unused and don't result in bounds checks). +use std::collections::HashSet; + +use im::Vector; +use noirc_errors::Location; + +use crate::ssa::{ + ir::{ + basic_block::BasicBlockId, + function::Function, + instruction::{BinaryOp, Instruction}, + post_order::PostOrder, + types::Type, + value::ValueId, + }, + ssa_gen::{Ssa, SSA_WORD_SIZE}, + unused::{is_unused, mark_terminator_values_as_used, mark_used_instruction_results}, +}; + +impl Ssa { + #[tracing::instrument(level = "trace", skip(self))] + pub(crate) fn replace_unused_array_instructions_with_bounds_checks(mut self) -> Ssa { + for function in self.functions.values_mut() { + replace_unused_arrays_with_bounds_checks(function); + } + self + } +} + +fn replace_unused_arrays_with_bounds_checks(function: &mut Function) { + let mut context = Context::default(); + for call_data in &function.dfg.data_bus.call_data { + mark_used_instruction_results(&mut context.used_values, &function.dfg, call_data.array_id); + } + + let blocks = PostOrder::with_function(function); + for block in blocks.as_slice() { + context.replace_unused_arrays_with_bounds_checks(function, *block); + } +} + +/// Per function context for tracking unused values +#[derive(Default)] +struct Context { + used_values: HashSet, +} + +impl Context { + fn replace_unused_arrays_with_bounds_checks( + &mut self, + function: &mut Function, + block_id: BasicBlockId, + ) { + let block = &function.dfg[block_id]; + mark_terminator_values_as_used(&mut self.used_values, function, block); + + let instructions_len = block.instructions().len(); + + // Indexes of array instructions that might be out of bounds. + let mut possible_index_out_of_bounds_indexes = Vec::new(); + + for (instruction_index, instruction_id) in block.instructions().iter().rev().enumerate() { + let instruction = &function.dfg[*instruction_id]; + + if is_unused(&self.used_values, *instruction_id, function) { + if instruction_might_result_in_out_of_bounds(function, instruction) { + possible_index_out_of_bounds_indexes + .push(instructions_len - instruction_index - 1); + } + } else { + use Instruction::*; + if !matches!(instruction, IncrementRc { .. } | DecrementRc { .. }) { + instruction.for_each_value(|value| { + mark_used_instruction_results(&mut self.used_values, &function.dfg, value); + }); + } + } + } + + if possible_index_out_of_bounds_indexes.is_empty() { + return; + } + + self.replace_array_instructions_with_out_of_bounds_checks( + function, + block_id, + &mut possible_index_out_of_bounds_indexes, + ); + } + + /// Replaces unused ArrayGet/ArraySet instructions with out of bounds checks. + /// Returns `true` if at least one check was inserted. + /// Because some ArrayGet might happen in groups (for composite types), if just + /// some of the instructions in a group are used but not all of them, no check + /// is inserted, so this method might return `false`. + fn replace_array_instructions_with_out_of_bounds_checks( + &mut self, + function: &mut Function, + block_id: BasicBlockId, + possible_index_out_of_bounds_indexes: &mut Vec, + ) { + // Keep track of the current side effects condition + let mut side_effects_condition = None; + + // Keep track of the next index we need to handle + let mut next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); + + let instructions = function.dfg[block_id].take_instructions(); + for (index, instruction_id) in instructions.iter().enumerate() { + let instruction_id = *instruction_id; + let instruction = &function.dfg[instruction_id]; + + if let Instruction::EnableSideEffects { condition } = instruction { + side_effects_condition = Some(*condition); + + // We still need to keep the EnableSideEffects instruction + function.dfg[block_id].instructions_mut().push(instruction_id); + continue; + }; + + // If it's an ArrayGet we'll deal with groups of it in case the array type is a composite type, + // and adjust `next_out_of_bounds_index` and `possible_index_out_of_bounds_indexes` accordingly + if let Instruction::ArrayGet { array, .. } = instruction { + handle_array_get_group( + function, + array, + index, + &mut next_out_of_bounds_index, + possible_index_out_of_bounds_indexes, + ); + } + + let Some(out_of_bounds_index) = next_out_of_bounds_index else { + // No more out of bounds instructions to insert, just push the current instruction + function.dfg[block_id].instructions_mut().push(instruction_id); + continue; + }; + + if index != out_of_bounds_index { + // This instruction is not out of bounds: let's just push it + function.dfg[block_id].instructions_mut().push(instruction_id); + continue; + } + + // This is an instruction that might be out of bounds: let's add a constrain. + let (array, index) = match instruction { + Instruction::ArrayGet { array, index } + | Instruction::ArraySet { array, index, .. } => (array, index), + _ => panic!("Expected an ArrayGet or ArraySet instruction here"), + }; + + let call_stack = function.dfg.get_call_stack(instruction_id); + + let (lhs, rhs) = if function.dfg.get_numeric_constant(*index).is_some() { + // If we are here it means the index is known but out of bounds. That's always an error! + let false_const = function.dfg.make_constant(false.into(), Type::bool()); + let true_const = function.dfg.make_constant(true.into(), Type::bool()); + (false_const, true_const) + } else { + // `index` will be relative to the flattened array length, so we need to take that into account + let array_length = function.dfg.type_of_value(*array).flattened_size(); + + // If we are here it means the index is dynamic, so let's add a check that it's less than length + let index = function + .dfg + .insert_instruction_and_results( + Instruction::Cast(*index, Type::unsigned(SSA_WORD_SIZE)), + block_id, + None, + call_stack.clone(), + ) + .first(); + + let array_length = function + .dfg + .make_constant((array_length as u128).into(), Type::unsigned(SSA_WORD_SIZE)); + let is_index_out_of_bounds = function + .dfg + .insert_instruction_and_results( + Instruction::binary(BinaryOp::Lt, index, array_length), + block_id, + None, + call_stack.clone(), + ) + .first(); + let true_const = function.dfg.make_constant(true.into(), Type::bool()); + (is_index_out_of_bounds, true_const) + }; + + let (lhs, rhs) = apply_side_effects( + side_effects_condition, + lhs, + rhs, + function, + block_id, + call_stack.clone(), + ); + + let message = Some("Index out of bounds".to_owned().into()); + function.dfg.insert_instruction_and_results( + Instruction::Constrain(lhs, rhs, message), + block_id, + None, + call_stack, + ); + + next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); + } + } +} + +fn instruction_might_result_in_out_of_bounds( + function: &Function, + instruction: &Instruction, +) -> bool { + use Instruction::*; + match instruction { + ArrayGet { array, index } | ArraySet { array, index, .. } => { + if function.dfg.try_get_array_length(*array).is_some() { + if let Some(known_index) = function.dfg.get_numeric_constant(*index) { + // `index` will be relative to the flattened array length, so we need to take that into account + let typ = function.dfg.type_of_value(*array); + let array_length = typ.flattened_size(); + known_index >= array_length.into() + } else { + // A dynamic index might always be out of bounds + true + } + } else { + // Slice operations might be out of bounds, but there's no way we + // can insert a check because we don't know a slice's length + false + } + } + _ => false, + } +} + +fn handle_array_get_group( + function: &Function, + array: &ValueId, + index: usize, + next_out_of_bounds_index: &mut Option, + possible_index_out_of_bounds_indexes: &mut Vec, +) { + let Some(array_length) = function.dfg.try_get_array_length(*array) else { + // Nothing to do for slices + return; + }; + + let flattened_size = function.dfg.type_of_value(*array).flattened_size(); + let element_size = flattened_size / array_length; + if element_size <= 1 { + // Not a composite type + return; + }; + + // It's a composite type. + // When doing ArrayGet on a composite type, this **always** results in instructions like these + // (assuming element_size == 3): + // + // 1. v27 = array_get v1, index v26 + // 2. v28 = add v26, u32 1 + // 3. v29 = array_get v1, index v28 + // 4. v30 = add v26, u32 2 + // 5. v31 = array_get v1, index v30 + // + // That means that after this instructions, (element_size - 1) instructions will be + // part of this composite array get, and they'll be two instructions apart. + // + // Now three things can happen: + // a) none of the array_get instructions are unused: in this case they won't be in + // `possible_index_out_of_bounds_indexes` and they won't be removed, nothing to do here + // b) all of the array_get instructions are unused: in this case we can replace **all** + // of them with just one constrain: no need to do one per array_get + // c) some of the array_get instructions are unused, but not all: in this case + // we don't need to insert any constrain, because on a later stage array bound checks + // will be performed anyway. We'll let DIE remove the unused ones, without replacing + // them with bounds checks, and leave the used ones. + // + // To check in which scenario we are we can get from `possible_index_out_of_bounds_indexes` + // (starting from `next_out_of_bounds_index`) while we are in the group ranges + // (1..=5 in the example above) + + let Some(out_of_bounds_index) = *next_out_of_bounds_index else { + // No next unused instruction, so this is case a) and nothing needs to be done here + return; + }; + + if index != out_of_bounds_index { + // The next index is not the one for the current instructions, + // so we are in case a), and nothing needs to be done here + return; + } + + // What's the last instruction that's part of the group? (5 in the example above) + let last_instruction_index = index + 2 * (element_size - 1); + // How many unused instructions are in this group? + let mut unused_count = 1; + loop { + *next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); + if let Some(out_of_bounds_index) = *next_out_of_bounds_index { + if out_of_bounds_index <= last_instruction_index { + unused_count += 1; + if unused_count == element_size { + // We are in case b): we need to insert just one constrain. + // Since we popped all of the group indexes, and given that we + // are analyzing the first instruction in the group, we can + // set `next_out_of_bounds_index` to the current index: + // then a check will be inserted, and no other check will be + // inserted for the rest of the group. + *next_out_of_bounds_index = Some(index); + break; + } else { + continue; + } + } + } + + // We are in case c): some of the instructions are unused. + // We don't need to insert any checks, and given that we already popped + // all of the indexes in the group, there's nothing else to do here. + break; + } +} + +// Given `lhs` and `rhs` values, if there's a side effects condition this will +// return (`lhs * condition`, `rhs * condition`), otherwise just (`lhs`, `rhs`) +fn apply_side_effects( + side_effects_condition: Option, + lhs: ValueId, + rhs: ValueId, + function: &mut Function, + block_id: BasicBlockId, + call_stack: Vector, +) -> (ValueId, ValueId) { + // See if there's an active "enable side effects" condition + let Some(condition) = side_effects_condition else { + return (lhs, rhs); + }; + + // Condition needs to be cast to argument type in order to multiply them together. + // In our case, lhs is always a boolean. + let casted_condition = function + .dfg + .insert_instruction_and_results( + Instruction::Cast(condition, Type::bool()), + block_id, + None, + call_stack.clone(), + ) + .first(); + let lhs = function + .dfg + .insert_instruction_and_results( + Instruction::binary(BinaryOp::Mul, lhs, casted_condition), + block_id, + None, + call_stack.clone(), + ) + .first(); + let rhs = function + .dfg + .insert_instruction_and_results( + Instruction::binary(BinaryOp::Mul, rhs, casted_condition), + block_id, + None, + call_stack, + ) + .first(); + (lhs, rhs) +} diff --git a/compiler/noirc_evaluator/src/ssa/unused.rs b/compiler/noirc_evaluator/src/ssa/unused.rs new file mode 100644 index 00000000000..8b567907e50 --- /dev/null +++ b/compiler/noirc_evaluator/src/ssa/unused.rs @@ -0,0 +1,70 @@ +use std::collections::HashSet; + +use crate::ssa::ir::{ + basic_block::BasicBlock, + dfg::DataFlowGraph, + function::Function, + instruction::{Instruction, InstructionId, Intrinsic}, + value::{Value, ValueId}, +}; + +/// Returns true if an instruction can be removed. +/// +/// An instruction can be removed as long as it has no side-effects, and none of its result +/// values have been referenced. +pub(crate) fn is_unused( + used_values: &HashSet, + instruction_id: InstructionId, + function: &Function, +) -> bool { + let instruction = &function.dfg[instruction_id]; + + if instruction.can_eliminate_if_unused(&function.dfg) { + let results = function.dfg.instruction_results(instruction_id); + results.iter().all(|result| !used_values.contains(result)) + } else if let Instruction::Call { func, arguments } = instruction { + // TODO: make this more general for instructions which don't have results but have side effects "sometimes" like `Intrinsic::AsWitness` + let as_witness_id = function.dfg.get_intrinsic(Intrinsic::AsWitness); + as_witness_id == Some(func) && !used_values.contains(&arguments[0]) + } else { + // If the instruction has side effects we should never remove it. + false + } +} + +/// Adds values referenced by the terminator to the set of used values. +pub(crate) fn mark_terminator_values_as_used( + used_values: &mut HashSet, + function: &Function, + block: &BasicBlock, +) { + block.unwrap_terminator().for_each_value(|value| { + mark_used_instruction_results(used_values, &function.dfg, value); + }); +} + +/// Inspects a value recursively (as it could be an array) and marks all comprised instruction +/// results as used. +pub(crate) fn mark_used_instruction_results( + used_values: &mut HashSet, + dfg: &DataFlowGraph, + value_id: ValueId, +) { + let value_id = dfg.resolve(value_id); + match &dfg[value_id] { + Value::Instruction { .. } => { + used_values.insert(value_id); + } + Value::Array { array, .. } => { + for elem in array { + mark_used_instruction_results(used_values, dfg, *elem); + } + } + Value::Param { .. } => { + used_values.insert(value_id); + } + _ => { + // Does not comprise of any instruction results + } + } +} From b44203f1bbe4a28a61a2daf687775469adc37bb7 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 9 Aug 2024 17:05:00 -0300 Subject: [PATCH 28/29] Revert "Extract `replace_unused_array_instructions_with_bounds_checks` optimization" This reverts commit ef07ccbe0275451db7c23df121cbac528eaaa4b3. --- compiler/noirc_evaluator/src/ssa.rs | 5 - compiler/noirc_evaluator/src/ssa/opt/die.rs | 423 +++++++++++++++++- compiler/noirc_evaluator/src/ssa/opt/mod.rs | 1 - ...d_array_instructions_with_bounds_checks.rs | 374 ---------------- compiler/noirc_evaluator/src/ssa/unused.rs | 70 --- 5 files changed, 406 insertions(+), 467 deletions(-) delete mode 100644 compiler/noirc_evaluator/src/ssa/opt/replace_unused_array_instructions_with_bounds_checks.rs delete mode 100644 compiler/noirc_evaluator/src/ssa/unused.rs diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 66144a8f974..032e3d07d7b 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -46,7 +46,6 @@ pub(super) mod function_builder; pub mod ir; mod opt; pub mod ssa_gen; -mod unused; pub struct SsaEvaluatorOptions { /// Emit debug information for the intermediate SSA IR @@ -114,10 +113,6 @@ pub(crate) fn optimize_into_acir( .run_pass(Ssa::fold_constants, "After Constant Folding:") .run_pass(Ssa::remove_enable_side_effects, "After EnableSideEffects removal:") .run_pass(Ssa::fold_constants_using_constraints, "After Constraint Folding:") - .run_pass( - Ssa::replace_unused_array_instructions_with_bounds_checks, - "After replacing unused array instructions with bounds checks:", - ) .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") .run_pass(Ssa::array_set_optimization, "After Array Set Optimizations:") .finish(); diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index c52b3897434..f488ef390e4 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -2,17 +2,20 @@ //! which the results are unused. use std::collections::HashSet; +use im::Vector; +use noirc_errors::Location; + use crate::ssa::{ ir::{ - basic_block::BasicBlockId, + basic_block::{BasicBlock, BasicBlockId}, dfg::DataFlowGraph, function::Function, - instruction::{Instruction, InstructionId}, + instruction::{BinaryOp, Instruction, InstructionId, Intrinsic}, post_order::PostOrder, - value::ValueId, + types::Type, + value::{Value, ValueId}, }, - ssa_gen::Ssa, - unused::{is_unused, mark_terminator_values_as_used, mark_used_instruction_results}, + ssa_gen::{Ssa, SSA_WORD_SIZE}, }; impl Ssa { @@ -21,7 +24,7 @@ impl Ssa { #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn dead_instruction_elimination(mut self) -> Ssa { for function in self.functions.values_mut() { - dead_instruction_elimination(function); + dead_instruction_elimination(function, true); } self } @@ -33,16 +36,29 @@ impl Ssa { /// instructions that reference results from an instruction in another block are evaluated first. /// If we did not iterate blocks in this order we could not safely say whether or not the results /// of its instructions are needed elsewhere. -fn dead_instruction_elimination(function: &mut Function) { +fn dead_instruction_elimination(function: &mut Function, insert_out_of_bounds_checks: bool) { let mut context = Context::default(); for call_data in &function.dfg.data_bus.call_data { - mark_used_instruction_results(&mut context.used_values, &function.dfg, call_data.array_id); + context.mark_used_instruction_results(&function.dfg, call_data.array_id); } - let blocks = PostOrder::with_function(function); + let mut inserted_out_of_bounds_checks = false; + let blocks = PostOrder::with_function(function); for block in blocks.as_slice() { - context.remove_unused_instructions_in_block(function, *block); + inserted_out_of_bounds_checks |= context.remove_unused_instructions_in_block( + function, + *block, + insert_out_of_bounds_checks, + ); + } + + // If we inserted out of bounds check, let's run the pass again with those new + // instructions (we don't want to remove those checks, or instructions that are + // dependencies of those checks) + if inserted_out_of_bounds_checks { + dead_instruction_elimination(function, false); + return; } context.remove_rc_instructions(&mut function.dfg); @@ -72,34 +88,120 @@ impl Context { /// values set. This allows DIE to identify whole chains of unused instructions. (If the /// values referenced by an unused instruction were considered to be used, only the head of /// such chains would be removed.) + /// + /// If `insert_out_of_bounds_checks` is true and there are unused ArrayGet/ArraySet that + /// might be out of bounds, this method will insert out of bounds checks instead of + /// removing unused instructions and return `true`. The idea then is to later call this + /// function again with `insert_out_of_bounds_checks` set to false to effectively remove + /// unused instructions but leave the out of bounds checks. fn remove_unused_instructions_in_block( &mut self, function: &mut Function, block_id: BasicBlockId, - ) { + insert_out_of_bounds_checks: bool, + ) -> bool { let block = &function.dfg[block_id]; - mark_terminator_values_as_used(&mut self.used_values, function, block); + self.mark_terminator_values_as_used(function, block); - for instruction_id in block.instructions().iter().rev() { - if is_unused(&self.used_values, *instruction_id, function) { + let instructions_len = block.instructions().len(); + + // Indexes of instructions that might be out of bounds. + // We'll remove those, but before that we'll insert bounds checks for them. + let mut possible_index_out_of_bounds_indexes = Vec::new(); + + for (instruction_index, instruction_id) in block.instructions().iter().rev().enumerate() { + let instruction = &function.dfg[*instruction_id]; + + if self.is_unused(*instruction_id, function) { self.instructions_to_remove.insert(*instruction_id); - } else { - let instruction = &function.dfg[*instruction_id]; + if insert_out_of_bounds_checks + && instruction_might_result_in_out_of_bounds(function, instruction) + { + possible_index_out_of_bounds_indexes + .push(instructions_len - instruction_index - 1); + } + } else { use Instruction::*; if matches!(instruction, IncrementRc { .. } | DecrementRc { .. }) { self.rc_instructions.push((*instruction_id, block_id)); } else { instruction.for_each_value(|value| { - mark_used_instruction_results(&mut self.used_values, &function.dfg, value); + self.mark_used_instruction_results(&function.dfg, value); }); } } } + // If there are some instructions that might trigger an out of bounds error, + // first add constrain checks. Then run the DIE pass again, which will remove those + // but leave the constrains (any any value needed by those constrains) + if !possible_index_out_of_bounds_indexes.is_empty() { + let inserted_check = self.replace_array_instructions_with_out_of_bounds_checks( + function, + block_id, + &mut possible_index_out_of_bounds_indexes, + ); + // There's a slight chance we didn't insert any checks, so we could proceed with DIE. + if inserted_check { + return true; + } + } + function.dfg[block_id] .instructions_mut() .retain(|instruction| !self.instructions_to_remove.contains(instruction)); + + false + } + + /// Returns true if an instruction can be removed. + /// + /// An instruction can be removed as long as it has no side-effects, and none of its result + /// values have been referenced. + fn is_unused(&self, instruction_id: InstructionId, function: &Function) -> bool { + let instruction = &function.dfg[instruction_id]; + + if instruction.can_eliminate_if_unused(&function.dfg) { + let results = function.dfg.instruction_results(instruction_id); + results.iter().all(|result| !self.used_values.contains(result)) + } else if let Instruction::Call { func, arguments } = instruction { + // TODO: make this more general for instructions which don't have results but have side effects "sometimes" like `Intrinsic::AsWitness` + let as_witness_id = function.dfg.get_intrinsic(Intrinsic::AsWitness); + as_witness_id == Some(func) && !self.used_values.contains(&arguments[0]) + } else { + // If the instruction has side effects we should never remove it. + false + } + } + + /// Adds values referenced by the terminator to the set of used values. + fn mark_terminator_values_as_used(&mut self, function: &Function, block: &BasicBlock) { + block.unwrap_terminator().for_each_value(|value| { + self.mark_used_instruction_results(&function.dfg, value); + }); + } + + /// Inspects a value recursively (as it could be an array) and marks all comprised instruction + /// results as used. + fn mark_used_instruction_results(&mut self, dfg: &DataFlowGraph, value_id: ValueId) { + let value_id = dfg.resolve(value_id); + match &dfg[value_id] { + Value::Instruction { .. } => { + self.used_values.insert(value_id); + } + Value::Array { array, .. } => { + for elem in array { + self.mark_used_instruction_results(dfg, *elem); + } + } + Value::Param { .. } => { + self.used_values.insert(value_id); + } + _ => { + // Does not comprise of any instruction results + } + } } fn remove_rc_instructions(self, dfg: &mut DataFlowGraph) { @@ -118,6 +220,293 @@ impl Context { } } } + + /// Replaces unused ArrayGet/ArraySet instructions with out of bounds checks. + /// Returns `true` if at least one check was inserted. + /// Because some ArrayGet might happen in groups (for composite types), if just + /// some of the instructions in a group are used but not all of them, no check + /// is inserted, so this method might return `false`. + fn replace_array_instructions_with_out_of_bounds_checks( + &mut self, + function: &mut Function, + block_id: BasicBlockId, + possible_index_out_of_bounds_indexes: &mut Vec, + ) -> bool { + let mut inserted_check = false; + + // Keep track of the current side effects condition + let mut side_effects_condition = None; + + // Keep track of the next index we need to handle + let mut next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); + + let instructions = function.dfg[block_id].take_instructions(); + for (index, instruction_id) in instructions.iter().enumerate() { + let instruction_id = *instruction_id; + let instruction = &function.dfg[instruction_id]; + + if let Instruction::EnableSideEffects { condition } = instruction { + side_effects_condition = Some(*condition); + + // We still need to keep the EnableSideEffects instruction + function.dfg[block_id].instructions_mut().push(instruction_id); + continue; + }; + + // If it's an ArrayGet we'll deal with groups of it in case the array type is a composite type, + // and adjust `next_out_of_bounds_index` and `possible_index_out_of_bounds_indexes` accordingly + if let Instruction::ArrayGet { array, .. } = instruction { + handle_array_get_group( + function, + array, + index, + &mut next_out_of_bounds_index, + possible_index_out_of_bounds_indexes, + ); + } + + let Some(out_of_bounds_index) = next_out_of_bounds_index else { + // No more out of bounds instructions to insert, just push the current instruction + function.dfg[block_id].instructions_mut().push(instruction_id); + continue; + }; + + if index != out_of_bounds_index { + // This instruction is not out of bounds: let's just push it + function.dfg[block_id].instructions_mut().push(instruction_id); + continue; + } + + // This is an instruction that might be out of bounds: let's add a constrain. + let (array, index) = match instruction { + Instruction::ArrayGet { array, index } + | Instruction::ArraySet { array, index, .. } => (array, index), + _ => panic!("Expected an ArrayGet or ArraySet instruction here"), + }; + + let call_stack = function.dfg.get_call_stack(instruction_id); + + let (lhs, rhs) = if function.dfg.get_numeric_constant(*index).is_some() { + // If we are here it means the index is known but out of bounds. That's always an error! + let false_const = function.dfg.make_constant(false.into(), Type::bool()); + let true_const = function.dfg.make_constant(true.into(), Type::bool()); + (false_const, true_const) + } else { + // `index` will be relative to the flattened array length, so we need to take that into account + let array_length = function.dfg.type_of_value(*array).flattened_size(); + + // If we are here it means the index is dynamic, so let's add a check that it's less than length + let index = function + .dfg + .insert_instruction_and_results( + Instruction::Cast(*index, Type::unsigned(SSA_WORD_SIZE)), + block_id, + None, + call_stack.clone(), + ) + .first(); + + let array_length = function + .dfg + .make_constant((array_length as u128).into(), Type::unsigned(SSA_WORD_SIZE)); + let is_index_out_of_bounds = function + .dfg + .insert_instruction_and_results( + Instruction::binary(BinaryOp::Lt, index, array_length), + block_id, + None, + call_stack.clone(), + ) + .first(); + let true_const = function.dfg.make_constant(true.into(), Type::bool()); + (is_index_out_of_bounds, true_const) + }; + + let (lhs, rhs) = apply_side_effects( + side_effects_condition, + lhs, + rhs, + function, + block_id, + call_stack.clone(), + ); + + let message = Some("Index out of bounds".to_owned().into()); + function.dfg.insert_instruction_and_results( + Instruction::Constrain(lhs, rhs, message), + block_id, + None, + call_stack, + ); + inserted_check = true; + + next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); + } + + inserted_check + } +} + +fn instruction_might_result_in_out_of_bounds( + function: &Function, + instruction: &Instruction, +) -> bool { + use Instruction::*; + match instruction { + ArrayGet { array, index } | ArraySet { array, index, .. } => { + if function.dfg.try_get_array_length(*array).is_some() { + if let Some(known_index) = function.dfg.get_numeric_constant(*index) { + // `index` will be relative to the flattened array length, so we need to take that into account + let typ = function.dfg.type_of_value(*array); + let array_length = typ.flattened_size(); + known_index >= array_length.into() + } else { + // A dynamic index might always be out of bounds + true + } + } else { + // Slice operations might be out of bounds, but there's no way we + // can insert a check because we don't know a slice's length + false + } + } + _ => false, + } +} + +fn handle_array_get_group( + function: &Function, + array: &ValueId, + index: usize, + next_out_of_bounds_index: &mut Option, + possible_index_out_of_bounds_indexes: &mut Vec, +) { + let Some(array_length) = function.dfg.try_get_array_length(*array) else { + // Nothing to do for slices + return; + }; + + let flattened_size = function.dfg.type_of_value(*array).flattened_size(); + let element_size = flattened_size / array_length; + if element_size <= 1 { + // Not a composite type + return; + }; + + // It's a composite type. + // When doing ArrayGet on a composite type, this **always** results in instructions like these + // (assuming element_size == 3): + // + // 1. v27 = array_get v1, index v26 + // 2. v28 = add v26, u32 1 + // 3. v29 = array_get v1, index v28 + // 4. v30 = add v26, u32 2 + // 5. v31 = array_get v1, index v30 + // + // That means that after this instructions, (element_size - 1) instructions will be + // part of this composite array get, and they'll be two instructions apart. + // + // Now three things can happen: + // a) none of the array_get instructions are unused: in this case they won't be in + // `possible_index_out_of_bounds_indexes` and they won't be removed, nothing to do here + // b) all of the array_get instructions are unused: in this case we can replace **all** + // of them with just one constrain: no need to do one per array_get + // c) some of the array_get instructions are unused, but not all: in this case + // we don't need to insert any constrain, because on a later stage array bound checks + // will be performed anyway. We'll let DIE remove the unused ones, without replacing + // them with bounds checks, and leave the used ones. + // + // To check in which scenario we are we can get from `possible_index_out_of_bounds_indexes` + // (starting from `next_out_of_bounds_index`) while we are in the group ranges + // (1..=5 in the example above) + + let Some(out_of_bounds_index) = *next_out_of_bounds_index else { + // No next unused instruction, so this is case a) and nothing needs to be done here + return; + }; + + if index != out_of_bounds_index { + // The next index is not the one for the current instructions, + // so we are in case a), and nothing needs to be done here + return; + } + + // What's the last instruction that's part of the group? (5 in the example above) + let last_instruction_index = index + 2 * (element_size - 1); + // How many unused instructions are in this group? + let mut unused_count = 1; + loop { + *next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); + if let Some(out_of_bounds_index) = *next_out_of_bounds_index { + if out_of_bounds_index <= last_instruction_index { + unused_count += 1; + if unused_count == element_size { + // We are in case b): we need to insert just one constrain. + // Since we popped all of the group indexes, and given that we + // are analyzing the first instruction in the group, we can + // set `next_out_of_bounds_index` to the current index: + // then a check will be inserted, and no other check will be + // inserted for the rest of the group. + *next_out_of_bounds_index = Some(index); + break; + } else { + continue; + } + } + } + + // We are in case c): some of the instructions are unused. + // We don't need to insert any checks, and given that we already popped + // all of the indexes in the group, there's nothing else to do here. + break; + } +} + +// Given `lhs` and `rhs` values, if there's a side effects condition this will +// return (`lhs * condition`, `rhs * condition`), otherwise just (`lhs`, `rhs`) +fn apply_side_effects( + side_effects_condition: Option, + lhs: ValueId, + rhs: ValueId, + function: &mut Function, + block_id: BasicBlockId, + call_stack: Vector, +) -> (ValueId, ValueId) { + // See if there's an active "enable side effects" condition + let Some(condition) = side_effects_condition else { + return (lhs, rhs); + }; + + // Condition needs to be cast to argument type in order to multiply them together. + // In our case, lhs is always a boolean. + let casted_condition = function + .dfg + .insert_instruction_and_results( + Instruction::Cast(condition, Type::bool()), + block_id, + None, + call_stack.clone(), + ) + .first(); + let lhs = function + .dfg + .insert_instruction_and_results( + Instruction::binary(BinaryOp::Mul, lhs, casted_condition), + block_id, + None, + call_stack.clone(), + ) + .first(); + let rhs = function + .dfg + .insert_instruction_and_results( + Instruction::binary(BinaryOp::Mul, rhs, casted_condition), + block_id, + None, + call_stack, + ) + .first(); + (lhs, rhs) } #[cfg(test)] diff --git a/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 48673e317bf..4e5fa262696 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -17,7 +17,6 @@ mod rc; mod remove_bit_shifts; mod remove_enable_side_effects; mod remove_if_else; -mod replace_unused_array_instructions_with_bounds_checks; mod resolve_is_unconstrained; mod runtime_separation; mod simplify_cfg; diff --git a/compiler/noirc_evaluator/src/ssa/opt/replace_unused_array_instructions_with_bounds_checks.rs b/compiler/noirc_evaluator/src/ssa/opt/replace_unused_array_instructions_with_bounds_checks.rs deleted file mode 100644 index e62b8f5af44..00000000000 --- a/compiler/noirc_evaluator/src/ssa/opt/replace_unused_array_instructions_with_bounds_checks.rs +++ /dev/null @@ -1,374 +0,0 @@ -//! This optimization initially does what DIE (die.rs) does: compute unused instructions. -//! Then, it will try to replace any unused ArrayGet/ArraySet instructions with out of bounds -//! checks (or remove them if they are unused and don't result in bounds checks). -use std::collections::HashSet; - -use im::Vector; -use noirc_errors::Location; - -use crate::ssa::{ - ir::{ - basic_block::BasicBlockId, - function::Function, - instruction::{BinaryOp, Instruction}, - post_order::PostOrder, - types::Type, - value::ValueId, - }, - ssa_gen::{Ssa, SSA_WORD_SIZE}, - unused::{is_unused, mark_terminator_values_as_used, mark_used_instruction_results}, -}; - -impl Ssa { - #[tracing::instrument(level = "trace", skip(self))] - pub(crate) fn replace_unused_array_instructions_with_bounds_checks(mut self) -> Ssa { - for function in self.functions.values_mut() { - replace_unused_arrays_with_bounds_checks(function); - } - self - } -} - -fn replace_unused_arrays_with_bounds_checks(function: &mut Function) { - let mut context = Context::default(); - for call_data in &function.dfg.data_bus.call_data { - mark_used_instruction_results(&mut context.used_values, &function.dfg, call_data.array_id); - } - - let blocks = PostOrder::with_function(function); - for block in blocks.as_slice() { - context.replace_unused_arrays_with_bounds_checks(function, *block); - } -} - -/// Per function context for tracking unused values -#[derive(Default)] -struct Context { - used_values: HashSet, -} - -impl Context { - fn replace_unused_arrays_with_bounds_checks( - &mut self, - function: &mut Function, - block_id: BasicBlockId, - ) { - let block = &function.dfg[block_id]; - mark_terminator_values_as_used(&mut self.used_values, function, block); - - let instructions_len = block.instructions().len(); - - // Indexes of array instructions that might be out of bounds. - let mut possible_index_out_of_bounds_indexes = Vec::new(); - - for (instruction_index, instruction_id) in block.instructions().iter().rev().enumerate() { - let instruction = &function.dfg[*instruction_id]; - - if is_unused(&self.used_values, *instruction_id, function) { - if instruction_might_result_in_out_of_bounds(function, instruction) { - possible_index_out_of_bounds_indexes - .push(instructions_len - instruction_index - 1); - } - } else { - use Instruction::*; - if !matches!(instruction, IncrementRc { .. } | DecrementRc { .. }) { - instruction.for_each_value(|value| { - mark_used_instruction_results(&mut self.used_values, &function.dfg, value); - }); - } - } - } - - if possible_index_out_of_bounds_indexes.is_empty() { - return; - } - - self.replace_array_instructions_with_out_of_bounds_checks( - function, - block_id, - &mut possible_index_out_of_bounds_indexes, - ); - } - - /// Replaces unused ArrayGet/ArraySet instructions with out of bounds checks. - /// Returns `true` if at least one check was inserted. - /// Because some ArrayGet might happen in groups (for composite types), if just - /// some of the instructions in a group are used but not all of them, no check - /// is inserted, so this method might return `false`. - fn replace_array_instructions_with_out_of_bounds_checks( - &mut self, - function: &mut Function, - block_id: BasicBlockId, - possible_index_out_of_bounds_indexes: &mut Vec, - ) { - // Keep track of the current side effects condition - let mut side_effects_condition = None; - - // Keep track of the next index we need to handle - let mut next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); - - let instructions = function.dfg[block_id].take_instructions(); - for (index, instruction_id) in instructions.iter().enumerate() { - let instruction_id = *instruction_id; - let instruction = &function.dfg[instruction_id]; - - if let Instruction::EnableSideEffects { condition } = instruction { - side_effects_condition = Some(*condition); - - // We still need to keep the EnableSideEffects instruction - function.dfg[block_id].instructions_mut().push(instruction_id); - continue; - }; - - // If it's an ArrayGet we'll deal with groups of it in case the array type is a composite type, - // and adjust `next_out_of_bounds_index` and `possible_index_out_of_bounds_indexes` accordingly - if let Instruction::ArrayGet { array, .. } = instruction { - handle_array_get_group( - function, - array, - index, - &mut next_out_of_bounds_index, - possible_index_out_of_bounds_indexes, - ); - } - - let Some(out_of_bounds_index) = next_out_of_bounds_index else { - // No more out of bounds instructions to insert, just push the current instruction - function.dfg[block_id].instructions_mut().push(instruction_id); - continue; - }; - - if index != out_of_bounds_index { - // This instruction is not out of bounds: let's just push it - function.dfg[block_id].instructions_mut().push(instruction_id); - continue; - } - - // This is an instruction that might be out of bounds: let's add a constrain. - let (array, index) = match instruction { - Instruction::ArrayGet { array, index } - | Instruction::ArraySet { array, index, .. } => (array, index), - _ => panic!("Expected an ArrayGet or ArraySet instruction here"), - }; - - let call_stack = function.dfg.get_call_stack(instruction_id); - - let (lhs, rhs) = if function.dfg.get_numeric_constant(*index).is_some() { - // If we are here it means the index is known but out of bounds. That's always an error! - let false_const = function.dfg.make_constant(false.into(), Type::bool()); - let true_const = function.dfg.make_constant(true.into(), Type::bool()); - (false_const, true_const) - } else { - // `index` will be relative to the flattened array length, so we need to take that into account - let array_length = function.dfg.type_of_value(*array).flattened_size(); - - // If we are here it means the index is dynamic, so let's add a check that it's less than length - let index = function - .dfg - .insert_instruction_and_results( - Instruction::Cast(*index, Type::unsigned(SSA_WORD_SIZE)), - block_id, - None, - call_stack.clone(), - ) - .first(); - - let array_length = function - .dfg - .make_constant((array_length as u128).into(), Type::unsigned(SSA_WORD_SIZE)); - let is_index_out_of_bounds = function - .dfg - .insert_instruction_and_results( - Instruction::binary(BinaryOp::Lt, index, array_length), - block_id, - None, - call_stack.clone(), - ) - .first(); - let true_const = function.dfg.make_constant(true.into(), Type::bool()); - (is_index_out_of_bounds, true_const) - }; - - let (lhs, rhs) = apply_side_effects( - side_effects_condition, - lhs, - rhs, - function, - block_id, - call_stack.clone(), - ); - - let message = Some("Index out of bounds".to_owned().into()); - function.dfg.insert_instruction_and_results( - Instruction::Constrain(lhs, rhs, message), - block_id, - None, - call_stack, - ); - - next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); - } - } -} - -fn instruction_might_result_in_out_of_bounds( - function: &Function, - instruction: &Instruction, -) -> bool { - use Instruction::*; - match instruction { - ArrayGet { array, index } | ArraySet { array, index, .. } => { - if function.dfg.try_get_array_length(*array).is_some() { - if let Some(known_index) = function.dfg.get_numeric_constant(*index) { - // `index` will be relative to the flattened array length, so we need to take that into account - let typ = function.dfg.type_of_value(*array); - let array_length = typ.flattened_size(); - known_index >= array_length.into() - } else { - // A dynamic index might always be out of bounds - true - } - } else { - // Slice operations might be out of bounds, but there's no way we - // can insert a check because we don't know a slice's length - false - } - } - _ => false, - } -} - -fn handle_array_get_group( - function: &Function, - array: &ValueId, - index: usize, - next_out_of_bounds_index: &mut Option, - possible_index_out_of_bounds_indexes: &mut Vec, -) { - let Some(array_length) = function.dfg.try_get_array_length(*array) else { - // Nothing to do for slices - return; - }; - - let flattened_size = function.dfg.type_of_value(*array).flattened_size(); - let element_size = flattened_size / array_length; - if element_size <= 1 { - // Not a composite type - return; - }; - - // It's a composite type. - // When doing ArrayGet on a composite type, this **always** results in instructions like these - // (assuming element_size == 3): - // - // 1. v27 = array_get v1, index v26 - // 2. v28 = add v26, u32 1 - // 3. v29 = array_get v1, index v28 - // 4. v30 = add v26, u32 2 - // 5. v31 = array_get v1, index v30 - // - // That means that after this instructions, (element_size - 1) instructions will be - // part of this composite array get, and they'll be two instructions apart. - // - // Now three things can happen: - // a) none of the array_get instructions are unused: in this case they won't be in - // `possible_index_out_of_bounds_indexes` and they won't be removed, nothing to do here - // b) all of the array_get instructions are unused: in this case we can replace **all** - // of them with just one constrain: no need to do one per array_get - // c) some of the array_get instructions are unused, but not all: in this case - // we don't need to insert any constrain, because on a later stage array bound checks - // will be performed anyway. We'll let DIE remove the unused ones, without replacing - // them with bounds checks, and leave the used ones. - // - // To check in which scenario we are we can get from `possible_index_out_of_bounds_indexes` - // (starting from `next_out_of_bounds_index`) while we are in the group ranges - // (1..=5 in the example above) - - let Some(out_of_bounds_index) = *next_out_of_bounds_index else { - // No next unused instruction, so this is case a) and nothing needs to be done here - return; - }; - - if index != out_of_bounds_index { - // The next index is not the one for the current instructions, - // so we are in case a), and nothing needs to be done here - return; - } - - // What's the last instruction that's part of the group? (5 in the example above) - let last_instruction_index = index + 2 * (element_size - 1); - // How many unused instructions are in this group? - let mut unused_count = 1; - loop { - *next_out_of_bounds_index = possible_index_out_of_bounds_indexes.pop(); - if let Some(out_of_bounds_index) = *next_out_of_bounds_index { - if out_of_bounds_index <= last_instruction_index { - unused_count += 1; - if unused_count == element_size { - // We are in case b): we need to insert just one constrain. - // Since we popped all of the group indexes, and given that we - // are analyzing the first instruction in the group, we can - // set `next_out_of_bounds_index` to the current index: - // then a check will be inserted, and no other check will be - // inserted for the rest of the group. - *next_out_of_bounds_index = Some(index); - break; - } else { - continue; - } - } - } - - // We are in case c): some of the instructions are unused. - // We don't need to insert any checks, and given that we already popped - // all of the indexes in the group, there's nothing else to do here. - break; - } -} - -// Given `lhs` and `rhs` values, if there's a side effects condition this will -// return (`lhs * condition`, `rhs * condition`), otherwise just (`lhs`, `rhs`) -fn apply_side_effects( - side_effects_condition: Option, - lhs: ValueId, - rhs: ValueId, - function: &mut Function, - block_id: BasicBlockId, - call_stack: Vector, -) -> (ValueId, ValueId) { - // See if there's an active "enable side effects" condition - let Some(condition) = side_effects_condition else { - return (lhs, rhs); - }; - - // Condition needs to be cast to argument type in order to multiply them together. - // In our case, lhs is always a boolean. - let casted_condition = function - .dfg - .insert_instruction_and_results( - Instruction::Cast(condition, Type::bool()), - block_id, - None, - call_stack.clone(), - ) - .first(); - let lhs = function - .dfg - .insert_instruction_and_results( - Instruction::binary(BinaryOp::Mul, lhs, casted_condition), - block_id, - None, - call_stack.clone(), - ) - .first(); - let rhs = function - .dfg - .insert_instruction_and_results( - Instruction::binary(BinaryOp::Mul, rhs, casted_condition), - block_id, - None, - call_stack, - ) - .first(); - (lhs, rhs) -} diff --git a/compiler/noirc_evaluator/src/ssa/unused.rs b/compiler/noirc_evaluator/src/ssa/unused.rs deleted file mode 100644 index 8b567907e50..00000000000 --- a/compiler/noirc_evaluator/src/ssa/unused.rs +++ /dev/null @@ -1,70 +0,0 @@ -use std::collections::HashSet; - -use crate::ssa::ir::{ - basic_block::BasicBlock, - dfg::DataFlowGraph, - function::Function, - instruction::{Instruction, InstructionId, Intrinsic}, - value::{Value, ValueId}, -}; - -/// Returns true if an instruction can be removed. -/// -/// An instruction can be removed as long as it has no side-effects, and none of its result -/// values have been referenced. -pub(crate) fn is_unused( - used_values: &HashSet, - instruction_id: InstructionId, - function: &Function, -) -> bool { - let instruction = &function.dfg[instruction_id]; - - if instruction.can_eliminate_if_unused(&function.dfg) { - let results = function.dfg.instruction_results(instruction_id); - results.iter().all(|result| !used_values.contains(result)) - } else if let Instruction::Call { func, arguments } = instruction { - // TODO: make this more general for instructions which don't have results but have side effects "sometimes" like `Intrinsic::AsWitness` - let as_witness_id = function.dfg.get_intrinsic(Intrinsic::AsWitness); - as_witness_id == Some(func) && !used_values.contains(&arguments[0]) - } else { - // If the instruction has side effects we should never remove it. - false - } -} - -/// Adds values referenced by the terminator to the set of used values. -pub(crate) fn mark_terminator_values_as_used( - used_values: &mut HashSet, - function: &Function, - block: &BasicBlock, -) { - block.unwrap_terminator().for_each_value(|value| { - mark_used_instruction_results(used_values, &function.dfg, value); - }); -} - -/// Inspects a value recursively (as it could be an array) and marks all comprised instruction -/// results as used. -pub(crate) fn mark_used_instruction_results( - used_values: &mut HashSet, - dfg: &DataFlowGraph, - value_id: ValueId, -) { - let value_id = dfg.resolve(value_id); - match &dfg[value_id] { - Value::Instruction { .. } => { - used_values.insert(value_id); - } - Value::Array { array, .. } => { - for elem in array { - mark_used_instruction_results(used_values, dfg, *elem); - } - } - Value::Param { .. } => { - used_values.insert(value_id); - } - _ => { - // Does not comprise of any instruction results - } - } -} From be0ef17427f10b6342b5f107521010ba8f830677 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 13 Aug 2024 12:58:19 -0300 Subject: [PATCH 29/29] Improve formatting --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 93 ++++++++++----------- 1 file changed, 44 insertions(+), 49 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index f488ef390e4..1aa0c2efbd0 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -296,28 +296,24 @@ impl Context { let array_length = function.dfg.type_of_value(*array).flattened_size(); // If we are here it means the index is dynamic, so let's add a check that it's less than length - let index = function - .dfg - .insert_instruction_and_results( - Instruction::Cast(*index, Type::unsigned(SSA_WORD_SIZE)), - block_id, - None, - call_stack.clone(), - ) - .first(); - - let array_length = function - .dfg - .make_constant((array_length as u128).into(), Type::unsigned(SSA_WORD_SIZE)); - let is_index_out_of_bounds = function - .dfg - .insert_instruction_and_results( - Instruction::binary(BinaryOp::Lt, index, array_length), - block_id, - None, - call_stack.clone(), - ) - .first(); + let index = function.dfg.insert_instruction_and_results( + Instruction::Cast(*index, Type::unsigned(SSA_WORD_SIZE)), + block_id, + None, + call_stack.clone(), + ); + let index = index.first(); + + let array_typ = Type::unsigned(SSA_WORD_SIZE); + let array_length = + function.dfg.make_constant((array_length as u128).into(), array_typ); + let is_index_out_of_bounds = function.dfg.insert_instruction_and_results( + Instruction::binary(BinaryOp::Lt, index, array_length), + block_id, + None, + call_stack.clone(), + ); + let is_index_out_of_bounds = is_index_out_of_bounds.first(); let true_const = function.dfg.make_constant(true.into(), Type::bool()); (is_index_out_of_bounds, true_const) }; @@ -477,35 +473,34 @@ fn apply_side_effects( return (lhs, rhs); }; + let dfg = &mut function.dfg; + // Condition needs to be cast to argument type in order to multiply them together. // In our case, lhs is always a boolean. - let casted_condition = function - .dfg - .insert_instruction_and_results( - Instruction::Cast(condition, Type::bool()), - block_id, - None, - call_stack.clone(), - ) - .first(); - let lhs = function - .dfg - .insert_instruction_and_results( - Instruction::binary(BinaryOp::Mul, lhs, casted_condition), - block_id, - None, - call_stack.clone(), - ) - .first(); - let rhs = function - .dfg - .insert_instruction_and_results( - Instruction::binary(BinaryOp::Mul, rhs, casted_condition), - block_id, - None, - call_stack, - ) - .first(); + let casted_condition = dfg.insert_instruction_and_results( + Instruction::Cast(condition, Type::bool()), + block_id, + None, + call_stack.clone(), + ); + let casted_condition = casted_condition.first(); + + let lhs = dfg.insert_instruction_and_results( + Instruction::binary(BinaryOp::Mul, lhs, casted_condition), + block_id, + None, + call_stack.clone(), + ); + let lhs = lhs.first(); + + let rhs = dfg.insert_instruction_and_results( + Instruction::binary(BinaryOp::Mul, rhs, casted_condition), + block_id, + None, + call_stack, + ); + let rhs = rhs.first(); + (lhs, rhs) }