From 63b48be0fc88b1d4cfb8b6f60e9f8e8c44c8e021 Mon Sep 17 00:00:00 2001 From: AztecBot Date: Thu, 6 Aug 2026 16:15:14 +0000 Subject: [PATCH 1/6] fix(ssa): remove dead reference arrays between mem2reg iterations --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 211 +++++++++++++++++- 1 file changed, 210 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 6be732ed768..9948a22cf73 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -89,6 +89,20 @@ impl Function { // - v0 can't be optimized out because it's stored in v2 // - v2 can and will be optimized out // - now running it again will lead to optimizing out v0, etc. + // + // The same chain can be built through an array of references: + // + // ``` + // v0 = allocate -> &mut u16 + // store u16 1 at v0 + // v2 = make_array [v0] : [&mut u16; 1] + // v3 = allocate -> &mut [&mut u16; 1] + // store v2 at v3 + // ``` + // + // Here `v0` is ineligible because it appears in `v2`, and `v2` stays alive only because it + // is stored into `v3`. Once `v3` is optimized out, `v2` has no users left, so it is dropped + // between iterations to let the next one reach `v0`. loop { let mut inserter = FunctionInserter::new(self); @@ -145,6 +159,8 @@ impl Function { // mem2reg can no longer simplify the program. break; } + + remove_unused_make_arrays(self, &blocks); } } } @@ -542,6 +558,61 @@ fn collect_eligible_variables_and_def_sites( (variables, def_sites, has_ineligible_variables) } +/// Remove every [`MakeArray`][Instruction::MakeArray] instruction whose result has no users left. +/// +/// An array holding a reference makes that reference ineligible for promotion, even when the array +/// itself is dead. Dropping such arrays here is what lets the surrounding loop peel one level of +/// nesting per iteration; without it the pass stops at the outermost reference cell and leaves the +/// inner `allocate`/`store` pairs behind for good, because dead-store removal is mem2reg's job and +/// no later pass in the pipeline revisits them. +/// +/// Only `MakeArray` is considered: it is side-effect free and, unlike an array read, carries no +/// out-of-bounds check that dead instruction elimination would have to preserve. +fn remove_unused_make_arrays(function: &mut Function, blocks: &[BasicBlockId]) { + loop { + // The databus arrays are roots alongside the instructions and terminators: they are named + // by the function's ABI rather than by anything in the instruction stream. + let mut used = HashSet::default(); + for block in blocks.iter().copied() { + for instruction_id in function.dfg[block].instructions() { + function.dfg[*instruction_id].for_each_value(|value| { + used.insert(value); + }); + } + if let Some(terminator) = function.dfg[block].terminator() { + terminator.for_each_value(|value| { + used.insert(value); + }); + } + } + for call_data in &function.dfg.data_bus.call_data { + used.insert(call_data.array_id); + } + used.extend(function.dfg.data_bus.return_data); + + let mut removed_any = false; + for block in blocks.iter().copied() { + let mut instructions = function.dfg[block].take_instructions(); + instructions.retain(|instruction_id| { + let keep = !matches!(function.dfg[*instruction_id], Instruction::MakeArray { .. }) + || function + .dfg + .instruction_results(*instruction_id) + .iter() + .any(|result| used.contains(result)); + removed_any |= !keep; + keep + }); + *function.dfg[block].instructions_mut() = instructions; + } + + // An array can hold another array, so removing one can free up the next. + if !removed_any { + break; + } + } +} + /// Commit to all changes made by the pass: /// - Map any values mapped from the inserter to their new values in the function /// - Remove all Allocate, Load, and Store instructions from the eligible variables @@ -583,7 +654,10 @@ fn commit( mod tests { use crate::{ assert_ssa_snapshot, - ssa::{opt::assert_ssa_does_not_change, ssa_gen::Ssa}, + ssa::{ + opt::{assert_pass_does_not_affect_execution, assert_ssa_does_not_change}, + ssa_gen::Ssa, + }, }; #[test] @@ -1744,4 +1818,139 @@ brillig(inline) fn main f0 { super::get_value_from_visited_predecessor(dummy_var, b1, &cfg, &block_states).is_none() ); } + + /// A reference stored into an array that is itself only kept alive by a store to a dead + /// reference cell must still be promoted. Nothing later in the pipeline removes dead stores, + /// so an `allocate`/`store` pair surviving here reaches `mutable_array_set_optimization` and + /// ACIR generation, neither of which accepts memory operations. + #[test] + fn dead_reference_array_does_not_block_promotion() { + let src = " + acir(inline) fn main f0 { + b0(): + v0 = allocate -> &mut Field + store Field 1 at v0 + v2 = make_array [v0] : [&mut Field; 1] + v3 = allocate -> &mut [&mut Field; 1] + store v2 at v3 + return + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let (ssa, _) = assert_pass_does_not_affect_execution(ssa, vec![], Ssa::mem2reg); + assert_ssa_snapshot!(ssa, @r" + acir(inline) fn main f0 { + b0(): + return + } + "); + } + + /// Each level of reference nesting is peeled by one iteration of the pass, so a chain deeper + /// than the one in `dead_reference_array_does_not_block_promotion` must also be fully removed + /// by a single `mem2reg` run. + #[test] + fn nested_dead_reference_arrays_are_fully_promoted() { + let src = " + acir(inline) fn main f0 { + b0(): + v0 = allocate -> &mut Field + store Field 1 at v0 + v2 = make_array [v0] : [&mut Field; 1] + v3 = allocate -> &mut [&mut Field; 1] + store v2 at v3 + v5 = make_array [v3] : [&mut [&mut Field; 1]; 1] + v6 = allocate -> &mut [&mut [&mut Field; 1]; 1] + store v5 at v6 + return + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let (ssa, _) = assert_pass_does_not_affect_execution(ssa, vec![], Ssa::mem2reg); + assert_ssa_snapshot!(ssa, @r" + acir(inline) fn main f0 { + b0(): + return + } + "); + } + + /// An array of references that is actually read from keeps the references inside it aliased, + /// so neither the array nor the `allocate`/`store` pair backing the reference may be dropped. + #[test] + fn live_reference_array_still_prevents_optimization() { + let src = " + brillig(inline) fn func f0 { + b0(): + v0 = allocate -> &mut Field + store Field 1 at v0 + v2 = make_array [v0] : [&mut Field; 1] + v3 = array_get v2, index u32 0 -> &mut Field + v4 = load v3 -> Field + return v4 + } + "; + assert_ssa_does_not_change(src, Ssa::mem2reg); + } + + /// A reference count instruction is a use of the array it names, so an array that is only + /// reachable through `inc_rc` must be kept: dropping it would leave the `inc_rc` dangling. + #[test] + fn make_array_used_by_rc_instruction_is_kept() { + let src = " + brillig(inline) fn func f0 { + b0(): + v0 = allocate -> &mut Field + store Field 1 at v0 + v2 = make_array [v0] : [&mut Field; 1] + inc_rc v2 + v3 = allocate -> &mut [&mut Field; 1] + store v2 at v3 + return + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let (ssa, _) = assert_pass_does_not_affect_execution(ssa, vec![], Ssa::mem2reg); + assert_ssa_snapshot!(ssa, @r" + brillig(inline) fn func f0 { + b0(): + v0 = allocate -> &mut Field + store Field 1 at v0 + v2 = make_array [v0] : [&mut Field; 1] + inc_rc v2 + return + } + "); + } + + /// A databus array is named by the function's ABI, not by any instruction, so it has no + /// instruction users at all. It must survive the sweep that drops arrays which became dead + /// once the reference cell holding them was promoted. + #[test] + fn databus_arrays_are_kept() { + let src = " + acir(inline) fn main f0 { + call_data(0): array: v1, indices: [] + return_data: v3 + b0(v0: Field): + v1 = make_array [v0] : [Field; 1] + v2 = allocate -> &mut [Field; 1] + store v1 at v2 + v3 = make_array [v0] : [Field; 1] + return v3 + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let ssa = ssa.mem2reg(); + assert_ssa_snapshot!(ssa, @r" + acir(inline) fn main f0 { + call_data(0): array: v1, indices: [] + return_data: v2 + b0(v0: Field): + v1 = make_array [v0] : [Field; 1] + v2 = make_array [v0] : [Field; 1] + return v2 + } + "); + } } From ca4a65d69331e42daae9a4deec1d6ab9c7d0b0f6 Mon Sep 17 00:00:00 2001 From: AztecBot Date: Wed, 12 Aug 2026 10:15:06 +0000 Subject: [PATCH 2/6] update PR #13478 --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 127 ++++++++++++------ 1 file changed, 84 insertions(+), 43 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index f8e0cccf80f..6eebb2b690f 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -559,58 +559,70 @@ fn collect_eligible_variables_and_def_sites( (variables, def_sites, has_ineligible_variables) } -/// Remove every [`MakeArray`][Instruction::MakeArray] instruction whose result has no users left. +/// Remove every [`MakeArray`][Instruction::MakeArray] instruction whose results are unused. /// -/// An array holding a reference makes that reference ineligible for promotion, even when the array -/// itself is dead. Dropping such arrays here is what lets the surrounding loop peel one level of +/// An array holding a reference makes that reference ineligible for promotion even when the array +/// itself is dead. Dropping such arrays is what lets [`Function::mem2reg`]'s loop peel one level of /// nesting per iteration; without it the pass stops at the outermost reference cell and leaves the -/// inner `allocate`/`store` pairs behind for good, because dead-store removal is mem2reg's job and -/// no later pass in the pipeline revisits them. +/// inner `allocate`/`store` pairs behind for good, since removing a dead store needs the alias +/// information only mem2reg has and no later pass in the pipeline revisits them. /// /// Only `MakeArray` is considered: it is side-effect free and, unlike an array read, carries no -/// out-of-bounds check that dead instruction elimination would have to preserve. +/// out-of-bounds check that would have to be preserved in its place. +/// +/// Like [`Ssa::dead_instruction_elimination`], blocks are visited in post order and their +/// instructions in reverse, so a whole chain of nested dead arrays is removed in one traversal: +/// a value is only referenced after the instruction defining it, so every use of a result has +/// been seen by the time its defining instruction is reached. fn remove_unused_make_arrays(function: &mut Function, blocks: &[BasicBlockId]) { - loop { - // The databus arrays are roots alongside the instructions and terminators: they are named - // by the function's ABI rather than by anything in the instruction stream. - let mut used = HashSet::default(); - for block in blocks.iter().copied() { - for instruction_id in function.dfg[block].instructions() { - function.dfg[*instruction_id].for_each_value(|value| { - used.insert(value); - }); - } - if let Some(terminator) = function.dfg[block].terminator() { - terminator.for_each_value(|value| { - used.insert(value); + // Databus arrays are roots alongside the instructions and terminators: they are named by the + // function's ABI rather than by anything in the instruction stream. + let mut used_values: HashSet = function + .dfg + .data_bus + .call_data + .iter() + .map(|call_data| call_data.array_id) + .chain(function.dfg.data_bus.return_data) + .collect(); + + let mut instructions_to_remove = HashSet::default(); + + // `blocks` is in reverse post order, so iterating it backwards gives post order. + for block in blocks.iter().rev().copied() { + function.dfg[block].unwrap_terminator().for_each_value(|value| { + used_values.insert(value); + }); + + for instruction_id in function.dfg[block].instructions().iter().rev() { + let instruction = &function.dfg[*instruction_id]; + let is_unused_array = matches!(instruction, Instruction::MakeArray { .. }) + && function + .dfg + .instruction_results(*instruction_id) + .iter() + .all(|result| !used_values.contains(result)); + + if is_unused_array { + // Leaving the array's elements out of `used_values` is what exposes an array whose + // only use is another dead array. + instructions_to_remove.insert(*instruction_id); + } else { + instruction.for_each_value(|value| { + used_values.insert(value); }); } } - for call_data in &function.dfg.data_bus.call_data { - used.insert(call_data.array_id); - } - used.extend(function.dfg.data_bus.return_data); - - let mut removed_any = false; - for block in blocks.iter().copied() { - let mut instructions = function.dfg[block].take_instructions(); - instructions.retain(|instruction_id| { - let keep = !matches!(function.dfg[*instruction_id], Instruction::MakeArray { .. }) - || function - .dfg - .instruction_results(*instruction_id) - .iter() - .any(|result| used.contains(result)); - removed_any |= !keep; - keep - }); - *function.dfg[block].instructions_mut() = instructions; - } + } - // An array can hold another array, so removing one can free up the next. - if !removed_any { - break; - } + if instructions_to_remove.is_empty() { + return; + } + + for block in blocks.iter().copied() { + function.dfg[block] + .instructions_mut() + .retain(|instruction_id| !instructions_to_remove.contains(instruction_id)); } } @@ -1876,6 +1888,35 @@ brillig(inline) fn main f0 { "); } + /// The array and the reference cell keeping it alive need not sit in the same block, so a dead + /// array must also be found when its only use is in a successor block. + #[test] + fn dead_reference_array_used_in_another_block_is_removed() { + let src = " + acir(inline) fn main f0 { + b0(): + v0 = allocate -> &mut Field + store Field 1 at v0 + v2 = make_array [v0] : [&mut Field; 1] + jmp b1() + b1(): + v3 = allocate -> &mut [&mut Field; 1] + store v2 at v3 + return + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let (ssa, _) = assert_pass_does_not_affect_execution(ssa, vec![], Ssa::mem2reg); + assert_ssa_snapshot!(ssa, @r" + acir(inline) fn main f0 { + b0(): + jmp b1() + b1(): + return + } + "); + } + /// An array of references that is actually read from keeps the references inside it aliased, /// so neither the array nor the `allocate`/`store` pair backing the reference may be dropped. #[test] From bd51f0a4d176e0b050ba5027d3d6009634461083 Mon Sep 17 00:00:00 2001 From: AztecBot Date: Wed, 12 Aug 2026 11:23:56 +0000 Subject: [PATCH 3/6] update PR #13478 --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 106 +++++++++++++++++- 1 file changed, 104 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 6eebb2b690f..3edf495b0ba 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -572,8 +572,10 @@ fn collect_eligible_variables_and_def_sites( /// /// Like [`Ssa::dead_instruction_elimination`], blocks are visited in post order and their /// instructions in reverse, so a whole chain of nested dead arrays is removed in one traversal: -/// a value is only referenced after the instruction defining it, so every use of a result has -/// been seen by the time its defining instruction is reached. +/// every use of a result has been seen by the time its defining instruction is reached. Back +/// edges do not disturb this. A use is always dominated by its definition, and the post-order is +/// the reverse of a topological order of the CFG with back edges removed, so a block is always +/// visited before any block dominating it. fn remove_unused_make_arrays(function: &mut Function, blocks: &[BasicBlockId]) { // Databus arrays are roots alongside the instructions and terminators: they are named by the // function's ABI rather than by anything in the instruction stream. @@ -668,6 +670,7 @@ mod tests { use crate::{ assert_ssa_snapshot, ssa::{ + interpreter::value::Value, opt::{assert_pass_does_not_affect_execution, assert_ssa_does_not_change}, ssa_gen::Ssa, }, @@ -1917,6 +1920,105 @@ brillig(inline) fn main f0 { "); } + /// A back edge orders the block defining an array after the block using it in the CFG's + /// depth-first walk, so the sweep must still see the use first. The array here is defined in + /// the loop preheader and only used from inside the loop body. + #[test] + fn dead_reference_array_defined_before_loop_is_removed() { + let src = " + brillig(inline) fn main f0 { + b0(v0: u1): + v1 = allocate -> &mut Field + store Field 1 at v1 + v3 = make_array [v1] : [&mut Field; 1] + jmp b1() + b1(): + jmpif v0 then: b2(), else: b3() + b2(): + v4 = allocate -> &mut [&mut Field; 1] + store v3 at v4 + jmp b1() + b3(): + return + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let (ssa, _) = + assert_pass_does_not_affect_execution(ssa, vec![Value::bool(false)], Ssa::mem2reg); + assert_ssa_snapshot!(ssa, @r" + brillig(inline) fn main f0 { + b0(v0: u1): + jmp b1() + b1(): + jmpif v0 then: b2(), else: b3() + b2(): + jmp b1() + b3(): + return + } + "); + } + + /// A dead reference array built inside a loop body is removed like any other, letting the + /// reference it holds be promoted on the following iteration of the pass. + #[test] + fn dead_reference_array_inside_loop_body_is_removed() { + let src = " + brillig(inline) fn main f0 { + b0(v0: u1): + jmp b1() + b1(): + jmpif v0 then: b2(), else: b3() + b2(): + v1 = allocate -> &mut Field + store Field 1 at v1 + v3 = make_array [v1] : [&mut Field; 1] + v4 = allocate -> &mut [&mut Field; 1] + store v3 at v4 + jmp b1() + b3(): + return + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let (ssa, _) = + assert_pass_does_not_affect_execution(ssa, vec![Value::bool(false)], Ssa::mem2reg); + assert_ssa_snapshot!(ssa, @r" + brillig(inline) fn main f0 { + b0(v0: u1): + jmp b1() + b1(): + jmpif v0 then: b2(), else: b3() + b2(): + jmp b1() + b3(): + return + } + "); + } + + /// An array carried around a loop as a block argument is used by the terminators on both the + /// entry and back edges, so it is live and the reference inside it stays ineligible. + #[test] + fn reference_array_carried_across_back_edge_is_kept() { + let src = " + brillig(inline) fn main f0 { + b0(v0: u1): + v1 = allocate -> &mut Field + store Field 1 at v1 + v3 = make_array [v1] : [&mut Field; 1] + jmp b1(v3) + b1(v4: [&mut Field; 1]): + jmpif v0 then: b2(), else: b3() + b2(): + jmp b1(v4) + b3(): + return + } + "; + assert_ssa_does_not_change(src, Ssa::mem2reg); + } + /// An array of references that is actually read from keeps the references inside it aliased, /// so neither the array nor the `allocate`/`store` pair backing the reference may be dropped. #[test] From e1afea9d7d4cea51eafab59400254aa25766e379 Mon Sep 17 00:00:00 2001 From: AztecBot Date: Wed, 12 Aug 2026 12:22:54 +0000 Subject: [PATCH 4/6] update PR #13478 --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 181 ++++++++++++++++++ 1 file changed, 181 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 3edf495b0ba..40bbc892f49 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -670,8 +670,10 @@ mod tests { use crate::{ assert_ssa_snapshot, ssa::{ + SsaBuilder, SsaEvaluatorOptions, interpreter::value::Value, opt::{assert_pass_does_not_affect_execution, assert_ssa_does_not_change}, + primary_passes, ssa_gen::Ssa, }, }; @@ -2019,6 +2021,185 @@ brillig(inline) fn main f0 { assert_ssa_does_not_change(src, Ssa::mem2reg); } + /// The AST fuzzer found this program with seed `0xbc347a7e00100000`. A dead array of + /// references keeps the allocation inside it ineligible for promotion, so `mem2reg` reaches + /// its fixed point with an `allocate`/`store` pair still live. Dead instruction elimination + /// then drops the array but deliberately keeps the store, and the surviving memory operation + /// reaches ACIR generation, which rejects it. + /// + /// The program is checked in rather than the seed because the fuzzer's generator evolves: + /// replaying a seed against a later generator produces a different program, so the seed stops + /// being a reproduction while the compiler bug it found is still there. This is that seed's + /// own output, reduced to the part which fails. + /// + /// It has no equivalent in Noir source. Keeping the reference-holding array alive this far + /// into the pipeline needs a dynamic index into a vector whose elements contain references, + /// which the frontend rejects and `verify_no_dynamic_indices_to_references` re-checks; with + /// constant indices the array is optimized away early and nothing fails. + #[test] + fn dead_reference_array_from_ast_fuzzer_does_not_reach_acir() { + let src = r#" + g2 = make_array [u1 1] : [u1] + g4 = make_array [u1 1] : [u1] + acir(inline) fn main f0 { + b0(v11: [u8; 4]): + v13 = allocate -> &mut u32 + jmpif u1 1 then: b2(), else: b3() + b1(v14: u1): + return v14 + b2(): + v17 = make_array [Field 1, Field 1] : [Field; 2] + v21 = make_array b"WRD" + v25 = make_array b"VEA" + v28 = make_array b"EMN" + v29 = make_array [v21, v25, v28] : [[u8; 3]; 3] + v34 = make_array b"A" + v35 = allocate -> &mut [u8; 1] + store v34 at v35 + v37 = make_array b"A" + v38 = allocate -> &mut [u8; 1] + store v37 at v38 + v39 = make_array [v35, v38] : [&[u8; 1]; 2] + v40 = allocate -> &mut [&[u8; 1]; 2] + store v39 at v40 + v42 = make_array [v17, v29, Field 1, Field 1, Field 1, v40, v17, v29, Field 1, Field 1, Field 1, v40] : [([Field; 2], [[u8; 3]; 3], Field, Field, Field, &[&[u8; 1]; 2])] + v44 = allocate -> &mut [([Field; 2], [[u8; 3]; 3], Field, Field, Field, &[&[u8; 1]; 2])] + store v42 at v44 + v48 = load v44 -> [([Field; 2], [[u8; 3]; 3], Field, Field, Field, &[&[u8; 1]; 2])] + v51 = unchecked_mul u32 9, u32 6 + v52 = array_get v48, index v51 -> [Field; 2] + v66 = array_get v52, index u32 1 -> Field + v67 = load v13 -> u32 + v68 = call f1(v66, v67) -> [Field; 2] + v69 = array_get v68, index u32 1 -> Field + v73 = load v44 -> [([Field; 2], [[u8; 3]; 3], Field, Field, Field, &[&[u8; 1]; 2])] + v75 = unchecked_mul u32 9, u32 6 + v76 = array_get v73, index v75 -> [Field; 2] + v87 = array_get v76, index u32 1 -> Field + v88 = load v13 -> u32 + v89 = call f1(v87, v88) -> [Field; 2] + v90 = array_get v89, index u32 0 -> Field + v91 = eq v69, v90 + v92 = not v91 + jmpif v92 then: b4(), else: b5() + b3(): + jmpif u1 0 then: b17(), else: b18() + b4(): + jmp b6(i16 0) + b5(): + jmp b15() + b6(v98: i16): + jmp b15() + b15(): + v122 = array_get g2, index u32 9 -> u1 + jmp b16(v122) + b16(v123: u1): + jmp b22(v123) + b17(): + jmp b19(u1 0) + b18(): + v128 = array_get g2, index u32 9 -> u1 + jmp b20(v128) + b19(v124: u1): + jmp b21(v124) + b20(v125: u1): + jmp b21(v125) + b21(v129: u1): + jmp b22(v129) + b22(v130: u1): + jmp b1(v130) + } + brillig(inline_always) fn func_1_proxy f1 { + b0(v11: Field, v12: u32): + v13 = allocate -> &mut u32 + store v12 at v13 + v15 = call f2(v11, v13) -> [Field; 2] + return v15 + } + brillig(inline_always) fn func_1 f2 { + b0(v11: Field, v12: &mut u32): + v23 = eq v11, Field 1 + jmpif v23 then: b5(), else: b6() + b3(v81: [Field; 2]): + return v81 + b4(v21: Field): + v44 = array_get g4, index u32 9 -> u1 + v45 = not v44 + jmpif v45 then: b14(), else: b15() + b5(): + v25 = array_get g4, index u32 9 -> u1 + v26 = cast v25 as Field + v28 = array_get g4, index u32 9 -> u1 + v29 = cast v28 as Field + v30 = div v26, v29 + v32 = div v30, Field 0 + jmp b7(v32) + b6(): + v35 = eq v11, Field 1 + jmpif v35 then: b8(), else: b9() + b7(v33: Field): + jmp b13(v33) + b8(): + jmp b10(v11) + b9(): + jmp b11(v11) + b10(v36: Field): + jmp b12(v36) + b11(v37: Field): + jmp b12(v37) + b12(v38: Field): + jmp b13(v38) + b13(v39: Field): + jmp b4(v39) + b14(): + v46 = call f2(v11, v12) -> [Field; 2] + v47 = array_get v46, index u32 0 -> Field + jmp b16(v47) + b15(): + v48 = call f2(v11, v12) -> [Field; 2] + v49 = array_get v48, index u32 1 -> Field + jmp b16(v49) + b16(v50: Field): + v51 = call f2(v50, v12) -> [Field; 2] + v52 = array_get v51, index u32 0 -> Field + v53 = make_array [v21, v52] : [Field; 2] + v79 = allocate -> &mut [Field; 2] + store v53 at v79 + v80 = load v79 -> [Field; 2] + jmp b3(v80) + } + "#; + let ssa = Ssa::from_str(src).unwrap(); + let options = SsaEvaluatorOptions::default(); + let builder = SsaBuilder::from_ssa( + ssa, + options.ssa_logging.clone(), + options.ssa_logging_hide_unchanged, + false, + None, + ); + // A memory operation surviving into ACIR makes the pipeline's own post-check panic with + // "Load or Store instruction found"; the assertion below states the same invariant on the + // finished SSA. + let ssa = + builder.run_passes(&primary_passes(&options)).expect("passes should run").finish(); + + for function in ssa.functions.values().filter(|function| function.runtime().is_acir()) { + for block in function.reachable_blocks() { + for instruction_id in function.dfg[block].instructions() { + assert!( + !matches!( + function.dfg[*instruction_id], + super::Instruction::Load { .. } | super::Instruction::Store { .. } + ), + "a memory operation reached ACIR generation in {}", + function.name() + ); + } + } + } + } + /// An array of references that is actually read from keeps the references inside it aliased, /// so neither the array nor the `allocate`/`store` pair backing the reference may be dropped. #[test] From 4c55ce7e4dbd02d9a72896e826738f665602cdff Mon Sep 17 00:00:00 2001 From: AztecBot Date: Wed, 12 Aug 2026 13:05:18 +0000 Subject: [PATCH 5/6] update PR #13478 --- .../noirc_evaluator/src/ssa/opt/checks.rs | 65 ++- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 535 +----------------- .../src/ssa/opt/mutable_array_set.rs | 4 +- .../ssa/validation/dynamic_array_indices.rs | 29 + .../dynamic_array_index_references/Nargo.toml | 6 + .../Prover.toml | 1 + .../src/main.nr | 14 + .../execute__tests__stderr.snap | 16 + 8 files changed, 129 insertions(+), 541 deletions(-) create mode 100644 test_programs/compile_failure/dynamic_array_index_references/Nargo.toml create mode 100644 test_programs/compile_failure/dynamic_array_index_references/Prover.toml create mode 100644 test_programs/compile_failure/dynamic_array_index_references/src/main.nr create mode 100644 tooling/nargo_cli/tests/snapshots/compile_failure/dynamic_array_index_references/execute__tests__stderr.snap diff --git a/compiler/noirc_evaluator/src/ssa/opt/checks.rs b/compiler/noirc_evaluator/src/ssa/opt/checks.rs index 484639524ed..c94289a8d98 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/checks.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/checks.rs @@ -32,7 +32,7 @@ use crate::ssa::ir::{ /// ```ignore /// checks::for_each_instruction(function, |instruction, dfg| { /// checks::assert_not_if_else(instruction); -/// checks::assert_not_load_or_store(instruction); +/// checks::assert_not_load_or_store(function, instruction, dfg); /// }); /// ``` pub(super) fn for_each_instruction( @@ -117,10 +117,35 @@ pub(super) fn assert_not_if_else(instruction: &Instruction) { } /// Panics if the instruction is a Load or Store. -pub(super) fn assert_not_load_or_store(instruction: &Instruction) { - assert!( - !matches!(instruction, Instruction::Load { .. } | Instruction::Store { .. }), - "Load or Store instruction found" +/// +/// ACIR has no memory operations, so by this point `mem2reg` must have promoted every allocation. +/// One surviving here means it could not: something kept the address first-class past the last +/// `mem2reg` run, which is usually an aggregate that still holds the reference — an array of +/// references which is live, or one which is dead but was not recognised as such, leaving the +/// address ineligible for promotion. +/// +/// Dynamically selecting a reference out of an array is the known way to keep an address +/// first-class that far, and is reported to the user as +/// [`RuntimeError::DynamicIndexingWithReference`][crate::errors::RuntimeError::DynamicIndexingWithReference] +/// by [`verify_no_dynamic_indices_to_references`][crate::ssa::validation::dynamic_array_indices::verify_no_dynamic_indices_to_references], +/// which runs earlier in the pipeline. Anything reaching this assertion got past that check, so +/// the printed address and its type are the place to start. +pub(super) fn assert_not_load_or_store( + function: &Function, + instruction: &Instruction, + dfg: &DataFlowGraph, +) { + let (kind, address) = match instruction { + Instruction::Load { address } => ("Load", *address), + Instruction::Store { address, .. } => ("Store", *address), + _ => return, + }; + + panic!( + "{kind} instruction found in ACIR function '{}': the address {address} of type {} was not \ + promoted by mem2reg, so a memory operation reached ACIR generation", + function.name(), + dfg.type_of_value(address), ); } @@ -203,3 +228,33 @@ fn is_signed_binary_op(instruction: &Instruction, dfg: &DataFlowGraph, op: Binar false } } + +#[cfg(test)] +mod tests { + use crate::ssa::ssa_gen::Ssa; + + use super::assert_not_load_or_store; + + /// A memory operation surviving into ACIR is a compiler bug with no user-facing cause to + /// report, so the panic has to carry enough to start debugging from: which function, which + /// address, and what that address is a reference to. + #[test] + #[should_panic( + expected = "Store instruction found in ACIR function 'main': the address v0 of type &mut Field was not promoted by mem2reg" + )] + fn load_or_store_assertion_names_the_function_and_address() { + let src = " + acir(inline) fn main f0 { + b0(): + v0 = allocate -> &mut Field + store Field 1 at v0 + return + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let function = ssa.main(); + super::for_each_instruction(function, |instruction, dfg| { + assert_not_load_or_store(function, instruction, dfg); + }); + } +} diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 40bbc892f49..d75705ae55b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -89,20 +89,6 @@ impl Function { // - v0 can't be optimized out because it's stored in v2 // - v2 can and will be optimized out // - now running it again will lead to optimizing out v0, etc. - // - // The same chain can be built through an array of references: - // - // ``` - // v0 = allocate -> &mut u16 - // store u16 1 at v0 - // v2 = make_array [v0] : [&mut u16; 1] - // v3 = allocate -> &mut [&mut u16; 1] - // store v2 at v3 - // ``` - // - // Here `v0` is ineligible because it appears in `v2`, and `v2` stays alive only because it - // is stored into `v3`. Once `v3` is optimized out, `v2` has no users left, so it is dropped - // between iterations to let the next one reach `v0`. loop { let mut inserter = FunctionInserter::new(self); @@ -159,8 +145,6 @@ impl Function { // mem2reg can no longer simplify the program. break; } - - remove_unused_make_arrays(self, &blocks); } } } @@ -559,75 +543,6 @@ fn collect_eligible_variables_and_def_sites( (variables, def_sites, has_ineligible_variables) } -/// Remove every [`MakeArray`][Instruction::MakeArray] instruction whose results are unused. -/// -/// An array holding a reference makes that reference ineligible for promotion even when the array -/// itself is dead. Dropping such arrays is what lets [`Function::mem2reg`]'s loop peel one level of -/// nesting per iteration; without it the pass stops at the outermost reference cell and leaves the -/// inner `allocate`/`store` pairs behind for good, since removing a dead store needs the alias -/// information only mem2reg has and no later pass in the pipeline revisits them. -/// -/// Only `MakeArray` is considered: it is side-effect free and, unlike an array read, carries no -/// out-of-bounds check that would have to be preserved in its place. -/// -/// Like [`Ssa::dead_instruction_elimination`], blocks are visited in post order and their -/// instructions in reverse, so a whole chain of nested dead arrays is removed in one traversal: -/// every use of a result has been seen by the time its defining instruction is reached. Back -/// edges do not disturb this. A use is always dominated by its definition, and the post-order is -/// the reverse of a topological order of the CFG with back edges removed, so a block is always -/// visited before any block dominating it. -fn remove_unused_make_arrays(function: &mut Function, blocks: &[BasicBlockId]) { - // Databus arrays are roots alongside the instructions and terminators: they are named by the - // function's ABI rather than by anything in the instruction stream. - let mut used_values: HashSet = function - .dfg - .data_bus - .call_data - .iter() - .map(|call_data| call_data.array_id) - .chain(function.dfg.data_bus.return_data) - .collect(); - - let mut instructions_to_remove = HashSet::default(); - - // `blocks` is in reverse post order, so iterating it backwards gives post order. - for block in blocks.iter().rev().copied() { - function.dfg[block].unwrap_terminator().for_each_value(|value| { - used_values.insert(value); - }); - - for instruction_id in function.dfg[block].instructions().iter().rev() { - let instruction = &function.dfg[*instruction_id]; - let is_unused_array = matches!(instruction, Instruction::MakeArray { .. }) - && function - .dfg - .instruction_results(*instruction_id) - .iter() - .all(|result| !used_values.contains(result)); - - if is_unused_array { - // Leaving the array's elements out of `used_values` is what exposes an array whose - // only use is another dead array. - instructions_to_remove.insert(*instruction_id); - } else { - instruction.for_each_value(|value| { - used_values.insert(value); - }); - } - } - } - - if instructions_to_remove.is_empty() { - return; - } - - for block in blocks.iter().copied() { - function.dfg[block] - .instructions_mut() - .retain(|instruction_id| !instructions_to_remove.contains(instruction_id)); - } -} - /// Commit to all changes made by the pass: /// - Map any values mapped from the inserter to their new values in the function /// - Remove all Allocate, Load, and Store instructions from the eligible variables @@ -669,13 +584,7 @@ fn commit( mod tests { use crate::{ assert_ssa_snapshot, - ssa::{ - SsaBuilder, SsaEvaluatorOptions, - interpreter::value::Value, - opt::{assert_pass_does_not_affect_execution, assert_ssa_does_not_change}, - primary_passes, - ssa_gen::Ssa, - }, + ssa::{opt::assert_ssa_does_not_change, ssa_gen::Ssa}, }; #[test] @@ -1836,446 +1745,4 @@ brillig(inline) fn main f0 { super::get_value_from_visited_predecessor(dummy_var, b1, &cfg, &block_states).is_none() ); } - - /// A reference stored into an array that is itself only kept alive by a store to a dead - /// reference cell must still be promoted. Nothing later in the pipeline removes dead stores, - /// so an `allocate`/`store` pair surviving here reaches `mutable_array_set_optimization` and - /// ACIR generation, neither of which accepts memory operations. - #[test] - fn dead_reference_array_does_not_block_promotion() { - let src = " - acir(inline) fn main f0 { - b0(): - v0 = allocate -> &mut Field - store Field 1 at v0 - v2 = make_array [v0] : [&mut Field; 1] - v3 = allocate -> &mut [&mut Field; 1] - store v2 at v3 - return - } - "; - let ssa = Ssa::from_str(src).unwrap(); - let (ssa, _) = assert_pass_does_not_affect_execution(ssa, vec![], Ssa::mem2reg); - assert_ssa_snapshot!(ssa, @r" - acir(inline) fn main f0 { - b0(): - return - } - "); - } - - /// Each level of reference nesting is peeled by one iteration of the pass, so a chain deeper - /// than the one in `dead_reference_array_does_not_block_promotion` must also be fully removed - /// by a single `mem2reg` run. - #[test] - fn nested_dead_reference_arrays_are_fully_promoted() { - let src = " - acir(inline) fn main f0 { - b0(): - v0 = allocate -> &mut Field - store Field 1 at v0 - v2 = make_array [v0] : [&mut Field; 1] - v3 = allocate -> &mut [&mut Field; 1] - store v2 at v3 - v5 = make_array [v3] : [&mut [&mut Field; 1]; 1] - v6 = allocate -> &mut [&mut [&mut Field; 1]; 1] - store v5 at v6 - return - } - "; - let ssa = Ssa::from_str(src).unwrap(); - let (ssa, _) = assert_pass_does_not_affect_execution(ssa, vec![], Ssa::mem2reg); - assert_ssa_snapshot!(ssa, @r" - acir(inline) fn main f0 { - b0(): - return - } - "); - } - - /// The array and the reference cell keeping it alive need not sit in the same block, so a dead - /// array must also be found when its only use is in a successor block. - #[test] - fn dead_reference_array_used_in_another_block_is_removed() { - let src = " - acir(inline) fn main f0 { - b0(): - v0 = allocate -> &mut Field - store Field 1 at v0 - v2 = make_array [v0] : [&mut Field; 1] - jmp b1() - b1(): - v3 = allocate -> &mut [&mut Field; 1] - store v2 at v3 - return - } - "; - let ssa = Ssa::from_str(src).unwrap(); - let (ssa, _) = assert_pass_does_not_affect_execution(ssa, vec![], Ssa::mem2reg); - assert_ssa_snapshot!(ssa, @r" - acir(inline) fn main f0 { - b0(): - jmp b1() - b1(): - return - } - "); - } - - /// A back edge orders the block defining an array after the block using it in the CFG's - /// depth-first walk, so the sweep must still see the use first. The array here is defined in - /// the loop preheader and only used from inside the loop body. - #[test] - fn dead_reference_array_defined_before_loop_is_removed() { - let src = " - brillig(inline) fn main f0 { - b0(v0: u1): - v1 = allocate -> &mut Field - store Field 1 at v1 - v3 = make_array [v1] : [&mut Field; 1] - jmp b1() - b1(): - jmpif v0 then: b2(), else: b3() - b2(): - v4 = allocate -> &mut [&mut Field; 1] - store v3 at v4 - jmp b1() - b3(): - return - } - "; - let ssa = Ssa::from_str(src).unwrap(); - let (ssa, _) = - assert_pass_does_not_affect_execution(ssa, vec![Value::bool(false)], Ssa::mem2reg); - assert_ssa_snapshot!(ssa, @r" - brillig(inline) fn main f0 { - b0(v0: u1): - jmp b1() - b1(): - jmpif v0 then: b2(), else: b3() - b2(): - jmp b1() - b3(): - return - } - "); - } - - /// A dead reference array built inside a loop body is removed like any other, letting the - /// reference it holds be promoted on the following iteration of the pass. - #[test] - fn dead_reference_array_inside_loop_body_is_removed() { - let src = " - brillig(inline) fn main f0 { - b0(v0: u1): - jmp b1() - b1(): - jmpif v0 then: b2(), else: b3() - b2(): - v1 = allocate -> &mut Field - store Field 1 at v1 - v3 = make_array [v1] : [&mut Field; 1] - v4 = allocate -> &mut [&mut Field; 1] - store v3 at v4 - jmp b1() - b3(): - return - } - "; - let ssa = Ssa::from_str(src).unwrap(); - let (ssa, _) = - assert_pass_does_not_affect_execution(ssa, vec![Value::bool(false)], Ssa::mem2reg); - assert_ssa_snapshot!(ssa, @r" - brillig(inline) fn main f0 { - b0(v0: u1): - jmp b1() - b1(): - jmpif v0 then: b2(), else: b3() - b2(): - jmp b1() - b3(): - return - } - "); - } - - /// An array carried around a loop as a block argument is used by the terminators on both the - /// entry and back edges, so it is live and the reference inside it stays ineligible. - #[test] - fn reference_array_carried_across_back_edge_is_kept() { - let src = " - brillig(inline) fn main f0 { - b0(v0: u1): - v1 = allocate -> &mut Field - store Field 1 at v1 - v3 = make_array [v1] : [&mut Field; 1] - jmp b1(v3) - b1(v4: [&mut Field; 1]): - jmpif v0 then: b2(), else: b3() - b2(): - jmp b1(v4) - b3(): - return - } - "; - assert_ssa_does_not_change(src, Ssa::mem2reg); - } - - /// The AST fuzzer found this program with seed `0xbc347a7e00100000`. A dead array of - /// references keeps the allocation inside it ineligible for promotion, so `mem2reg` reaches - /// its fixed point with an `allocate`/`store` pair still live. Dead instruction elimination - /// then drops the array but deliberately keeps the store, and the surviving memory operation - /// reaches ACIR generation, which rejects it. - /// - /// The program is checked in rather than the seed because the fuzzer's generator evolves: - /// replaying a seed against a later generator produces a different program, so the seed stops - /// being a reproduction while the compiler bug it found is still there. This is that seed's - /// own output, reduced to the part which fails. - /// - /// It has no equivalent in Noir source. Keeping the reference-holding array alive this far - /// into the pipeline needs a dynamic index into a vector whose elements contain references, - /// which the frontend rejects and `verify_no_dynamic_indices_to_references` re-checks; with - /// constant indices the array is optimized away early and nothing fails. - #[test] - fn dead_reference_array_from_ast_fuzzer_does_not_reach_acir() { - let src = r#" - g2 = make_array [u1 1] : [u1] - g4 = make_array [u1 1] : [u1] - acir(inline) fn main f0 { - b0(v11: [u8; 4]): - v13 = allocate -> &mut u32 - jmpif u1 1 then: b2(), else: b3() - b1(v14: u1): - return v14 - b2(): - v17 = make_array [Field 1, Field 1] : [Field; 2] - v21 = make_array b"WRD" - v25 = make_array b"VEA" - v28 = make_array b"EMN" - v29 = make_array [v21, v25, v28] : [[u8; 3]; 3] - v34 = make_array b"A" - v35 = allocate -> &mut [u8; 1] - store v34 at v35 - v37 = make_array b"A" - v38 = allocate -> &mut [u8; 1] - store v37 at v38 - v39 = make_array [v35, v38] : [&[u8; 1]; 2] - v40 = allocate -> &mut [&[u8; 1]; 2] - store v39 at v40 - v42 = make_array [v17, v29, Field 1, Field 1, Field 1, v40, v17, v29, Field 1, Field 1, Field 1, v40] : [([Field; 2], [[u8; 3]; 3], Field, Field, Field, &[&[u8; 1]; 2])] - v44 = allocate -> &mut [([Field; 2], [[u8; 3]; 3], Field, Field, Field, &[&[u8; 1]; 2])] - store v42 at v44 - v48 = load v44 -> [([Field; 2], [[u8; 3]; 3], Field, Field, Field, &[&[u8; 1]; 2])] - v51 = unchecked_mul u32 9, u32 6 - v52 = array_get v48, index v51 -> [Field; 2] - v66 = array_get v52, index u32 1 -> Field - v67 = load v13 -> u32 - v68 = call f1(v66, v67) -> [Field; 2] - v69 = array_get v68, index u32 1 -> Field - v73 = load v44 -> [([Field; 2], [[u8; 3]; 3], Field, Field, Field, &[&[u8; 1]; 2])] - v75 = unchecked_mul u32 9, u32 6 - v76 = array_get v73, index v75 -> [Field; 2] - v87 = array_get v76, index u32 1 -> Field - v88 = load v13 -> u32 - v89 = call f1(v87, v88) -> [Field; 2] - v90 = array_get v89, index u32 0 -> Field - v91 = eq v69, v90 - v92 = not v91 - jmpif v92 then: b4(), else: b5() - b3(): - jmpif u1 0 then: b17(), else: b18() - b4(): - jmp b6(i16 0) - b5(): - jmp b15() - b6(v98: i16): - jmp b15() - b15(): - v122 = array_get g2, index u32 9 -> u1 - jmp b16(v122) - b16(v123: u1): - jmp b22(v123) - b17(): - jmp b19(u1 0) - b18(): - v128 = array_get g2, index u32 9 -> u1 - jmp b20(v128) - b19(v124: u1): - jmp b21(v124) - b20(v125: u1): - jmp b21(v125) - b21(v129: u1): - jmp b22(v129) - b22(v130: u1): - jmp b1(v130) - } - brillig(inline_always) fn func_1_proxy f1 { - b0(v11: Field, v12: u32): - v13 = allocate -> &mut u32 - store v12 at v13 - v15 = call f2(v11, v13) -> [Field; 2] - return v15 - } - brillig(inline_always) fn func_1 f2 { - b0(v11: Field, v12: &mut u32): - v23 = eq v11, Field 1 - jmpif v23 then: b5(), else: b6() - b3(v81: [Field; 2]): - return v81 - b4(v21: Field): - v44 = array_get g4, index u32 9 -> u1 - v45 = not v44 - jmpif v45 then: b14(), else: b15() - b5(): - v25 = array_get g4, index u32 9 -> u1 - v26 = cast v25 as Field - v28 = array_get g4, index u32 9 -> u1 - v29 = cast v28 as Field - v30 = div v26, v29 - v32 = div v30, Field 0 - jmp b7(v32) - b6(): - v35 = eq v11, Field 1 - jmpif v35 then: b8(), else: b9() - b7(v33: Field): - jmp b13(v33) - b8(): - jmp b10(v11) - b9(): - jmp b11(v11) - b10(v36: Field): - jmp b12(v36) - b11(v37: Field): - jmp b12(v37) - b12(v38: Field): - jmp b13(v38) - b13(v39: Field): - jmp b4(v39) - b14(): - v46 = call f2(v11, v12) -> [Field; 2] - v47 = array_get v46, index u32 0 -> Field - jmp b16(v47) - b15(): - v48 = call f2(v11, v12) -> [Field; 2] - v49 = array_get v48, index u32 1 -> Field - jmp b16(v49) - b16(v50: Field): - v51 = call f2(v50, v12) -> [Field; 2] - v52 = array_get v51, index u32 0 -> Field - v53 = make_array [v21, v52] : [Field; 2] - v79 = allocate -> &mut [Field; 2] - store v53 at v79 - v80 = load v79 -> [Field; 2] - jmp b3(v80) - } - "#; - let ssa = Ssa::from_str(src).unwrap(); - let options = SsaEvaluatorOptions::default(); - let builder = SsaBuilder::from_ssa( - ssa, - options.ssa_logging.clone(), - options.ssa_logging_hide_unchanged, - false, - None, - ); - // A memory operation surviving into ACIR makes the pipeline's own post-check panic with - // "Load or Store instruction found"; the assertion below states the same invariant on the - // finished SSA. - let ssa = - builder.run_passes(&primary_passes(&options)).expect("passes should run").finish(); - - for function in ssa.functions.values().filter(|function| function.runtime().is_acir()) { - for block in function.reachable_blocks() { - for instruction_id in function.dfg[block].instructions() { - assert!( - !matches!( - function.dfg[*instruction_id], - super::Instruction::Load { .. } | super::Instruction::Store { .. } - ), - "a memory operation reached ACIR generation in {}", - function.name() - ); - } - } - } - } - - /// An array of references that is actually read from keeps the references inside it aliased, - /// so neither the array nor the `allocate`/`store` pair backing the reference may be dropped. - #[test] - fn live_reference_array_still_prevents_optimization() { - let src = " - brillig(inline) fn func f0 { - b0(): - v0 = allocate -> &mut Field - store Field 1 at v0 - v2 = make_array [v0] : [&mut Field; 1] - v3 = array_get v2, index u32 0 -> &mut Field - v4 = load v3 -> Field - return v4 - } - "; - assert_ssa_does_not_change(src, Ssa::mem2reg); - } - - /// A reference count instruction is a use of the array it names, so an array that is only - /// reachable through `inc_rc` must be kept: dropping it would leave the `inc_rc` dangling. - #[test] - fn make_array_used_by_rc_instruction_is_kept() { - let src = " - brillig(inline) fn func f0 { - b0(): - v0 = allocate -> &mut Field - store Field 1 at v0 - v2 = make_array [v0] : [&mut Field; 1] - inc_rc v2 - v3 = allocate -> &mut [&mut Field; 1] - store v2 at v3 - return - } - "; - let ssa = Ssa::from_str(src).unwrap(); - let (ssa, _) = assert_pass_does_not_affect_execution(ssa, vec![], Ssa::mem2reg); - assert_ssa_snapshot!(ssa, @r" - brillig(inline) fn func f0 { - b0(): - v0 = allocate -> &mut Field - store Field 1 at v0 - v2 = make_array [v0] : [&mut Field; 1] - inc_rc v2 - return - } - "); - } - - /// A databus array is named by the function's ABI, not by any instruction, so it has no - /// instruction users at all. It must survive the sweep that drops arrays which became dead - /// once the reference cell holding them was promoted. - #[test] - fn databus_arrays_are_kept() { - let src = " - acir(inline) fn main f0 { - call_data(0): array: v1, indices: [] - return_data: v3 - b0(v0: Field): - v1 = make_array [v0] : [Field; 1] - v2 = allocate -> &mut [Field; 1] - store v1 at v2 - v3 = make_array [v0] : [Field; 1] - return v3 - } - "; - let ssa = Ssa::from_str(src).unwrap(); - let ssa = ssa.mem2reg(); - assert_ssa_snapshot!(ssa, @r" - acir(inline) fn main f0 { - call_data(0): array: v1, indices: [] - return_data: v2 - b0(v0: Field): - v1 = make_array [v0] : [Field; 1] - v2 = make_array [v0] : [Field; 1] - return v2 - } - "); - } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/mutable_array_set.rs b/compiler/noirc_evaluator/src/ssa/opt/mutable_array_set.rs index a6b1114d795..7a785aa6b9d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mutable_array_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mutable_array_set.rs @@ -64,11 +64,11 @@ fn mutable_array_set_optimization_pre_check(func: &Function) { // flatten_cfg must have run super::checks::assert_cfg_is_flattened(func); - super::checks::for_each_instruction(func, |instruction, _dfg| { + super::checks::for_each_instruction(func, |instruction, dfg| { // remove_if_else must have run super::checks::assert_not_if_else(instruction); // mem2reg must have run (no Load/Store remaining) - super::checks::assert_not_load_or_store(instruction); + super::checks::assert_not_load_or_store(func, instruction, dfg); // No mutable array sets should exist yet (they are created by this pass) super::checks::assert_not_mutable_array_set(instruction); }); diff --git a/compiler/noirc_evaluator/src/ssa/validation/dynamic_array_indices.rs b/compiler/noirc_evaluator/src/ssa/validation/dynamic_array_indices.rs index 0ec0ad43e56..016bf9d1111 100644 --- a/compiler/noirc_evaluator/src/ssa/validation/dynamic_array_indices.rs +++ b/compiler/noirc_evaluator/src/ssa/validation/dynamic_array_indices.rs @@ -117,6 +117,35 @@ mod tests { assert!(matches!(result, Err(RuntimeError::DynamicIndexingWithReference { .. }))); } + #[test] + fn dynamic_array_set_of_references() { + // Writing a reference into an array at a dynamic index selects which reference the array + // ends up aliasing, so it is as unresolvable before ACIR generation as reading one out. + // fn main(c: u32) -> pub bool { + // let mut x = false; + // let mut b: [&mut bool; 2] = [&mut false, &mut true]; + // b[c] = &mut x; + // *b[0] + // } + let src = r#" + acir(inline) predicate_pure fn main f0 { + b0(v0: u32): + v1 = allocate -> &mut u1 + v2 = allocate -> &mut u1 + store u1 0 at v1 + store u1 1 at v2 + v3 = make_array [v1, v2] : [&mut u1; 2] + v4 = array_set v3, index v0, value v1 + v5 = array_get v4, index u32 0 -> &mut u1 + v6 = load v5 -> u1 + return v6 + }"#; + + let ssa = Ssa::from_str(src).unwrap(); + let result = verify_no_dynamic_indices_to_references(&ssa); + assert!(matches!(result, Err(RuntimeError::DynamicIndexingWithReference { .. }))); + } + #[test] fn no_error_in_brillig() { // unconstrained fn main(c: u32) -> pub bool { diff --git a/test_programs/compile_failure/dynamic_array_index_references/Nargo.toml b/test_programs/compile_failure/dynamic_array_index_references/Nargo.toml new file mode 100644 index 00000000000..b0ec3cde9b8 --- /dev/null +++ b/test_programs/compile_failure/dynamic_array_index_references/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "dynamic_array_index_references" +type = "bin" +authors = [""] + +[dependencies] diff --git a/test_programs/compile_failure/dynamic_array_index_references/Prover.toml b/test_programs/compile_failure/dynamic_array_index_references/Prover.toml new file mode 100644 index 00000000000..bfa1f5e58ca --- /dev/null +++ b/test_programs/compile_failure/dynamic_array_index_references/Prover.toml @@ -0,0 +1 @@ +i = "1" diff --git a/test_programs/compile_failure/dynamic_array_index_references/src/main.nr b/test_programs/compile_failure/dynamic_array_index_references/src/main.nr new file mode 100644 index 00000000000..76dae7fa655 --- /dev/null +++ b/test_programs/compile_failure/dynamic_array_index_references/src/main.nr @@ -0,0 +1,14 @@ +// Reading an array of references at a dynamic index is a dynamic selection of a reference, which +// cannot be resolved before ACIR generation. Rejecting it here is what keeps the address out of +// ACIR: were it to survive, mem2reg would have no way to promote the allocation behind it and a +// later pass would hit its "no memory operations in ACIR" assertion instead of reporting anything +// a user can act on. +fn main(i: u32) -> pub Field { + let mut x = 10; + let mut y = 20; + + let r: [&mut Field; 2] = [&mut x, &mut y]; + + y = 77; + *r[i] +} diff --git a/tooling/nargo_cli/tests/snapshots/compile_failure/dynamic_array_index_references/execute__tests__stderr.snap b/tooling/nargo_cli/tests/snapshots/compile_failure/dynamic_array_index_references/execute__tests__stderr.snap new file mode 100644 index 00000000000..299759cf137 --- /dev/null +++ b/tooling/nargo_cli/tests/snapshots/compile_failure/dynamic_array_index_references/execute__tests__stderr.snap @@ -0,0 +1,16 @@ +--- +source: tooling/nargo_cli/tests/execute.rs +expression: stderr +--- +--- The SSA failed to validate after 'Dead Instruction Elimination': Set the NOIR_SHOW_INVALID_SSA env var to see the SSA. +error: Only constant indices are supported when indexing an array containing reference values + ┌─ src/main.nr:13:6 + │ +13 │ *r[i] + │ ---- + │ + = Call stack: + 1: main + at src/main.nr:13:6 + +Aborting due to 1 previous error From 8a7602f89fdafc1c8d3f580cb50a52745d29ec78 Mon Sep 17 00:00:00 2001 From: AztecBot Date: Wed, 12 Aug 2026 13:51:05 +0000 Subject: [PATCH 6/6] update PR #13478 --- tooling/ast_fuzzer/src/program/func.rs | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/tooling/ast_fuzzer/src/program/func.rs b/tooling/ast_fuzzer/src/program/func.rs index e20191855b2..d2331ec7dca 100644 --- a/tooling/ast_fuzzer/src/program/func.rs +++ b/tooling/ast_fuzzer/src/program/func.rs @@ -991,14 +991,24 @@ impl<'a> FunctionContext<'a> { let len_expr = self.call_array_len(Expression::Ident(ident_1), src_type.clone()); // The rules around dynamic indexing is the same as for arrays. + let no_dynamic = + self.in_no_dynamic || !self.unconstrained() && types::contains_reference(item_type); + let was_in_no_dynamic = std::mem::replace(&mut self.in_no_dynamic, no_dynamic); + let (idx_expr, idx_dyn) = if max_depth == 0 || bool::arbitrary(u)? { // Avoid any stack overflow where we look for an index in the vector itself. - (self.gen_literal(u, &types::U32)?, false) - } else { - let no_dynamic = - self.in_no_dynamic || !self.unconstrained() && types::contains_reference(item_type); - let was_in_no_dynamic = std::mem::replace(&mut self.in_no_dynamic, no_dynamic); + let mut idx_expr = self.gen_literal(u, &types::U32)?; + + // A literal index is bounded here rather than by the caller, because a vector's + // length is not known at compile time: unlike an array, an out-of-bounds constant + // is not rejected during compilation, and reaches ACIR generation as an index which + // could not be simplified away. + if self.avoid_index_out_of_bounds(u)? { + idx_expr = expr::modulo(idx_expr, len_expr); + } + (idx_expr, false) + } else { // Choose a random index. let (mut idx_expr, idx_dyn) = self.gen_expr(u, &types::U32, max_depth.saturating_sub(1), Flags::NESTED)?; @@ -1010,10 +1020,11 @@ impl<'a> FunctionContext<'a> { idx_expr = expr::modulo(idx_expr, len_expr); } - self.in_no_dynamic = was_in_no_dynamic; (idx_expr, idx_dyn) }; + self.in_no_dynamic = was_in_no_dynamic; + // Access the item by index let item_expr = access_item(self, ident_2, idx_expr);