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: use predicate for curve operations #5076

Merged
merged 12 commits into from
May 30, 2024
10 changes: 9 additions & 1 deletion compiler/noirc_evaluator/src/ssa/ir/instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,12 @@ impl Intrinsic {
| Intrinsic::IsUnconstrained => false,

// Some black box functions have side-effects
Intrinsic::BlackBox(func) => matches!(func, BlackBoxFunc::RecursiveAggregation),
Intrinsic::BlackBox(func) => matches!(
func,
BlackBoxFunc::RecursiveAggregation
| BlackBoxFunc::MultiScalarMul
| BlackBoxFunc::EmbeddedCurveAdd
),
}
}

Expand Down Expand Up @@ -340,6 +345,9 @@ impl Instruction {

// Some `Intrinsic`s have side effects so we must check what kind of `Call` this is.
Call { func, .. } => match dfg[*func] {
// Explicitly allows removal of unused ec operations, even if they can fail
Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::MultiScalarMul))
| Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::EmbeddedCurveAdd)) => true,
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
Value::Intrinsic(intrinsic) => !intrinsic.has_side_effects(),

// All foreign functions are treated as having side effects.
Expand Down
47 changes: 46 additions & 1 deletion compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@
use fxhash::FxHashMap as HashMap;
use std::collections::{BTreeMap, HashSet};

use acvm::{acir::AcirField, FieldElement};
use acvm::{acir::AcirField, acir::BlackBoxFunc, FieldElement};
use iter_extended::vecmap;

use crate::ssa::{
Expand Down Expand Up @@ -769,7 +769,38 @@ impl<'f> Context<'f> {

Instruction::Call { func, arguments }
}
//Issue #5045: We set curve points to infinity if condition is false
Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::EmbeddedCurveAdd)) => {
arguments[2] = self.var_or_one(arguments[2], condition, call_stack.clone());
arguments[5] = self.var_or_one(arguments[5], condition, call_stack.clone());

Instruction::Call { func, arguments }
}
Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::MultiScalarMul)) => {
let mut array_with_predicate = im::Vector::new();
let array_typ;
if let Value::Array { array, typ } =
&self.inserter.function.dfg[arguments[0]]
{
array_typ = typ.clone();
for (i, value) in array.clone().iter().enumerate() {
if i % 3 == 2 {
array_with_predicate.push_back(self.var_or_one(
*value,
condition,
call_stack.clone(),
));
} else {
array_with_predicate.push_back(*value);
}
}
} else {
unreachable!();
}
arguments[0] =
self.inserter.function.dfg.make_array(array_with_predicate, array_typ);
Instruction::Call { func, arguments }
}
_ => Instruction::Call { func, arguments },
},
other => other,
Expand All @@ -779,6 +810,20 @@ impl<'f> Context<'f> {
}
}

// Computes: if condition { var } else { 1 }
fn var_or_one(&mut self, var: ValueId, condition: ValueId, call_stack: CallStack) -> ValueId {
let field = self.insert_instruction(
Instruction::binary(BinaryOp::Mul, var, condition),
call_stack.clone(),
);
let not_condition =
self.insert_instruction(Instruction::Not(condition), call_stack.clone());
self.insert_instruction(
Instruction::binary(BinaryOp::Add, field, not_condition),
call_stack,
)
}

fn undo_stores_in_then_branch(&mut self, store_values: &HashMap<ValueId, Store>) {
for (address, store) in store_values {
let address = *address;
Expand Down
7 changes: 7 additions & 0 deletions test_programs/execution_success/regression_5045/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "regression_5045"
version = "0.1.0"
type = "bin"
authors = [""]

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
is_active = false
20 changes: 20 additions & 0 deletions test_programs/execution_success/regression_5045/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
use dep::std::embedded_curve_ops::EmbeddedCurvePoint;
use dep::std::embedded_curve_ops::EmbeddedCurveScalar;

fn main(is_active: bool) {
let a = EmbeddedCurvePoint {
x: 0x1d8eb4378a3bde41e0b6a9a8dcbd21b7ff9c51bdd6ca13ce989abbbf90df3666,
y: 0x06075b63354f2504f9cddba0b94ed0cef35fc88615e69ec1f853b51eb79a24a0,
is_infinite: false
};

if is_active {
let bad = EmbeddedCurvePoint { x: 0, y: 5, is_infinite: false };
let d = bad.double();
let e = dep::std::embedded_curve_ops::multi_scalar_mul(
[a, bad],
[EmbeddedCurveScalar { lo: 1, hi: 0 }, EmbeddedCurveScalar { lo: 1, hi: 0 }]
);
assert(e[0] != d.x);
}
}
Loading