Skip to content

fix: don't deduplicate binary math of unsigned types #6848

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.

Sign up for GitHub

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

Merged
asterite merged 9 commits into from
Dec 19, 2024
Merged

fix: don't deduplicate binary math of unsigned types #6848

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
55153df
Add a regression test for 6834
asterite Dec 17, 2024
0be4c79
account for has_side_effects in ResultCache get
vezenovm Dec 17, 2024
f3a8d49
Reduce the program a bit more
asterite Dec 18, 2024
4f04bc3
Revert "account for has_side_effects in ResultCache get"
asterite Dec 18, 2024
31395ae
Binary math that can overflow is true in requires_acir_gen_predicate
asterite Dec 18, 2024
48fd9e4
Remove comment from test program
asterite Dec 18, 2024
9ae5af7
Only consider unsigned types
asterite Dec 18, 2024
b46bafb
Fix test
asterite Dec 18, 2024
d1afd2f
Fix a couple more tests
asterite Dec 18, 2024
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
21 changes: 16 additions & 5 deletions compiler/noirc_evaluator/src/ssa/ir/instruction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,10 +556,22 @@ impl Instruction {
/// 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) => {
// Some binary math can overflow or underflow
match binary.operator {
BinaryOp::Add
| BinaryOp::Sub
| BinaryOp::Mul
| BinaryOp::Div
| BinaryOp::Mod => true,
BinaryOp::Eq
| BinaryOp::Lt
| BinaryOp::And
| BinaryOp::Or
| BinaryOp::Xor
| BinaryOp::Shl
| BinaryOp::Shr => false,
}
}

Instruction::ArrayGet { array, index } => {
Expand All @@ -577,7 +589,6 @@ impl Instruction {
_ => false,
},
Instruction::Cast(_, _)
| Instruction::Binary(_)
| Instruction::Not(_)
| Instruction::Truncate { .. }
| Instruction::Constrain(_, _, _)
Expand Down
7 changes: 7 additions & 0 deletions test_programs/execution_success/regression_6834/Nargo.toml
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
}
}