diff --git a/compiler/noirc_evaluator/src/acir/arrays.rs b/compiler/noirc_evaluator/src/acir/arrays.rs index 418653b5aef..a2e8b8a49b5 100644 --- a/compiler/noirc_evaluator/src/acir/arrays.rs +++ b/compiler/noirc_evaluator/src/acir/arrays.rs @@ -201,8 +201,8 @@ impl Context<'_> { // Pass the instruction between array methods rather than the internal fields themselves let (array, index, store_value) = match dfg[instruction] { - Instruction::ArrayGet { array, index, offset: _ } => (array, index, None), - Instruction::ArraySet { array, index, value, mutable, offset: _ } => { + Instruction::ArrayGet { array, index } => (array, index, None), + Instruction::ArraySet { array, index, value, mutable } => { mutable_array_set = mutable; (array, index, Some(value)) } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 4bb481d2718..fdeeda8e695 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -8,7 +8,7 @@ use crate::brillig::brillig_ir::registers::RegisterAllocator; use crate::brillig::brillig_ir::{ BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, BrilligBinaryOp, BrilligContext, ReservedRegisters, }; -use crate::ssa::ir::instruction::{ArrayOffset, ConstrainError, Hint}; +use crate::ssa::ir::instruction::{ConstrainError, Hint}; use crate::ssa::ir::{ basic_block::BasicBlockId, dfg::DataFlowGraph, @@ -846,7 +846,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { let source_variable = self.convert_ssa_single_addr_value(*value, dfg); self.convert_cast(destination_variable, source_variable); } - Instruction::ArrayGet { array, index, offset } => { + Instruction::ArrayGet { array, index } => { let result_ids = dfg.instruction_results(instruction_id); let destination_variable = self.variables.define_variable( self.function_context, @@ -858,13 +858,8 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { let array_variable = self.convert_ssa_value(*array, dfg); let index_variable = self.convert_ssa_single_addr_value(*index, dfg); - let has_offset = if dfg.is_constant(*index) { - // For constant indices it must be the case that they have been offset during SSA - assert!(*offset != ArrayOffset::None); - true - } else { - false - }; + // Constants are assumed to have been offset just before Brillig gen. + let has_offset = dfg.is_constant(*index); self.convert_ssa_array_get( array_variable, @@ -873,7 +868,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { has_offset, ); } - Instruction::ArraySet { array, index, value, mutable, offset } => { + Instruction::ArraySet { array, index, value, mutable } => { let source_variable = self.convert_ssa_value(*array, dfg); let index_register = self.convert_ssa_single_addr_value(*index, dfg); let value_variable = self.convert_ssa_value(*value, dfg); @@ -886,13 +881,8 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { dfg, ); - let has_offset = if dfg.is_constant(*index) { - // For constant indices it must be the case that they have been offset during SSA - assert!(*offset != ArrayOffset::None); - true - } else { - false - }; + // Constants are assumed to have been offset just before Brillig gen. + let has_offset = dfg.is_constant(*index); self.convert_ssa_array_set( source_variable, diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index 46edaa7987d..bab867dff91 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -522,7 +522,7 @@ impl DependencyContext { // For array get operations, we check the Brillig calls for // results involving the array in question, to properly // populate the array element tainted sets - Instruction::ArrayGet { array, index, offset: _ } => { + Instruction::ArrayGet { array, index } => { self.process_array_get(*array, *index, &results, function); // Record all the used arguments as parents of the results self.update_children(&arguments, &results); diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/data_bus.rs b/compiler/noirc_evaluator/src/ssa/function_builder/data_bus.rs index db5065cd74d..cb84348fb0c 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/data_bus.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/data_bus.rs @@ -2,7 +2,6 @@ use std::{collections::BTreeMap, sync::Arc}; use crate::ssa::ir::{ function::RuntimeType, - instruction::ArrayOffset, types::{NumericType, Type}, value::{ValueId, ValueMapping}, }; @@ -167,9 +166,7 @@ impl FunctionBuilder { if let Type::Array(_, 0) = subitem_typ { continue; } - let offset = ArrayOffset::None; - let element = - self.insert_array_get(value, index_var, offset, subitem_typ.clone()); + let element = self.insert_array_get(value, index_var, subitem_typ.clone()); index += match subitem_typ { Type::Array(_, _) | Type::Slice(_) => subitem_typ.element_size(), Type::Numeric(_) => 1, diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 23cfa0c1cc6..dae78079133 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -23,7 +23,7 @@ use super::{ basic_block::BasicBlock, dfg::{GlobalsGraph, InsertInstructionResult}, function::RuntimeType, - instruction::{ArrayOffset, ConstrainError, InstructionId, Intrinsic}, + instruction::{ConstrainError, InstructionId, Intrinsic}, types::NumericType, }, opt::pure::FunctionPurities, @@ -353,12 +353,10 @@ impl FunctionBuilder { &mut self, array: ValueId, index: ValueId, - offset: ArrayOffset, element_type: Type, ) -> ValueId { let element_type = Some(vec![element_type]); - self.insert_instruction(Instruction::ArrayGet { array, index, offset }, element_type) - .first() + self.insert_instruction(Instruction::ArrayGet { array, index }, element_type).first() } /// Insert an instruction to create a new array with the given index replaced with a new value @@ -368,9 +366,8 @@ impl FunctionBuilder { index: ValueId, value: ValueId, mutable: bool, - offset: ArrayOffset, ) -> ValueId { - let instruction = Instruction::ArraySet { array, index, value, mutable, offset }; + let instruction = Instruction::ArraySet { array, index, value, mutable }; self.insert_instruction(instruction, None).first() } diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs index c1a6ecc3b1f..6a64f3c5bba 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs @@ -5,9 +5,7 @@ use super::{ ir::{ dfg::DataFlowGraph, function::{Function, FunctionId, RuntimeType}, - instruction::{ - ArrayOffset, Binary, BinaryOp, ConstrainError, Instruction, TerminatorInstruction, - }, + instruction::{Binary, BinaryOp, ConstrainError, Instruction, TerminatorInstruction}, types::Type, value::ValueId, }, @@ -585,19 +583,17 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { self.side_effects_enabled = self.lookup_bool(*condition, "enable_side_effects")?; Ok(()) } - Instruction::ArrayGet { array, index, offset } => { - self.interpret_array_get(*array, *index, *offset, results[0], side_effects_enabled) + Instruction::ArrayGet { array, index } => { + self.interpret_array_get(*array, *index, results[0], side_effects_enabled) } - Instruction::ArraySet { array, index, value, mutable, offset } => self - .interpret_array_set( - *array, - *index, - *value, - *mutable, - *offset, - results[0], - side_effects_enabled, - ), + Instruction::ArraySet { array, index, value, mutable } => self.interpret_array_set( + *array, + *index, + *value, + *mutable, + results[0], + side_effects_enabled, + ), Instruction::IncrementRc { value } => self.interpret_inc_rc(*value), Instruction::DecrementRc { value } => self.interpret_dec_rc(*value), Instruction::IfElse { then_condition, then_value, else_condition, else_value } => self @@ -892,10 +888,10 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { &mut self, array: ValueId, index: ValueId, - offset: ArrayOffset, result: ValueId, side_effects_enabled: bool, ) -> IResult<()> { + let offset = self.dfg().array_offset(array, index); let array = self.lookup_array_or_slice(array, "array get")?; let index = self.lookup_u32(index, "array get index")?; let mut index = index - offset.to_u32(); @@ -959,10 +955,10 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { index: ValueId, value: ValueId, mutable: bool, - offset: ArrayOffset, result: ValueId, side_effects_enabled: bool, ) -> IResult<()> { + let offset = self.dfg().array_offset(array, index); let array = self.lookup_array_or_slice(array, "array set")?; let result_array = if side_effects_enabled { diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs b/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs index dd33ddc3aec..7cfd91b077e 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs @@ -490,9 +490,9 @@ fn shr_signed() { " acir(inline) fn main f0 { b0(): - v0 = shr i16 65520, i16 2 - v1 = shr i16 65533, i16 1 - v2 = shr i16 65528, i16 3 + v0 = shr i16 65520, i16 2 + v1 = shr i16 65533, i16 1 + v2 = shr i16 65528, i16 3 return v0, v1, v2 } ", @@ -829,10 +829,10 @@ fn array_get() { fn array_get_with_offset() { let value = expect_value( r#" - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(): v0 = make_array [Field 1, Field 2] : [Field; 2] - v1 = array_get v0, index u32 4 minus 3 -> Field + v1 = array_get v0, index u32 2 minus 1 -> Field return v1 } "#, @@ -949,10 +949,11 @@ fn array_set_disabled_by_enable_side_effects() { fn array_set_with_offset() { let values = expect_values( " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(): v0 = make_array [Field 1, Field 2] : [Field; 2] - v1 = array_set v0, index u32 4 minus 3, value Field 5 + inc_rc v0 + v1 = array_set v0, index u32 2 minus 1, value Field 5 return v0, v1 } ", @@ -961,18 +962,15 @@ fn array_set_with_offset() { let v0 = values[0].as_array_or_slice().unwrap(); let v1 = values[1].as_array_or_slice().unwrap(); - // acir function, so all rcs are 1 - assert_eq!(*v0.rc.borrow(), 1); + assert_eq!(*v0.rc.borrow(), 2); assert_eq!(*v1.rc.borrow(), 1); let one = from_constant(1u32.into(), NumericType::NativeField); let two = from_constant(2u32.into(), NumericType::NativeField); let five = from_constant(5u32.into(), NumericType::NativeField); - // v0 was not mutated - assert_eq!(*v0.elements.borrow(), vec![one.clone(), two.clone()]); - // v1 was mutated - assert_eq!(*v1.elements.borrow(), vec![one, five]); + assert_eq!(*v0.elements.borrow(), vec![one.clone(), two.clone()], "v0 should not be mutated"); + assert_eq!(*v1.elements.borrow(), vec![one, five], "v1 should be mutated"); } #[test] diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 376b7894f26..0eb40c8762f 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -2,6 +2,7 @@ use std::{borrow::Cow, sync::Arc}; use crate::ssa::{ function_builder::data_bus::DataBus, + ir::instruction::ArrayOffset, opt::pure::{FunctionPurities, Purity}, }; @@ -107,6 +108,9 @@ pub(crate) struct DataFlowGraph { #[serde(skip)] pub(crate) function_purities: Arc, + + /// Indicate whether the Brillig array index offset optimizations have been performed. + pub(crate) brillig_arrays_offset: bool, } /// The GlobalsGraph contains the actual global data. @@ -766,6 +770,21 @@ impl DataFlowGraph { pub(crate) fn purity_of(&self, function: FunctionId) -> Option { self.function_purities.get(&function).copied() } + + /// Determine the appropriate [ArrayOffset] to use for indexing an array or slice. + pub(crate) fn array_offset(&self, array: ValueId, index: ValueId) -> ArrayOffset { + if !self.runtime.is_brillig() + || !self.brillig_arrays_offset + || self.get_numeric_constant(index).is_none() + { + return ArrayOffset::None; + } + match self.type_of_value(array) { + Type::Array(_, _) => ArrayOffset::Array, + Type::Slice(_) => ArrayOffset::Slice, + _ => ArrayOffset::None, + } + } } impl std::ops::Index for DataFlowGraph { diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify.rs index 9a33bc72ab3..b1ec80bd61b 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify.rs @@ -2,7 +2,7 @@ use crate::ssa::{ ir::{ basic_block::BasicBlockId, instruction::{ - ArrayOffset, Binary, BinaryOp, ConstrainError, Instruction, + Binary, BinaryOp, ConstrainError, Instruction, binary::{truncate, truncate_field}, }, types::{NumericType, Type}, @@ -110,7 +110,7 @@ pub(crate) fn simplify( } } Instruction::ConstrainNotEqual(..) => None, - Instruction::ArrayGet { array, index, offset } => { + Instruction::ArrayGet { array, index } => { if let Some(index) = dfg.get_numeric_constant(*index) { return try_optimize_array_get_from_previous_set(dfg, *array, index); } @@ -121,7 +121,7 @@ pub(crate) fn simplify( { // 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 +342,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( @@ -354,11 +353,7 @@ fn optimize_length_one_array_read( let result = try_optimize_array_get_from_previous_set(dfg, array, FieldElement::zero()); if let SimplifyResult::None = result { - SimplifyResult::SimplifiedToInstruction(Instruction::ArrayGet { - array, - index: zero, - offset, - }) + SimplifyResult::SimplifiedToInstruction(Instruction::ArrayGet { array, index: zero }) } else { result } @@ -456,8 +451,8 @@ fn try_optimize_array_set_from_previous_get( target_value: ValueId, ) -> SimplifyResult { let array_from_get = match dfg.get_local_or_global_instruction(target_value) { - Some(Instruction::ArrayGet { array, index, offset }) => { - if *offset == ArrayOffset::None && *array == array_id && *index == target_index { + Some(Instruction::ArrayGet { array, index }) => { + if *array == array_id && *index == target_index { // If array and index match from the value, we can immediately simplify return SimplifyResult::SimplifiedTo(array_id); } else if *index == target_index { diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify/call.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify/call.rs index f446b90e1dc..c0888484a21 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify/call.rs @@ -11,7 +11,7 @@ use crate::ssa::{ ir::{ basic_block::BasicBlockId, dfg::DataFlowGraph, - instruction::{ArrayOffset, Binary, BinaryOp, Endian, Hint, Instruction, Intrinsic}, + instruction::{Binary, BinaryOp, Endian, Hint, Instruction, Intrinsic}, types::{NumericType, Type}, value::{Value, ValueId}, }, @@ -511,7 +511,6 @@ fn simplify_slice_push_back( index: arguments[0], value: arguments[2], mutable: false, - offset: ArrayOffset::None, }; let set_last_slice_value = dfg @@ -565,11 +564,8 @@ fn simplify_slice_pop_back( // Iterating through element types in reverse here since we're popping from the end for element_type in element_types.iter().rev() { flattened_len = decrement_slice_length(flattened_len, dfg, block, call_stack); - let get_last_elem_instr = Instruction::ArrayGet { - array: arguments[1], - index: flattened_len, - offset: ArrayOffset::None, - }; + let get_last_elem_instr = + Instruction::ArrayGet { array: arguments[1], index: flattened_len }; let element_type = Some(vec![element_type.clone()]); let get_last_elem = dfg diff --git a/compiler/noirc_evaluator/src/ssa/ir/function.rs b/compiler/noirc_evaluator/src/ssa/ir/function.rs index 8b6bc0fa241..7e758a986e3 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/function.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/function.rs @@ -125,6 +125,7 @@ impl Function { new_function.set_runtime(another.runtime()); new_function.set_globals(another.dfg.globals.clone()); new_function.dfg.set_function_purities(another.dfg.function_purities.clone()); + new_function.dfg.brillig_arrays_offset = another.dfg.brillig_arrays_offset; new_function } diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 9ad9400bf41..0ea0acb6600 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -355,14 +355,12 @@ pub enum Instruction { EnableSideEffectsIf { condition: ValueId }, /// Retrieve a value from an array at the given index - /// `offset` determines whether the index has been shifted by some offset. - ArrayGet { array: ValueId, index: ValueId, offset: ArrayOffset }, + ArrayGet { array: ValueId, index: ValueId }, /// Creates a new array with the new value at the given index. All other elements are identical /// to those in the given array. This will not modify the original array unless `mutable` is /// set. This flag is off by default and only enabled when optimizations determine it is safe. - /// `offset` determines whether the index has been shifted by some offset. - ArraySet { array: ValueId, index: ValueId, value: ValueId, mutable: bool, offset: ArrayOffset }, + ArraySet { array: ValueId, index: ValueId, value: ValueId, mutable: bool }, /// An instruction to increment the reference count of a value. /// @@ -453,7 +451,7 @@ impl Instruction { match self { Instruction::Binary(binary) => binary.requires_acir_gen_predicate(dfg), - Instruction::ArrayGet { array, index, offset: _ } => { + Instruction::ArrayGet { array, index } => { // `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 @@ -567,7 +565,7 @@ 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 } => !dfg.is_safe_index(*index, *array), // ArraySet has side effects ArraySet { .. } => true, @@ -642,18 +640,15 @@ impl Instruction { Instruction::EnableSideEffectsIf { condition } => { Instruction::EnableSideEffectsIf { condition: f(*condition) } } - Instruction::ArrayGet { array, index, offset } => { - Instruction::ArrayGet { array: f(*array), index: f(*index), offset: *offset } - } - Instruction::ArraySet { array, index, value, mutable, offset } => { - Instruction::ArraySet { - array: f(*array), - index: f(*index), - value: f(*value), - mutable: *mutable, - offset: *offset, - } + Instruction::ArrayGet { array, index } => { + Instruction::ArrayGet { array: f(*array), index: f(*index) } } + Instruction::ArraySet { array, index, value, mutable } => Instruction::ArraySet { + array: f(*array), + index: f(*index), + value: f(*value), + mutable: *mutable, + }, Instruction::IncrementRc { value } => Instruction::IncrementRc { value: f(*value) }, Instruction::DecrementRc { value } => Instruction::DecrementRc { value: f(*value) }, Instruction::RangeCheck { value, max_bit_size, assert_message } => { @@ -716,11 +711,11 @@ impl Instruction { Instruction::EnableSideEffectsIf { condition } => { *condition = f(*condition); } - Instruction::ArrayGet { array, index, offset: _ } => { + Instruction::ArrayGet { array, index } => { *array = f(*array); *index = f(*index); } - Instruction::ArraySet { array, index, value, mutable: _, offset: _ } => { + Instruction::ArraySet { array, index, value, mutable: _ } => { *array = f(*array); *index = f(*index); *value = f(*value); @@ -782,11 +777,11 @@ impl Instruction { f(*value); } Instruction::Allocate => (), - Instruction::ArrayGet { array, index, offset: _ } => { + Instruction::ArrayGet { array, index } => { f(*array); f(*index); } - Instruction::ArraySet { array, index, value, mutable: _, offset: _ } => { + Instruction::ArraySet { array, index, value, mutable: _ } => { f(*array); f(*index); f(*value); diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index 706cc1c3aa2..146baf93739 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -338,17 +338,19 @@ fn display_instruction_inner( Instruction::EnableSideEffectsIf { condition } => { write!(f, "enable_side_effects {}", show(*condition)) } - Instruction::ArrayGet { array, index, offset } => { + Instruction::ArrayGet { array, index } => { + let offset = dfg.array_offset(*array, *index); write!( f, "array_get {}, index {}{}{}", show(*array), show(*index), - display_array_offset(offset), + display_array_offset(&offset), result_types(dfg, results) ) } - Instruction::ArraySet { array, index, value, mutable, offset } => { + Instruction::ArraySet { array, index, value, mutable } => { + let offset = dfg.array_offset(*array, *index); let array = show(*array); let index = show(*index); let value = show(*value); @@ -359,7 +361,7 @@ fn display_instruction_inner( mutable, array, index, - display_array_offset(offset), + display_array_offset(&offset), value ) } diff --git a/compiler/noirc_evaluator/src/ssa/mod.rs b/compiler/noirc_evaluator/src/ssa/mod.rs index 589030d3c21..700d5b96add 100644 --- a/compiler/noirc_evaluator/src/ssa/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/mod.rs @@ -223,10 +223,6 @@ pub fn primary_passes(options: &SsaEvaluatorOptions) -> Vec> { SsaPass::new(Ssa::fold_constants_with_brillig, "Inlining Brillig Calls"), SsaPass::new(Ssa::remove_unreachable_instructions, "Remove Unreachable Instructions") .and_then(Ssa::remove_unreachable_functions), - // This pass makes transformations specific to Brillig generation. - // It must be the last pass to either alter or add new instructions before Brillig generation, - // as other semantics in the compiler can potentially break (e.g. inserting instructions). - SsaPass::new(Ssa::brillig_array_get_and_set, "Brillig Array Get and Set Optimizations"), SsaPass::new(Ssa::dead_instruction_elimination, "Dead Instruction Elimination") // A function can be potentially unreachable post-DIE if all calls to that function were removed. .and_then(Ssa::remove_unreachable_functions), @@ -261,10 +257,6 @@ pub fn minimal_passes() -> Vec> { // which was called in the AST not being called in the SSA. Such functions would cause // panics later, when we are looking for global allocations. SsaPass::new(Ssa::remove_unreachable_functions, "Removing Unreachable Functions"), - // We need to add an offset to constant array indices in Brillig. - // This can change which globals are used, because constant creation might result - // in the (re)use of otherwise unused global values. - SsaPass::new(Ssa::brillig_array_get_and_set, "Brillig Array Get and Set Optimizations"), ] } @@ -284,6 +276,20 @@ pub fn optimize_ssa_builder_into_acir( drop(ssa_gen_span_guard); + // Shift the array offsets in Brillig functions to benefit from the constant allocation logic, + // but do this outside the normal SSA passes, so we don't need to make the SSA interpreter and + // every other pass aware of the possibility that indexes are shifted, which could result in + // them conceptually not aligning with the positions of complex item types for example. + // This can change which globals are used, because constant creation might result + // in the (re)use of otherwise unused global values. + // It must be the last pass to either alter or add new instructions before Brillig generation, + // as other semantics in the compiler can potentially break (e.g. inserting instructions). + // Running it through the builder so it can be printed, for transparency. + let builder = builder.run_passes(&[SsaPass::new( + Ssa::brillig_array_get_and_set, + "Brillig Array Get and Set Optimizations", + )])?; + let brillig = time("SSA to Brillig", options.print_codegen_timings, || { builder.ssa().to_brillig(&options.brillig_options) }); diff --git a/compiler/noirc_evaluator/src/ssa/opt/basic_conditional.rs b/compiler/noirc_evaluator/src/ssa/opt/basic_conditional.rs index a30d025b0ef..c241b30cdaf 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/basic_conditional.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/basic_conditional.rs @@ -201,7 +201,7 @@ fn block_cost(block: BasicBlockId, dfg: &DataFlowGraph) -> u32 { | Instruction::Store { .. } | Instruction::ArraySet { .. } => return u32::MAX, - Instruction::ArrayGet { array, index, offset: _ } => { + Instruction::ArrayGet { array, index } => { // A get can fail because of out-of-bound index let mut in_bound = false; // check if index is in bound 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..2ae6f8f96df 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 @@ -21,16 +21,16 @@ //! v1 = array_get v0, index u32 4 minus 1 -> Field //! ``` //! -//! 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 +//! Now the index to retrieve is 4 and there's no need to offset it in Brillig, +//! avoiding one addition. +//! +//! 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,18 +44,22 @@ //! b0(v0: [Field]): //! v1 = array_get v0, index u32 6 minus 3 -> Field //! ``` -use acvm::FieldElement; +//! +//! This pass must be the very last, just before generating ACIR and Brillig opcodes from the SSA. +//! Shifting indexes can break some of the assumptions of earlier compiler operations, so this is +//! done outside the normal SSA pipeline, as an internal step, and must be run only once. +//! +//! 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 crate::{ brillig::brillig_ir::BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, ssa::{ Ssa, - ir::{ - function::Function, - instruction::{ArrayOffset, Instruction}, - types::{NumericType, Type}, - value::ValueId, - }, + ir::{function::Function, instruction::Instruction, types::NumericType, value::ValueId}, }, }; @@ -79,38 +83,30 @@ impl Function { return; } + assert!(!self.dfg.brillig_arrays_offset, "Brillig arrays can be offset at most once!"); + self.dfg.brillig_arrays_offset = true; + self.simple_optimization(|context| { let instruction = context.instruction(); match instruction { - Instruction::ArrayGet { array, index, offset } => { - // This pass should run at most once - assert!(*offset == ArrayOffset::None); - + Instruction::ArrayGet { array, index } => { let array = *array; let index = *index; - let Some(constant_index) = context.dfg.get_numeric_constant(index) else { + let Some(index) = compute_offset_index(context, array, index) else { return; }; - - let (index, offset) = compute_index_and_offset(context, array, constant_index); - let new_instruction = Instruction::ArrayGet { array, index, offset }; + let new_instruction = Instruction::ArrayGet { array, index }; context.replace_current_instruction_with(new_instruction); } - Instruction::ArraySet { array, index, value, mutable, offset } => { - // This pass should run at most once - assert!(*offset == ArrayOffset::None); - + Instruction::ArraySet { array, index, value, mutable } => { let array = *array; let index = *index; let value = *value; let mutable = *mutable; - let Some(constant_index) = context.dfg.get_numeric_constant(index) else { + let Some(index) = compute_offset_index(context, array, index) else { return; }; - - let (index, offset) = compute_index_and_offset(context, array, constant_index); - let new_instruction = - Instruction::ArraySet { array, index, value, mutable, offset }; + let new_instruction = Instruction::ArraySet { array, index, value, mutable }; context.replace_current_instruction_with(new_instruction); } _ => (), @@ -119,23 +115,19 @@ impl Function { } } -/// Given an array or slice value and a constant index, returns an offset (shifted) index -/// together with which type of [`ArrayOffset`] was used to shift it. -fn compute_index_and_offset( +/// Given an array or slice value and a constant index, returns an offset (shifted) index. +fn compute_offset_index( context: &mut SimpleOptimizationContext, array_or_slice: ValueId, - constant_index: FieldElement, -) -> (ValueId, ArrayOffset) { - let offset = if matches!(context.dfg.type_of_value(array_or_slice), Type::Array(..)) { - ArrayOffset::Array - } else { - ArrayOffset::Slice - }; + index: ValueId, +) -> Option { + let constant_index = context.dfg.get_numeric_constant(index)?; + let offset = context.dfg.array_offset(array_or_slice, index); let index = context.dfg.make_constant( constant_index + offset.to_u32().into(), NumericType::unsigned(BRILLIG_MEMORY_ADDRESSING_BIT_SIZE), ); - (index, offset) + Some(index) } #[cfg(test)] @@ -279,4 +271,53 @@ mod tests { "; assert_ssa_does_not_change(src, Ssa::brillig_array_get_and_set); } + + // This test is here to demonstrate how trying to use the common machinery + // after this pass would lead to unexpected results. + #[test] + fn is_safe_index_unexpected_after_pass() { + let src = " + brillig(inline) fn main f0 { + b0(v0: [Field; 1]): + v1 = array_get v0, index u32 0 -> Field + return v1 + } + "; + + fn has_side_effects(ssa: &Ssa) -> bool { + let func = ssa.main(); + let b0 = &func.dfg[func.entry_block()]; + let instruction = &func.dfg[b0.instructions()[0]]; + instruction.has_side_effects(&func.dfg) + } + + let ssa = Ssa::from_str(src).unwrap(); + + assert!( + !has_side_effects(&ssa), + "Indexing 1-element array with index 0 should be safe and have no side effects." + ); + + let ssa = ssa.brillig_array_get_and_set(); + + assert!( + has_side_effects(&ssa), + "It should have no side effects, but the index is now considered unsafe." + ); + } + + #[test] + #[should_panic(expected = "offset at most once")] + fn only_executes_once() { + let src = " + brillig(inline) fn main f0 { + b0(v0: [Field]): + v2 = array_get v0, index u32 3 minus 3 -> Field + return v2 + } + "; + + let ssa = Ssa::from_str(src).unwrap(); + ssa.brillig_array_get_and_set(); + } } 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 cf17ab4128a..ab4a2895963 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding/mod.rs @@ -26,7 +26,7 @@ use crate::ssa::{ dfg::DataFlowGraph, dom::DominatorTree, function::{Function, FunctionId}, - instruction::{ArrayOffset, Instruction, InstructionId}, + instruction::{Instruction, InstructionId}, types::NumericType, value::{Value, ValueId, ValueMapping}, }, @@ -394,9 +394,7 @@ impl Context { if let Instruction::ArraySet { index, value, .. } = instruction { let predicate = self.use_constraint_info.then_some(side_effects_enabled_var); - let offset = ArrayOffset::None; - let array_get = - Instruction::ArrayGet { array: instruction_results[0], index: *index, offset }; + let array_get = Instruction::ArrayGet { array: instruction_results[0], index: *index }; // If we encounter an array_get for this address, we know what the result will be. self.cached_instruction_results.cache(array_get, predicate, block, vec![*value]); @@ -1124,9 +1122,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" diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 985234dee35..de329234839 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -556,7 +556,6 @@ mod test { function_builder::FunctionBuilder, ir::{ function::RuntimeType, - instruction::ArrayOffset, map::Id, types::{NumericType, Type}, }, @@ -700,8 +699,7 @@ mod test { let v3 = builder.insert_load(v2, array_type); let one = builder.numeric_constant(1u128, NumericType::unsigned(32)); let mutable = false; - let offset = ArrayOffset::None; - let v5 = builder.insert_array_set(v3, zero, one, mutable, offset); + let v5 = builder.insert_array_set(v3, zero, one, mutable); builder.terminate_with_return(vec![v5]); let ssa = builder.finish(); diff --git a/compiler/noirc_evaluator/src/ssa/opt/die/array_oob_checks.rs b/compiler/noirc_evaluator/src/ssa/opt/die/array_oob_checks.rs index 55b31ee7f10..2d02ecc2610 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die/array_oob_checks.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die/array_oob_checks.rs @@ -4,7 +4,7 @@ use crate::ssa::{ ir::{ basic_block::BasicBlockId, function::Function, - instruction::{ArrayOffset, BinaryOp, Instruction}, + instruction::{BinaryOp, Instruction}, types::{NumericType, Type}, value::ValueId, }, @@ -155,11 +155,7 @@ pub(super) fn should_insert_oob_check(function: &Function, instruction: &Instruc use Instruction::*; match instruction { - ArrayGet { array, index, offset } | ArraySet { array, index, offset, .. } => { - assert!( - matches!(offset, ArrayOffset::None), - "ICE: The array offset should always be `None` for ACIR" - ); + ArrayGet { array, index } | ArraySet { array, index, .. } => { // We only care about arrays here as slices are expected to have explicit checks laid down in the initial SSA. function.dfg.try_get_array_length(*array).is_some() && !function.dfg.is_safe_index(*index, *array) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs index 15fced2415f..bd6890a0045 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs @@ -6,7 +6,7 @@ use crate::{ ssa::ir::{ basic_block::BasicBlockId, dfg::DataFlowGraph, - instruction::{ArrayOffset, BinaryOp, Instruction}, + instruction::{BinaryOp, Instruction}, types::{NumericType, Type}, value::ValueId, }, @@ -157,8 +157,7 @@ impl<'a> ValueMerger<'a> { let typevars = Some(vec![element_type.clone()]); let mut get_element = |array, typevars| { - let offset = ArrayOffset::None; - let get = Instruction::ArrayGet { array, index, offset }; + let get = Instruction::ArrayGet { array, index }; self.dfg .insert_instruction_and_results(get, self.block, typevars, self.call_stack) .first() @@ -228,8 +227,7 @@ impl<'a> ValueMerger<'a> { if len <= index_u32 { panic!("get_element invoked with an out of bounds index"); } else { - let offset = ArrayOffset::None; - let get = Instruction::ArrayGet { array, index, offset }; + let get = Instruction::ArrayGet { array, index }; let results = self.dfg.insert_instruction_and_results( get, self.block, diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 99420c7c6e2..8570c9954a8 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -718,7 +718,7 @@ impl<'f> LoopInvariantContext<'f> { use Instruction::*; match instruction { - ArrayGet { array, index, offset: _ } => { + ArrayGet { array, index } => { let array_typ = self.inserter.function.dfg.type_of_value(*array); let upper_bound = self.outer_induction_variables.get(index).map(|bounds| bounds.1); if let (Type::Array(_, len), Some(upper_bound)) = (array_typ, upper_bound) { diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs index 0eb15046065..36023f37aeb 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs @@ -67,9 +67,7 @@ use acvm::{FieldElement, acir::AcirField}; use crate::ssa::{ ir::{ function::Function, - instruction::{ - ArrayOffset, Binary, BinaryOp, ConstrainError, Endian, Instruction, Intrinsic, - }, + instruction::{Binary, BinaryOp, ConstrainError, Endian, Instruction, Intrinsic}, types::{NumericType, Type}, value::ValueId, }, @@ -414,8 +412,7 @@ impl Context<'_, '_, '_> { /// Insert an instruction to extract an element from an array fn insert_array_get(&mut self, array: ValueId, index: ValueId, element_type: Type) -> ValueId { let element_type = Some(vec![element_type]); - let offset = ArrayOffset::None; - let instruction = Instruction::ArrayGet { array, index, offset }; + let instruction = Instruction::ArrayGet { array, index }; self.context.insert_instruction(instruction, element_type).first() } } 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 5c34af41157..08acc96471f 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable_instructions.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable_instructions.rs @@ -276,8 +276,8 @@ impl Function { Reachability::Unreachable } } - Instruction::ArrayGet { array, index, offset } - | Instruction::ArraySet { array, index, offset, .. } + Instruction::ArrayGet { array, index } + | Instruction::ArraySet { array, index, .. } if context.dfg.runtime().is_acir() => { let array_type = context.dfg.type_of_value(*array); @@ -288,7 +288,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 { @@ -513,7 +513,7 @@ fn should_replace_instruction_with_defaults(context: &SimpleOptimizationContext) let instruction = context.instruction(); // ArrayGet needs special handling: if we replaced the index with a default value, it could be invalid. - if let Instruction::ArrayGet { array, index, offset: _ } = instruction { + if let Instruction::ArrayGet { array, index } = instruction { // If we replaced the index with a default, it's going to be zero. let index_zero = context.dfg.get_numeric_constant(*index).is_some_and(|c| c.is_zero()); diff --git a/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs b/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs index 159264ad1e3..b382fa82cfe 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs @@ -12,7 +12,7 @@ use crate::ssa::{ basic_block::BasicBlockId, dfg::GlobalsGraph, function::{Function, FunctionId}, - instruction::{ConstrainError, Instruction}, + instruction::{ArrayOffset, ConstrainError, Instruction}, value::ValueId, }, opt::pure::FunctionPurities, @@ -288,16 +288,18 @@ impl Translator { self.define_variable(target, value_id)?; } ParsedInstruction::ArrayGet { target, element_type, array, index, offset } => { + self.set_offset(&target, offset)?; let array = self.translate_value(array)?; let index = self.translate_value(index)?; - let value_id = self.builder.insert_array_get(array, index, offset, element_type); + let value_id = self.builder.insert_array_get(array, index, element_type); self.define_variable(target, value_id)?; } ParsedInstruction::ArraySet { target, array, index, value, mutable, offset } => { + self.set_offset(&target, offset)?; let array = self.translate_value(array)?; let index = self.translate_value(index)?; let value = self.translate_value(value)?; - let value_id = self.builder.insert_array_set(array, index, value, mutable, offset); + let value_id = self.builder.insert_array_set(array, index, value, mutable); self.define_variable(target, value_id)?; } ParsedInstruction::BinaryOp { target, lhs, op, rhs } => { @@ -595,4 +597,16 @@ impl Translator { fn current_function_id(&self) -> FunctionId { self.builder.current_function.id() } + + /// If any array instruction has an offset, mark the DFG as using offsets in general. + fn set_offset(&mut self, target: &Identifier, offset: ArrayOffset) -> Result<(), SsaError> { + if offset == ArrayOffset::None { + return Ok(()); + } + if !self.builder.current_function.dfg.runtime().is_brillig() { + return Err(SsaError::IllegalOffset(target.clone(), offset)); + } + self.builder.current_function.dfg.brillig_arrays_offset = true; + Ok(()) + } } diff --git a/compiler/noirc_evaluator/src/ssa/parser/mod.rs b/compiler/noirc_evaluator/src/ssa/parser/mod.rs index 68ca089bc2c..4cc4b1835e2 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/mod.rs @@ -7,7 +7,7 @@ use std::{ use super::{ Ssa, ir::{ - instruction::{ArrayOffset, BinaryOp}, + instruction::BinaryOp, types::{NumericType, Type}, }, opt::pure::Purity, @@ -27,7 +27,10 @@ use noirc_frontend::{ use thiserror::Error; use token::{Keyword, SpannedToken, Token}; -use crate::ssa::{ir::function::RuntimeType, parser::ast::ParsedTerminator}; +use crate::ssa::{ + ir::{function::RuntimeType, instruction::ArrayOffset}, + parser::ast::ParsedTerminator, +}; mod ast; mod into_ssa; @@ -130,6 +133,8 @@ pub(crate) enum SsaError { VariableAlreadyDefined(Identifier), #[error("Global '{0}' already defined")] GlobalAlreadyDefined(Identifier), + #[error("Illegal use of offset in non-Brillig function '{0:?}'")] + IllegalOffset(Identifier, ArrayOffset), } impl SsaError { @@ -141,7 +146,8 @@ impl SsaError { | SsaError::UnknownBlock(identifier) | SsaError::VariableAlreadyDefined(identifier) | SsaError::GlobalAlreadyDefined(identifier) - | SsaError::UnknownFunction(identifier) => identifier.span, + | SsaError::UnknownFunction(identifier) + | SsaError::IllegalOffset(identifier, _) => identifier.span, SsaError::MismatchedReturnValues { returns, expected: _ } => returns[0].span, } } diff --git a/compiler/noirc_evaluator/src/ssa/parser/tests.rs b/compiler/noirc_evaluator/src/ssa/parser/tests.rs index b8e7e7f7f06..ca80a934dc9 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/tests.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/tests.rs @@ -7,6 +7,7 @@ use crate::{ fn assert_ssa_roundtrip(src: &str) { let ssa = Ssa::from_str(src).unwrap(); + println!("offset: {}", ssa.main().dfg.brillig_arrays_offset); let ssa = ssa.print_without_locations().to_string(); let ssa = trim_leading_whitespace_from_lines(&ssa); let src = trim_leading_whitespace_from_lines(src); @@ -382,7 +383,7 @@ fn test_array_get() { #[test] fn test_array_get_with_index_minus_1() { let src: &'static str = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: [Field; 3]): v2 = array_get v0, index u32 3 minus 1 -> Field return @@ -394,8 +395,8 @@ fn test_array_get_with_index_minus_1() { #[test] fn test_array_get_with_index_minus_3() { let src: &'static str = " - acir(inline) fn main f0 { - b0(v0: [Field; 3]): + brillig(inline) fn main f0 { + b0(v0: [Field]): v2 = array_get v0, index u32 6 minus 3 -> Field return } @@ -430,7 +431,7 @@ fn test_mutable_array_set() { #[test] fn test_array_set_with_index_minus_1() { let src = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: [Field; 3]): v3 = array_set v0, index u32 2 minus 1, value Field 1 return @@ -442,8 +443,8 @@ fn test_array_set_with_index_minus_1() { #[test] fn test_array_set_with_index_minus_3() { let src = " - acir(inline) fn main f0 { - b0(v0: [Field; 3]): + brillig(inline) fn main f0 { + b0(v0: [Field]): v3 = array_set v0, index u32 4 minus 3, value Field 1 return } @@ -882,3 +883,16 @@ fn unknown_function_global_function_pointer() { "; let _ = Ssa::from_str_no_validation(src).unwrap(); } + +#[test] +#[should_panic(expected = "Illegal use of offset")] +fn illegal_offset_in_acir_function() { + let src = " + acir(inline) fn main f0 { + b0(v0: [Field; 3]): + v3 = array_set v0, index u32 2 minus 1, value Field 1 + return + } + "; + let _ = Ssa::from_str_no_validation(src).unwrap(); +} diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index de7d7de1304..de6edb147a1 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -16,7 +16,7 @@ use crate::ssa::function_builder::FunctionBuilder; use crate::ssa::ir::basic_block::BasicBlockId; use crate::ssa::ir::function::FunctionId as IrFunctionId; use crate::ssa::ir::function::{Function, RuntimeType}; -use crate::ssa::ir::instruction::{ArrayOffset, BinaryOp}; +use crate::ssa::ir::instruction::BinaryOp; use crate::ssa::ir::map::AtomicCounter; use crate::ssa::ir::types::{NumericType, Type}; use crate::ssa::ir::value::ValueId; @@ -838,8 +838,7 @@ impl<'a> FunctionContext<'a> { new_value.for_each(|value| { let value = value.eval(self); let mutable = false; - let offset = ArrayOffset::None; - array = self.builder.insert_array_set(array, index, value, mutable, offset); + array = self.builder.insert_array_set(array, index, value, mutable); // Unchecked add because this can't overflow (it would have overflowed when creating the array) index = self.builder.insert_binary(index, BinaryOp::Add { unchecked: true }, one); }); diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 262a0b6eb77..006de36d3a0 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -29,7 +29,7 @@ use self::{ use super::ir::basic_block::BasicBlockId; use super::ir::dfg::GlobalsGraph; -use super::ir::instruction::{ArrayOffset, ErrorType}; +use super::ir::instruction::ErrorType; use super::ir::types::NumericType; use super::validation::validate_function; use super::{ @@ -510,8 +510,7 @@ impl FunctionContext<'_> { // Reference counting in brillig relies on us incrementing reference // counts when nested arrays/slices are constructed or indexed. This // has no effect in ACIR code. - let offset = ArrayOffset::None; - let result = self.builder.insert_array_get(array, index, offset, typ); + let result = self.builder.insert_array_get(array, index, typ); result.into() })) } diff --git a/tooling/ast_fuzzer/src/lib.rs b/tooling/ast_fuzzer/src/lib.rs index fa23f242b21..3b870cf997c 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, diff --git a/tooling/ssa_fuzzer/src/builder.rs b/tooling/ssa_fuzzer/src/builder.rs index 4f65154be03..4c501ba434d 100644 --- a/tooling/ssa_fuzzer/src/builder.rs +++ b/tooling/ssa_fuzzer/src/builder.rs @@ -5,7 +5,6 @@ use noirc_driver::{CompileOptions, CompiledProgram}; use noirc_evaluator::ssa::function_builder::FunctionBuilder; use noirc_evaluator::ssa::ir::basic_block::BasicBlockId; use noirc_evaluator::ssa::ir::function::{Function, RuntimeType}; -use noirc_evaluator::ssa::ir::instruction::ArrayOffset; use noirc_evaluator::ssa::ir::instruction::BinaryOp; use noirc_evaluator::ssa::ir::map::Id; use noirc_evaluator::ssa::ir::types::Type as SsaType; @@ -465,7 +464,6 @@ impl FuzzerBuilder { let byte = self.builder.insert_array_get( array.value_id, index, - ArrayOffset::None, SsaType::Numeric(NumericType::U8.into()), ); let byte_as_field = self.builder.insert_cast(byte, NumericType::Field.into()); @@ -703,7 +701,6 @@ impl FuzzerBuilder { let res = self.builder.insert_array_get( array.value_id, index.value_id, - ArrayOffset::None, element_type.clone().into(), ); TypedValue::new(res, element_type) @@ -731,13 +728,8 @@ impl FuzzerBuilder { } else { index }; - let res = self.builder.insert_array_set( - array.value_id, - index.value_id, - value.value_id, - false, - ArrayOffset::None, - ); + let res = + self.builder.insert_array_set(array.value_id, index.value_id, value.value_id, false); TypedValue::new(res, Type::Array(array_type.clone(), array_length)) } @@ -799,18 +791,8 @@ impl FuzzerBuilder { let result = result[0]; let x_idx = self.builder.numeric_constant(0_u32, NumericType::U32.into()); let y_idx = self.builder.numeric_constant(1_u32, NumericType::U32.into()); - let x = self.builder.insert_array_get( - result, - x_idx, - ArrayOffset::None, - field_type.clone().into(), - ); - let y = self.builder.insert_array_get( - result, - y_idx, - ArrayOffset::None, - field_type.clone().into(), - ); + let x = self.builder.insert_array_get(result, x_idx, field_type.clone().into()); + let y = self.builder.insert_array_get(result, y_idx, field_type.clone().into()); Point { x: TypedValue::new(x, field_type.clone()), y: TypedValue::new(y, field_type), @@ -886,24 +868,10 @@ impl FuzzerBuilder { let x_idx = self.builder.numeric_constant(0_u32, NumericType::U32.into()); let y_idx = self.builder.numeric_constant(1_u32, NumericType::U32.into()); let is_infinite_idx = self.builder.numeric_constant(2_u32, NumericType::U32.into()); - let x = self.builder.insert_array_get( - result, - x_idx, - ArrayOffset::None, - field_type.clone().into(), - ); - let y = self.builder.insert_array_get( - result, - y_idx, - ArrayOffset::None, - field_type.clone().into(), - ); - let is_infinite = self.builder.insert_array_get( - result, - is_infinite_idx, - ArrayOffset::None, - boolean_type.clone().into(), - ); + let x = self.builder.insert_array_get(result, x_idx, field_type.clone().into()); + let y = self.builder.insert_array_get(result, y_idx, field_type.clone().into()); + let is_infinite = + self.builder.insert_array_get(result, is_infinite_idx, boolean_type.clone().into()); Point { x: TypedValue::new(x, field_type.clone()), y: TypedValue::new(y, field_type), @@ -935,24 +903,10 @@ impl FuzzerBuilder { let x_idx = self.builder.numeric_constant(0_u32, NumericType::U32.into()); let y_idx = self.builder.numeric_constant(1_u32, NumericType::U32.into()); let is_infinite_idx = self.builder.numeric_constant(2_u32, NumericType::U32.into()); - let x = self.builder.insert_array_get( - result, - x_idx, - ArrayOffset::None, - field_type.clone().into(), - ); - let y = self.builder.insert_array_get( - result, - y_idx, - ArrayOffset::None, - field_type.clone().into(), - ); - let is_infinite = self.builder.insert_array_get( - result, - is_infinite_idx, - ArrayOffset::None, - boolean_type.clone().into(), - ); + let x = self.builder.insert_array_get(result, x_idx, field_type.clone().into()); + let y = self.builder.insert_array_get(result, y_idx, field_type.clone().into()); + let is_infinite = + self.builder.insert_array_get(result, is_infinite_idx, boolean_type.clone().into()); Point { x: TypedValue::new(x, field_type.clone()), y: TypedValue::new(y, field_type),