fix(ssa): Remove array from cache in constant folding if it's an argument to a Call (#9040)

Author: aakoshh

compiler/noirc_evaluator/src/ssa.rs

@@ -211,7 +211,7 @@ pub fn primary_passes(options: &SsaEvaluatorOptions) -> Vec<SsaPass> {
         SsaPass::new(Ssa::fold_constants, "Constant Folding"),
         SsaPass::new(Ssa::flatten_basic_conditionals, "Simplify conditionals for unconstrained"),
         SsaPass::new(Ssa::remove_enable_side_effects, "EnableSideEffectsIf removal"),
-        SsaPass::new(Ssa::fold_constants_using_constraints, "Constraint Folding"),
+        SsaPass::new(Ssa::fold_constants_using_constraints, "Constraint Folding using constraints"),
         SsaPass::new_try(
             move |ssa| ssa.unroll_loops_iteratively(options.max_bytecode_increase_percent),
             "Unrolling",

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

@@ -715,15 +715,16 @@ impl<'brillig> Context<'brillig> {
         }
     }
 
+    /// Remove previously cached instructions that created arrays,
+    /// if the current instruction is such that it could modify that array.
     fn remove_possibly_mutated_cached_make_arrays(
         &mut self,
         instruction: &Instruction,
         function: &Function,
     ) {
-        use Instruction::{ArraySet, Store};
+        use Instruction::{ArraySet, Call, MakeArray, Store};
 
-        // Should we consider calls to slice_push_back and similar to be mutating operations as well?
-        if let Store { value, .. } | ArraySet { array: value, .. } = instruction {
+        let mut remove_if_array = |value: &ValueId| {
             if function.dfg.is_global(*value) {
                 // Early return as we expect globals to be immutable.
                 return;
@@ -739,9 +740,29 @@ impl<'brillig> Context<'brillig> {
                 _ => return,
             };
 
-            if matches!(instruction, Instruction::MakeArray { .. } | Instruction::Call { .. }) {
+            if matches!(instruction, MakeArray { .. } | Call { .. }) {
                 self.cached_instruction_results.remove(instruction);
             }
+        };
+
+        // Should we consider calls to slice_push_back and similar to be mutating operations as well?
+        match instruction {
+            Store { value, .. } | ArraySet { array: value, .. } => {
+                remove_if_array(value);
+            }
+            Call { arguments, func } if function.runtime().is_brillig() => {
+                let Value::Function(func_id) = &function.dfg[*func] else { return };
+                if matches!(function.dfg.purity_of(*func_id), None | Some(Purity::Impure)) {
+                    // Arrays passed to functions might be mutated by them if there are no `inc_rc` instructions
+                    // placed *before* the call to protect them. Currently we don't track the ref count in this
+                    // context, so be conservative and do not reuse any array shared with a callee.
+                    // In ACIR we don't track refcounts, so it should be fine.
+                    for arg in arguments {
+                        remove_if_array(arg);
+                    }
+                }
+            }
+            _ => {}
         }
     }
 }

test_programs/execution_success/regression_9037/Nargo.toml

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

test_programs/execution_success/regression_9037/Prover.toml

@@ -0,0 +1 @@
+return = [[false]]

test_programs/execution_success/regression_9037/src/main.nr

@@ -0,0 +1,28 @@
+global G_B: ([bool; 1],) = ([false],);
+unconstrained fn main() -> pub [[bool; 1]; 1] {
+    let mut ctx_limit: u32 = 25_u32;
+    let _ = func_3(([true],), [G_B.0], (&mut ctx_limit));
+    [G_B.0]
+}
+unconstrained fn func_3(a: ([bool; 1],), mut c: [[bool; 1]; 1], ctx_limit: &mut u32) -> bool {
+    if ((*ctx_limit) > 0_u32) {
+        *ctx_limit = ((*ctx_limit) - 1_u32);
+        if func_3(G_B, [a.0], ctx_limit) {
+            c[0_u32] = {
+                {
+                    let mut idx_d: u32 = 0_u32;
+                    loop {
+                        if (idx_d == 3_u32) {
+                            break
+                        } else {
+                            idx_d = (idx_d + 1_u32);
+                            ()
+                        }
+                    }
+                };
+                a.0
+            };
+        }
+    }
+    true
+}

tooling/ast_fuzzer/src/program/mod.rs

@@ -572,8 +572,6 @@ impl std::fmt::Display for DisplayAstAsNoir<'_> {
         printer.show_type_in_let = true;
         // Most of the time it doesn't affect testing, except the comptime tests where
         // we parse back the code. For that we use `DisplayAstAsNoirComptime`.
-        // Using it also inserts an extra `cast` in the SSA,
-        // which at the moment for example defeats loop unrolling.
         printer.show_type_of_int_literal = false;
         printer.print_program(self.0, f)
     }