diff --git a/compiler/noirc_evaluator/src/acir/arrays.rs b/compiler/noirc_evaluator/src/acir/arrays.rs index 0474b38b551..d3849564031 100644 --- a/compiler/noirc_evaluator/src/acir/arrays.rs +++ b/compiler/noirc_evaluator/src/acir/arrays.rs @@ -297,10 +297,6 @@ impl Context<'_> { index: ValueId, store_value: Option, ) -> Result { - if !self.acir_context.is_constant_zero(&self.current_side_effects_enabled_var) { - return Ok(false); - } - // The side-effects predicate is only this instruction's own for the instructions // [`crate::ssa::opt::remove_enable_side_effects`] fences, that is those reporting // `Instruction::requires_acir_gen_predicate == true`. A read at a statically safe index @@ -311,10 +307,18 @@ impl Context<'_> { // Resolving it as disabled is also unnecessary. A safe index is in bounds by construction, // so it never needs the predicate's fallback to a valid slot and the ordinary path emits // exactly the read the program asked for. + // + // This check runs before the predicate is inspected: a safe read's outcome here is + // "not handled" regardless of the predicate's value, so it is not a predicate read. if store_value.is_none() && dfg.is_safe_index(index, array) { return Ok(false); } + let predicate = self.read_predicate(); + if !self.acir_context.is_constant_zero(&predicate) { + return Ok(false); + } + let value = if store_value.is_some() { self.convert_value(array, dfg) } else { @@ -351,7 +355,8 @@ impl Context<'_> { } // Make sure this code is disabled, or fail with "Index out of bounds". let msg = "Index out of bounds, array has size 0".to_string(); - self.acir_context.assert_zero_var(self.current_side_effects_enabled_var, msg)?; + let predicate = self.read_predicate(); + self.acir_context.assert_zero_var(predicate, msg)?; Ok(true) } @@ -380,11 +385,27 @@ impl Context<'_> { call_stack: self.acir_context.get_call_stack(), })) } - AcirValue::Array(array) => { + AcirValue::Array(array_value) => { // `AcirValue::Array` supports reading/writing to constant indices at compile-time in some cases. if let Some(constant_index) = self.constant_index(index, dfg)? { - let store_value = store_value.map(|value| self.convert_value(value, dfg)); - self.handle_constant_index(instruction, dfg, array, constant_index, store_value) + let store = store_value.map(|value| self.convert_value(value, dfg)); + let resolved = self.handle_constant_index( + instruction, + dfg, + array_value, + constant_index, + store, + )?; + // A compile-time read at an index that is not statically safe reports + // `requires_acir_gen_predicate = true`, yet resolves optimistically + // without consulting the predicate: if the predicate were false the + // result is a don't-care that downstream predication masks anyway. + if resolved && store_value.is_none() && !dfg.is_safe_index(index, array) { + self.predicate_not_needed( + "constant in-bounds index resolved at compile time", + ); + } + Ok(resolved) } else { Ok(false) } @@ -454,8 +475,8 @@ impl Context<'_> { } if let Some(store_value) = store_value { - let side_effects_always_enabled = - self.acir_context.is_constant_one(&self.current_side_effects_enabled_var); + let predicate = self.read_predicate(); + let side_effects_always_enabled = self.acir_context.is_constant_one(&predicate); if side_effects_always_enabled { // If we know that this write will always occur then we can perform it at compile time. @@ -561,15 +582,18 @@ impl Context<'_> { let index_var = self.get_flattened_index(&array_typ, array_id, index_var, dfg, gating, shift)?; + // Reads need no store predication; only the store value depends on the predicate. + let Some(store) = store_value else { + return Ok((index_var, None)); + }; + // Side-effects are always enabled so we do not need to do any predication - if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) { - let store_value = store_value.map(|store| self.convert_value(store, dfg)); - return Ok((index_var, store_value)); + let predicate = self.read_predicate(); + if self.acir_context.is_constant_one(&predicate) { + return Ok((index_var, Some(self.convert_value(store, dfg)))); } - let new_value = store_value - .map(|store| self.predicated_store_value(store, dfg, array_id, index_var)) - .transpose()?; + let new_value = Some(self.predicated_store_value(store, dfg, array_id, index_var)?); Ok((index_var, new_value)) } @@ -600,11 +624,10 @@ impl Context<'_> { ) -> Result { match (store_value, dummy_value) { (AcirValue::Var(store_var, typ), AcirValue::Var(dummy_var, _)) => { - let true_pred = - self.acir_context.mul_var(*store_var, self.current_side_effects_enabled_var)?; + let predicate = self.read_predicate(); + let true_pred = self.acir_context.mul_var(*store_var, predicate)?; let one = self.acir_context.add_constant(FieldElement::one()); - let not_pred = - self.acir_context.sub_var(one, self.current_side_effects_enabled_var)?; + let not_pred = self.acir_context.sub_var(one, predicate)?; let false_pred = self.acir_context.mul_var(not_pred, *dummy_var)?; // predicate*value + (1-predicate)*dummy let new_value = self.acir_context.add_var(true_pred, false_pred)?; @@ -1245,6 +1268,11 @@ impl Context<'_> { .get(index as usize) .copied() { + if matches!(gating, IndexGating::Gated { .. }) { + self.predicate_not_needed( + "constant index resolved to a fixed flattened offset at compile time", + ); + } return Ok(self.acir_context.add_constant(offset)); } @@ -1256,7 +1284,8 @@ impl Context<'_> { let var_index = match gating { IndexGating::Safe => var_index, IndexGating::Gated { .. } => { - self.acir_context.mul_var(var_index, self.current_side_effects_enabled_var)? + let predicate = self.read_predicate(); + self.acir_context.mul_var(var_index, predicate)? } }; @@ -1276,8 +1305,8 @@ impl Context<'_> { match gating { IndexGating::Gated { fallback_offset } if fallback_offset != 0 => { let one = self.acir_context.add_constant(FieldElement::one()); - let not_pred = - self.acir_context.sub_var(one, self.current_side_effects_enabled_var)?; + let predicate = self.read_predicate(); + let not_pred = self.acir_context.sub_var(one, predicate)?; let offset_var = self.acir_context.add_constant(fallback_offset); let offset_term = self.acir_context.mul_var(offset_var, not_pred)?; Ok(self.acir_context.add_var(flat_index, offset_term)?) diff --git a/compiler/noirc_evaluator/src/acir/call/intrinsics/mod.rs b/compiler/noirc_evaluator/src/acir/call/intrinsics/mod.rs index 0e857d14993..8057b1e2aec 100644 --- a/compiler/noirc_evaluator/src/acir/call/intrinsics/mod.rs +++ b/compiler/noirc_evaluator/src/acir/call/intrinsics/mod.rs @@ -55,12 +55,23 @@ impl Context<'_> { .map(|result_id| dfg.type_of_value(*result_id).flattened_size()) .sum(); + // Only `RecursiveAggregation` consumes the side-effects predicate: it is + // injected as an extra witness input so aggregation can be conditionally + // executed. Other blackbox functions either take no predicate or receive + // one as an ordinary SSA-level argument (e.g. MSM, ECDSA). + let predicate = + if matches!(black_box, acvm::acir::BlackBoxFunc::RecursiveAggregation) { + Some(self.read_predicate()) + } else { + None + }; + let vars = self.acir_context.black_box_function( black_box, inputs, None, output_count, - Some(self.current_side_effects_enabled_var), + predicate, )?; Ok(self.convert_vars_to_values(vars, dfg, result_ids)) diff --git a/compiler/noirc_evaluator/src/acir/call/intrinsics/vector_ops.rs b/compiler/noirc_evaluator/src/acir/call/intrinsics/vector_ops.rs index 3026d4952ef..e895ead0aa2 100644 --- a/compiler/noirc_evaluator/src/acir/call/intrinsics/vector_ops.rs +++ b/compiler/noirc_evaluator/src/acir/call/intrinsics/vector_ops.rs @@ -197,6 +197,9 @@ impl Context<'_> { }); let new_vector_val = if let Some(len_const) = len_const { // Length is known at compile time - we can precisely determine where to write + self.predicate_not_needed( + "vector length folded to a compile-time constant; elements placed inline", + ); let mut new_vector = self.read_array_with_type(vector, &vector_typ)?; // length of Acir Values vector let len = len_const.to_u128() as usize * elements_to_push.len(); @@ -278,9 +281,8 @@ impl Context<'_> { // element-type-sizes table, but a whole-element append needs no per-member offsets: // multiply the length by the flattened element size directly and skip building that // table for this write. - let predicated_length = self - .acir_context - .mul_var(vector_length, self.current_side_effects_enabled_var)?; + let predicate = self.read_predicate(); + let predicated_length = self.acir_context.mul_var(vector_length, predicate)?; let element_flattened_size = self.acir_context.add_constant(elements_var.len()); self.acir_context.mul_var(predicated_length, element_flattened_size)? }; @@ -390,7 +392,8 @@ impl Context<'_> { if self.has_zero_length(vector_contents_id, dfg) { // Make sure this code is disabled, or fail with the empty-vector pop message. let msg = "Attempt to pop from an empty vector".to_string(); - self.acir_context.assert_zero_var(self.current_side_effects_enabled_var, msg)?; + let predicate = self.read_predicate(); + self.acir_context.assert_zero_var(predicate, msg)?; // Fill the result with default values. let mut results = Vec::with_capacity(result_ids.len()); @@ -472,12 +475,17 @@ impl Context<'_> { let assert_message = self.acir_context.generate_assertion_message_payload( "Attempt to pop from an empty vector".to_string(), ); + let predicate = self.read_predicate(); self.acir_context.assert_neq_var( vector_length_var, zero, - self.current_side_effects_enabled_var, + predicate, Some(assert_message), )?; + } else { + // A known-constant, nonzero length (the constant-zero case was handled by the + // caller) needs neither the runtime emptiness assertion nor index gating. + self.predicate_not_needed("vector length is a known nonzero constant"); } let one = self.acir_context.add_constant(FieldElement::one()); @@ -487,9 +495,8 @@ impl Context<'_> { // to ensure we don't end up trying to look up an item at index -1, when the semantic length is 0, // which can fail a circuit even when the side effects are disabled. if is_unknown_length { - new_vector_length_var = self - .acir_context - .mul_var(new_vector_length_var, self.current_side_effects_enabled_var)?; + let predicate = self.read_predicate(); + new_vector_length_var = self.acir_context.mul_var(new_vector_length_var, predicate)?; } Ok(new_vector_length_var) @@ -551,7 +558,8 @@ impl Context<'_> { if self.has_zero_length(vector_contents_id, dfg) { // Make sure this code is disabled, or fail with the empty-vector pop message. let msg = "Attempt to pop from an empty vector".to_string(); - self.acir_context.assert_zero_var(self.current_side_effects_enabled_var, msg)?; + let predicate = self.read_predicate(); + self.acir_context.assert_zero_var(predicate, msg)?; // Fill the result with default values. let mut results = Vec::with_capacity(result_ids.len()); @@ -661,6 +669,11 @@ impl Context<'_> { // Fetch the flattened index from the user provided index argument. let item_size = self.acir_context.add_constant(elements_to_insert.len()); let is_safe_index = Self::is_index_safe(arguments[2], dfg, &vector_typ, vector_size); + if is_safe_index { + // A statically safe insert index needs no gating, so no lowering path below + // consults the predicate. + self.predicate_not_needed("insert index is statically safe"); + } let insert_index = self.acir_context.mul_var(insert_index, item_size)?; // Because the insert index might be at the end of the vector, the element type sizes we @@ -877,7 +890,8 @@ impl Context<'_> { if self.has_zero_length(vector_contents, dfg) { // Make sure this code is disabled, or fail with "Index out of bounds". let msg = "Index out of bounds, vector has size 0".to_string(); - self.acir_context.assert_zero_var(self.current_side_effects_enabled_var, msg)?; + let predicate = self.read_predicate(); + self.acir_context.assert_zero_var(predicate, msg)?; // Fill the result with default values. let mut results = Vec::with_capacity(result_ids.len()); @@ -920,6 +934,11 @@ impl Context<'_> { let item_size_var = self.acir_context.add_constant(item_size); let remove_index = self.acir_context.mul_var(remove_index, item_size_var)?; let is_safe_index = Self::is_index_safe(arguments[2], dfg, &vector_typ, vector_size); + if is_safe_index { + // A statically safe remove index needs no gating, so no lowering path below + // consults the predicate. + self.predicate_not_needed("remove index is statically safe"); + } // Fetch the flattened index from the user provided index argument. let flat_user_index = self.get_flattened_index( diff --git a/compiler/noirc_evaluator/src/acir/call/mod.rs b/compiler/noirc_evaluator/src/acir/call/mod.rs index 6e9c5f1ba9c..ff277c3eddc 100644 --- a/compiler/noirc_evaluator/src/acir/call/mod.rs +++ b/compiler/noirc_evaluator/src/acir/call/mod.rs @@ -118,11 +118,12 @@ impl Context<'_> { ); }; + let predicate = self.read_predicate(); let output_vars = self.acir_context.call_acir_function( AcirFunctionId::new(acir_function_id), inputs, output_count, - self.current_side_effects_enabled_var, + predicate, )?; let output_values = self.convert_vars_to_values(output_vars, dfg, result_ids); @@ -143,13 +144,14 @@ impl Context<'_> { vecmap(result_ids, |result_id| dfg.type_of_value(*result_id).as_ref().into()); // Reuse or generate Brillig code + let predicate = self.read_predicate(); let output_values = if let Some(generated_pointer) = self.shared_context.generated_brillig_pointer(func.id(), arguments.clone()) { let code = self.shared_context.generated_brillig(generated_pointer.as_usize()); let skip_output_range_checks = false; self.acir_context.brillig_call( - self.current_side_effects_enabled_var, + predicate, code, inputs, outputs, @@ -162,7 +164,7 @@ impl Context<'_> { let generated_pointer = self.shared_context.new_generated_pointer(); let skip_output_range_checks = false; let output_values = self.acir_context.brillig_call( - self.current_side_effects_enabled_var, + predicate, &code, inputs, outputs, diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index d5f6fcdd67c..41ce08fdcfd 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -71,6 +71,11 @@ struct Context<'a> { /// `SideEffectsEnabled` instruction. current_side_effects_enabled_var: AcirVar, + /// Whether the instruction currently being converted consulted the side-effects + /// predicate (set by [`Self::read_predicate`] / [`Self::predicate_not_needed`]). + /// Compared against `Instruction::requires_acir_gen_predicate` after each instruction. + predicate_was_read: bool, + /// Manages and builds the `AcirVar`s to which the converted SSA values refer. acir_context: AcirContext, @@ -140,6 +145,7 @@ impl<'a> Context<'a> { Context { ssa_values: HashMap::default(), current_side_effects_enabled_var, + predicate_was_read: false, acir_context, initialized_arrays: HashSet::default(), memory_blocks: HashMap::default(), @@ -466,6 +472,22 @@ impl<'a> Context<'a> { Ok(acir_var) } + /// Returns the current side-effects predicate, recording that the instruction being + /// converted consulted it. Every read of `current_side_effects_enabled_var` during + /// instruction conversion must go through here so the recorded flag stays accurate. + fn read_predicate(&mut self) -> AcirVar { + self.predicate_was_read = true; + self.current_side_effects_enabled_var + } + + /// Records that the instruction being converted deliberately skipped consulting the + /// side-effects predicate on this lowering path, even though its + /// `requires_acir_gen_predicate` reports `true` (e.g. every relevant operand folded to + /// a compile-time constant, so no predication was necessary). + fn predicate_not_needed(&mut self, _reason: &str) { + self.predicate_was_read = true; + } + /// Converts an SSA instruction into its ACIR representation fn convert_ssa_instruction( &mut self, @@ -476,12 +498,13 @@ impl<'a> Context<'a> { let instruction = &dfg[instruction_id]; self.acir_context.set_call_stack(dfg.get_instruction_call_stack(instruction_id)); let mut warnings = Vec::new(); + self.predicate_was_read = false; match instruction { Instruction::Binary(binary) => { // Disable the side effects if the binary instruction does not require them let predicate = if instruction.requires_acir_gen_predicate(dfg) { - self.current_side_effects_enabled_var + self.read_predicate() } else { self.acir_context.add_constant(FieldElement::one()) }; @@ -498,7 +521,7 @@ impl<'a> Context<'a> { let lhs = self.convert_numeric_value(*lhs, dfg)?; let rhs = self.convert_numeric_value(*rhs, dfg)?; let assert_payload = self.convert_constrain_error(dfg, assert_message)?; - let predicate = self.current_side_effects_enabled_var; + let predicate = self.read_predicate(); self.acir_context.assert_neq_var(lhs, rhs, predicate, assert_payload)?; } Instruction::Cast(value_id, _) => { @@ -567,6 +590,19 @@ impl<'a> Context<'a> { Instruction::Noop => (), } + // The lowering above must consult the side-effects predicate exactly when + // `requires_acir_gen_predicate` says it may. `EnableSideEffectsIf` is excluded: it + // writes the predicate rather than reading it. + if !matches!(instruction, Instruction::EnableSideEffectsIf { .. }) { + let requires = instruction.requires_acir_gen_predicate(dfg); + assert_eq!( + requires, self.predicate_was_read, + "ACIR-gen predicate use mismatch: requires_acir_gen_predicate = {requires} but \ + predicate_was_read = {} for instruction {instruction:?}", + self.predicate_was_read, + ); + } + self.acir_context.set_call_stack(CallStack::empty()); Ok(warnings) } diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index ae087af3c69..d89957b065e 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -563,10 +563,12 @@ impl Instruction { // `get_flattened_index` which reads `current_side_effects_enabled_var` // to guard the element-type-sizes memory lookup. Intrinsic::VectorPushBack => true, - // Technically these don't use the side effects predicate, but they fail on empty vectors, - // and by pretending that they require the predicate, we can preserve any current side - // effect variable in the SSA and use it to optimize out memory operations that we know - // would fail, but they shouldn't because they might be disabled. + // These consult the side effects predicate during ACIR gen: a pop from a + // vector whose backing store is empty asserts the predicate is false, and + // a pop of a non-constant length predicates its emptiness assertion and + // gates the decremented index. Reporting `true` also preserves the current + // side effect variable in the SSA, which is used to optimize out memory + // operations that would fail but shouldn't because they might be disabled. Intrinsic::VectorPopFront | Intrinsic::VectorPopBack => true, // RecursiveAggregation's predicate is injected implicitly from // `current_side_effects_enabled_var` during ACIR generation, so we