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: prevent incorrect ACIRgen caused by noop truncations #7456

Merged
merged 3 commits into from
Feb 20, 2025
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
25 changes: 13 additions & 12 deletions compiler/noirc_evaluator/src/acir/acir_variable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -842,18 +842,19 @@ impl<F: AcirField, B: BlackBoxFunctionSolver<F>> AcirContext<F, B> {
}

// maximum bit size for q and for [r and rhs]
let mut max_q_bits = bit_size;
let mut max_rhs_bits = bit_size;
// when rhs is constant, we can better estimate the maximum bit sizes
if let Some(rhs_const) = rhs_expr.to_const() {
max_rhs_bits = rhs_const.num_bits();
if max_rhs_bits != 0 {
if max_rhs_bits > bit_size {
return Ok((zero, zero));
}
max_q_bits = bit_size - max_rhs_bits + 1;
}
}
let (max_q_bits, max_rhs_bits) = if let Some(rhs_const) = rhs_expr.to_const() {
// when rhs is constant, we can better estimate the maximum bit sizes
let max_rhs_bits = rhs_const.num_bits();
assert!(
max_rhs_bits <= bit_size,
"attempted to divide by constant larger than operand type"
);

let max_q_bits = bit_size - max_rhs_bits + 1;
(max_q_bits, max_rhs_bits)
} else {
(bit_size, bit_size)
};

let [q_value, r_value]: [AcirValue; 2] = self
.brillig_call(
Expand Down
6 changes: 6 additions & 0 deletions compiler/noirc_evaluator/src/acir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2094,6 +2094,12 @@ impl<'a> Context<'a> {
max_bit_size: u32,
dfg: &DataFlowGraph,
) -> Result<AcirVar, RuntimeError> {
assert_ne!(bit_size, max_bit_size, "Attempted to generate a noop truncation");
assert!(
bit_size < max_bit_size,
"Attempted to generate a truncation into large size than max input"
);

let mut var = self.convert_numeric_value(value_id, dfg)?;
match &dfg[value_id] {
Value::Instruction { instruction, .. } => {
Expand Down
20 changes: 12 additions & 8 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 @@ -107,7 +107,7 @@
}

let operator = if lhs_type == NumericType::NativeField {
// Unchecked operations between fields or bools don't make sense, so we convert those to non-unchecked

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

View workflow job for this annotation

GitHub Actions / Code

Unknown word (bools)
// to reduce noise and confusion in the generated SSA.
match operator {
BinaryOp::Add { unchecked: true } => BinaryOp::Add { unchecked: false },
Expand All @@ -116,7 +116,7 @@
_ => operator,
}
} else if lhs_type == NumericType::bool() {
// Unchecked mul between bools doesn't make sense, so we convert that to non-unchecked

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

View workflow job for this annotation

GitHub Actions / Code

Unknown word (bools)
if let BinaryOp::Mul { unchecked: true } = operator {
BinaryOp::Mul { unchecked: false }
} else {
Expand Down Expand Up @@ -289,7 +289,7 @@
}
if lhs_type == NumericType::bool() {
// Boolean AND is equivalent to multiplication, which is a cheaper operation.
// (mul unchecked because these are bools so it doesn't matter really)

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

View workflow job for this annotation

GitHub Actions / Code

Unknown word (bools)
let instruction =
Instruction::binary(BinaryOp::Mul { unchecked: true }, lhs, rhs);
return SimplifyResult::SimplifiedToInstruction(instruction);
Expand All @@ -306,14 +306,18 @@
let bitmask_plus_one = bitmask.to_u128() + 1;
if bitmask_plus_one.is_power_of_two() {
let value = if lhs_value.is_some() { rhs } else { lhs };
let num_bits = bitmask_plus_one.ilog2();
return SimplifyResult::SimplifiedToInstruction(
Instruction::Truncate {
value,
bit_size: num_bits,
max_bit_size: lhs_type.bit_size(),
},
);
let bit_size = bitmask_plus_one.ilog2();
let max_bit_size = lhs_type.bit_size();

if bit_size == max_bit_size {
// If we're truncating a value into the full size of its type then
// the truncation is a noop.
return SimplifyResult::SimplifiedTo(value);
} else {
return SimplifyResult::SimplifiedToInstruction(
Instruction::Truncate { value, bit_size, max_bit_size },
);
}
}
}

Expand Down Expand Up @@ -645,7 +649,7 @@

let integer_modulus = BigUint::from(2_u128).pow(bit_size);
let truncated_as_bigint = BigUint::from(input)
.modpow(&BigUint::one(), &integer_modulus);

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

View workflow job for this annotation

GitHub Actions / Code

Unknown word (modpow)
let truncated_as_bigint = FieldElement::from_be_bytes_reduce(&truncated_as_bigint.to_bytes_be());
prop_assert_eq!(truncated_as_field, truncated_as_bigint);
}
Expand Down
22 changes: 22 additions & 0 deletions compiler/noirc_evaluator/src/ssa/ir/instruction/constrain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,4 +204,26 @@ mod tests {
";
assert_normalized_ssa_equals(ssa, expected);
}

#[test]
fn simplifies_out_noop_bitwise_ands() {
// Regression test for https://github.com/noir-lang/noir/issues/7451
let src = "
acir(inline) predicate_pure fn main f0 {
b0(v0: u8):
v1 = and u8 255, v0
return v1
}
";

let ssa = Ssa::from_str_simplifying(src).unwrap();

let expected = "
acir(inline) fn main f0 {
b0(v0: u8):
return v0
}
";
assert_normalized_ssa_equals(ssa, expected);
}
}
6 changes: 6 additions & 0 deletions test_programs/execution_success/regression_7451/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[package]
name = "regression_7451"
type = "bin"
authors = [""]

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
x="1"
12 changes: 12 additions & 0 deletions test_programs/execution_success/regression_7451/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
fn main(x: u8) {
// Safety: test code
let predicate = unsafe { return_true() };
let tmp: u8 = if predicate { 0xff } else { 0 };

let y = tmp & x;
assert_eq(y, 1);
}

unconstrained fn return_true() -> bool {
true
}
Loading