From 1f8fd51fb28b62e05f4b0c0829d446e43e8b85cc Mon Sep 17 00:00:00 2001 From: jfecher Date: Thu, 13 Jul 2023 13:52:03 -0500 Subject: [PATCH] fix: Add Result to acir gen (#1927) Add Result to acir gen --- crates/noirc_evaluator/src/ssa_refactor.rs | 4 +- .../acir_gen/acir_ir/acir_variable.rs | 10 +- .../ssa_refactor/acir_gen/acir_ir/errors.rs | 55 +++++- .../acir_gen/acir_ir/generated_acir.rs | 1 + .../src/ssa_refactor/acir_gen/mod.rs | 159 +++++++++--------- 5 files changed, 140 insertions(+), 89 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor.rs b/crates/noirc_evaluator/src/ssa_refactor.rs index 4c9dc848b0e..29a718b590e 100644 --- a/crates/noirc_evaluator/src/ssa_refactor.rs +++ b/crates/noirc_evaluator/src/ssa_refactor.rs @@ -32,7 +32,7 @@ pub(crate) fn optimize_into_acir( program: Program, allow_log_ops: bool, print_ssa_passes: bool, -) -> GeneratedAcir { +) -> Result { let abi_distinctness = program.return_distinctness; let mut ssa = ssa_gen::generate_ssa(program) .print(print_ssa_passes, "Initial SSA:") @@ -71,7 +71,7 @@ pub fn experimental_create_circuit( ) -> Result<(Circuit, DebugInfo, Abi), RuntimeError> { let func_sig = program.main_function_signature.clone(); let GeneratedAcir { current_witness_index, opcodes, return_witnesses, locations, .. } = - optimize_into_acir(program, show_output, enable_logging); + optimize_into_acir(program, show_output, enable_logging)?; let abi = gen_abi(func_sig, return_witnesses.clone()); let public_abi = abi.clone().public_abi(); diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs index 8f713575075..98f551239b0 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs @@ -124,6 +124,10 @@ impl AcirContext { self.add_data(var_data) } + pub(crate) fn get_location(&mut self) -> Option { + self.acir_ir.current_location + } + pub(crate) fn set_location(&mut self, location: Option) { self.acir_ir.current_location = location; } @@ -271,7 +275,11 @@ impl AcirContext { Ok(()) } else { // Constraint is always false - this program is unprovable - Err(AcirGenError::BadConstantEquality { lhs: *lhs_const, rhs: *rhs_const }) + Err(AcirGenError::BadConstantEquality { + lhs: *lhs_const, + rhs: *rhs_const, + location: self.get_location(), + }) } } else { self.acir_ir.assert_is_zero( diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/errors.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/errors.rs index e209f8617c3..fa98bbead19 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/errors.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/errors.rs @@ -1,30 +1,69 @@ use acvm::FieldElement; +use noirc_errors::Location; + +use crate::errors::{RuntimeError, RuntimeErrorKind}; #[derive(Debug, PartialEq, Eq, Clone)] pub(crate) enum AcirGenError { - InvalidRangeConstraint { num_bits: u32 }, - IndexOutOfBounds { index: usize, array_size: usize }, - UnsupportedIntegerSize { num_bits: u32, max_num_bits: u32 }, - BadConstantEquality { lhs: FieldElement, rhs: FieldElement }, + InvalidRangeConstraint { num_bits: u32, location: Option }, + IndexOutOfBounds { index: usize, array_size: usize, location: Option }, + UnsupportedIntegerSize { num_bits: u32, max_num_bits: u32, location: Option }, + BadConstantEquality { lhs: FieldElement, rhs: FieldElement, location: Option }, } impl AcirGenError { pub(crate) fn message(&self) -> String { match self { - AcirGenError::InvalidRangeConstraint { num_bits } => { + AcirGenError::InvalidRangeConstraint { num_bits, .. } => { // Don't apply any constraints if the range is for the maximum number of bits or more. format!( "All Witnesses are by default u{num_bits} Applying this type does not apply any constraints.\n We also currently do not allow integers of size more than {num_bits}, this will be handled by BigIntegers.") } - AcirGenError::IndexOutOfBounds { index, array_size } => { + AcirGenError::IndexOutOfBounds { index, array_size, .. } => { format!("Index out of bounds, array has size {array_size}, but index was {index}") } - AcirGenError::UnsupportedIntegerSize { num_bits, max_num_bits } => { + AcirGenError::UnsupportedIntegerSize { num_bits, max_num_bits, .. } => { format!("Integer sized {num_bits} is over the max supported size of {max_num_bits}") } - AcirGenError::BadConstantEquality { lhs, rhs } => { + AcirGenError::BadConstantEquality { lhs, rhs, .. } => { format!("{lhs} and {rhs} constrained to be equal though they never can be") } } } } + +impl From for RuntimeError { + fn from(error: AcirGenError) -> Self { + match error { + AcirGenError::InvalidRangeConstraint { num_bits, location } => { + let kind = RuntimeErrorKind::UnstructuredError { + message: format!( + "Failed range constraint when constraining to {num_bits} bits" + ), + }; + RuntimeError::new(kind, location) + } + AcirGenError::IndexOutOfBounds { index, array_size, location } => { + let kind = RuntimeErrorKind::ArrayOutOfBounds { + index: index as u128, + bound: array_size as u128, + }; + RuntimeError::new(kind, location) + } + AcirGenError::UnsupportedIntegerSize { num_bits, max_num_bits, location } => { + let kind = RuntimeErrorKind::UnstructuredError { + message: format!("Unsupported integer size of {num_bits} bits. The maximum supported size is {max_num_bits} bits.") + }; + RuntimeError::new(kind, location) + } + AcirGenError::BadConstantEquality { lhs: _, rhs: _, location } => { + // We avoid showing the actual lhs and rhs since most of the time they are just 0 + // and 1 respectively. This would confuse users if a constraint such as + // assert(foo < bar) fails with "failed constraint: 0 = 1." + let kind = + RuntimeErrorKind::UnstructuredError { message: "Failed constraint".into() }; + RuntimeError::new(kind, location) + } + } + } +} diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/generated_acir.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/generated_acir.rs index 90aa1e11f6b..edd9777b127 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/generated_acir.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/generated_acir.rs @@ -662,6 +662,7 @@ impl GeneratedAcir { if num_bits >= FieldElement::max_num_bits() { return Err(AcirGenError::InvalidRangeConstraint { num_bits: FieldElement::max_num_bits(), + location: self.current_location, }); }; diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs index 3c365e68e71..72194828f5c 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs @@ -2,9 +2,12 @@ use std::collections::HashMap; -use crate::brillig::{ - brillig_gen::brillig_fn::FunctionContext as BrilligFunctionContext, - brillig_ir::artifact::BrilligArtifact, Brillig, +use crate::{ + brillig::{ + brillig_gen::brillig_fn::FunctionContext as BrilligFunctionContext, + brillig_ir::artifact::BrilligArtifact, Brillig, + }, + errors::RuntimeError, }; use self::acir_ir::{ @@ -28,7 +31,7 @@ use acvm::{ acir::{brillig::Opcode, native_types::Expression}, FieldElement, }; -use iter_extended::vecmap; +use iter_extended::{try_vecmap, vecmap}; pub(crate) use acir_ir::generated_acir::GeneratedAcir; use noirc_abi::AbiDistinctness; @@ -82,9 +85,9 @@ impl Ssa { brillig: Brillig, abi_distinctness: AbiDistinctness, allow_log_ops: bool, - ) -> GeneratedAcir { + ) -> Result { let context = Context::new(); - let mut generated_acir = context.convert_ssa(self, brillig, allow_log_ops); + let mut generated_acir = context.convert_ssa(self, brillig, allow_log_ops)?; match abi_distinctness { AbiDistinctness::Distinct => { @@ -101,9 +104,9 @@ impl Ssa { .collect(); generated_acir.return_witnesses = distinct_return_witness; - generated_acir + Ok(generated_acir) } - AbiDistinctness::DuplicationAllowed => generated_acir, + AbiDistinctness::DuplicationAllowed => Ok(generated_acir), } } } @@ -117,7 +120,12 @@ impl Context { } /// Converts SSA into ACIR - fn convert_ssa(self, ssa: Ssa, brillig: Brillig, allow_log_ops: bool) -> GeneratedAcir { + fn convert_ssa( + self, + ssa: Ssa, + brillig: Brillig, + allow_log_ops: bool, + ) -> Result { let main_func = ssa.main(); match main_func.runtime() { RuntimeType::Acir => self.convert_acir_main(main_func, &ssa, brillig, allow_log_ops), @@ -131,28 +139,32 @@ impl Context { ssa: &Ssa, brillig: Brillig, allow_log_ops: bool, - ) -> GeneratedAcir { + ) -> Result { let dfg = &main_func.dfg; let entry_block = &dfg[main_func.entry_block()]; - self.convert_ssa_block_params(entry_block.parameters(), dfg); + self.convert_ssa_block_params(entry_block.parameters(), dfg)?; for instruction_id in entry_block.instructions() { - self.convert_ssa_instruction(*instruction_id, dfg, ssa, &brillig, allow_log_ops); + self.convert_ssa_instruction(*instruction_id, dfg, ssa, &brillig, allow_log_ops)?; } - self.convert_ssa_return(entry_block.terminator().unwrap(), dfg); + self.convert_ssa_return(entry_block.unwrap_terminator(), dfg); - self.acir_context.finish() + Ok(self.acir_context.finish()) } - fn convert_brillig_main(mut self, main_func: &Function, brillig: Brillig) -> GeneratedAcir { + fn convert_brillig_main( + mut self, + main_func: &Function, + brillig: Brillig, + ) -> Result { let dfg = &main_func.dfg; - let inputs = vecmap(dfg[main_func.entry_block()].parameters(), |param_id| { + let inputs = try_vecmap(dfg[main_func.entry_block()].parameters(), |param_id| { let typ = dfg.type_of_value(*param_id); - self.create_value_from_type(&typ, &mut |this, _| this.acir_context.add_variable()) - }); + self.create_value_from_type(&typ, &mut |this, _| Ok(this.acir_context.add_variable())) + })?; let outputs: Vec = vecmap(main_func.returns(), |result_id| dfg.type_of_value(*result_id).into()); @@ -170,42 +182,47 @@ impl Context { self.acir_context.return_var(acir_var); } - self.acir_context.finish() + Ok(self.acir_context.finish()) } /// Adds and binds `AcirVar`s for each numeric block parameter or block parameter array element. - fn convert_ssa_block_params(&mut self, params: &[ValueId], dfg: &DataFlowGraph) { + fn convert_ssa_block_params( + &mut self, + params: &[ValueId], + dfg: &DataFlowGraph, + ) -> Result<(), AcirGenError> { for param_id in params { let typ = dfg.type_of_value(*param_id); - let value = self.convert_ssa_block_param(&typ); + let value = self.convert_ssa_block_param(&typ)?; self.ssa_values.insert(*param_id, value); } + Ok(()) } - fn convert_ssa_block_param(&mut self, param_type: &Type) -> AcirValue { + fn convert_ssa_block_param(&mut self, param_type: &Type) -> Result { self.create_value_from_type(param_type, &mut |this, typ| this.add_numeric_input_var(&typ)) } fn create_value_from_type( &mut self, param_type: &Type, - make_var: &mut impl FnMut(&mut Self, NumericType) -> AcirVar, - ) -> AcirValue { + make_var: &mut impl FnMut(&mut Self, NumericType) -> Result, + ) -> Result { match param_type { Type::Numeric(numeric_type) => { let typ = AcirType::new(*numeric_type); - AcirValue::Var(make_var(self, *numeric_type), typ) + Ok(AcirValue::Var(make_var(self, *numeric_type)?, typ)) } Type::Array(element_types, length) => { let mut elements = im::Vector::new(); for _ in 0..*length { for element in element_types.iter() { - elements.push_back(self.create_value_from_type(element, make_var)); + elements.push_back(self.create_value_from_type(element, make_var)?); } } - AcirValue::Array(elements) + Ok(AcirValue::Array(elements)) } _ => unreachable!("ICE: Params to the program should only contains numbers and arrays"), } @@ -216,14 +233,15 @@ impl Context { /// /// This function is used not only for adding numeric block parameters, but also for adding /// any array elements that belong to reference type block parameters. - fn add_numeric_input_var(&mut self, numeric_type: &NumericType) -> AcirVar { + fn add_numeric_input_var( + &mut self, + numeric_type: &NumericType, + ) -> Result { let acir_var = self.acir_context.add_variable(); if matches!(numeric_type, NumericType::Signed { .. } | NumericType::Unsigned { .. }) { - self.acir_context - .range_constrain_var(acir_var, numeric_type) - .expect("invalid range constraint was applied {numeric_type}"); + self.acir_context.range_constrain_var(acir_var, numeric_type)?; } - acir_var + Ok(acir_var) } /// Converts an SSA instruction into its ACIR representation @@ -234,26 +252,20 @@ impl Context { ssa: &Ssa, brillig: &Brillig, allow_log_ops: bool, - ) { + ) -> Result<(), AcirGenError> { let instruction = &dfg[instruction_id]; self.acir_context.set_location(dfg.get_location(&instruction_id)); match instruction { Instruction::Binary(binary) => { - let result_acir_var = self - .convert_ssa_binary(binary, dfg) - .expect("add Result types to all methods so errors bubble up"); + let result_acir_var = self.convert_ssa_binary(binary, dfg)?; self.define_result_var(dfg, instruction_id, result_acir_var); } Instruction::Constrain(value_id) => { let constrain_condition = self.convert_numeric_value(*value_id, dfg); - self.acir_context - .assert_eq_one(constrain_condition) - .expect("add Result types to all methods so errors bubble up"); + self.acir_context.assert_eq_one(constrain_condition)?; } Instruction::Cast(value_id, typ) => { - let result_acir_var = self - .convert_ssa_cast(value_id, typ, dfg) - .expect("add Result types to all methods so errors bubble up"); + let result_acir_var = self.convert_ssa_cast(value_id, typ, dfg)?; self.define_result_var(dfg, instruction_id, result_acir_var); } Instruction::Call { func, arguments } => { @@ -290,7 +302,7 @@ impl Context { dfg, allow_log_ops, result_ids, - ); + )?; // Issue #1438 causes this check to fail with intrinsics that return 0 // results but the ssa form instead creates 1 unit result value. @@ -311,16 +323,12 @@ impl Context { AcirValue::Var(acir_var, typ) => (acir_var, typ), _ => unreachable!("NOT is only applied to numerics"), }; - let result_acir_var = self - .acir_context - .not_var(acir_var, typ) - .expect("add Result types to all methods so errors bubble up"); + let result_acir_var = self.acir_context.not_var(acir_var, typ)?; self.define_result_var(dfg, instruction_id, result_acir_var); } Instruction::Truncate { value, bit_size, max_bit_size } => { - let result_acir_var = self - .convert_ssa_truncate(*value, *bit_size, *max_bit_size, dfg) - .expect("add Result types to all methods so errors bubble up"); + let result_acir_var = + self.convert_ssa_truncate(*value, *bit_size, *max_bit_size, dfg)?; self.define_result_var(dfg, instruction_id, result_acir_var); } Instruction::EnableSideEffects { condition } => { @@ -328,10 +336,10 @@ impl Context { self.current_side_effects_enabled_var = acir_var; } Instruction::ArrayGet { array, index } => { - self.handle_array_operation(instruction_id, *array, *index, None, dfg); + self.handle_array_operation(instruction_id, *array, *index, None, dfg)?; } Instruction::ArraySet { array, index, value } => { - self.handle_array_operation(instruction_id, *array, *index, Some(*value), dfg); + self.handle_array_operation(instruction_id, *array, *index, Some(*value), dfg)?; } Instruction::Allocate => { unreachable!("Expected all allocate instructions to be removed before acir_gen") @@ -344,6 +352,7 @@ impl Context { } } self.acir_context.set_location(None); + Ok(()) } fn gen_brillig_for(&self, func: &Function, brillig: &Brillig) -> Vec { @@ -374,7 +383,7 @@ impl Context { index: ValueId, store_value: Option, dfg: &DataFlowGraph, - ) { + ) -> Result<(), AcirGenError> { let array = self.convert_array_value(array, dfg); let index = dfg .get_numeric_constant(index) @@ -382,16 +391,17 @@ impl Context { .try_to_u64() .expect("Expected array index to fit into a u64") as usize; - if index >= array.len() { + let array_size = array.len(); + if index >= array_size { // Ignore the error if side effects are disabled. if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) { - // TODO: Can we save a source Location for this error? - panic!("Index {} is out of bounds for array of length {}", index, array.len()); + let location = self.acir_context.get_location(); + return Err(AcirGenError::IndexOutOfBounds { index, array_size, location }); } let result_type = dfg.type_of_value(dfg.instruction_results(instruction)[0]); - let value = self.create_default_value(&result_type); + let value = self.create_default_value(&result_type)?; self.define_result(dfg, instruction, value); - return; + return Ok(()); } let value = match store_value { @@ -403,6 +413,7 @@ impl Context { }; self.define_result(dfg, instruction, value); + Ok(()) } /// Remember the result of an instruction returning a single value @@ -523,6 +534,7 @@ impl Context { return Err(AcirGenError::UnsupportedIntegerSize { num_bits: *bit_size, max_num_bits: max_integer_bit_size, + location: self.acir_context.get_location(), }); } } @@ -676,17 +688,14 @@ impl Context { dfg: &DataFlowGraph, allow_log_ops: bool, result_ids: &[ValueId], - ) -> Vec { + ) -> Result, AcirGenError> { match intrinsic { Intrinsic::BlackBox(black_box) => { let inputs = vecmap(arguments, |arg| self.convert_value(*arg, dfg)); - let vars = self - .acir_context - .black_box_function(black_box, inputs) - .expect("add Result types to all methods so errors bubble up"); + let vars = self.acir_context.black_box_function(black_box, inputs)?; - Self::convert_vars_to_values(vars, dfg, result_ids) + Ok(Self::convert_vars_to_values(vars, dfg, result_ids)) } Intrinsic::ToRadix(endian) => { let field = self.convert_value(arguments[0], dfg).into_var(); @@ -694,27 +703,21 @@ impl Context { let limb_size = self.convert_value(arguments[2], dfg).into_var(); let result_type = Self::array_element_type(dfg, result_ids[0]); - self.acir_context - .radix_decompose(endian, field, radix, limb_size, result_type) - .expect("add Result types to all methods so errors bubble up") + self.acir_context.radix_decompose(endian, field, radix, limb_size, result_type) } Intrinsic::ToBits(endian) => { let field = self.convert_value(arguments[0], dfg).into_var(); let bit_size = self.convert_value(arguments[1], dfg).into_var(); let result_type = Self::array_element_type(dfg, result_ids[0]); - self.acir_context - .bit_decompose(endian, field, bit_size, result_type) - .expect("add Result types to all methods so errors bubble up") + self.acir_context.bit_decompose(endian, field, bit_size, result_type) } Intrinsic::Println => { let inputs = vecmap(arguments, |arg| self.convert_value(*arg, dfg)); if allow_log_ops { - self.acir_context - .print(inputs) - .expect("add Result types to all methods so errors bubble up"); + self.acir_context.print(inputs)?; } - Vec::new() + Ok(Vec::new()) } Intrinsic::Sort => { let inputs = vecmap(arguments, |arg| self.convert_value(*arg, dfg)); @@ -739,7 +742,7 @@ impl Context { let out_vars = self.acir_context.sort(input_vars, bit_size).expect("Could not sort"); - Self::convert_vars_to_values(out_vars, dfg, result_ids) + Ok(Self::convert_vars_to_values(out_vars, dfg, result_ids)) } _ => todo!("expected a black box function"), } @@ -824,9 +827,9 @@ impl Context { } /// Creates a default, meaningless value meant only to be a valid value of the given type. - fn create_default_value(&mut self, param_type: &Type) -> AcirValue { + fn create_default_value(&mut self, param_type: &Type) -> Result { self.create_value_from_type(param_type, &mut |this, _| { - this.acir_context.add_constant(FieldElement::zero()) + Ok(this.acir_context.add_constant(FieldElement::zero())) }) } } @@ -872,7 +875,7 @@ mod tests { let ssa = builder.finish(); let context = Context::new(); - let acir = context.convert_ssa(ssa, Brillig::default(), false); + let acir = context.convert_ssa(ssa, Brillig::default(), false).unwrap(); let expected_opcodes = vec![Opcode::Arithmetic(&Expression::one() - &Expression::from(Witness(1)))];