From 68bc5e9bb521263c33bcbf3b3918d2543ebb8cb2 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 1 Jul 2025 10:30:40 +0100 Subject: [PATCH 1/4] Move DIE post-check to the end --- compiler/noirc_evaluator/src/ssa.rs | 8 ++++++- compiler/noirc_evaluator/src/ssa/opt/die.rs | 25 ++++++++++++--------- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index eae0928efb6..f5c72504319 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -245,7 +245,13 @@ pub fn primary_passes(options: &SsaEvaluatorOptions) -> Vec { SsaPass::new_try( Ssa::verify_no_dynamic_indices_to_references, "Verifying no dynamic array indices to reference value elements", - ), + ) + .and_then(|ssa| { + // Deferred sanity checks that don't modify the SSA, just panic if we have something unexpected + // that we don't know how to attribute to a concrete error with the Noir code. + ssa.dead_instruction_elimination_post_check(true); + ssa + }), ] } diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 68d8bb9028d..e23e5556ab6 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -54,14 +54,6 @@ impl Ssa { flattened: bool, skip_brillig: bool, ) -> Ssa { - // Perform post-checks on the SSA. - let check = |ssa: Ssa| { - // Check that we have established the properties expected from this pass. - #[cfg(debug_assertions)] - ssa.functions.values().for_each(|f| die_post_check(f, flattened)); - ssa - }; - let mut previous_unused_params = None; loop { let (new_ssa, result) = @@ -75,13 +67,13 @@ impl Ssa { // If there are no unused parameters, return early if !has_unused { - return check(new_ssa); + return new_ssa; } if let Some(previous) = &previous_unused_params { // If no changes to dead parameters occurred, return early if previous == &result.unused_parameters { - return check(new_ssa); + return new_ssa; } } @@ -114,6 +106,19 @@ impl Ssa { (self, result) } + + /// Sanity check on the final SSA, panicking if the assumptions don't hold. + /// + /// Done as a separate step so that we can put it after other passes which provide + /// concrete feedback about where the problem with the Noir code might be, such as + /// dynamic indexing of arrays with references in ACIR. We can look up the callstack + /// of the offending instruction here as well, it's just not clear what error message + /// to say, besides the fact that mem2reg was unable to eliminate something. + #[cfg_attr(not(debug_assertions), allow(unused_variables))] + pub(crate) fn dead_instruction_elimination_post_check(&self, flattened: bool) { + #[cfg(debug_assertions)] + self.functions.values().for_each(|f| die_post_check(f, flattened)); + } } impl Function { From 9d8da7ca763969c23be71c84c0d4c7c658e6afbd Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 1 Jul 2025 10:56:44 +0100 Subject: [PATCH 2/4] Do not use arrays with references under dynamic input regions --- tooling/ast_fuzzer/src/program/func.rs | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/tooling/ast_fuzzer/src/program/func.rs b/tooling/ast_fuzzer/src/program/func.rs index 71e138a3abf..1b00d70d21f 100644 --- a/tooling/ast_fuzzer/src/program/func.rs +++ b/tooling/ast_fuzzer/src/program/func.rs @@ -196,6 +196,9 @@ pub(super) struct FunctionContext<'a> { in_loop: bool, /// Indicator of computing an expression that should not contain dynamic input. in_no_dynamic: bool, + /// Indicator of being affected by dynamic input, in which case we should refrain + /// from using expression that requires no-dynamic mode. + in_dynamic: bool, /// All the functions callable from this one, with the types we can /// produce from their return value. call_targets: BTreeMap>, @@ -261,6 +264,7 @@ impl<'a> FunctionContext<'a> { dynamics, in_loop: false, in_no_dynamic: false, + in_dynamic: false, call_targets, next_ident_id: 0, has_call: false, @@ -337,6 +341,11 @@ impl<'a> FunctionContext<'a> { self.dynamics.contains(id) } + /// Check if a type can be used inside a dynamic input context. + fn can_be_used_in_dynamic(&self, typ: &Type) -> bool { + self.unconstrained() || !(types::contains_reference(typ) && types::is_array_or_slice(typ)) + } + /// Choose a producer for a type, preferring local variables over global ones. fn choose_producer( &self, @@ -345,10 +354,11 @@ impl<'a> FunctionContext<'a> { ) -> arbitrary::Result> { // Check if we have something that produces this exact type. if u.ratio(7, 10)? { - let producer = if self.in_no_dynamic { - self.locals - .current() - .choose_producer_filtered(u, typ, |id, _| !self.is_dynamic(id))? + let producer = if self.in_no_dynamic || self.in_dynamic { + self.locals.current().choose_producer_filtered(u, typ, |id, _| { + (!self.in_no_dynamic || !self.is_dynamic(id)) + && (!self.in_dynamic || self.can_be_used_in_dynamic(typ)) + })? } else { self.locals.current().choose_producer(u, typ)? }; @@ -369,6 +379,7 @@ impl<'a> FunctionContext<'a> { *mutable && (typ.as_ref() == prod || !types::is_array_or_slice(prod)) && (!self.in_no_dynamic || !self.is_dynamic(id)) + && (!self.in_dynamic || self.can_be_used_in_dynamic(typ.as_ref())) }) .map(|id| id.map(VariableId::Local)); } @@ -1116,6 +1127,12 @@ impl<'a> FunctionContext<'a> { let (cond, cond_dyn) = self.gen_expr(u, &Type::Bool, max_depth, Flags::CONDITION)?; + // If the `if` condition uses dynamic input, we cannot access certain constructs in the body in ACIR. + // Note that this would be the case for `while` and `for` as well, however `while` is not used in ACIR, + // and `for` has its own restriction of having to have compile-time boundaries, so this is only done where necessary. + let in_dynamic = self.in_dynamic || cond_dyn; + let was_in_dynamic = std::mem::replace(&mut self.in_dynamic, in_dynamic); + let (cons, cons_dyn) = { if flags.allow_blocks { self.gen_block(u, typ)? @@ -1136,6 +1153,7 @@ impl<'a> FunctionContext<'a> { Some(expr) }; + self.in_dynamic = was_in_dynamic; let alt_dyn = alt.as_ref().is_some_and(|(_, d)| *d); let is_dyn = cond_dyn || cons_dyn || alt_dyn; From 05b8fa370dbfc0a23ea6c735ee01d2f4fb4af1c4 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 1 Jul 2025 11:12:19 +0100 Subject: [PATCH 3/4] Comment --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index e23e5556ab6..077636910e7 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -113,7 +113,7 @@ impl Ssa { /// concrete feedback about where the problem with the Noir code might be, such as /// dynamic indexing of arrays with references in ACIR. We can look up the callstack /// of the offending instruction here as well, it's just not clear what error message - /// to say, besides the fact that mem2reg was unable to eliminate something. + /// to return, besides the fact that mem2reg was unable to eliminate something. #[cfg_attr(not(debug_assertions), allow(unused_variables))] pub(crate) fn dead_instruction_elimination_post_check(&self, flattened: bool) { #[cfg(debug_assertions)] From 55bafd08587d3456571cbaa8fd22b62ccc5aefc7 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 1 Jul 2025 12:51:14 +0100 Subject: [PATCH 4/4] Also filter in gen_expr_from_source --- tooling/ast_fuzzer/src/program/func.rs | 38 ++++++++++++++++++-------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/tooling/ast_fuzzer/src/program/func.rs b/tooling/ast_fuzzer/src/program/func.rs index 1b00d70d21f..71ee6306b64 100644 --- a/tooling/ast_fuzzer/src/program/func.rs +++ b/tooling/ast_fuzzer/src/program/func.rs @@ -341,9 +341,14 @@ impl<'a> FunctionContext<'a> { self.dynamics.contains(id) } - /// Check if a type can be used inside a dynamic input context. - fn can_be_used_in_dynamic(&self, typ: &Type) -> bool { - self.unconstrained() || !(types::contains_reference(typ) && types::is_array_or_slice(typ)) + /// Check if a source type can be used inside a dynamic input context to produce some target type. + fn can_be_used_in_dynamic(&self, src_type: &Type, tgt_type: &Type) -> bool { + // Dynamic inputs are restricted only in ACIR + self.unconstrained() + // If we are looking for the exact type, it's okay. + || src_type == tgt_type + // But we can't index an array with references. + || !(types::is_array_or_slice(src_type) && types::contains_reference(src_type) ) } /// Choose a producer for a type, preferring local variables over global ones. @@ -355,10 +360,14 @@ impl<'a> FunctionContext<'a> { // Check if we have something that produces this exact type. if u.ratio(7, 10)? { let producer = if self.in_no_dynamic || self.in_dynamic { - self.locals.current().choose_producer_filtered(u, typ, |id, _| { - (!self.in_no_dynamic || !self.is_dynamic(id)) - && (!self.in_dynamic || self.can_be_used_in_dynamic(typ)) - })? + self.locals.current().choose_producer_filtered( + u, + typ, + |id, (_, _, producer_type)| { + (!self.in_no_dynamic || !self.is_dynamic(id)) + && (!self.in_dynamic || self.can_be_used_in_dynamic(producer_type, typ)) + }, + )? } else { self.locals.current().choose_producer(u, typ)? }; @@ -375,11 +384,13 @@ impl<'a> FunctionContext<'a> { return self .locals .current() - .choose_producer_filtered(u, typ.as_ref(), |id, (mutable, _, prod)| { + .choose_producer_filtered(u, typ.as_ref(), |id, (mutable, _, producer_type)| { *mutable - && (typ.as_ref() == prod || !types::is_array_or_slice(prod)) + && (typ.as_ref() == producer_type + || !types::is_array_or_slice(producer_type)) && (!self.in_no_dynamic || !self.is_dynamic(id)) - && (!self.in_dynamic || self.can_be_used_in_dynamic(typ.as_ref())) + && (!self.in_dynamic + || self.can_be_used_in_dynamic(producer_type, typ.as_ref())) }) .map(|id| id.map(VariableId::Local)); } @@ -585,7 +596,12 @@ impl<'a> FunctionContext<'a> { return Ok(Some((src_expr, src_dyn))); } - // Mutable references to array elements are currently unsupported + // Some types cannot be accessed in certain contexts. + if self.in_dynamic && !self.can_be_used_in_dynamic(src_type, tgt_type) { + return Ok(None); + } + + // Mutable references to array elements are currently unsupported. if types::is_array_or_slice(src_type) { src_mutable = false; }