chore(ssa): Assert that array offset are not set

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

compiler/noirc_evaluator/src/ssa/ir/instruction.rs

@@ -453,14 +453,19 @@ 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
                 // it is expected that OOB checks are inserted separately. However, it would
                 // 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,

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,16 @@
 //! 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.
+//!
+//! 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::{
@@ -279,4 +289,36 @@ mod tests {
         ";
         assert_ssa_does_not_change(src, Ssa::brillig_array_get_and_set);
     }
+
+    #[test]
+    #[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]):
+            v1 = array_get v0, index u32 0 -> Field
+            return v1
+        }
+        ";
+
+        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"
+        );
+    }
 }

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"

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 {

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,
             },
             (

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),
@@ -120,7 +120,7 @@ impl Default for Config {
             ("let", 20),
             ("call", 5),
             ("print", 15),
-            ("constrain", 10),
+            ("constrain", 15),
         ]);
         Self {
             max_globals: 3,