Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: right shift is not a regular division #6400

Merged
merged 6 commits into from
Nov 5, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@
Or,
/// Bitwise xor (^)
Xor,
/// Bitshift left (<<)

Check warning on line 42 in compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Bitshift)
Shl,
/// Bitshift right (>>)

Check warning on line 44 in compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Bitshift)
Shr,
}

Expand Down Expand Up @@ -281,15 +281,7 @@
let zero = dfg.make_constant(FieldElement::zero(), operand_type);
return SimplifyResult::SimplifiedTo(zero);
}

// `two_pow_rhs` is limited to be at most `2 ^ {operand_bitsize - 1}` so it fits in `operand_type`.
let two_pow_rhs = FieldElement::from(2u128).pow(&rhs_const);
let two_pow_rhs = dfg.make_constant(two_pow_rhs, operand_type);
return SimplifyResult::SimplifiedToInstruction(Instruction::binary(
BinaryOp::Div,
self.lhs,
two_pow_rhs,
));
return SimplifyResult::None;
}
}
};
Expand Down
33 changes: 23 additions & 10 deletions compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ impl Context<'_> {
}

/// Insert ssa instructions which computes lhs >> rhs by doing lhs/2^rhs
/// For negative signed integers, we do the division on the 1-complement representation of lhs,
/// before converting back the result to the 2-complement representation.
pub(crate) fn insert_shift_right(
&mut self,
lhs: ValueId,
Expand All @@ -153,16 +155,27 @@ impl Context<'_> {
) -> ValueId {
let lhs_typ = self.function.dfg.type_of_value(lhs);
let base = self.field_constant(FieldElement::from(2_u128));
// we can safely cast to unsigned because overflow_checks prevent bit-shift with a negative value
let rhs_unsigned = self.insert_cast(rhs, Type::unsigned(bit_size));
let pow = self.pow(base, rhs_unsigned);
// We need at least one more bit for the case where rhs == bit_size
let div_type = Type::unsigned(bit_size + 1);
let casted_lhs = self.insert_cast(lhs, div_type.clone());
let casted_pow = self.insert_cast(pow, div_type);
let div_result = self.insert_binary(casted_lhs, BinaryOp::Div, casted_pow);
// We have to cast back to the original type
self.insert_cast(div_result, lhs_typ)
let pow = self.pow(base, rhs);
if lhs_typ.is_unsigned() {
// unsigned right bit shift is just a normal division
self.insert_binary(lhs, BinaryOp::Div, pow)
} else {
// Get the sign of the operand; positive signed operand will just do a division as well
let zero = self.numeric_constant(FieldElement::zero(), Type::signed(bit_size));
let lhs_sign = self.insert_binary(lhs, BinaryOp::Lt, zero);
let lhs_sign_as_field = self.insert_cast(lhs_sign, Type::field());
let lhs_as_field = self.insert_cast(lhs, Type::field());
// 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);
let one_complement = self.insert_truncate(one_complement, bit_size, bit_size + 1);
let one_complement = self.insert_cast(one_complement, Type::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);
self.insert_truncate(shifted, bit_size, bit_size + 1)
}
}

/// Computes lhs^rhs via square&multiply, using the bits decomposition of rhs
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ fn main(x: u64) {
assert(x << 63 == 0);

assert_eq((1 as u64) << 32, 0x0100000000);

//regression for 6201
let a: i16 = -769;
assert_eq(a >> 3, -97);
}

fn regression_2250() {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
fn main(x: u64, y: u8) {
fn main(x: u64, y: u8, a: i16) {
// runtime shifts on compile-time known values
guipublic marked this conversation as resolved.
Show resolved Hide resolved
assert(64 << y == 128);
assert(64 >> y == 32);
Expand All @@ -17,4 +17,6 @@ fn main(x: u64, y: u8) {
assert(a << y == -2);

assert(x >> (x as u8) == 0);

assert_eq(a >> 3, -97);
}
guipublic marked this conversation as resolved.
Show resolved Hide resolved
Loading