From 00d4633359e9e8cdd1395d96d2693b94e9dcde6d Mon Sep 17 00:00:00 2001 From: Joss Date: Wed, 26 Apr 2023 12:50:50 +0100 Subject: [PATCH] chore(noir): dedup within output also. Futhermore: - reorder keywords (distinct first) - add test case - fix up comments & typos - tidy up --- crates/noirc_abi/src/lib.rs | 13 +++++++------ crates/noirc_evaluator/src/lib.rs | 11 +++++++---- .../src/ssa/acir_gen/operations/return.rs | 4 ++-- .../noirc_frontend/src/hir/resolution/errors.rs | 2 +- .../src/hir/resolution/resolver.rs | 4 +++- .../noirc_frontend/src/monomorphization/ast.rs | 4 ++-- crates/noirc_frontend/src/parser/parser.rs | 17 +++++++---------- 7 files changed, 29 insertions(+), 26 deletions(-) diff --git a/crates/noirc_abi/src/lib.rs b/crates/noirc_abi/src/lib.rs index 0f79b245b1c..191128b9407 100644 --- a/crates/noirc_abi/src/lib.rs +++ b/crates/noirc_abi/src/lib.rs @@ -87,13 +87,14 @@ impl std::fmt::Display for AbiVisibility { #[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] -/// Represents whether the return value should be encoded into witness indexes that are distinct -/// from the input parameters. +/// Represents whether the return value should compromise of unique witness indices such that no +/// index occurs within the program's abi more than once. /// -/// This is useful for apllication stacks that require an uniform abi across across multiple -/// circuits. When duplication is allowed, the compiler may indentify that a public input reaches -/// the output unaltered and is thus references directly, causing the input and output witness -/// indcies to overlap. +/// This is useful for application stacks that require an uniform abi across across multiple +/// circuits. When index duplication is allowed, the compiler may identify that a public input +/// reaches the output unaltered and is thus referenced directly, causing the input and output +/// witness indices to overlap. Similarly, repetitions of copied values in the output may be +/// optimized away. pub enum AbiDistinctness { Distinct, DuplicationAllowed, diff --git a/crates/noirc_evaluator/src/lib.rs b/crates/noirc_evaluator/src/lib.rs index 9e84d8124dd..64a02061b0f 100644 --- a/crates/noirc_evaluator/src/lib.rs +++ b/crates/noirc_evaluator/src/lib.rs @@ -54,8 +54,8 @@ pub struct Evaluator { // and increasing as for `public_parameters`. We then use a `Vec` rather // than a `BTreeSet` to preserve this order for the ABI. return_values: Vec, - // If true, indicates that the resulting ACIR should prevent input and output witness indicies - // from overlapping by having extra contraints if necessary. + // If true, indicates that the resulting ACIR should enforce that all inputs and outputs are + // comprised of unique witness indices by having extra constraints if necessary. return_is_distinct: bool, opcodes: Vec, @@ -105,7 +105,7 @@ pub fn create_circuit( } impl Evaluator { - // Returns true if the `witnees_index` appears in the program's input parameters. + // Returns true if the `witness_index` appears in the program's input parameters. fn is_abi_input(&self, witness_index: Witness) -> bool { witness_index.as_usize() <= self.num_witnesses_abi_len } @@ -125,8 +125,11 @@ impl Evaluator { self.is_abi_input(witness_index) && !is_public_input } + // True if the main function return has the `distinct` keyword and this particular witness + // index has already occurred elsewhere in the abi's inputs and outputs. fn should_proxy_witness_for_abi_output(&self, witness_index: Witness) -> bool { - self.return_is_distinct && self.is_abi_input(witness_index) + self.return_is_distinct + && (self.is_abi_input(witness_index) || self.return_values.contains(&witness_index)) } // Creates a new Witness index diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/operations/return.rs b/crates/noirc_evaluator/src/ssa/acir_gen/operations/return.rs index 5ed04ca578d..6aaa3b2fbbd 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/operations/return.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/operations/return.rs @@ -48,8 +48,8 @@ pub(crate) fn evaluate( "we do not allow private ABI inputs to be returned as public outputs", ))); } - // Check if the outputted witness needs separating from an occurrence in the program's - // input. This behaviour stems from usage of the `distinct` keyword. + // Check if the outputted witness needs separating from an existing occurrence in the + // abi. This behavior stems from usage of the `distinct` keyword. let return_witness = if evaluator.should_proxy_witness_for_abi_output(witness) { let proxy_constraint = Expression::from(witness); evaluator.create_intermediate_variable(proxy_constraint) diff --git a/crates/noirc_frontend/src/hir/resolution/errors.rs b/crates/noirc_frontend/src/hir/resolution/errors.rs index 365109f2bee..c57e4c890d2 100644 --- a/crates/noirc_frontend/src/hir/resolution/errors.rs +++ b/crates/noirc_frontend/src/hir/resolution/errors.rs @@ -187,7 +187,7 @@ impl From for Diagnostic { ident.0.span(), ); - diag.add_note("The `distinct` keyword is only valid when used on the main function of a program as it's only purpose is to prevent a program's parameter and return witness indexes from overlapping".to_owned()); + diag.add_note("The `distinct` keyword is only valid when used on the main function of a program, as its only purpose is to ensure that all witness indices that occur in the abi are unique".to_owned()); diag } ResolverError::ExpectedComptimeVariable { name, span } => Diagnostic::simple_error( diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index b18f62f2e24..ebc67b6181a 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -681,9 +681,11 @@ impl<'a> Resolver<'a> { } } - /// True if the 'distinct' keyword is allowed on parameters in this function + /// True if the `distinct` keyword is allowed on function's return type fn distinct_allowed(&self, func: &NoirFunction) -> bool { if self.in_contract() { + // "open" and "unconstrained" functions are compiled to brillig and thus duplication of + // witness indices in their abis is not a concern. !func.def.is_unconstrained && !func.def.is_open } else { func.name() == "main" diff --git a/crates/noirc_frontend/src/monomorphization/ast.rs b/crates/noirc_frontend/src/monomorphization/ast.rs index bb4ea1015ab..04aec9a6726 100644 --- a/crates/noirc_frontend/src/monomorphization/ast.rs +++ b/crates/noirc_frontend/src/monomorphization/ast.rs @@ -221,9 +221,9 @@ impl Type { pub struct Program { pub functions: Vec, pub main_function_signature: FunctionSignature, - /// Indicates whether the resulting ACIR should keep input and output witness indcies separate. + /// Indicates whether witness indices are allowed to reoccur in the ABI of the resulting ACIR. /// - /// Note: this has no impact on monomorphisation, and is simply attached here for ease of + /// Note: this has no impact on monomorphization, and is simply attached here for ease of /// forwarding to the next phase. pub return_distinctness: noirc_abi::AbiDistinctness, } diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index 200a2f346e9..065b6362fb4 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -162,7 +162,7 @@ fn function_definition(allow_self: bool) -> impl NoirParser { |( ( ((((attribute, (is_unconstrained, is_open)), name), generics), parameters), - (return_visibility, return_distinctness, return_type), + ((return_distinctness, return_visibility), return_type), ), body, )| { @@ -236,19 +236,15 @@ fn lambda_return_type() -> impl NoirParser { .map(|ret| ret.unwrap_or(UnresolvedType::Unspecified)) } -fn function_return_type() -> impl NoirParser<(AbiVisibility, AbiDistinctness, UnresolvedType)> { +fn function_return_type() -> impl NoirParser<((AbiDistinctness, AbiVisibility), UnresolvedType)> { just(Token::Arrow) - .ignore_then(optional_visibility()) - .then(optional_distinctness()) + .ignore_then(optional_distinctness()) + .then(optional_visibility()) .then(parse_type()) .or_not() .map(|ret| { - ret.map(|((visibility, distinctness), return_type)| { - (visibility, distinctness, return_type) - }) - .unwrap_or(( - AbiVisibility::Private, - AbiDistinctness::DuplicationAllowed, + ret.unwrap_or(( + (AbiDistinctness::DuplicationAllowed, AbiVisibility::Private), UnresolvedType::Unit, )) }) @@ -1275,6 +1271,7 @@ mod test { "fn f(f: pub Field, y : Field, z : comptime Field) -> u8 { x + a }", "fn func_name(f: Field, y : pub Field, z : pub [u8;5],) {}", "fn func_name(x: [Field], y : [Field;2],y : pub [Field;2], z : pub [u8;5]) {}", + "fn main(x: pub u8, y: pub u8) -> distinct pub [u8; 2] { [x, y] }" ], );