From e17dfa55719f0cfb1080dd25eeda7b70ed44b60d Mon Sep 17 00:00:00 2001 From: guipublic <47281315+guipublic@users.noreply.github.com> Date: Wed, 18 Sep 2024 19:50:14 +0200 Subject: [PATCH] fix: Initialise databus using return values (#6074) # Description ## Problem\* Resolves #5715 ## Summary\* Use ABI's return values for the Databus return_data ## Additional Context ## Documentation\* Check one: - [X] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [X] I have tested the changes locally. - [X] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../src/ssa/acir_gen/acir_ir/acir_variable.rs | 9 +++ .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 68 +++++++++---------- 2 files changed, 42 insertions(+), 35 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index 9586d08e10c..3d428e97622 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -1944,6 +1944,15 @@ impl AcirContext { Ok(()) } + /// Insert the MemoryInit for the Return Data array, using the provided witnesses + pub(crate) fn initialize_return_data(&mut self, block_id: BlockId, init: Vec) { + self.acir_ir.push_opcode(Opcode::MemoryInit { + block_id, + init, + block_type: BlockType::ReturnData, + }); + } + /// Initializes an array in memory with the given values `optional_values`. /// If `optional_values` is empty, then the array is initialized with zeros. pub(crate) fn initialize_array( diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 15b44fde65d..430801dacf6 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -434,16 +434,9 @@ impl<'a> Context<'a> { for instruction_id in entry_block.instructions() { warnings.extend(self.convert_ssa_instruction(*instruction_id, dfg, ssa, brillig)?); } - let (return_vars, return_warnings) = self.convert_ssa_return(entry_block.unwrap_terminator(), dfg)?; - let call_data_arrays: Vec = - self.data_bus.call_data.iter().map(|cd| cd.array_id).collect(); - for call_data_array in call_data_arrays { - self.ensure_array_is_initialized(call_data_array, dfg)?; - } - // TODO: This is a naive method of assigning the return values to their witnesses as // we're likely to get a number of constraints which are asserting one witness to be equal to another. // @@ -452,6 +445,7 @@ impl<'a> Context<'a> { self.acir_context.assert_eq_var(*witness_var, return_var, None)?; } + self.initialize_databus(&return_witnesses, dfg)?; warnings.extend(return_warnings); warnings.extend(self.acir_context.warnings.clone()); @@ -459,6 +453,34 @@ impl<'a> Context<'a> { Ok(self.acir_context.finish(input_witness, return_witnesses, warnings)) } + fn initialize_databus( + &mut self, + witnesses: &Vec, + dfg: &DataFlowGraph, + ) -> Result<(), RuntimeError> { + // Initialize return_data using provided witnesses + if let Some(return_data) = self.data_bus.return_data { + let block_id = self.block_id(&return_data); + let already_initialized = self.initialized_arrays.contains(&block_id); + if !already_initialized { + // We hijack ensure_array_is_initialized() because we want the return data to use the return value witnesses, + // but the databus contains the computed values instead, that have just been asserted to be equal to the return values. + // We do not use initialize_array either for the case where a constant value is returned. + // In that case, the constant value has already been assigned a witness and the returned acir vars will be + // converted to it, instead of the corresponding return value witness. + self.acir_context.initialize_return_data(block_id, witnesses.to_owned()); + } + } + + // Initialize call_data + let call_data_arrays: Vec = + self.data_bus.call_data.iter().map(|cd| cd.array_id).collect(); + for call_data_array in call_data_arrays { + self.ensure_array_is_initialized(call_data_array, dfg)?; + } + Ok(()) + } + fn convert_brillig_main( mut self, main_func: &Function, @@ -1792,19 +1814,9 @@ impl<'a> Context<'a> { _ => unreachable!("ICE: Program must have a singular return"), }; - return_values.iter().fold(0, |acc, value_id| { - let is_databus = self - .data_bus - .return_data - .map_or(false, |return_databus| dfg[*value_id] == dfg[return_databus]); - - if is_databus { - // We do not return value for the data bus. - acc - } else { - acc + dfg.type_of_value(*value_id).flattened_size() - } - }) + return_values + .iter() + .fold(0, |acc, value_id| acc + dfg.type_of_value(*value_id).flattened_size()) } /// Converts an SSA terminator's return values into their ACIR representations @@ -1824,27 +1836,13 @@ impl<'a> Context<'a> { let mut has_constant_return = false; let mut return_vars: Vec = Vec::new(); for value_id in return_values { - let is_databus = self - .data_bus - .return_data - .map_or(false, |return_databus| dfg[*value_id] == dfg[return_databus]); let value = self.convert_value(*value_id, dfg); // `value` may or may not be an array reference. Calling `flatten` will expand the array if there is one. let acir_vars = self.acir_context.flatten(value)?; for (acir_var, _) in acir_vars { has_constant_return |= self.acir_context.is_constant(&acir_var); - if is_databus { - // We do not return value for the data bus. - self.ensure_array_is_initialized( - self.data_bus.return_data.expect( - "`is_databus == true` implies `data_bus.return_data` is `Some`", - ), - dfg, - )?; - } else { - return_vars.push(acir_var); - } + return_vars.push(acir_var); } }