From 2a23588317f23b8ef051698e8ba900766bdf9476 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Fri, 5 Apr 2024 11:29:18 +0000 Subject: [PATCH 1/4] fix: unknown slice lengths --- compiler/noirc_evaluator/src/ssa.rs | 1 + compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 24 +++++++ .../src/ssa/opt/as_slice_length.rs | 62 +++++++++++++++++++ compiler/noirc_evaluator/src/ssa/opt/mod.rs | 1 + 4 files changed, 88 insertions(+) create mode 100644 compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index a45cf3af608..8e2ad5b47d6 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -50,6 +50,7 @@ pub(crate) fn optimize_into_acir( let ssa = SsaBuilder::new(program, print_passes, force_brillig_output, print_timings)? .run_pass(Ssa::defunctionalize, "After Defunctionalization:") .run_pass(Ssa::remove_paired_rc, "After Removing Paired rc_inc & rc_decs:") + .run_pass(Ssa::as_slice_optimization, "After As slice optimization") .run_pass(Ssa::inline_functions, "After Inlining:") // Run mem2reg with the CFG separated into blocks .run_pass(Ssa::mem2reg, "After Mem2Reg:") diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 870b5e602f1..6b950c327cf 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -378,6 +378,30 @@ impl DataFlowGraph { value_id } + /// Replaces an instruction result with a fresh id. + pub(crate) fn replace_result( + &mut self, + instruction_id: InstructionId, + prev_value_id: ValueId, + ) -> ValueId { + let typ = self.type_of_value(prev_value_id); + let results = self.results.get_mut(&instruction_id).unwrap(); + let res_position = results + .iter() + .position(|&id| id == prev_value_id) + .expect("Result id not found while replacing"); + + let value_id = self.values.insert(Value::Instruction { + typ, + position: res_position, + instruction: instruction_id, + }); + + // Replace the value in list of results for this instruction + results[res_position] = value_id; + value_id + } + /// Returns the number of instructions /// inserted into functions. pub(crate) fn num_instructions(&self) -> usize { diff --git a/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs new file mode 100644 index 00000000000..14ec1f8be4f --- /dev/null +++ b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs @@ -0,0 +1,62 @@ +use crate::ssa::{ + ir::{ + function::Function, + instruction::{Instruction, InstructionId, Intrinsic}, + types::Type, + value::Value, + }, + ssa_gen::Ssa, +}; +use fxhash::FxHashMap as HashMap; + +impl Ssa { + #[tracing::instrument(level = "trace", skip(self))] + pub(crate) fn as_slice_optimization(mut self) -> Self { + for func in self.functions.values_mut() { + let known_slice_lengths = known_slice_lengths(func); + replace_known_slice_lengths(func, known_slice_lengths); + } + self + } +} + +fn known_slice_lengths(func: &Function) -> HashMap { + let mut known_slice_lengths = HashMap::default(); + for block_id in func.reachable_blocks() { + let block = &func.dfg[block_id]; + for instruction_id in block.instructions() { + let (target_func, arguments) = match &func.dfg[*instruction_id] { + Instruction::Call { func, arguments } => (func, arguments), + _ => continue, + }; + + match &func.dfg[*target_func] { + Value::Intrinsic(Intrinsic::AsSlice) => { + let array_typ = func.dfg.type_of_value(arguments[0]); + if let Type::Array(_, length) = array_typ { + known_slice_lengths.insert(*instruction_id, length); + } else { + unreachable!("AsSlice called with non-array {}", array_typ); + } + } + _ => continue, + }; + } + } + known_slice_lengths +} + +fn replace_known_slice_lengths( + func: &mut Function, + known_slice_lengths: HashMap, +) { + known_slice_lengths.into_iter().for_each(|(instruction_id, known_length)| { + let call_returns = func.dfg.instruction_results(instruction_id); + let original_slice_length = call_returns[0]; + + // We won't use the new id for the original unknown length. + func.dfg.replace_result(instruction_id, original_slice_length); + let known_length = func.dfg.make_constant(known_length.into(), Type::length_type()); + func.dfg.set_value_from_id(original_slice_length, known_length); + }); +} diff --git a/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 479010b1ed8..f37f45b30bb 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -4,6 +4,7 @@ //! simpler form until the IR only has a single function remaining with 1 block within it. //! Generally, these passes are also expected to minimize the final amount of instructions. mod array_set; +mod as_slice_length; mod assert_constant; mod bubble_up_constrains; mod constant_folding; From 014971a7e63c65ad507d9717a3bf991ee58db1df Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 5 Apr 2024 13:06:50 +0100 Subject: [PATCH 2/4] chore: add regression test --- .../array_to_slice_constant_length/Nargo.toml | 7 +++++++ .../array_to_slice_constant_length/Prover.toml | 1 + .../array_to_slice_constant_length/src/main.nr | 10 ++++++++++ 3 files changed, 18 insertions(+) create mode 100644 test_programs/execution_success/array_to_slice_constant_length/Nargo.toml create mode 100644 test_programs/execution_success/array_to_slice_constant_length/Prover.toml create mode 100644 test_programs/execution_success/array_to_slice_constant_length/src/main.nr diff --git a/test_programs/execution_success/array_to_slice_constant_length/Nargo.toml b/test_programs/execution_success/array_to_slice_constant_length/Nargo.toml new file mode 100644 index 00000000000..b338cf9b6ae --- /dev/null +++ b/test_programs/execution_success/array_to_slice_constant_length/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "array_to_slice_constant_length" +type = "bin" +authors = [""] +compiler_version = ">=0.26.0" + +[dependencies] diff --git a/test_programs/execution_success/array_to_slice_constant_length/Prover.toml b/test_programs/execution_success/array_to_slice_constant_length/Prover.toml new file mode 100644 index 00000000000..a52e9d3c46a --- /dev/null +++ b/test_programs/execution_success/array_to_slice_constant_length/Prover.toml @@ -0,0 +1 @@ +val = "42" diff --git a/test_programs/execution_success/array_to_slice_constant_length/src/main.nr b/test_programs/execution_success/array_to_slice_constant_length/src/main.nr new file mode 100644 index 00000000000..e81dd4a0c5f --- /dev/null +++ b/test_programs/execution_success/array_to_slice_constant_length/src/main.nr @@ -0,0 +1,10 @@ +// Regression test for https://github.com/noir-lang/noir/issues/4722 + +unconstrained fn return_array(val: Field) -> [Field; 1] { + [val; 1] +} + +fn main(val: Field) { + let array = return_array(val); + assert_constant(array.as_slice().len()); +} From a393b2dd5a53c944351b7a778f9d68b7df9935ee Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 5 Apr 2024 13:07:44 +0100 Subject: [PATCH 3/4] chore: delay pass until before `evaluate_assert_constant` --- compiler/noirc_evaluator/src/ssa.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 8e2ad5b47d6..4b2f1060c88 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -50,10 +50,10 @@ pub(crate) fn optimize_into_acir( let ssa = SsaBuilder::new(program, print_passes, force_brillig_output, print_timings)? .run_pass(Ssa::defunctionalize, "After Defunctionalization:") .run_pass(Ssa::remove_paired_rc, "After Removing Paired rc_inc & rc_decs:") - .run_pass(Ssa::as_slice_optimization, "After As slice optimization") .run_pass(Ssa::inline_functions, "After Inlining:") // Run mem2reg with the CFG separated into blocks .run_pass(Ssa::mem2reg, "After Mem2Reg:") + .run_pass(Ssa::as_slice_optimization, "After `as_slice` optimization") .try_run_pass(Ssa::evaluate_assert_constant, "After Assert Constant:")? .try_run_pass(Ssa::unroll_loops, "After Unrolling:")? .run_pass(Ssa::simplify_cfg, "After Simplifying:") From 89ab865ded8b619bca9b8936bbb3165f4bca2dd8 Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 5 Apr 2024 13:12:49 +0100 Subject: [PATCH 4/4] chore: small explanation of SSA pass --- compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs index 14ec1f8be4f..69eab1da0ed 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs @@ -10,6 +10,13 @@ use crate::ssa::{ use fxhash::FxHashMap as HashMap; impl Ssa { + /// A simple SSA pass to find any calls to `Intrinsic::AsSlice` and replacing any references to the length of the + /// resulting slice with the length of the array from which it was generated. + /// + /// This allows the length of a slice generated from an array to be used in locations where a constant value is + /// necessary when the value of the array is unknown. + /// + /// Note that this pass must be placed before loop unrolling to be useful. #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn as_slice_optimization(mut self) -> Self { for func in self.functions.values_mut() { @@ -55,6 +62,8 @@ fn replace_known_slice_lengths( let original_slice_length = call_returns[0]; // We won't use the new id for the original unknown length. + // This isn't strictly necessary as a new result will be defined the next time for which the instruction + // is reinserted but this avoids leaving the program in an invalid state. func.dfg.replace_result(instruction_id, original_slice_length); let known_length = func.dfg.make_constant(known_length.into(), Type::length_type()); func.dfg.set_value_from_id(original_slice_length, known_length);