From b8ec570ddb7f4f67f1ac8fecb71cbe04594791fb Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 2 Dec 2024 11:38:32 -0300 Subject: [PATCH 1/4] Add test that shows issue with hoisting sub --- .../src/ssa/opt/constant_folding.rs | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 41c84c935b1..92d499bb356 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -1457,6 +1457,32 @@ mod test { assert_normalized_ssa_equals(ssa, src); } + #[test] + fn does_not_hoist_sub_to_common_ancestor() { + let src = " + brillig(inline) fn main f0 { + b0(v0: u32): + v2 = eq v0, u32 0 + jmpif v2 then: b1, else: b2 + b1(): + v3 = sub v0, u32 1 + jmp b5() + b2(): + jmpif v0 then: b3, else: b4 + b3(): + v4 = sub v0, u32 1 // We can't hoist this because v0 is zero here and it will lead to an underflow + jmp b5() + b4(): + jmp b5() + b5(): + return + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let mut ssa = ssa.fold_constants_using_constraints(); + assert_normalized_ssa_equals(ssa, src); + } + #[test] fn deduplicates_side_effecting_intrinsics() { let src = " From e066da7d5c5b3a9a5adfef2c38b71314b71f9d83 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 2 Dec 2024 12:03:00 -0300 Subject: [PATCH 2/4] fix: don't hoist binary ops that can overflow/underflow --- .../noirc_evaluator/src/ssa/ir/instruction.rs | 16 +++++++++++++-- .../src/ssa/opt/constant_folding.rs | 20 +++++++++---------- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 6737b335b7d..699a8e530f1 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -389,9 +389,21 @@ impl Instruction { // This should never be side-effectful MakeArray { .. } => false, + // Some binary math can overflow or underflow + Binary(binary) => match binary.operator { + BinaryOp::Add | BinaryOp::Sub | BinaryOp::Mul | BinaryOp::Div => true, + BinaryOp::Mod + | BinaryOp::Eq + | BinaryOp::Lt + | BinaryOp::And + | BinaryOp::Or + | BinaryOp::Xor + | BinaryOp::Shl + | BinaryOp::Shr => false, + }, + // These can have different behavior depending on the EnableSideEffectsIf context. - Binary(_) - | Cast(_, _) + Cast(_, _) | Not(_) | Truncate { .. } | IfElse { .. } diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 92d499bb356..e414ce8f430 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -1460,26 +1460,26 @@ mod test { #[test] fn does_not_hoist_sub_to_common_ancestor() { let src = " - brillig(inline) fn main f0 { + acir(inline) fn main f0 { b0(v0: u32): v2 = eq v0, u32 0 - jmpif v2 then: b1, else: b2 - b1(): - v3 = sub v0, u32 1 + jmpif v2 then: b4, else: b1 + b4(): + v5 = sub v0, u32 1 jmp b5() - b2(): - jmpif v0 then: b3, else: b4 + b5(): + return + b1(): + jmpif v0 then: b3, else: b2 b3(): v4 = sub v0, u32 1 // We can't hoist this because v0 is zero here and it will lead to an underflow jmp b5() - b4(): + b2(): jmp b5() - b5(): - return } "; let ssa = Ssa::from_str(src).unwrap(); - let mut ssa = ssa.fold_constants_using_constraints(); + let ssa = ssa.fold_constants_using_constraints(); assert_normalized_ssa_equals(ssa, src); } From affd9cdbd357f32ed76ae6423243edec8626193d Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 2 Dec 2024 12:42:27 -0300 Subject: [PATCH 3/4] Fix test --- compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index e414ce8f430..39bda435842 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -1187,7 +1187,7 @@ mod test { v2 = lt u32 1000, v0 jmpif v2 then: b1, else: b2 b1(): - v4 = add v0, u32 1 + v4 = shl v0, u32 1 v5 = lt v0, v4 constrain v5 == u1 1 jmp b2() @@ -1195,7 +1195,7 @@ mod test { v7 = lt u32 1000, v0 jmpif v7 then: b3, else: b4 b3(): - v8 = add v0, u32 1 + v8 = shl v0, u32 1 v9 = lt v0, v8 constrain v9 == u1 1 jmp b4() @@ -1213,10 +1213,10 @@ mod test { brillig(inline) fn main f0 { b0(v0: u32): v2 = lt u32 1000, v0 - v4 = add v0, u32 1 + v4 = shl v0, u32 1 jmpif v2 then: b1, else: b2 b1(): - v5 = add v0, u32 1 + v5 = shl v0, u32 1 v6 = lt v0, v5 constrain v6 == u1 1 jmp b2() From 1ddba8acd980e306d5e4c620b47538cb1e8218dc Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 2 Dec 2024 12:43:24 -0300 Subject: [PATCH 4/4] Also consider Mod as potentially failing --- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 699a8e530f1..0d7b9d10f4c 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -391,9 +391,10 @@ impl Instruction { // Some binary math can overflow or underflow Binary(binary) => match binary.operator { - BinaryOp::Add | BinaryOp::Sub | BinaryOp::Mul | BinaryOp::Div => true, - BinaryOp::Mod - | BinaryOp::Eq + BinaryOp::Add | BinaryOp::Sub | BinaryOp::Mul | BinaryOp::Div | BinaryOp::Mod => { + true + } + BinaryOp::Eq | BinaryOp::Lt | BinaryOp::And | BinaryOp::Or