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 980fab05d9..e8e23bbb79 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs @@ -134,22 +134,22 @@ impl Context<'_> { let pow = self.pow(base, rhs); let pow = self.insert_cast(pow, typ); - // TODO: should this be unchecked? + // Unchecked mul because predicate is 0 or 1 ( FieldElement::max_num_bits(), - self.insert_binary(predicate, BinaryOp::Mul { unchecked: false }, pow), + self.insert_binary(predicate, BinaryOp::Mul { unchecked: true }, pow), ) }; if max_bit <= bit_size { - // TODO: should this be unchecked? - self.insert_binary(lhs, BinaryOp::Mul { unchecked: false }, 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); - // TODO: should this be unchecked? + // Unchecked mul as this is a wrapping operation that we later truncate let result = - self.insert_binary(lhs_field, BinaryOp::Mul { unchecked: false }, pow_field); + 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) } @@ -177,10 +177,10 @@ 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 - // TODO: should this be unchecked? + // Unchecked add as these are fields let one_complement = self.insert_binary( lhs_sign_as_field, - BinaryOp::Add { unchecked: false }, + BinaryOp::Add { unchecked: true }, lhs_as_field, ); let one_complement = self.insert_truncate(one_complement, bit_size, bit_size + 1); @@ -189,10 +189,10 @@ impl Context<'_> { 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); - // TODO: should this be unchecked? + // This shouldn't underflow let shifted = self.insert_binary( shifted_complement, - BinaryOp::Sub { unchecked: false }, + BinaryOp::Sub { unchecked: true }, lhs_sign_as_int, ); self.insert_truncate(shifted, bit_size, bit_size + 1) @@ -219,21 +219,17 @@ impl Context<'_> { let one = self.field_constant(FieldElement::one()); let mut r = one; for i in 1..bit_size + 1 { - // TODO: should this be unchecked? - let r_squared = self.insert_binary(r, BinaryOp::Mul { unchecked: false }, r); - // TODO: should this be unchecked? - let a = self.insert_binary(r_squared, BinaryOp::Mul { unchecked: false }, lhs); + // All of these operations are unchecked because they deal with fields + 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); - // TODO: should this be unchecked? - let r1 = self.insert_binary(a, BinaryOp::Mul { unchecked: false }, b); - // TODO: should this be unchecked? - let r2 = self.insert_binary(r_squared, BinaryOp::Mul { unchecked: false }, not_b); - // TODO: should this be unchecked? - r = self.insert_binary(r1, BinaryOp::Add { unchecked: false }, 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/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 12ca9aeb5e..d9da8c94d9 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -317,23 +317,21 @@ 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); - // TODO: should this be unchecked? + + // All of these operations are unchecked because they deal with fields let positive_predicate = - self.builder.insert_binary(sign_field, BinaryOp::Mul { unchecked: false }, as_field); - // TODO: should this be unchecked? + self.builder.insert_binary(sign_field, BinaryOp::Mul { unchecked: true }, as_field); let two_complement = - self.builder.insert_binary(bit_width, BinaryOp::Sub { unchecked: false }, as_field); + self.builder.insert_binary(bit_width, BinaryOp::Sub { unchecked: true }, as_field); let sign_not_field = self.builder.insert_cast(sign_not, NumericType::NativeField); - // TODO: should this be unchecked? let negative_predicate = self.builder.insert_binary( sign_not_field, - BinaryOp::Mul { unchecked: false }, + BinaryOp::Mul { unchecked: true }, two_complement, ); - // TODO: should this be unchecked? self.builder.insert_binary( positive_predicate, - BinaryOp::Add { unchecked: false }, + BinaryOp::Add { unchecked: true }, negative_predicate, ) } @@ -490,10 +488,10 @@ 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); - // TODO: should this be unchecked? + // Unchecked mul because sign_diff is a boolean let sign_diff_with_predicate = self.builder.insert_binary( sign_diff, - BinaryOp::Mul { unchecked: false }, + BinaryOp::Mul { unchecked: true }, same_sign, ); let overflow_check = Instruction::Constrain( @@ -508,12 +506,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); - // TODO: should this be unchecked? - let product_field = self.builder.insert_binary( - lhs_abs, - BinaryOp::Mul { unchecked: false }, - 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, @@ -527,10 +522,10 @@ 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); - // TODO: should this be unchecked + // This shouldn't overflow so it's unchecked let positive_maximum_with_offset = self.builder.insert_binary( half_width, - BinaryOp::Add { unchecked: false }, + BinaryOp::Add { unchecked: true }, not_same_sign_field, ); let product_overflow_check = @@ -895,10 +890,11 @@ 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 - // TODO: should this be unchecked? + // 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: false }, + BinaryOp::Mul { unchecked: true }, element_size, ); let one = self.builder.numeric_constant(FieldElement::one(), NumericType::length_type()); @@ -906,8 +902,8 @@ impl<'a> FunctionContext<'a> { new_value.for_each(|value| { let value = value.eval(self); array = self.builder.insert_array_set(array, index, value); - // TODO: should this be unchecked? - index = self.builder.insert_binary(index, BinaryOp::Add { unchecked: false }, 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 } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index d68dab9a0b..28ff544b85 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -446,10 +446,11 @@ 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()); - // TODO: should this be unchecked? + // 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: false }, + BinaryOp::Mul { unchecked: true }, type_size, ); @@ -706,7 +707,7 @@ 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 - // TODO: should this be unchecked? + // 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 },