diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index e57f5d1883..fcfb04eb5c 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -1949,9 +1949,9 @@ impl<'a> Context<'a> { let bit_count = binary_type.bit_size::(); let num_type = binary_type.to_numeric_type(); let result = match binary.operator { - BinaryOp::Add => self.acir_context.add_var(lhs, rhs), - BinaryOp::Sub => self.acir_context.sub_var(lhs, rhs), - BinaryOp::Mul => self.acir_context.mul_var(lhs, rhs), + BinaryOp::Add { .. } => self.acir_context.add_var(lhs, rhs), + BinaryOp::Sub { .. } => self.acir_context.sub_var(lhs, rhs), + BinaryOp::Mul { .. } => self.acir_context.mul_var(lhs, rhs), BinaryOp::Div => self.acir_context.div_var( lhs, rhs, @@ -2070,7 +2070,7 @@ impl<'a> Context<'a> { Value::Instruction { instruction, .. } => { if matches!( &dfg[*instruction], - Instruction::Binary(Binary { operator: BinaryOp::Sub, .. }) + Instruction::Binary(Binary { operator: BinaryOp::Sub { .. }, .. }) ) { // Subtractions must first have the integer modulus added before truncation can be // applied. This is done in order to prevent underflow. @@ -3159,7 +3159,11 @@ mod test { let func_with_nested_call_v1 = builder.add_parameter(Type::field()); let two = builder.field_constant(2u128); - let v0_plus_two = builder.insert_binary(func_with_nested_call_v0, BinaryOp::Add, two); + let v0_plus_two = builder.insert_binary( + func_with_nested_call_v0, + BinaryOp::Add { unchecked: false }, + two, + ); let foo_id = Id::test_new(2); let foo_call = builder.import_function(foo_id); diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 31c99bf433..c986aefb34 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1318,9 +1318,9 @@ impl<'block> BrilligBlock<'block> { BrilligBinaryOp::Modulo } } - BinaryOp::Add => BrilligBinaryOp::Add, - BinaryOp::Sub => BrilligBinaryOp::Sub, - BinaryOp::Mul => BrilligBinaryOp::Mul, + BinaryOp::Add { .. } => BrilligBinaryOp::Add, + BinaryOp::Sub { .. } => BrilligBinaryOp::Sub, + BinaryOp::Mul { .. } => BrilligBinaryOp::Mul, BinaryOp::Eq => BrilligBinaryOp::Equals, BinaryOp::Lt => { if is_signed { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs index d6851a9ecf..1c908f7014 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs @@ -382,14 +382,14 @@ mod test { builder.switch_to_block(b2); let twenty_seven = builder.field_constant(27u128); - let v7 = builder.insert_binary(v0, BinaryOp::Add, twenty_seven); + let v7 = builder.insert_binary(v0, BinaryOp::Add { unchecked: false }, twenty_seven); builder.insert_store(v3, v7); builder.terminate_with_jmp(b3, vec![]); builder.switch_to_block(b1); - let v6 = builder.insert_binary(v1, BinaryOp::Add, twenty_seven); + let v6 = builder.insert_binary(v1, BinaryOp::Add { unchecked: false }, twenty_seven); builder.insert_store(v3, v6); builder.terminate_with_jmp(b3, vec![]); @@ -501,7 +501,7 @@ mod test { builder.switch_to_block(b2); - let v6 = builder.insert_binary(v4, BinaryOp::Mul, v4); + let v6 = builder.insert_binary(v4, BinaryOp::Mul { unchecked: false }, v4); builder.terminate_with_jmp(b4, vec![v0]); @@ -526,7 +526,7 @@ mod test { let v12 = builder.insert_load(v3, Type::field()); - let v13 = builder.insert_binary(v12, BinaryOp::Add, v6); + let v13 = builder.insert_binary(v12, BinaryOp::Add { unchecked: false }, v6); builder.insert_store(v3, v13); @@ -535,13 +535,13 @@ mod test { builder.switch_to_block(b8); let one = builder.field_constant(1u128); - let v15 = builder.insert_binary(v7, BinaryOp::Add, one); + let v15 = builder.insert_binary(v7, BinaryOp::Add { unchecked: false }, one); builder.terminate_with_jmp(b4, vec![v15]); builder.switch_to_block(b6); - let v16 = builder.insert_binary(v4, BinaryOp::Add, one); + let v16 = builder.insert_binary(v4, BinaryOp::Add { unchecked: false }, one); builder.terminate_with_jmp(b1, vec![v16]); diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index d08d533923..3d7e0b06d5 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -251,14 +251,6 @@ impl FunctionBuilder { operator: BinaryOp, rhs: ValueId, ) -> ValueId { - let lhs_type = self.type_of_value(lhs); - let rhs_type = self.type_of_value(rhs); - if operator != BinaryOp::Shl && operator != BinaryOp::Shr { - assert_eq!( - lhs_type, rhs_type, - "ICE - Binary instruction operands must have the same type" - ); - } let instruction = Instruction::Binary(Binary { lhs, rhs, operator }); self.insert_instruction(instruction, None).first() } diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 8425e4d5e5..8a4b3fa1ce 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -230,6 +230,7 @@ impl DataFlowGraph { if !self.is_handled_by_runtime(&instruction) { return InsertInstructionResult::InstructionRemoved; } + match instruction.simplify(self, block, ctrl_typevars.clone(), call_stack) { SimplifyResult::SimplifiedTo(simplification) => { InsertInstructionResult::SimplifiedTo(simplification) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 17cde96cdd..67210d59aa 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -412,10 +412,15 @@ impl Instruction { // Some binary math can overflow or underflow Binary(binary) => match binary.operator { - BinaryOp::Add | BinaryOp::Sub | BinaryOp::Mul | BinaryOp::Div | BinaryOp::Mod => { - true - } - BinaryOp::Eq + BinaryOp::Add { unchecked: false } + | BinaryOp::Sub { unchecked: false } + | BinaryOp::Mul { unchecked: false } + | BinaryOp::Div + | BinaryOp::Mod => true, + BinaryOp::Add { unchecked: true } + | BinaryOp::Sub { unchecked: true } + | BinaryOp::Mul { unchecked: true } + | BinaryOp::Eq | BinaryOp::Lt | BinaryOp::And | BinaryOp::Or @@ -566,16 +571,19 @@ impl Instruction { match self { Instruction::Binary(binary) => { match binary.operator { - BinaryOp::Add - | BinaryOp::Sub - | BinaryOp::Mul + BinaryOp::Add { unchecked: false } + | BinaryOp::Sub { unchecked: false } + | BinaryOp::Mul { unchecked: false } | BinaryOp::Div | BinaryOp::Mod => { // Some binary math can overflow or underflow, but this is only the case // for unsigned types (here we assume the type of binary.lhs is the same) dfg.type_of_value(binary.rhs).is_unsigned() } - BinaryOp::Eq + BinaryOp::Add { unchecked: true } + | BinaryOp::Sub { unchecked: true } + | BinaryOp::Mul { unchecked: true } + | BinaryOp::Eq | BinaryOp::Lt | BinaryOp::And | BinaryOp::Or diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs index 28e58e2cbb..df1e8f537d 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs @@ -15,11 +15,11 @@ use super::{ #[derive(Debug, PartialEq, Eq, Hash, Copy, Clone, Serialize, Deserialize)] pub(crate) enum BinaryOp { /// Addition of lhs + rhs. - Add, + Add { unchecked: bool }, /// Subtraction of lhs - rhs. - Sub, + Sub { unchecked: bool }, /// Multiplication of lhs * rhs. - Mul, + Mul { unchecked: bool }, /// Division of lhs / rhs. Div, /// Modulus of lhs % rhs. @@ -48,9 +48,12 @@ pub(crate) enum BinaryOp { impl std::fmt::Display for BinaryOp { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - BinaryOp::Add => write!(f, "add"), - BinaryOp::Sub => write!(f, "sub"), - BinaryOp::Mul => write!(f, "mul"), + BinaryOp::Add { unchecked: false } => write!(f, "add"), + BinaryOp::Add { unchecked: true } => write!(f, "unchecked_add"), + BinaryOp::Sub { unchecked: false } => write!(f, "sub"), + BinaryOp::Sub { unchecked: true } => write!(f, "unchecked_sub"), + BinaryOp::Mul { unchecked: false } => write!(f, "mul"), + BinaryOp::Mul { unchecked: true } => write!(f, "unchecked_mul"), BinaryOp::Div => write!(f, "div"), BinaryOp::Eq => write!(f, "eq"), BinaryOp::Mod => write!(f, "mod"), @@ -86,28 +89,61 @@ impl Binary { /// Try to simplify this binary instruction, returning the new value if possible. pub(super) fn simplify(&self, dfg: &mut DataFlowGraph) -> SimplifyResult { - let lhs = dfg.get_numeric_constant(self.lhs); - let rhs = dfg.get_numeric_constant(self.rhs); - let operand_type = dfg.type_of_value(self.lhs).unwrap_numeric(); + let lhs_value = dfg.get_numeric_constant(self.lhs); + let rhs_value = dfg.get_numeric_constant(self.rhs); + + let lhs_type = dfg.type_of_value(self.lhs).unwrap_numeric(); + let rhs_type = dfg.type_of_value(self.rhs).unwrap_numeric(); + + let operator = self.operator; + if operator != BinaryOp::Shl && operator != BinaryOp::Shr { + assert_eq!( + lhs_type, rhs_type, + "ICE - Binary instruction operands must have the same type" + ); + } + + let operator = if lhs_type == NumericType::NativeField { + // Unchecked operations between fields or bools don't make sense, so we convert those to non-unchecked + // to reduce noise and confusion in the generated SSA. + match operator { + BinaryOp::Add { unchecked: true } => BinaryOp::Add { unchecked: false }, + BinaryOp::Sub { unchecked: true } => BinaryOp::Sub { unchecked: false }, + BinaryOp::Mul { unchecked: true } => BinaryOp::Mul { unchecked: false }, + _ => operator, + } + } else if lhs_type == NumericType::bool() { + // Unchecked mul between bools doesn't make sense, so we convert that to non-unchecked + if let BinaryOp::Mul { unchecked: true } = operator { + BinaryOp::Mul { unchecked: false } + } else { + operator + } + } else { + operator + }; + + // We never return `SimplifyResult::None` here because `operator` might have changed. + let simplified = Instruction::Binary(Binary { lhs: self.lhs, rhs: self.rhs, operator }); - if let (Some(lhs), Some(rhs)) = (lhs, rhs) { - return match eval_constant_binary_op(lhs, rhs, self.operator, operand_type) { + if let (Some(lhs), Some(rhs)) = (lhs_value, rhs_value) { + return match eval_constant_binary_op(lhs, rhs, operator, lhs_type) { Some((result, result_type)) => { let value = dfg.make_constant(result, result_type); SimplifyResult::SimplifiedTo(value) } - None => SimplifyResult::None, + None => SimplifyResult::SimplifiedToInstruction(simplified), }; } - let lhs_is_zero = lhs.map_or(false, |lhs| lhs.is_zero()); - let rhs_is_zero = rhs.map_or(false, |rhs| rhs.is_zero()); + let lhs_is_zero = lhs_value.map_or(false, |lhs| lhs.is_zero()); + let rhs_is_zero = rhs_value.map_or(false, |rhs| rhs.is_zero()); - let lhs_is_one = lhs.map_or(false, |lhs| lhs.is_one()); - let rhs_is_one = rhs.map_or(false, |rhs| rhs.is_one()); + let lhs_is_one = lhs_value.map_or(false, |lhs| lhs.is_one()); + let rhs_is_one = rhs_value.map_or(false, |rhs| rhs.is_one()); match self.operator { - BinaryOp::Add => { + BinaryOp::Add { .. } => { if lhs_is_zero { return SimplifyResult::SimplifiedTo(self.rhs); } @@ -115,12 +151,12 @@ impl Binary { return SimplifyResult::SimplifiedTo(self.lhs); } } - BinaryOp::Sub => { + BinaryOp::Sub { .. } => { if rhs_is_zero { return SimplifyResult::SimplifiedTo(self.lhs); } } - BinaryOp::Mul => { + BinaryOp::Mul { .. } => { if lhs_is_one { return SimplifyResult::SimplifiedTo(self.rhs); } @@ -128,7 +164,7 @@ impl Binary { return SimplifyResult::SimplifiedTo(self.lhs); } if lhs_is_zero || rhs_is_zero { - let zero = dfg.make_constant(FieldElement::zero(), operand_type); + let zero = dfg.make_constant(FieldElement::zero(), lhs_type); return SimplifyResult::SimplifiedTo(zero); } if dfg.get_value_max_num_bits(self.lhs) == 1 { @@ -141,7 +177,7 @@ impl Binary { if let Instruction::Binary(Binary { lhs, rhs, operator }) = dfg[*instruction] { - if operator == BinaryOp::Mul + if matches!(operator, BinaryOp::Mul { .. }) && (dfg.resolve(self.lhs) == dfg.resolve(lhs) || dfg.resolve(self.lhs) == dfg.resolve(rhs)) { @@ -156,7 +192,7 @@ impl Binary { if let Instruction::Binary(Binary { lhs, rhs, operator }) = dfg[*instruction] { - if operator == BinaryOp::Mul + if matches!(operator, BinaryOp::Mul { .. }) && (dfg.resolve(self.rhs) == dfg.resolve(lhs) || dfg.resolve(self.rhs) == dfg.resolve(rhs)) { @@ -173,13 +209,13 @@ impl Binary { } BinaryOp::Mod => { if rhs_is_one { - let zero = dfg.make_constant(FieldElement::zero(), operand_type); + let zero = dfg.make_constant(FieldElement::zero(), lhs_type); return SimplifyResult::SimplifiedTo(zero); } - if operand_type.is_unsigned() { + if lhs_type.is_unsigned() { // lhs % 2**bit_size is equivalent to truncating `lhs` to `bit_size` bits. // We then convert to a truncation for consistency, allowing more optimizations. - if let Some(modulus) = rhs { + if let Some(modulus) = rhs_value { let modulus = modulus.to_u128(); if modulus.is_power_of_two() { let bit_size = modulus.ilog2(); @@ -187,7 +223,7 @@ impl Binary { Instruction::Truncate { value: self.lhs, bit_size, - max_bit_size: operand_type.bit_size(), + max_bit_size: lhs_type.bit_size(), }, ); } @@ -200,7 +236,7 @@ impl Binary { return SimplifyResult::SimplifiedTo(one); } - if operand_type == NumericType::bool() { + if lhs_type == NumericType::bool() { // Simplify forms of `(boolean == true)` into `boolean` if lhs_is_one { return SimplifyResult::SimplifiedTo(self.rhs); @@ -222,13 +258,13 @@ impl Binary { let zero = dfg.make_constant(FieldElement::zero(), NumericType::bool()); return SimplifyResult::SimplifiedTo(zero); } - if operand_type.is_unsigned() { + if lhs_type.is_unsigned() { if rhs_is_zero { // Unsigned values cannot be less than zero. let zero = dfg.make_constant(FieldElement::zero(), NumericType::bool()); return SimplifyResult::SimplifiedTo(zero); } else if rhs_is_one { - let zero = dfg.make_constant(FieldElement::zero(), operand_type); + let zero = dfg.make_constant(FieldElement::zero(), lhs_type); return SimplifyResult::SimplifiedToInstruction(Instruction::binary( BinaryOp::Eq, self.lhs, @@ -239,35 +275,37 @@ impl Binary { } BinaryOp::And => { if lhs_is_zero || rhs_is_zero { - let zero = dfg.make_constant(FieldElement::zero(), operand_type); + let zero = dfg.make_constant(FieldElement::zero(), lhs_type); return SimplifyResult::SimplifiedTo(zero); } if dfg.resolve(self.lhs) == dfg.resolve(self.rhs) { return SimplifyResult::SimplifiedTo(self.lhs); } - if operand_type == NumericType::bool() { + if lhs_type == NumericType::bool() { // Boolean AND is equivalent to multiplication, which is a cheaper operation. - let instruction = Instruction::binary(BinaryOp::Mul, self.lhs, self.rhs); + // (mul unchecked because these are bools so it doesn't matter really) + let instruction = + Instruction::binary(BinaryOp::Mul { unchecked: true }, self.lhs, self.rhs); return SimplifyResult::SimplifiedToInstruction(instruction); } - if operand_type.is_unsigned() { + if lhs_type.is_unsigned() { // It's common in other programming languages to truncate values to a certain bit size using // a bitwise AND with a bit mask. However this operation is quite inefficient inside a snark. // // We then replace this bitwise operation with an equivalent truncation instruction. - match (lhs, rhs) { + match (lhs_value, rhs_value) { (Some(bitmask), None) | (None, Some(bitmask)) => { // This substitution requires the bitmask to retain all of the lower bits. // The bitmask must then be one less than a power of 2. let bitmask_plus_one = bitmask.to_u128() + 1; if bitmask_plus_one.is_power_of_two() { - let value = if lhs.is_some() { self.rhs } else { self.lhs }; + let value = if lhs_value.is_some() { self.rhs } else { self.lhs }; let num_bits = bitmask_plus_one.ilog2(); return SimplifyResult::SimplifiedToInstruction( Instruction::Truncate { value, bit_size: num_bits, - max_bit_size: operand_type.bit_size(), + max_bit_size: lhs_type.bit_size(), }, ); } @@ -284,8 +322,8 @@ impl Binary { if rhs_is_zero { return SimplifyResult::SimplifiedTo(self.lhs); } - if operand_type == NumericType::bool() && (lhs_is_one || rhs_is_one) { - let one = dfg.make_constant(FieldElement::one(), operand_type); + if lhs_type == NumericType::bool() && (lhs_is_one || rhs_is_one) { + let one = dfg.make_constant(FieldElement::one(), lhs_type); return SimplifyResult::SimplifiedTo(one); } if dfg.resolve(self.lhs) == dfg.resolve(self.rhs) { @@ -300,24 +338,24 @@ impl Binary { return SimplifyResult::SimplifiedTo(self.lhs); } if dfg.resolve(self.lhs) == dfg.resolve(self.rhs) { - let zero = dfg.make_constant(FieldElement::zero(), operand_type); + let zero = dfg.make_constant(FieldElement::zero(), lhs_type); return SimplifyResult::SimplifiedTo(zero); } } - BinaryOp::Shl => return SimplifyResult::None, + BinaryOp::Shl => return SimplifyResult::SimplifiedToInstruction(simplified), BinaryOp::Shr => { // Bit shifts by constants can be treated as divisions. - if let Some(rhs_const) = rhs { - if rhs_const >= FieldElement::from(operand_type.bit_size() as u128) { + if let Some(rhs_const) = rhs_value { + if rhs_const >= FieldElement::from(lhs_type.bit_size() as u128) { // Shifting by the full width of the operand type, any `lhs` goes to zero. - let zero = dfg.make_constant(FieldElement::zero(), operand_type); + let zero = dfg.make_constant(FieldElement::zero(), lhs_type); return SimplifyResult::SimplifiedTo(zero); } - return SimplifyResult::None; + return SimplifyResult::SimplifiedToInstruction(simplified); } } }; - SimplifyResult::None + SimplifyResult::SimplifiedToInstruction(simplified) } /// Check if unsigned overflow is possible, and if so return some message to be used if it fails. @@ -331,14 +369,14 @@ impl Binary { let max_rhs_bits = dfg.get_value_max_num_bits(self.rhs); let msg = match self.operator { - BinaryOp::Add => { + BinaryOp::Add { unchecked: false } => { if std::cmp::max(max_lhs_bits, max_rhs_bits) < bit_size { // `lhs` and `rhs` have both been casted up from smaller types and so cannot overflow. return None; } "attempt to add with overflow" } - BinaryOp::Sub => { + BinaryOp::Sub { unchecked: false } => { if dfg.is_constant(self.lhs) && max_lhs_bits > max_rhs_bits { // `lhs` is a fixed constant and `rhs` is restricted such that `lhs - rhs > 0` // Note strict inequality as `rhs > lhs` while `max_lhs_bits == max_rhs_bits` is possible. @@ -346,7 +384,7 @@ impl Binary { } "attempt to subtract with overflow" } - BinaryOp::Mul => { + BinaryOp::Mul { unchecked: false } => { if bit_size == 1 || max_lhs_bits + max_rhs_bits <= bit_size || max_lhs_bits == 1 @@ -470,9 +508,9 @@ fn truncate(int: u128, bit_size: u32) -> u128 { impl BinaryOp { fn get_field_function(self) -> Option FieldElement> { match self { - BinaryOp::Add => Some(std::ops::Add::add), - BinaryOp::Sub => Some(std::ops::Sub::sub), - BinaryOp::Mul => Some(std::ops::Mul::mul), + BinaryOp::Add { .. } => Some(std::ops::Add::add), + BinaryOp::Sub { .. } => Some(std::ops::Sub::sub), + BinaryOp::Mul { .. } => Some(std::ops::Mul::mul), BinaryOp::Div => Some(std::ops::Div::div), BinaryOp::Eq => Some(|x, y| (x == y).into()), BinaryOp::Lt => Some(|x, y| (x < y).into()), @@ -488,9 +526,9 @@ impl BinaryOp { fn get_u128_function(self) -> fn(u128, u128) -> Option { match self { - BinaryOp::Add => u128::checked_add, - BinaryOp::Sub => u128::checked_sub, - BinaryOp::Mul => u128::checked_mul, + BinaryOp::Add { .. } => u128::checked_add, + BinaryOp::Sub { .. } => u128::checked_sub, + BinaryOp::Mul { .. } => u128::checked_mul, BinaryOp::Div => u128::checked_div, BinaryOp::Mod => u128::checked_rem, BinaryOp::And => |x, y| Some(x & y), @@ -505,9 +543,9 @@ impl BinaryOp { fn get_i128_function(self) -> fn(i128, i128) -> Option { match self { - BinaryOp::Add => i128::checked_add, - BinaryOp::Sub => i128::checked_sub, - BinaryOp::Mul => i128::checked_mul, + BinaryOp::Add { .. } => i128::checked_add, + BinaryOp::Sub { .. } => i128::checked_sub, + BinaryOp::Mul { .. } => i128::checked_mul, BinaryOp::Div => i128::checked_div, BinaryOp::Mod => i128::checked_rem, BinaryOp::And => |x, y| Some(x & y), diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index f1ba21f1c1..992c633ffc 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -142,8 +142,7 @@ pub(super) fn simplify_call( slice.push_back(*elem); } - let new_slice_length = - update_slice_length(arguments[0], dfg, BinaryOp::Add, block); + let new_slice_length = increment_slice_length(arguments[0], dfg, block); let new_slice = make_array(dfg, slice, element_type, block, call_stack); return SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]); @@ -161,7 +160,7 @@ pub(super) fn simplify_call( slice.push_front(*elem); } - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); + let new_slice_length = increment_slice_length(arguments[0], dfg, block); let new_slice = make_array(dfg, slice, element_type, block, call_stack); SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) @@ -201,7 +200,7 @@ pub(super) fn simplify_call( slice.pop_front().expect("There are no elements in this slice to be removed") }); - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); + let new_slice_length = decrement_slice_length(arguments[0], dfg, block); results.push(new_slice_length); @@ -234,7 +233,7 @@ pub(super) fn simplify_call( index += 1; } - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); + let new_slice_length = increment_slice_length(arguments[0], dfg, block); let new_slice = make_array(dfg, slice, typ, block, call_stack); SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) @@ -272,7 +271,7 @@ pub(super) fn simplify_call( let new_slice = make_array(dfg, slice, typ, block, call_stack); results.insert(0, new_slice); - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); + let new_slice_length = decrement_slice_length(arguments[0], dfg, block); results.insert(0, new_slice_length); @@ -388,6 +387,22 @@ fn update_slice_length( dfg.insert_instruction_and_results(instruction, block, None, call_stack).first() } +fn increment_slice_length( + slice_len: ValueId, + dfg: &mut DataFlowGraph, + block: BasicBlockId, +) -> ValueId { + update_slice_length(slice_len, dfg, BinaryOp::Add { unchecked: false }, block) +} + +fn decrement_slice_length( + slice_len: ValueId, + dfg: &mut DataFlowGraph, + block: BasicBlockId, +) -> ValueId { + update_slice_length(slice_len, dfg, BinaryOp::Sub { unchecked: true }, block) +} + fn simplify_slice_push_back( mut slice: im::Vector, element_type: Type, @@ -408,7 +423,7 @@ fn simplify_slice_push_back( .insert_instruction_and_results(len_not_equals_capacity_instr, block, None, call_stack) .first(); - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); + let new_slice_length = increment_slice_length(arguments[0], dfg, block); for elem in &arguments[2..] { slice.push_back(*elem); @@ -457,14 +472,17 @@ fn simplify_slice_pop_back( let element_count = element_types.len(); let mut results = VecDeque::with_capacity(element_count + 1); - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); + let new_slice_length = decrement_slice_length(arguments[0], dfg, block); let element_size = dfg.make_constant((element_count as u128).into(), NumericType::length_type()); - let flattened_len_instr = Instruction::binary(BinaryOp::Mul, arguments[0], element_size); + // Compute the flattened length doing an unchecked mul + // (it shouldn't overflow because it would have overflowed before when the slice was created) + let flattened_len_instr = + Instruction::binary(BinaryOp::Mul { unchecked: true }, arguments[0], element_size); let mut flattened_len = dfg.insert_instruction_and_results(flattened_len_instr, block, None, call_stack).first(); - flattened_len = update_slice_length(flattened_len, dfg, BinaryOp::Sub, block); + flattened_len = decrement_slice_length(flattened_len, dfg, block); // We must pop multiple elements in the case of a slice of tuples // Iterating through element types in reverse here since we're popping from the end @@ -478,7 +496,7 @@ fn simplify_slice_pop_back( .first(); results.push_front(get_last_elem); - flattened_len = update_slice_length(flattened_len, dfg, BinaryOp::Sub, block); + flattened_len = decrement_slice_length(flattened_len, dfg, block); } results.push_front(arguments[1]); diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/constrain.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/constrain.rs index 5ae6a642a5..a3881419a8 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/constrain.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/constrain.rs @@ -44,7 +44,7 @@ pub(super) fn decompose_constrain( vec![Instruction::Constrain(lhs, rhs, msg.clone())] } - Instruction::Binary(Binary { lhs, rhs, operator: BinaryOp::Mul }) + Instruction::Binary(Binary { lhs, rhs, operator: BinaryOp::Mul { .. } }) if constant.is_one() && dfg.type_of_value(lhs) == Type::bool() => { // Replace an equality assertion on a boolean multiplication diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 7fdcb4c26c..eed1af8251 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -507,16 +507,18 @@ fn apply_side_effects( let casted_condition = dfg.insert_instruction_and_results(cast, block_id, None, call_stack); let casted_condition = casted_condition.first(); + // Unchecked mul because the side effects var is always 0 or 1 let lhs = dfg.insert_instruction_and_results( - Instruction::binary(BinaryOp::Mul, lhs, casted_condition), + Instruction::binary(BinaryOp::Mul { unchecked: true }, lhs, casted_condition), block_id, None, call_stack, ); let lhs = lhs.first(); + // Unchecked mul because the side effects var is always 0 or 1 let rhs = dfg.insert_instruction_and_results( - Instruction::binary(BinaryOp::Mul, rhs, casted_condition), + Instruction::binary(BinaryOp::Mul { unchecked: true }, rhs, casted_condition), block_id, None, call_stack, diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 748867c740..76f8495c00 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -654,20 +654,10 @@ impl<'f> Context<'f> { // Replace constraint `lhs == rhs` with `condition * lhs == condition * rhs`. // Condition needs to be cast to argument type in order to multiply them together. - let argument_type = self.inserter.function.dfg.type_of_value(lhs); - - let cast = Instruction::Cast(condition, argument_type.unwrap_numeric()); - let casted_condition = self.insert_instruction(cast, call_stack); - - let lhs = self.insert_instruction( - Instruction::binary(BinaryOp::Mul, lhs, casted_condition), - call_stack, - ); - let rhs = self.insert_instruction( - Instruction::binary(BinaryOp::Mul, rhs, casted_condition), - call_stack, - ); - + let casted_condition = + self.cast_condition_to_value_type(condition, lhs, call_stack); + let lhs = self.mul_by_condition(lhs, casted_condition, call_stack); + let rhs = self.mul_by_condition(rhs, casted_condition, call_stack); Instruction::Constrain(lhs, rhs, message) } Instruction::Store { address, value } => { @@ -700,28 +690,18 @@ impl<'f> Context<'f> { // Replace value with `value * predicate` to zero out value when predicate is inactive. // Condition needs to be cast to argument type in order to multiply them together. - let argument_type = self.inserter.function.dfg.type_of_value(value); - let cast = Instruction::Cast(condition, argument_type.unwrap_numeric()); - let casted_condition = self.insert_instruction(cast, call_stack); - - let value = self.insert_instruction( - Instruction::binary(BinaryOp::Mul, value, casted_condition), - call_stack, - ); + let casted_condition = + self.cast_condition_to_value_type(condition, value, call_stack); + let value = self.mul_by_condition(value, casted_condition, call_stack); Instruction::RangeCheck { value, max_bit_size, assert_message } } Instruction::Call { func, mut arguments } => match self.inserter.function.dfg[func] { Value::Intrinsic(Intrinsic::ToBits(_) | Intrinsic::ToRadix(_)) => { let field = arguments[0]; - let argument_type = self.inserter.function.dfg.type_of_value(field); - - let cast = Instruction::Cast(condition, argument_type.unwrap_numeric()); - let casted_condition = self.insert_instruction(cast, call_stack); - let field = self.insert_instruction( - Instruction::binary(BinaryOp::Mul, field, casted_condition), - call_stack, - ); + let casted_condition = + self.cast_condition_to_value_type(condition, field, call_stack); + let field = self.mul_by_condition(field, casted_condition, call_stack); arguments[0] = field; @@ -765,6 +745,30 @@ impl<'f> Context<'f> { } } + fn cast_condition_to_value_type( + &mut self, + condition: ValueId, + value: ValueId, + call_stack: CallStackId, + ) -> ValueId { + let argument_type = self.inserter.function.dfg.type_of_value(value); + let cast = Instruction::Cast(condition, argument_type.unwrap_numeric()); + self.insert_instruction(cast, call_stack) + } + + fn mul_by_condition( + &mut self, + value: ValueId, + condition: ValueId, + call_stack: CallStackId, + ) -> ValueId { + // Unchecked mul because the condition is always 0 or 1 + self.insert_instruction( + Instruction::binary(BinaryOp::Mul { unchecked: true }, value, condition), + call_stack, + ) + } + /// When a MSM is done under a predicate, we need to apply the predicate /// to the is_infinity property of the input points in order to ensure /// that the points will be on the curve no matter what. @@ -797,11 +801,11 @@ impl<'f> Context<'f> { // Computes: if condition { var } else { 1 } fn var_or_one(&mut self, var: ValueId, condition: ValueId, call_stack: CallStackId) -> ValueId { - let field = - self.insert_instruction(Instruction::binary(BinaryOp::Mul, var, condition), call_stack); + let field = self.mul_by_condition(var, condition, call_stack); let not_condition = self.not_instruction(condition, call_stack); + // Unchecked add because of the values is guaranteed to be 0 self.insert_instruction( - Instruction::binary(BinaryOp::Add, field, not_condition), + Instruction::binary(BinaryOp::Add { unchecked: true }, field, not_condition), call_stack, ) } @@ -1279,15 +1283,15 @@ mod test { v12 = not v5 v13 = cast v4 as u8 v14 = cast v12 as u8 - v15 = mul v13, v10 - v16 = mul v14, v11 - v17 = add v15, v16 + v15 = unchecked_mul v13, v10 + v16 = unchecked_mul v14, v11 + v17 = unchecked_add v15, v16 store v17 at v6 enable_side_effects v12 v18 = load v6 -> u8 v19 = cast v12 as u8 v20 = cast v4 as u8 - v21 = mul v20, v18 + v21 = unchecked_mul v20, v18 store v21 at v6 enable_side_effects u1 1 constrain v5 == u1 1 diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs index df351d6c0c..f4638cf85e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs @@ -121,13 +121,18 @@ impl<'a> ValueMerger<'a> { let else_condition = dfg.insert_instruction_and_results(cast, block, None, call_stack).first(); - let mul = Instruction::binary(BinaryOp::Mul, then_condition, then_value); + // Unchecked mul because `then_condition` will be 1 or 0 + let mul = + Instruction::binary(BinaryOp::Mul { unchecked: true }, then_condition, then_value); let then_value = dfg.insert_instruction_and_results(mul, block, None, call_stack).first(); - let mul = Instruction::binary(BinaryOp::Mul, else_condition, else_value); + // Unchecked mul because `else_condition` will be 1 or 0 + let mul = + Instruction::binary(BinaryOp::Mul { unchecked: true }, else_condition, else_value); let else_value = dfg.insert_instruction_and_results(mul, block, None, call_stack).first(); - let add = Instruction::binary(BinaryOp::Add, then_value, else_value); + // Unchecked add because one of the values will always be 0 + let add = Instruction::binary(BinaryOp::Add { unchecked: true }, then_value, else_value); dfg.insert_instruction_and_results(add, block, None, call_stack).first() } diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 3695548072..990e8954b1 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -936,7 +936,8 @@ mod test { // Compiling square f1 builder.new_function("square".into(), square_id, InlineType::default()); let square_v0 = builder.add_parameter(Type::field()); - let square_v2 = builder.insert_binary(square_v0, BinaryOp::Mul, square_v0); + let square_v2 = + builder.insert_binary(square_v0, BinaryOp::Mul { unchecked: false }, square_v0); builder.terminate_with_return(vec![square_v2]); // Compiling id1 f2 @@ -1001,9 +1002,9 @@ mod test { builder.switch_to_block(b2); let factorial_id = builder.import_function(factorial_id); - let v2 = builder.insert_binary(v0, BinaryOp::Sub, one); + let v2 = builder.insert_binary(v0, BinaryOp::Sub { unchecked: false }, one); let v3 = builder.insert_call(factorial_id, vec![v2], vec![Type::field()])[0]; - let v4 = builder.insert_binary(v0, BinaryOp::Mul, v3); + let v4 = builder.insert_binary(v0, BinaryOp::Mul { unchecked: false }, v3); builder.terminate_with_return(vec![v4]); let ssa = builder.finish(); diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index c1f2a6b88e..f0f677e5db 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -233,7 +233,7 @@ impl<'f> LoopInvariantContext<'f> { } } Instruction::Binary(binary) => { - if !matches!(binary.operator, BinaryOp::Add | BinaryOp::Mul) { + if !matches!(binary.operator, BinaryOp::Add { .. } | BinaryOp::Mul { .. }) { return false; } diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 29ddfff323..96a2458383 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -1002,7 +1002,7 @@ mod tests { let two = builder.field_constant(2u128); builder.insert_store(v5, two); let one = builder.field_constant(1u128); - let v3_plus_one = builder.insert_binary(v3, BinaryOp::Add, one); + let v3_plus_one = builder.insert_binary(v3, BinaryOp::Add { unchecked: false }, one); builder.terminate_with_jmp(b1, vec![v3_plus_one]); builder.switch_to_block(b3); diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs index 0af8fbb0b5..e36be71aee 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs @@ -133,15 +133,23 @@ impl Context<'_> { let predicate = self.insert_cast(overflow, typ); let pow = self.pow(base, rhs); let pow = self.insert_cast(pow, typ); - (FieldElement::max_num_bits(), self.insert_binary(predicate, BinaryOp::Mul, pow)) + + // Unchecked mul because `predicate` will be 1 or 0 + ( + FieldElement::max_num_bits(), + self.insert_binary(predicate, BinaryOp::Mul { unchecked: true }, pow), + ) }; if max_bit <= bit_size { - self.insert_binary(lhs, BinaryOp::Mul, pow) + // Unchecked mul as it can't overflow + self.insert_binary(lhs, BinaryOp::Mul { unchecked: true }, pow) } else { let lhs_field = self.insert_cast(lhs, NumericType::NativeField); let pow_field = self.insert_cast(pow, NumericType::NativeField); - let result = self.insert_binary(lhs_field, BinaryOp::Mul, pow_field); + // Unchecked mul as this is a wrapping operation that we later truncate + let result = + self.insert_binary(lhs_field, BinaryOp::Mul { unchecked: true }, pow_field); let result = self.insert_truncate(result, bit_size, max_bit); self.insert_cast(result, typ) } @@ -159,6 +167,7 @@ impl Context<'_> { let lhs_typ = self.function.dfg.type_of_value(lhs).unwrap_numeric(); let base = self.field_constant(FieldElement::from(2_u128)); let pow = self.pow(base, rhs); + let pow = self.insert_cast(pow, lhs_typ); if lhs_typ.is_unsigned() { // unsigned right bit shift is just a normal division self.insert_binary(lhs, BinaryOp::Div, pow) @@ -169,14 +178,29 @@ impl Context<'_> { let lhs_sign_as_field = self.insert_cast(lhs_sign, NumericType::NativeField); let lhs_as_field = self.insert_cast(lhs, NumericType::NativeField); // For negative numbers, convert to 1-complement using wrapping addition of a + 1 - let one_complement = self.insert_binary(lhs_sign_as_field, BinaryOp::Add, lhs_as_field); + // Unchecked add as these are fields + let one_complement = self.insert_binary( + lhs_sign_as_field, + BinaryOp::Add { unchecked: true }, + lhs_as_field, + ); let one_complement = self.insert_truncate(one_complement, bit_size, bit_size + 1); let one_complement = self.insert_cast(one_complement, NumericType::signed(bit_size)); // Performs the division on the 1-complement (or the operand if positive) let shifted_complement = self.insert_binary(one_complement, BinaryOp::Div, pow); // Convert back to 2-complement representation if operand is negative let lhs_sign_as_int = self.insert_cast(lhs_sign, lhs_typ); - let shifted = self.insert_binary(shifted_complement, BinaryOp::Sub, lhs_sign_as_int); + + // The requirements for this to underflow are all of these: + // - lhs < 0 + // - ones_complement(lhs) / (2^rhs) == 0 + // As the upper bit is set for the ones complement of negative numbers we'd need 2^rhs + // to be larger than the lhs bitsize for this to overflow. + let shifted = self.insert_binary( + shifted_complement, + BinaryOp::Sub { unchecked: true }, + lhs_sign_as_int, + ); self.insert_truncate(shifted, bit_size, bit_size + 1) } } @@ -200,17 +224,18 @@ impl Context<'_> { let rhs_bits = rhs_bits[0]; let one = self.field_constant(FieldElement::one()); let mut r = one; + // All operations are unchecked as we're acting on Field types (which are always unchecked) for i in 1..bit_size + 1 { - let r_squared = self.insert_binary(r, BinaryOp::Mul, r); - let a = self.insert_binary(r_squared, BinaryOp::Mul, lhs); + let r_squared = self.insert_binary(r, BinaryOp::Mul { unchecked: true }, r); + let a = self.insert_binary(r_squared, BinaryOp::Mul { unchecked: true }, lhs); let idx = self.field_constant(FieldElement::from((bit_size - i) as i128)); let b = self.insert_array_get(rhs_bits, idx, Type::bool()); let not_b = self.insert_not(b); let b = self.insert_cast(b, NumericType::NativeField); let not_b = self.insert_cast(not_b, NumericType::NativeField); - let r1 = self.insert_binary(a, BinaryOp::Mul, b); - let r2 = self.insert_binary(r_squared, BinaryOp::Mul, not_b); - r = self.insert_binary(r1, BinaryOp::Add, r2); + let r1 = self.insert_binary(a, BinaryOp::Mul { unchecked: true }, b); + let r2 = self.insert_binary(r_squared, BinaryOp::Mul { unchecked: true }, not_b); + r = self.insert_binary(r1, BinaryOp::Add { unchecked: true }, r2); } r } else { diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs index a22232ba49..ca9b75643b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs @@ -126,7 +126,7 @@ impl Context { use Instruction::*; match instruction { Binary(binary) => match binary.operator { - BinaryOp::Add | BinaryOp::Sub | BinaryOp::Mul => { + BinaryOp::Add { .. } | BinaryOp::Sub { .. } | BinaryOp::Mul { .. } => { dfg.type_of_value(binary.lhs).is_unsigned() } BinaryOp::Div | BinaryOp::Mod => { @@ -239,13 +239,13 @@ mod test { let one = builder.numeric_constant(1u128, NumericType::bool()); builder.insert_enable_side_effects_if(one); - builder.insert_binary(v0, BinaryOp::Mul, two); + builder.insert_binary(v0, BinaryOp::Mul { unchecked: false }, two); builder.insert_enable_side_effects_if(one); - builder.insert_binary(v0, BinaryOp::Mul, two); + builder.insert_binary(v0, BinaryOp::Mul { unchecked: false }, two); builder.insert_enable_side_effects_if(one); - builder.insert_binary(v0, BinaryOp::Mul, two); + builder.insert_binary(v0, BinaryOp::Mul { unchecked: false }, two); builder.insert_enable_side_effects_if(one); - builder.insert_binary(v0, BinaryOp::Mul, two); + builder.insert_binary(v0, BinaryOp::Mul { unchecked: false }, two); builder.insert_enable_side_effects_if(one); let ssa = builder.finish(); @@ -275,7 +275,10 @@ mod test { assert_eq!(instructions.len(), 4); for instruction in instructions.iter().take(4) { - assert_eq!(&main.dfg[*instruction], &Instruction::binary(BinaryOp::Mul, v0, two)); + assert_eq!( + &main.dfg[*instruction], + &Instruction::binary(BinaryOp::Mul { unchecked: false }, v0, two) + ); } } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index ab4256197b..79181b7e74 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -619,10 +619,16 @@ impl Loop { let header = &function.dfg[self.header]; let induction_var = header.parameters()[0]; - back.instructions().iter().filter(|instruction| { - let instruction = &function.dfg[**instruction]; - matches!(instruction, Instruction::Binary(Binary { lhs, operator: BinaryOp::Add, rhs: _ }) if *lhs == induction_var) - }).count() + back.instructions() + .iter() + .filter(|instruction| { + let instruction = &function.dfg[**instruction]; + matches!(instruction, + Instruction::Binary(Binary { lhs, operator: BinaryOp::Add { .. }, rhs: _ }) + if *lhs == induction_var + ) + }) + .count() } /// Decide if this loop is small enough that it can be inlined in a way that the number diff --git a/compiler/noirc_evaluator/src/ssa/parser/mod.rs b/compiler/noirc_evaluator/src/ssa/parser/mod.rs index 96aef1f3c3..143ba51187 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/mod.rs @@ -286,9 +286,9 @@ impl<'a> Parser<'a> { fn eat_binary_op(&mut self) -> ParseResult> { let op = match self.token.token() { - Token::Keyword(Keyword::Add) => BinaryOp::Add, - Token::Keyword(Keyword::Sub) => BinaryOp::Sub, - Token::Keyword(Keyword::Mul) => BinaryOp::Mul, + Token::Keyword(Keyword::Add) => BinaryOp::Add { unchecked: false }, + Token::Keyword(Keyword::Sub) => BinaryOp::Sub { unchecked: false }, + Token::Keyword(Keyword::Mul) => BinaryOp::Mul { unchecked: false }, Token::Keyword(Keyword::Div) => BinaryOp::Div, Token::Keyword(Keyword::Eq) => BinaryOp::Eq, Token::Keyword(Keyword::Mod) => BinaryOp::Mod, @@ -298,6 +298,9 @@ impl<'a> Parser<'a> { Token::Keyword(Keyword::Xor) => BinaryOp::Xor, Token::Keyword(Keyword::Shl) => BinaryOp::Shl, Token::Keyword(Keyword::Shr) => BinaryOp::Shr, + Token::Keyword(Keyword::UncheckedAdd) => BinaryOp::Add { unchecked: true }, + Token::Keyword(Keyword::UncheckedSub) => BinaryOp::Sub { unchecked: true }, + Token::Keyword(Keyword::UncheckedMul) => BinaryOp::Mul { unchecked: true }, _ => return Ok(None), }; diff --git a/compiler/noirc_evaluator/src/ssa/parser/tests.rs b/compiler/noirc_evaluator/src/ssa/parser/tests.rs index b5aac13cfd..8c24b2ec45 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/tests.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/tests.rs @@ -326,11 +326,27 @@ fn test_array_get_set_bug() { #[test] fn test_binary() { - for op in ["add", "sub", "mul", "div", "eq", "mod", "lt", "and", "or", "xor", "shl", "shr"] { + for op in [ + "add", + "sub", + "mul", + "div", + "eq", + "mod", + "lt", + "and", + "or", + "xor", + "shl", + "shr", + "unchecked_add", + "unchecked_sub", + "unchecked_mul", + ] { let src = format!( " acir(inline) fn main f0 {{ - b0(v0: Field, v1: Field): + b0(v0: u32, v1: u32): v2 = {op} v0, v1 return }} diff --git a/compiler/noirc_evaluator/src/ssa/parser/token.rs b/compiler/noirc_evaluator/src/ssa/parser/token.rs index 83a2a1d1ed..eb09209466 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/token.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/token.rs @@ -160,6 +160,9 @@ pub(crate) enum Keyword { Then, To, Truncate, + UncheckedAdd, + UncheckedSub, + UncheckedMul, Value, Xor, } @@ -217,6 +220,9 @@ impl Keyword { "then" => Keyword::Then, "to" => Keyword::To, "truncate" => Keyword::Truncate, + "unchecked_add" => Keyword::UncheckedAdd, + "unchecked_sub" => Keyword::UncheckedSub, + "unchecked_mul" => Keyword::UncheckedMul, "value" => Keyword::Value, "xor" => Keyword::Xor, _ => return None, @@ -278,6 +284,9 @@ impl Display for Keyword { Keyword::Then => write!(f, "then"), Keyword::To => write!(f, "to"), Keyword::Truncate => write!(f, "truncate"), + Keyword::UncheckedAdd => write!(f, "unchecked_add"), + Keyword::UncheckedSub => write!(f, "unchecked_sub"), + Keyword::UncheckedMul => write!(f, "unchecked_mul"), Keyword::Value => write!(f, "value"), Keyword::Xor => write!(f, "xor"), } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index e89d1d2a0c..23166386dd 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -317,12 +317,24 @@ impl<'a> FunctionContext<'a> { // We use unsafe casts here, this is fine as we're casting to a `field` type. let as_field = self.builder.insert_cast(input, NumericType::NativeField); let sign_field = self.builder.insert_cast(sign, NumericType::NativeField); - let positive_predicate = self.builder.insert_binary(sign_field, BinaryOp::Mul, as_field); - let two_complement = self.builder.insert_binary(bit_width, BinaryOp::Sub, as_field); + + // All of these operations are unchecked because they deal with fields + let positive_predicate = + self.builder.insert_binary(sign_field, BinaryOp::Mul { unchecked: true }, as_field); + let two_complement = + self.builder.insert_binary(bit_width, BinaryOp::Sub { unchecked: true }, as_field); let sign_not_field = self.builder.insert_cast(sign_not, NumericType::NativeField); - let negative_predicate = - self.builder.insert_binary(sign_not_field, BinaryOp::Mul, two_complement); - self.builder.insert_binary(positive_predicate, BinaryOp::Add, negative_predicate) + let negative_predicate = self.builder.insert_binary( + sign_not_field, + BinaryOp::Mul { unchecked: true }, + two_complement, + ); + // Unchecked addition because either `positive_predicate` or `negative_predicate` will be 0 + self.builder.insert_binary( + positive_predicate, + BinaryOp::Add { unchecked: true }, + negative_predicate, + ) } /// Insert constraints ensuring that the operation does not overflow the bit size of the result @@ -477,8 +489,12 @@ impl<'a> FunctionContext<'a> { //Check the result has the same sign as its inputs let result_sign = self.builder.insert_binary(result, BinaryOp::Lt, half_width); let sign_diff = self.builder.insert_binary(result_sign, BinaryOp::Eq, lhs_sign); - let sign_diff_with_predicate = - self.builder.insert_binary(sign_diff, BinaryOp::Mul, same_sign); + // Unchecked multiplication because boolean inputs + let sign_diff_with_predicate = self.builder.insert_binary( + sign_diff, + BinaryOp::Mul { unchecked: true }, + same_sign, + ); let overflow_check = Instruction::Constrain( sign_diff_with_predicate, same_sign, @@ -491,7 +507,9 @@ impl<'a> FunctionContext<'a> { // First we compute the absolute value of operands, and their product let lhs_abs = self.absolute_value_helper(lhs, lhs_sign, bit_size); let rhs_abs = self.absolute_value_helper(rhs, rhs_sign, bit_size); - let product_field = self.builder.insert_binary(lhs_abs, BinaryOp::Mul, rhs_abs); + // Unchecked mul because these are fields + let product_field = + self.builder.insert_binary(lhs_abs, BinaryOp::Mul { unchecked: true }, rhs_abs); // It must not already overflow the bit_size self.builder.set_location(location).insert_range_check( product_field, @@ -505,8 +523,12 @@ impl<'a> FunctionContext<'a> { let not_same = self.builder.insert_not(same_sign); let not_same_sign_field = self.insert_safe_cast(not_same, NumericType::unsigned(bit_size), location); - let positive_maximum_with_offset = - self.builder.insert_binary(half_width, BinaryOp::Add, not_same_sign_field); + // Unchecked add because adding 1 to half_width can't overflow + let positive_maximum_with_offset = self.builder.insert_binary( + half_width, + BinaryOp::Add { unchecked: true }, + not_same_sign_field, + ); let product_overflow_check = self.builder.insert_binary(product, BinaryOp::Lt, positive_maximum_with_offset); @@ -610,7 +632,8 @@ impl<'a> FunctionContext<'a> { if offset != 0 { let typ = self.builder.type_of_value(address).unwrap_numeric(); let offset = self.builder.numeric_constant(offset, typ); - address = self.builder.insert_binary(address, BinaryOp::Add, offset); + address = + self.builder.insert_binary(address, BinaryOp::Add { unchecked: true }, offset); } address } @@ -868,14 +891,20 @@ impl<'a> FunctionContext<'a> { self.builder.numeric_constant(self.element_size(array), NumericType::length_type()); // The actual base index is the user's index * the array element type's size - let mut index = - self.builder.set_location(location).insert_binary(index, BinaryOp::Mul, element_size); + // Unchecked mul because we are reaching for an array element: if it overflows here + // it would have overflowed when creating the array. + let mut index = self.builder.set_location(location).insert_binary( + index, + BinaryOp::Mul { unchecked: true }, + element_size, + ); let one = self.builder.numeric_constant(FieldElement::one(), NumericType::length_type()); new_value.for_each(|value| { let value = value.eval(self); array = self.builder.insert_array_set(array, index, value); - index = self.builder.insert_binary(index, BinaryOp::Add, one); + // Unchecked add because this can't overflow (it would have overflowed when creating the array) + index = self.builder.insert_binary(index, BinaryOp::Add { unchecked: true }, one); }); array } @@ -1000,9 +1029,9 @@ fn operator_requires_swapped_operands(op: BinaryOpKind) -> bool { /// to represent the full operation correctly. fn convert_operator(op: BinaryOpKind) -> BinaryOp { match op { - BinaryOpKind::Add => BinaryOp::Add, - BinaryOpKind::Subtract => BinaryOp::Sub, - BinaryOpKind::Multiply => BinaryOp::Mul, + BinaryOpKind::Add => BinaryOp::Add { unchecked: false }, + BinaryOpKind::Subtract => BinaryOp::Sub { unchecked: false }, + BinaryOpKind::Multiply => BinaryOp::Mul { unchecked: false }, BinaryOpKind::Divide => BinaryOp::Div, BinaryOpKind::Modulo => BinaryOp::Mod, BinaryOpKind::Equal => BinaryOp::Eq, diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 10b8129e8d..28ff544b85 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -446,8 +446,13 @@ impl<'a> FunctionContext<'a> { let type_size = Self::convert_type(element_type).size_of_type(); let type_size = self.builder.numeric_constant(type_size as u128, NumericType::length_type()); - let base_index = - self.builder.set_location(location).insert_binary(index, BinaryOp::Mul, type_size); + // This shouldn't overflow as we are reaching for an initial array offset + // (otherwise it would have overflowed when creating the array) + let base_index = self.builder.set_location(location).insert_binary( + index, + BinaryOp::Mul { unchecked: true }, + type_size, + ); let mut field_index = 0u128; Ok(Self::map_type(element_type, |typ| { @@ -702,7 +707,12 @@ impl<'a> FunctionContext<'a> { // We add one here in the case of a slice insert as a slice insert at the length of the slice // can be converted to a slice push back - let len_plus_one = self.builder.insert_binary(arguments[0], BinaryOp::Add, one); + // This is unchecked as the slice length could be u32::max + let len_plus_one = self.builder.insert_binary( + arguments[0], + BinaryOp::Add { unchecked: false }, + one, + ); self.codegen_slice_access_check(arguments[2], Some(len_plus_one)); } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index 98cdc0adad..068160bd7f 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -129,8 +129,8 @@ mod test { let one = builder.field_constant(1u128); let three = builder.field_constant(3u128); - let v1 = builder.insert_binary(v0, BinaryOp::Add, one); - let v2 = builder.insert_binary(v1, BinaryOp::Mul, three); + let v1 = builder.insert_binary(v0, BinaryOp::Add { unchecked: false }, one); + let v2 = builder.insert_binary(v1, BinaryOp::Mul { unchecked: false }, three); builder.terminate_with_return(vec![v2]); let ssa = builder.finish(); diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index 13a9400bd2..ec4d33c3ca 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -1424,6 +1424,7 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { let (mut lhs, lhs_is_negative) = match evaluated_lhs { Value::Field(value) => (value, false), + Value::U1(value) => ((value as u128).into(), false), Value::U8(value) => ((value as u128).into(), false), Value::U16(value) => ((value as u128).into(), false), Value::U32(value) => ((value as u128).into(), false), diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index 730bcd1be6..3d3d0721c6 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -822,10 +822,10 @@ fn to_le_radix( let value = get_field(value)?; let radix = get_u32(radix)?; - let limb_count = if let Type::Array(length, _) = return_type { + let (limb_count, element_type) = if let Type::Array(length, element_type) = return_type { if let Type::Constant(limb_count, kind) = *length { if kind.unifies(&Kind::u32()) { - limb_count + (limb_count, element_type) } else { return Err(InterpreterError::TypeAnnotationsNeededForMethodCall { location }); } @@ -836,14 +836,29 @@ fn to_le_radix( return Err(InterpreterError::TypeAnnotationsNeededForMethodCall { location }); }; + let return_type_is_bits = + *element_type == Type::Integer(Signedness::Unsigned, IntegerBitSize::One); + // Decompose the integer into its radix digits in little endian form. let decomposed_integer = compute_to_radix_le(value, radix); - let decomposed_integer = - vecmap(0..limb_count.to_u128() as usize, |i| match decomposed_integer.get(i) { - Some(digit) => Value::U8(*digit), - None => Value::U8(0), - }); - let result_type = byte_array_type(decomposed_integer.len()); + let decomposed_integer = vecmap(0..limb_count.to_u128() as usize, |i| { + let digit = match decomposed_integer.get(i) { + Some(digit) => *digit, + None => 0, + }; + // The only built-ins that use these either return `[u1; N]` or `[u8; N]` + if return_type_is_bits { + Value::U1(digit != 0) + } else { + Value::U8(digit) + } + }); + + let result_type = Type::Array( + Box::new(Type::Constant(decomposed_integer.len().into(), Kind::u32())), + element_type, + ); + Ok(Value::Array(decomposed_integer.into(), result_type)) }