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: mark field division and modulo as requiring predicate for all necessary types #7290

Merged
merged 3 commits into from
Feb 5, 2025
Merged
Show file tree
Hide file tree
Changes from all 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
13 changes: 8 additions & 5 deletions compiler/noirc_evaluator/src/acir/acir_variable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -549,12 +549,12 @@ impl<F: AcirField, B: BlackBoxFunctionSolver<F>> AcirContext<F, B> {
&mut self,
lhs: AcirVar,
rhs: AcirVar,
predicate: AcirVar,
assert_message: Option<AssertionPayload<F>>,
) -> Result<(), RuntimeError> {
let diff_var = self.sub_var(lhs, rhs)?;

let one = self.add_constant(F::one());
let _ = self.inv_var(diff_var, one)?;
let _ = self.inv_var(diff_var, predicate)?;
if let Some(payload) = assert_message {
self.acir_ir
.assertion_payloads
Expand Down Expand Up @@ -613,7 +613,7 @@ impl<F: AcirField, B: BlackBoxFunctionSolver<F>> AcirContext<F, B> {
}
NumericType::Signed { bit_size } => {
let (quotient_var, _remainder_var) =
self.signed_division_var(lhs, rhs, bit_size)?;
self.signed_division_var(lhs, rhs, bit_size, predicate)?;
Ok(quotient_var)
}
}
Expand Down Expand Up @@ -1050,6 +1050,7 @@ impl<F: AcirField, B: BlackBoxFunctionSolver<F>> AcirContext<F, B> {
lhs: AcirVar,
rhs: AcirVar,
bit_size: u32,
predicate: AcirVar,
) -> Result<(AcirVar, AcirVar), RuntimeError> {
// We derive the signed division from the unsigned euclidean division.
// note that this is not euclidean division!
Expand Down Expand Up @@ -1079,7 +1080,7 @@ impl<F: AcirField, B: BlackBoxFunctionSolver<F>> AcirContext<F, B> {

// Performs the division using the unsigned values of lhs and rhs
let (q1, r1) =
self.euclidean_division_var(unsigned_lhs, unsigned_rhs, bit_size - 1, one)?;
self.euclidean_division_var(unsigned_lhs, unsigned_rhs, bit_size - 1, predicate)?;

// Unsigned to signed: derive q and r from q1,r1 and the signs of lhs and rhs
// Quotient sign is lhs sign * rhs sign, whose resulting sign bit is the XOR of the sign bits
Expand Down Expand Up @@ -1117,7 +1118,9 @@ impl<F: AcirField, B: BlackBoxFunctionSolver<F>> AcirContext<F, B> {
};

let (_, remainder_var) = match numeric_type {
NumericType::Signed { bit_size } => self.signed_division_var(lhs, rhs, bit_size)?,
NumericType::Signed { bit_size } => {
self.signed_division_var(lhs, rhs, bit_size, predicate)?
}
_ => self.euclidean_division_var(lhs, rhs, bit_size, predicate)?,
};
Ok(remainder_var)
Expand Down
7 changes: 6 additions & 1 deletion compiler/noirc_evaluator/src/acir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -762,7 +762,12 @@ impl<'a> Context<'a> {
None
};

self.acir_context.assert_neq_var(lhs, rhs, assert_payload)?;
self.acir_context.assert_neq_var(
lhs,
rhs,
self.current_side_effects_enabled_var,
assert_payload,
)?;
}
Instruction::Cast(value_id, _) => {
let acir_var = self.convert_numeric_value(*value_id, dfg)?;
Expand Down
10 changes: 7 additions & 3 deletions compiler/noirc_evaluator/src/ssa/ir/instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@
// These can fail.
Constrain(..) | ConstrainNotEqual(..) | RangeCheck { .. } => true,

// This should never be side-effectful

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

View workflow job for this annotation

GitHub Actions / Code

Unknown word (effectful)
MakeArray { .. } | Noop => false,

// Some binary math can overflow or underflow
Expand Down Expand Up @@ -595,13 +595,17 @@
match binary.operator {
BinaryOp::Add { unchecked: false }
| BinaryOp::Sub { unchecked: false }
| BinaryOp::Mul { unchecked: false }
| BinaryOp::Div
| BinaryOp::Mod => {
| BinaryOp::Mul { unchecked: false } => {
// 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::Div | BinaryOp::Mod => {
// Div and Mod require a predicate if the RHS may be zero.
dfg.get_numeric_constant(binary.rhs)
.map(|rhs| !rhs.is_zero())
.unwrap_or(true)
}
BinaryOp::Add { unchecked: true }
| BinaryOp::Sub { unchecked: true }
| BinaryOp::Mul { unchecked: true }
Expand Down
61 changes: 61 additions & 0 deletions compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1724,4 +1724,65 @@ mod test {
let ssa = ssa.purity_analysis().fold_constants_using_constraints();
assert_normalized_ssa_equals(ssa, expected);
}

#[test]
fn does_not_deduplicate_field_divisions_under_different_predicates() {
// Regression test for https://github.com/noir-lang/noir/issues/7283
let src = "
acir(inline) fn main f0 {
b0(v0: Field, v1: Field, v2: u1):
enable_side_effects v2
v3 = div v1, v0
v4 = mul v3, v0
v5 = not v2
enable_side_effects v5
v6 = div v1, v0
return
}
";

let ssa = Ssa::from_str(src).unwrap();
let ssa = ssa.fold_constants();
assert_normalized_ssa_equals(ssa, src);
}

#[test]
fn does_not_deduplicate_unsigned_divisions_under_different_predicates() {
// Regression test for https://github.com/noir-lang/noir/issues/7283
let src = "
acir(inline) fn main f0 {
b0(v0: u32, v1: u32, v2: u1):
enable_side_effects v2
v3 = div v1, v0
v4 = not v2
enable_side_effects v4
v5 = div v1, v0
return
}
";

let ssa = Ssa::from_str(src).unwrap();
let ssa = ssa.fold_constants();
assert_normalized_ssa_equals(ssa, src);
}

#[test]
fn does_not_deduplicate_signed_divisions_under_different_predicates() {
// Regression test for https://github.com/noir-lang/noir/issues/7283
let src = "
acir(inline) fn main f0 {
b0(v0: i32, v1: i32, v2: u1):
enable_side_effects v2
v3 = div v1, v0
v4 = not v2
enable_side_effects v4
v5 = div v1, v0
return
}
";

let ssa = Ssa::from_str(src).unwrap();
let ssa = ssa.fold_constants();
assert_normalized_ssa_equals(ssa, src);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[package]
name = "signed_inactive_division_by_zero"
type = "bin"
authors = [""]

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
a = "1"
b = "0"
condition = false
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
fn main(a: i32, b: i32, condition: bool) -> pub i32 {
if condition {
// If `condition` is not set then we should not trigger an assertion failure here.
a / b
} else {
0
}
}