Skip to content

Commit

Permalink
fix: Disable if optimization (#5240)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves #5202 

## Summary\*

The linked issue was coming from our if condition optimization where we
assume the value of an if's condition to be true within the then branch
and false within the else branch. It seems the `map_value` calls from
this weren't being reset properly since the nested `if f1.sign { .. } {
.. }`'s else branch assumed `f1.sign` to be false, which was not reset
properly after the `if`. I've temporarily disabled the optimization
until we can add it back without breaking this test.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
jfecher authored Jun 13, 2024
1 parent 1c19dff commit a2816db
Show file tree
Hide file tree
Showing 7 changed files with 270 additions and 26 deletions.
27 changes: 1 addition & 26 deletions compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,6 @@ impl<'f> Context<'f> {
let old_condition = *condition;
let then_condition = self.inserter.resolve(old_condition);

let one = FieldElement::one();
let old_stores = std::mem::take(&mut self.store_values);
let old_allocations = std::mem::take(&mut self.local_allocations);
let branch = ConditionalBranch {
Expand All @@ -393,15 +392,6 @@ impl<'f> Context<'f> {
};
self.condition_stack.push(cond_context);
self.insert_current_side_effects_enabled();
// Optimization: within the then branch we know the condition to be true, so replace
// any references of it within this branch with true. Likewise, do the same with false
// with the else branch. We must be careful not to replace the condition if it is a
// known constant, otherwise we can end up setting 1 = 0 or vice-versa.
if self.inserter.function.dfg.get_numeric_constant(old_condition).is_none() {
let known_value = self.inserter.function.dfg.make_constant(one, Type::bool());

self.inserter.map_value(old_condition, known_value);
}
vec![self.branch_ends[if_entry], *else_destination, *then_destination]
}

Expand All @@ -414,7 +404,6 @@ impl<'f> Context<'f> {
self.insert_instruction(Instruction::Not(cond_context.condition), CallStack::new());
let else_condition = self.link_condition(else_condition);

let zero = FieldElement::zero();
// Make sure the else branch sees the previous values of each store
// rather than any values created in the 'then' branch.
let old_stores = std::mem::take(&mut cond_context.then_branch.store_values);
Expand All @@ -429,21 +418,12 @@ impl<'f> Context<'f> {
local_allocations: old_allocations,
last_block: *block,
};
let old_condition = else_branch.old_condition;
cond_context.then_branch.local_allocations.clear();
cond_context.else_branch = Some(else_branch);
self.condition_stack.push(cond_context);

self.insert_current_side_effects_enabled();
// Optimization: within the then branch we know the condition to be true, so replace
// any references of it within this branch with true. Likewise, do the same with false
// with the else branch. We must be careful not to replace the condition if it is a
// known constant, otherwise we can end up setting 1 = 0 or vice-versa.
if self.inserter.function.dfg.get_numeric_constant(old_condition).is_none() {
let known_value = self.inserter.function.dfg.make_constant(zero, Type::bool());

self.inserter.map_value(old_condition, known_value);
}

assert_eq!(self.cfg.successors(*block).len(), 1);
vec![self.cfg.successors(*block).next().unwrap()]
}
Expand Down Expand Up @@ -471,11 +451,6 @@ impl<'f> Context<'f> {
// known to be true/false within the then/else branch respectively.
self.insert_current_side_effects_enabled();

// We must map back to `then_condition` here. Mapping `old_condition` to itself would
// lose any previous mappings.
self.inserter
.map_value(cond_context.then_branch.old_condition, cond_context.then_branch.condition);

// While there is a condition on the stack we don't compile outside the condition
// until it is popped. This ensures we inline the full then and else branches
// before continuing from the end of the conditional here where they can be merged properly.
Expand Down
8 changes: 8 additions & 0 deletions test_programs/execution_success/regression_5202/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[package]
name = "regression_5202"
type = "bin"
authors = [""]
compiler_version = ">=0.30.0"

[dependencies]
fraction = { path = "fraction" }
21 changes: 21 additions & 0 deletions test_programs/execution_success/regression_5202/fraction/LICENSE
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
MIT License

Copyright (c) 2023 Resurgence Labs

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[package]
name = "fraction"
type = "lib"
authors = [""]

[dependencies]
10 changes: 10 additions & 0 deletions test_programs/execution_success/regression_5202/fraction/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# fraction
The Noir repository for accessing Fractions by Resurgence Labs

# usage
To Use as a Dependency, add the following line to your project's Nargo.toml under [dependencies]:
fraction = { tag = "v1.2.2", git = "https://github.com/resurgencelabs/fraction" }

Note: This is a highly dynamic repository. The code will change as more features are added, both at the repository level by Resurgence Labs, as well as at the language level itself by the Noir team.


201 changes: 201 additions & 0 deletions test_programs/execution_success/regression_5202/fraction/src/lib.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
use dep::std;

global MAX: Fraction = Fraction { sign: true, num: 3050913689, den: 1 };
global MIN: Fraction = Fraction { sign: false, num: 3050913689, den: 1 };
global ZERO: Fraction = Fraction { sign: true, num: 0, den: 1 };
global ONE: Fraction = Fraction { sign: true, num: 1, den: 1 };
global NEGATIVE_ONE: Fraction = Fraction { sign: false, num: 1, den: 1 };

struct Fraction {
sign: bool,
num: u32,
den: u32,
}

// Create a Fraction type variable without lenghtier code
pub fn toFraction(s: bool, n: u32, d: u32) -> Fraction {
assert(d != 0);
Fraction { sign: s, num: n, den: d }
}

// Swaps the numerator and denominator
fn invertFraction(f: Fraction) -> Fraction {
assert(f.num != 0);
Fraction { sign: f.sign, num: f.den, den: f.num }
}

// Changes the Sign of the Fraction
fn signChangeFraction(f: Fraction) -> Fraction {
Fraction { sign: !f.sign, num: f.num, den: f.den }
}

// this method will only work till numerator and denominator values are under 100
// this has been set for efficiency reasons, and will be modified once the Noir team
// can implement dynamic limit for loops
fn reduceFraction(f: Fraction) -> Fraction {
let mut a = f.num;
let mut b = f.den;
let mut j = 1;
let mut gcd = 1;

let min = if a > b { b } else { a };

for i in 2..100 {
j = i as u32;
if (j <= min) {
if (a % j == 0) & (b % j == 0) {
gcd = j;
}
}
}

Fraction { sign: f.sign, num: f.num / gcd, den: f.den / gcd }
}

// Adds two fractions
pub fn addFraction(f1: Fraction, f2: Fraction) -> Fraction {
let mut an = U128::from_integer(f1.num);
let mut ad = U128::from_integer(f1.den);
let mut bn = U128::from_integer(f2.num);
let mut bd = U128::from_integer(f2.den);
let mut m = f1;
let mut n = f2;

if f1.sign == f2.sign {
if ((ad * bd > U128::from_integer(2000000000))
| ((an * bd + ad * bn) > U128::from_integer(2000000000))
| ((an * bd) > U128::from_integer(2000000000))
| ((ad * bn) > U128::from_integer(2000000000))) {
m = reduceFraction(m);
n = reduceFraction(n);
}
an = U128::from_integer(m.num);
ad = U128::from_integer(m.den);
bn = U128::from_integer(n.num);
bd = U128::from_integer(n.den);
if ((ad * bd > U128::from_integer(2000000000))
| ((an * bd + ad * bn) > U128::from_integer(2000000000))
| ((an * bd) > U128::from_integer(2000000000))
| ((ad * bn) > U128::from_integer(2000000000))) {
let mut ddd = (an * bd + ad * bn) / (ad * bd);
let mut factor = U128::from_integer(1);
for _ in 1..5 {
if ddd * U128::from_integer(10) < U128::from_integer(2000000000) {
ddd *= U128::from_integer(10);
factor *= U128::from_integer(10);
}
}
let np: u32 = U128::to_integer(((an * bd + ad * bn) * factor) / (ad * bd));
let fx: u32 = U128::to_integer(factor);
Fraction { sign: f1.sign, num: np, den: fx }
} else {
Fraction { sign: f1.sign, num: (m.num * n.den + n.num * m.den), den: m.den * n.den }
}
} else if ((an * bd) > (bn * ad)) {
if ((ad * bd > U128::from_integer(2000000000))
| ((an * bd - ad * bn) > U128::from_integer(2000000000))
| ((an * bd) > U128::from_integer(2000000000))) {
m = reduceFraction(m);
n = reduceFraction(n);
}
an = U128::from_integer(m.num);
ad = U128::from_integer(m.den);
bn = U128::from_integer(n.num);
bd = U128::from_integer(n.den);

if ((ad * bd > U128::from_integer(2000000000))
| ((an * bd - ad * bn) > U128::from_integer(2000000000))
| ((an * bd) > U128::from_integer(2000000000))) {
let mut ddd = (an * bd - ad * bn) / (ad * bd);
let mut factor = U128::from_integer(1);
for _ in 1..5 {
if ddd * U128::from_integer(10) < U128::from_integer(2000000000) {
ddd *= U128::from_integer(10);
factor *= U128::from_integer(10);
}
}
let np: u32 = U128::to_integer(((an * bd - ad * bn) * factor) / (ad * bd));
let fx: u32 = U128::to_integer(factor);
Fraction { sign: f1.sign, num: np, den: fx }
} else {
Fraction { sign: f1.sign, num: (m.num * n.den - n.num * m.den), den: m.den * n.den }
}
} else {
if ((ad * bd > U128::from_integer(2000000000))
| ((bn * ad - bd * an) > U128::from_integer(2000000000))
| ((bn * ad) > U128::from_integer(2000000000))) {
m = reduceFraction(m);
n = reduceFraction(n);
}
an = U128::from_integer(m.num);
ad = U128::from_integer(m.den);
bn = U128::from_integer(n.num);
bd = U128::from_integer(n.den);
if ((ad * bd > U128::from_integer(2000000000))
| ((bn * ad - bd * an) > U128::from_integer(2000000000))
| ((bn * ad) > U128::from_integer(2000000000))) {
let mut ddd = (bn * ad - bd * an) / (ad * bd);
let mut factor = U128::from_integer(1);
for _ in 1..5 {
if ddd * U128::from_integer(10) < U128::from_integer(2000000000) {
ddd *= U128::from_integer(10);
factor *= U128::from_integer(10);
}
}
let np: u32 = U128::to_integer(((bn * ad - bd * an) * factor) / (ad * bd));
let fx: u32 = U128::to_integer(factor);
Fraction { sign: f2.sign, num: np, den: fx }
} else {
Fraction { sign: f2.sign, num: (n.num * m.den - m.num * n.den), den: m.den * n.den }
}
}
}

// Returns the closest but smaller Integer to the Given Fraction, but typecast to Fraction for convenience
pub fn floor(f: Fraction) -> Fraction {
let q = f.num / f.den;
if q * f.den == f.num {
Fraction { sign: f.sign, num: f.num, den: f.den }
} else if f.sign {
Fraction { sign: f.sign, num: q, den: 1 }
} else {
Fraction { sign: f.sign, num: q + 1, den: 1 }
}
}

#[test]
fn test_sum() {
let f1 = toFraction(true, 3, 5);
let f2 = toFraction(true, 2, 5);
let f = addFraction(f1, f2);
assert(f.num == f.den);
}

#[test]
fn test_reduce() {
let f1 = toFraction(true, 2, 10);
let f2 = reduceFraction(f1);
assert(f2.num == 1);
}

#[test]
fn test_floor() {
let f = toFraction(true, 7, 5);
let fl = floor(f);
assert(fl.num == 1);
assert(fl.den == 1);
}

#[test]
fn test_floor2() {
let f = toFraction(false, 12, 5);
let fl = floor(f);
assert(fl.num == 3);
assert(fl.den == 1);
}

#[test]
fn test_globals() {
let a = addFraction(ONE, NEGATIVE_ONE);
assert(a.num == ZERO.num);
}
23 changes: 23 additions & 0 deletions test_programs/execution_success/regression_5202/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
use dep::fraction::{Fraction, MAX, floor, toFraction, addFraction};

fn main() {
let g1 = toFraction(true, 33333333, 5);
let g2 = toFraction(true, 500000, 33333333);
let a = addFraction(g1, g2);

let f1 = floor(a);
let f2 = MAX;
assert(f1.sign);
assert(f2.sign);

if f1.sign != f2.sign {
if (f1.sign) { () } else { () }
} else {
// Test fails here before the fix to #5202.
// An optimization which assumes an if condition to be true/false
// within the then/else branch respectively wasn't being set properly
// causing f1.sign to be assumed to be false in this else branch.
assert(f1.sign);
assert(f2.sign);
}
}

0 comments on commit a2816db

Please sign in to comment.