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: don't deduplicate binary math of unsigned types #6848

Merged
merged 9 commits into from
Dec 19, 2024
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
24 changes: 19 additions & 5 deletions compiler/noirc_evaluator/src/ssa/ir/instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@
// These can fail.
Constrain(..) | RangeCheck { .. } => true,

// This should never be side-effectful

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

View workflow job for this annotation

GitHub Actions / Code

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

// Some binary math can overflow or underflow
Expand Down Expand Up @@ -556,10 +556,25 @@
/// If true the instruction will depend on `enable_side_effects` context during acir-gen.
pub(crate) fn requires_acir_gen_predicate(&self, dfg: &DataFlowGraph) -> bool {
match self {
Instruction::Binary(binary)
if matches!(binary.operator, BinaryOp::Div | BinaryOp::Mod) =>
{
true
Instruction::Binary(binary) => {
match binary.operator {
asterite marked this conversation as resolved.
Show resolved Hide resolved
BinaryOp::Add
| BinaryOp::Sub
| BinaryOp::Mul
| BinaryOp::Div
| BinaryOp::Mod => {
// 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::Eq
| BinaryOp::Lt
| BinaryOp::And
| BinaryOp::Or
| BinaryOp::Xor
| BinaryOp::Shl
| BinaryOp::Shr => false,
}
}

Instruction::ArrayGet { array, index } => {
Expand All @@ -577,7 +592,6 @@
_ => false,
},
Instruction::Cast(_, _)
| Instruction::Binary(_)
| Instruction::Not(_)
| Instruction::Truncate { .. }
| Instruction::Constrain(_, _, _)
Expand Down
92 changes: 46 additions & 46 deletions compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,17 +263,17 @@ mod test {
fn simple_loop_invariant_code_motion() {
let src = "
brillig(inline) fn main f0 {
b0(v0: u32, v1: u32):
jmp b1(u32 0)
b1(v2: u32):
v5 = lt v2, u32 4
b0(v0: i32, v1: i32):
jmp b1(i32 0)
b1(v2: i32):
v5 = lt v2, i32 4
jmpif v5 then: b3, else: b2
b2():
return
b3():
v6 = mul v0, v1
constrain v6 == u32 6
v8 = add v2, u32 1
constrain v6 == i32 6
v8 = add v2, i32 1
jmp b1(v8)
}
";
Expand All @@ -287,17 +287,17 @@ mod test {
// `v6 = mul v0, v1` in b3 should now be `v3 = mul v0, v1` in b0
let expected = "
brillig(inline) fn main f0 {
b0(v0: u32, v1: u32):
b0(v0: i32, v1: i32):
v3 = mul v0, v1
jmp b1(u32 0)
b1(v2: u32):
v6 = lt v2, u32 4
jmp b1(i32 0)
b1(v2: i32):
v6 = lt v2, i32 4
jmpif v6 then: b3, else: b2
b2():
return
b3():
constrain v3 == u32 6
v9 = add v2, u32 1
constrain v3 == i32 6
v9 = add v2, i32 1
jmp b1(v9)
}
";
Expand All @@ -312,25 +312,25 @@ mod test {
// is hoisted to the parent loop's pre-header block.
let src = "
brillig(inline) fn main f0 {
b0(v0: u32, v1: u32):
jmp b1(u32 0)
b1(v2: u32):
v6 = lt v2, u32 4
b0(v0: i32, v1: i32):
jmp b1(i32 0)
b1(v2: i32):
v6 = lt v2, i32 4
jmpif v6 then: b3, else: b2
b2():
return
b3():
jmp b4(u32 0)
b4(v3: u32):
v7 = lt v3, u32 4
jmp b4(i32 0)
b4(v3: i32):
v7 = lt v3, i32 4
jmpif v7 then: b6, else: b5
b5():
v9 = add v2, u32 1
v9 = add v2, i32 1
jmp b1(v9)
b6():
v10 = mul v0, v1
constrain v10 == u32 6
v12 = add v3, u32 1
constrain v10 == i32 6
v12 = add v3, i32 1
jmp b4(v12)
}
";
Expand All @@ -344,25 +344,25 @@ mod test {
// `v10 = mul v0, v1` in b6 should now be `v4 = mul v0, v1` in b0
let expected = "
brillig(inline) fn main f0 {
b0(v0: u32, v1: u32):
b0(v0: i32, v1: i32):
v4 = mul v0, v1
jmp b1(u32 0)
b1(v2: u32):
v7 = lt v2, u32 4
jmp b1(i32 0)
b1(v2: i32):
v7 = lt v2, i32 4
jmpif v7 then: b3, else: b2
b2():
return
b3():
jmp b4(u32 0)
b4(v3: u32):
v8 = lt v3, u32 4
jmp b4(i32 0)
b4(v3: i32):
v8 = lt v3, i32 4
jmpif v8 then: b6, else: b5
b5():
v10 = add v2, u32 1
v10 = add v2, i32 1
jmp b1(v10)
b6():
constrain v4 == u32 6
v12 = add v3, u32 1
constrain v4 == i32 6
v12 = add v3, i32 1
jmp b4(v12)
}
";
Expand All @@ -386,19 +386,19 @@ mod test {
// hoist `v7 = mul v6, v0`.
let src = "
brillig(inline) fn main f0 {
b0(v0: u32, v1: u32):
jmp b1(u32 0)
b1(v2: u32):
v5 = lt v2, u32 4
b0(v0: i32, v1: i32):
jmp b1(i32 0)
b1(v2: i32):
v5 = lt v2, i32 4
jmpif v5 then: b3, else: b2
b2():
return
b3():
v6 = mul v0, v1
v7 = mul v6, v0
v8 = eq v7, u32 12
constrain v7 == u32 12
v9 = add v2, u32 1
v8 = eq v7, i32 12
constrain v7 == i32 12
v9 = add v2, i32 1
jmp b1(v9)
}
";
Expand All @@ -411,19 +411,19 @@ mod test {

let expected = "
brillig(inline) fn main f0 {
b0(v0: u32, v1: u32):
b0(v0: i32, v1: i32):
v3 = mul v0, v1
v4 = mul v3, v0
v6 = eq v4, u32 12
jmp b1(u32 0)
b1(v2: u32):
v9 = lt v2, u32 4
v6 = eq v4, i32 12
jmp b1(i32 0)
b1(v2: i32):
v9 = lt v2, i32 4
jmpif v9 then: b3, else: b2
b2():
return
b3():
constrain v4 == u32 12
v11 = add v2, u32 1
constrain v4 == i32 12
v11 = add v2, i32 1
jmp b1(v11)
}
";
Expand Down
7 changes: 7 additions & 0 deletions test_programs/execution_success/regression_6834/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "regression_6834"
version = "0.1.0"
type = "bin"
authors = [""]

[dependencies]
2 changes: 2 additions & 0 deletions test_programs/execution_success/regression_6834/Prover.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
year = 1
min_age = 1
9 changes: 9 additions & 0 deletions test_programs/execution_success/regression_6834/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
fn main(year: u32, min_age: u8) -> pub u32 {
if min_age == 0 {
(year - 2) / 4
} else if year > 1 {
(year - 2) / 4
} else {
0
}
}
Loading