feat: only inject "out of bounds" checks in brillig

EXTERNAL_NOIR_LIBRARIES.yml

@@ -34,7 +34,7 @@ libraries:
     critical: true
   noir_bigcurve:
     repo: noir-lang/noir_bigcurve
-    timeout: 350
+    timeout: 400
     critical: true
   noir_base64:
     repo: noir-lang/noir_base64
@@ -83,7 +83,7 @@ libraries:
     repo: AztecProtocol/aztec-packages
     ref: *AZ_COMMIT
     path: noir-projects/noir-protocol-circuits/crates/blob
-    timeout: 200
+    timeout: 300
     critical: false
   protocol_circuits_parity_lib:
     repo: AztecProtocol/aztec-packages

compiler/noirc_evaluator/src/ssa/mod.rs

@@ -141,6 +141,7 @@ pub fn primary_passes(options: &SsaEvaluatorOptions) -> Vec<SsaPass> {
         ),
         SsaPass::new(Ssa::simplify_cfg, "Simplifying"),
         SsaPass::new(Ssa::mem2reg, "Mem2Reg"),
+        SsaPass::new(Ssa::simplify_cfg, "Simplifying"),
         SsaPass::new(Ssa::flatten_cfg, "Flattening"),
         SsaPass::new(Ssa::remove_bit_shifts, "Removing Bit Shifts"),
         // Run mem2reg once more with the flattened CFG to catch any remaining loads/stores

compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs

@@ -172,12 +172,75 @@ impl Ssa {
         }
 
         for function in self.functions.values_mut() {
+            // Disabled due to failures
+            // #[cfg(debug_assertions)]
+            // flatten_cfg_pre_check(function);
+
             flatten_function_cfg(function, &no_predicates);
+
+            // Disabled as we're getting failures, I would expect this to pass however.
+            // #[cfg(debug_assertions)]
+            // flatten_cfg_post_check(function);
         }
         self
     }
 }
 
+/// Pre-check condition for [Ssa::flatten_cfg].
+///
+/// Succeeds if:
+///   - no block contains a jmpif with a constant condition.
+///
+/// Otherwise panics.
+#[cfg(debug_assertions)]
+#[allow(dead_code)]
+fn flatten_cfg_pre_check(function: &Function) {
+    function.reachable_blocks().iter().for_each(|block| {
+        if let TerminatorInstruction::JmpIf { condition, .. } =
+            function.dfg[*block].unwrap_terminator()
+        {
+            if function
+                .dfg
+                .get_numeric_constant(*condition)
+                .is_some_and(|c| c.is_zero() || c.is_one())
+            {
+                // We have this check here as if we do not, the flattening pass will generate groups of opcodes which are
+                // under a zero predicate. These opcodes are not always removed by the die pass and so bloat the code.
+                //
+                // We could add handling for this inside of the pass itself but it's simpler to just run `simplify_cfg`
+                // immediately before flattening.
+                panic!(
+                    "Function {} has a jmpif with a constant condition {condition}",
+                    function.id()
+                );
+            }
+        }
+    });
+}
+
+/// Post-check condition for [Ssa::flatten_cfg].
+///
+/// Panics if:
+///   - Any `enable_side_effects u1 0` instructions exist.
+///
+/// Otherwise succeeds.
+#[cfg(debug_assertions)]
+#[allow(dead_code)]
+fn flatten_cfg_post_check(function: &Function) {
+    function.reachable_blocks().iter().for_each(|block| {
+        function.dfg[*block].instructions().iter().for_each(|instruction| {
+            if let Instruction::EnableSideEffectsIf { condition } = function.dfg[*instruction] {
+                if function.dfg.get_numeric_constant(condition).is_some_and(|c| c.is_zero()) {
+                    panic!(
+                        "Function {} has an enable_side_effects u1 0 instruction",
+                        function.id()
+                    );
+                }
+            }
+        });
+    });
+}
+
 pub(crate) struct Context<'f> {
     pub(crate) inserter: FunctionInserter<'f>,
 

compiler/noirc_evaluator/src/ssa/opt/remove_unreachable_instructions.rs

@@ -173,7 +173,76 @@ impl Function {
                         }
                     }
                 }
-                _ => {}
+                Instruction::ArrayGet { array, index, offset }
+                | Instruction::ArraySet { array, index, offset, .. } => {
+                    let array_or_slice_type = context.dfg.type_of_value(*array);
+                    let array_op_always_fails = match &array_or_slice_type {
+                        Type::Slice(_) => false,
+                        array_type @ Type::Array(_, len) => {
+                            *len == 0
+                                || context.dfg.get_numeric_constant(*index).is_some_and(|index| {
+                                    (index.try_to_u32().unwrap() - offset.to_u32())
+                                        >= (array_type.element_size() as u32 * len)
+                                })
+                        }
+
+                        _ => unreachable!(
+                            "Encountered non-array type during array read/write operation"
+                        ),
+                    };
+
+                    if array_op_always_fails {
+                        let is_predicate_constant_one =
+                            match context.dfg.get_numeric_constant(side_effects_condition) {
+                                Some(predicate) => predicate.is_one(),
+                                None => false, // The predicate is a variable
+                            };
+                        current_block_reachability = if is_predicate_constant_one {
+                            // If we have an array that contains references we no longer need to bother with resolution of those references.
+                            // However, we want a trap to still be triggered by an OOB array access.
+                            // Thus, we can replace our array with dummy numerics to avoid unnecessary allocations
+                            // making there way further down the compilation pipeline (e.g. ACIR where references are not supported).
+                            let (old_instruction, old_array, trap_array) = match array_or_slice_type
+                            {
+                                Type::Array(_, len) => {
+                                    let dummy_array_typ = Type::Array(
+                                        Arc::new(vec![Type::Numeric(NumericType::unsigned(1))]),
+                                        len,
+                                    );
+                                    (
+                                        instruction.clone(),
+                                        *array,
+                                        zeroed_value(
+                                            context.dfg,
+                                            func_id,
+                                            block_id,
+                                            &dummy_array_typ,
+                                        ),
+                                    )
+                                }
+                                _ => unreachable!("Expected an array type"),
+                            };
+                            let new_instruction = old_instruction.map_values(|value| {
+                                if value == old_array { trap_array } else { value }
+                            });
+                            let stack =
+                                context.dfg.get_instruction_call_stack_id(context.instruction_id);
+                            context.dfg.insert_instruction_and_results(
+                                new_instruction,
+                                block_id,
+                                Some(vec![Type::Numeric(NumericType::unsigned(1))]),
+                                stack,
+                            );
+                            // Remove the old failing array access in favor of the dummy one
+                            context.remove_current_instruction();
+
+                            Reachability::Unreachable
+                        } else {
+                            Reachability::UnreachableUnderPredicate
+                        };
+                    }
+                }
+                _ => (),
             };
 
             if current_block_reachability == Reachability::Unreachable {
@@ -794,4 +863,37 @@ mod test {
         }
         "#);
     }
+
+    #[test]
+    fn transforms_failing_array_access_to_work_on_dummy_array() {
+        let src = "
+        acir(inline) predicate_pure fn main f0 {
+          b0():
+            v0 = allocate -> &mut u8                      
+            store u8 0 at v0                               
+            v2 = make_array [u8 0, v0] : [(u8, &mut u8); 1]
+            v4 = array_get v2, index u32 2 -> u8         
+            v6 = array_get v2, index u32 3 -> &mut u8      
+            return v4
+        }
+        ";
+
+        let ssa = Ssa::from_str(src).unwrap();
+        let ssa = ssa.remove_unreachable_instructions();
+
+        // We expect the array containing references to no longer be in use,
+        // for the failing array get to now be over a dummy array.
+        // We expect the new assertion to also use the correct dummy type (u1) as to have a well formed SSA.
+        assert_ssa_snapshot!(ssa, @r"
+        acir(inline) predicate_pure fn main f0 {
+          b0():
+            v0 = allocate -> &mut u8
+            store u8 0 at v0
+            v2 = make_array [u8 0, v0] : [(u8, &mut u8); 1]
+            v4 = make_array [u1 0] : [u1; 1]
+            v6 = array_get v4, index u32 2 -> u1
+            unreachable
+        }
+        ");
+    }
 }

compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs

@@ -462,16 +462,25 @@ impl FunctionContext<'_> {
 
         // Checks for index Out-of-bounds
         match array_type {
+            Type::Array(_, len) => {
+                // Out of bounds array accesses are guaranteed to fail in ACIR so this check is performed implicitly.
+                // We then only need to inject it for brillig functions.
+                let runtime = self.builder.current_function.runtime();
+                if runtime.is_brillig() {
+                    let len =
+                        self.builder.numeric_constant(*len as u128, NumericType::length_type());
+                    self.codegen_access_check(index, len);
+                }
+            }
             Type::Slice(_) => {
+                // The slice length is dynamic however so we can't rely on it being equal to the underlying memory
+                // block as we can do for array types. We then inject a access check for both ACIR and brillig.
                 self.codegen_access_check(
                     index,
                     length.expect("ICE: a length must be supplied for checking index"),
                 );
             }
-            Type::Array(_, len) => {
-                let len = self.builder.numeric_constant(*len as u128, NumericType::length_type());
-                self.codegen_access_check(index, len);
-            }
+
             _ => unreachable!("must have array or slice but got {array_type}"),
         }
 

cspell.json

@@ -258,6 +258,7 @@
         "ripemd",
         "RMVD",
         "rngs",
+        "runtimes",
         "rustc",
         "rustfmt",
         "rustup",

test_programs/execution_success/empty_strings_in_composite_arrays/Nargo.toml

@@ -0,0 +1,6 @@
+[package]
+name = "empty_strings_in_composite_arrays"
+type = "bin"
+authors = [""]
+
+[dependencies]

test_programs/execution_success/empty_strings_in_composite_arrays/Prover.toml

@@ -0,0 +1 @@
+return = 0

test_programs/execution_success/empty_strings_in_composite_arrays/src/main.nr

@@ -0,0 +1,7 @@
+unconstrained fn main() -> pub bool {
+    func_2([(true, "", "")]) != func_2([(true, "", "")])
+}
+unconstrained fn func_2(b: [(bool, str<0>, str<0>); 1]) -> (str<0>, str<0>) {
+    println(b);
+    (b[0].1, b[0].2)
+}

test_programs/execution_success/regression_unused_nested_array_get/Nargo.toml

@@ -0,0 +1,6 @@
+[package]
+name = "regression_unused_nested_array_get"
+type = "bin"
+authors = [""]
+
+[dependencies]

test_programs/execution_success/regression_unused_nested_array_get/src/main.nr

@@ -0,0 +1,17 @@
+global G_A: [[Field; 1]; 2] =
+    [[-244112236639376348452645045105852040922], [-86306352895169753536594521383676370518]];
+fn main() {
+    if true {
+        for idx_a in 7845584091265575028_i64..7845584091265575034_i64 {
+            let mut b: [&mut u32; 1] = [(&mut 81586902_u32)];
+            let g: (i64, (i8, Field, Field, Field), u64) = (
+                idx_a,
+                (
+                    -118_i8, 1,
+                    (155342861061489534793205612322034234097 / G_A[1_u32][((*b[0_u32]) % 1_u32)]),
+                    G_A[((*b[0_u32]) % 2_u32)][((*b[0_u32]) % 1_u32)],
+                ), 1313306195659668909_u64,
+            );
+        }
+    }
+}