Skip to content

Commit

Permalink
Handle more TODOs
Browse files Browse the repository at this point in the history
  • Loading branch information
@asterite
asterite committed Jan 10, 2025
1 parent 626f0a2 commit 7840ced
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 45 deletions.
36 changes: 16 additions & 20 deletions compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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);
Expand All @@ -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)
Expand All @@ -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 {
Expand Down
40 changes: 18 additions & 22 deletions compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
}
Expand Down Expand Up @@ -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(
Expand All @@ -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,
Expand All @@ -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 =
Expand Down Expand Up @@ -895,19 +890,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
// 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());

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
}
Expand Down
7 changes: 4 additions & 3 deletions compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);

Expand Down Expand Up @@ -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 },
Expand Down

0 comments on commit 7840ced

Please sign in to comment.