From 6009cde84a2d6e788d0bf3034444ce450573a3c4 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 22 Sep 2025 15:49:42 +0100 Subject: [PATCH 1/7] Assert that array offset are not set --- .../src/ssa/ir/dfg/simplify.rs | 6 +-- .../noirc_evaluator/src/ssa/ir/instruction.rs | 16 ++++++- .../src/ssa/opt/brillig_array_get_and_set.rs | 42 +++++++++++++++++-- .../opt/remove_unreachable_instructions.rs | 6 ++- 4 files changed, 60 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify.rs index 9a33bc72ab3..1c27b731cad 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify.rs @@ -119,9 +119,10 @@ pub(crate) fn simplify( if matches!(array_or_slice_type, Type::Array(_, 1)) && array_or_slice_type.flattened_size() == 1 { + assert_eq!(*offset, ArrayOffset::None); // If the array is of length 1 then we know the only value which can be potentially read out of it. // We can then simply assert that the index is equal to zero and return the array's contained value. - optimize_length_one_array_read(dfg, block, call_stack, *array, *index, *offset) + optimize_length_one_array_read(dfg, block, call_stack, *array, *index) } else { None } @@ -342,7 +343,6 @@ fn optimize_length_one_array_read( call_stack: CallStackId, array: ValueId, index: ValueId, - offset: ArrayOffset, ) -> SimplifyResult { let zero = dfg.make_constant(FieldElement::zero(), NumericType::length_type()); let index_constraint = Instruction::Constrain( @@ -357,7 +357,7 @@ fn optimize_length_one_array_read( SimplifyResult::SimplifiedToInstruction(Instruction::ArrayGet { array, index: zero, - offset, + offset: ArrayOffset::None, }) } else { result diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 9ad9400bf41..8d76b6e9b2e 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -453,7 +453,7 @@ impl Instruction { match self { Instruction::Binary(binary) => binary.requires_acir_gen_predicate(dfg), - Instruction::ArrayGet { array, index, offset: _ } => { + Instruction::ArrayGet { array, index, offset } => { // `ArrayGet`s which read from "known good" indices from an array should not need a predicate. // This extra out of bounds (OOB) check is only inserted in the ACIR runtime. // Thus, in Brillig an `ArrayGet` is always a pure operation in isolation and @@ -461,6 +461,11 @@ impl Instruction { // not be safe to separate the `ArrayGet` from the OOB constraints that precede it, // because while it could read an array index, the returned data could be invalid, // and fail at runtime if we tried using it in the wrong context. + assert_eq!( + *offset, + ArrayOffset::None, + "Array offsets are not supposed to be set yet" + ); !dfg.is_safe_index(*index, *array) } @@ -567,7 +572,14 @@ impl Instruction { // used in the wrong context. Since we use this information to decide whether to hoist // instructions during deduplication, we consider unsafe values as potentially having // indirect side effects. - ArrayGet { array, index, offset: _ } => !dfg.is_safe_index(*index, *array), + ArrayGet { array, index, offset } => { + assert_eq!( + *offset, + ArrayOffset::None, + "Array offsets are not supposed to be set yet." + ); + !dfg.is_safe_index(*index, *array) + } // ArraySet has side effects ArraySet { .. } => true, diff --git a/compiler/noirc_evaluator/src/ssa/opt/brillig_array_get_and_set.rs b/compiler/noirc_evaluator/src/ssa/opt/brillig_array_get_and_set.rs index ff720efabad..42b4f86986c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/brillig_array_get_and_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/brillig_array_get_and_set.rs @@ -23,14 +23,14 @@ //! //! Now the index to retrieve is 4 and there's no need to offset it in Brillig, //! avoiding one addition. +//! //! The "minus 1" part is just there so that readers can understand that the index //! was offset and that the actual element index is 3. On the Brillig side, //! array operations with constant indexes are always assumed to have already been //! shifted. //! -//! In the case of slices, these are represented as Brillig vectors, where the items -//! pointer instead starts at three rather than one. -//! A Brillig vector is represented as [RC, Size, Capacity, ...items]. So for a slice +//! In the case of slices, they are represented as Brillig vectors as [RC, Size, Capacity, ...items], +//! thus the items pointer instead starts at three rather than one. So for a slice //! this pass will transform this: //! //! ```ssa @@ -44,6 +44,10 @@ //! b0(v0: [Field]): //! v1 = array_get v0, index u32 6 minus 3 -> Field //! ``` +//! +//! This pass meant to be one of the very last, just before generating ACIR and Brillig opcodes +//! from the SSA. Some of the earlier passes would not expect the offset to be set, and may +//! not take offset indexes correctly into account. use acvm::FieldElement; use crate::{ @@ -279,4 +283,36 @@ mod tests { "; assert_ssa_does_not_change(src, Ssa::brillig_array_get_and_set); } + + #[test] + #[should_panic] + fn is_safe_index_unusable_once_offset() { + let src = " + brillig(inline) fn main f0 { + b0(v0: [Field; 1]): + v2 = array_set v0, index u32 0, value Field 2 + return v2 + } + "; + + let ssa = Ssa::from_str(src).unwrap(); + + let func = ssa.main(); + let b0 = &func.dfg[func.entry_block()]; + let instruction = &func.dfg[b0.instructions()[0]]; + assert!( + !instruction.has_side_effects(&func.dfg), + "Indexing 1-element array with index 0 should be safe and have no side effects" + ); + + let ssa = ssa.brillig_array_get_and_set(); + + let func = ssa.main(); + let b0 = &func.dfg[func.entry_block()]; + let instruction = &func.dfg[b0.instructions()[0]]; + assert!( + !instruction.has_side_effects(&func.dfg), + "It should have no side effects, but it panics instead as it's not expected to be used" + ); + } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable_instructions.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable_instructions.rs index 6dafa4d2e1e..47ec66e1183 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable_instructions.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable_instructions.rs @@ -135,7 +135,8 @@ use crate::ssa::{ dfg::DataFlowGraph, function::{Function, FunctionId}, instruction::{ - Binary, BinaryOp, ConstrainError, Instruction, Intrinsic, TerminatorInstruction, + ArrayOffset, Binary, BinaryOp, ConstrainError, Instruction, Intrinsic, + TerminatorInstruction, binary::{BinaryEvaluationResult, eval_constant_binary_op}, }, types::{NumericType, Type}, @@ -287,6 +288,7 @@ impl Function { | Instruction::ArraySet { array, index, offset, .. } if context.dfg.runtime().is_acir() => { + assert_eq!(*offset, ArrayOffset::None); let array_type = context.dfg.type_of_value(*array); // We can only know a guaranteed out-of-bounds access for arrays, never for slices. let Type::Array(_, len) = array_type else { @@ -295,7 +297,7 @@ impl Function { let array_op_always_fails = len == 0 || context.dfg.get_numeric_constant(*index).is_some_and(|index| { - (index.try_to_u32().unwrap() - offset.to_u32()) + (index.try_to_u32().unwrap()) >= (array_type.element_size() as u32 * len) }); if !array_op_always_fails { From 0287700196388a72ef611b94ff9e5f8f88cfa78b Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 22 Sep 2025 15:56:10 +0100 Subject: [PATCH 2/7] Fix constant folding test --- compiler/noirc_evaluator/src/ssa/opt/constant_folding/mod.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding/mod.rs index 9059ee81e1a..d227459db4c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding/mod.rs @@ -1157,9 +1157,6 @@ mod test { } "; let ssa = Ssa::from_str(src).unwrap(); - // Need to run SSA pass that sets up Brillig array gets - let ssa = ssa.brillig_array_get_and_set(); - let ssa = ssa.fold_constants_with_brillig(); let ssa = ssa.remove_unreachable_functions(); assert_ssa_snapshot!(ssa, @r" From c18c015464095a73ed736b89f3039f505d05c9b5 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 22 Sep 2025 20:33:01 +0100 Subject: [PATCH 3/7] Increase constrain freq in brillig --- tooling/ast_fuzzer/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tooling/ast_fuzzer/src/lib.rs b/tooling/ast_fuzzer/src/lib.rs index 4d2e2886f9b..51c766c9730 100644 --- a/tooling/ast_fuzzer/src/lib.rs +++ b/tooling/ast_fuzzer/src/lib.rs @@ -120,7 +120,7 @@ impl Default for Config { ("let", 20), ("call", 5), ("print", 15), - ("constrain", 10), + ("constrain", 15), ]); Self { max_globals: 3, From 49ba189adefa0d0f6464a312ab18989be8311838 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 22 Sep 2025 20:53:56 +0100 Subject: [PATCH 4/7] Add comments about motivation --- .../src/ssa/opt/brillig_array_get_and_set.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/brillig_array_get_and_set.rs b/compiler/noirc_evaluator/src/ssa/opt/brillig_array_get_and_set.rs index 42b4f86986c..3dcde9f8fcb 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/brillig_array_get_and_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/brillig_array_get_and_set.rs @@ -48,6 +48,12 @@ //! This pass meant to be one of the very last, just before generating ACIR and Brillig opcodes //! from the SSA. Some of the earlier passes would not expect the offset to be set, and may //! not take offset indexes correctly into account. +//! +//! The main motivation behind this pass is to make it possible to reuse the same constants across +//! Brillig opcodes without having to re-allocate another register for them every time they appear. +//! For this to happen the constants need to be part of the DFG, where they can be hoisted and +//! deduplicated across instructions. Doing this during Brillig codegen would be too late, as at +//! that time we have a read-only DFG and we would be forced to generate more Brillig opcodes. use acvm::FieldElement; use crate::{ From 43dc73a00db6c1ee100ede987a679805ff2b944b Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 22 Sep 2025 23:43:20 +0100 Subject: [PATCH 5/7] Fix demonstration test --- .../src/ssa/opt/brillig_array_get_and_set.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/brillig_array_get_and_set.rs b/compiler/noirc_evaluator/src/ssa/opt/brillig_array_get_and_set.rs index 3dcde9f8fcb..754cecbdfad 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/brillig_array_get_and_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/brillig_array_get_and_set.rs @@ -291,13 +291,13 @@ mod tests { } #[test] - #[should_panic] + #[should_panic(expected = "Array offsets are not supposed to be set yet")] fn is_safe_index_unusable_once_offset() { let src = " brillig(inline) fn main f0 { b0(v0: [Field; 1]): - v2 = array_set v0, index u32 0, value Field 2 - return v2 + v1 = array_get v0, index u32 0 -> Field + return v1 } "; From bcbd59e68784948e6ad80ff7bf5a6ef85f9187fd Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 22 Sep 2025 23:47:11 +0100 Subject: [PATCH 6/7] Adjust for loop frequency in ACIR --- tooling/ast_fuzzer/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tooling/ast_fuzzer/src/lib.rs b/tooling/ast_fuzzer/src/lib.rs index 51c766c9730..86d606bb902 100644 --- a/tooling/ast_fuzzer/src/lib.rs +++ b/tooling/ast_fuzzer/src/lib.rs @@ -103,7 +103,7 @@ impl Default for Config { ("assign", 30), ("if", 10), ("match", 10), - ("for", 25), + ("for", 30), ("let", 25), ("call", 5), ("constrain", 4), From 5c471305891f8423e44f682d857abe20b8c59695 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 23 Sep 2025 00:15:35 +0100 Subject: [PATCH 7/7] Add error equivalency for overflow --- tooling/ast_fuzzer/src/compare/interpreted.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tooling/ast_fuzzer/src/compare/interpreted.rs b/tooling/ast_fuzzer/src/compare/interpreted.rs index ea52cde1044..26335d608a8 100644 --- a/tooling/ast_fuzzer/src/compare/interpreted.rs +++ b/tooling/ast_fuzzer/src/compare/interpreted.rs @@ -175,7 +175,11 @@ impl Comparable for ssa::interpreter::errors::InterpreterError { BinaryOp::Add { unchecked: false } => msg == "attempt to add with overflow", BinaryOp::Sub { unchecked: false } => msg == "attempt to subtract with overflow", BinaryOp::Mul { unchecked: false } => msg == "attempt to multiply with overflow", - BinaryOp::Shl | BinaryOp::Shr => msg == "attempt to bit-shift with overflow", + BinaryOp::Shl | BinaryOp::Shr => { + msg == "attempt to bit-shift with overflow" + || msg == "attempt to shift right with overflow" + || msg == "attempt to shift left with overflow" + } _ => false, }, (