From 730c9ff96eee34bce77fc1b41668cee45f06b503 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 11 Jul 2023 18:46:51 +0000 Subject: [PATCH 01/20] initial stdlib methods to start refactoring logign --- crates/nargo/src/ops/foreign_calls.rs | 8 ++++++++ crates/nargo/src/ops/mod.rs | 1 + .../test_data_ssa_refactor/debug_logs/Nargo.toml | 5 +++++ .../test_data_ssa_refactor/debug_logs/Prover.toml | 2 ++ .../test_data_ssa_refactor/debug_logs/src/main.nr | 7 +++++++ noir_stdlib/src/lib.nr | 14 ++++++++++++++ 6 files changed, 37 insertions(+) create mode 100644 crates/nargo/src/ops/foreign_calls.rs create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/Nargo.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/Prover.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr diff --git a/crates/nargo/src/ops/foreign_calls.rs b/crates/nargo/src/ops/foreign_calls.rs new file mode 100644 index 00000000000..2912603c626 --- /dev/null +++ b/crates/nargo/src/ops/foreign_calls.rs @@ -0,0 +1,8 @@ +/// This enumeration represents the Brillig foreign calls that are natively supported by nargo. +/// After resolution of a foreign call, nargo will restart execution of the ACVM +pub enum ForeignCall { + Println, + PrintlnFormat, + Sequence, + ReverseSequence, +} \ No newline at end of file diff --git a/crates/nargo/src/ops/mod.rs b/crates/nargo/src/ops/mod.rs index 8a0cce4b8c5..f82fbffef7d 100644 --- a/crates/nargo/src/ops/mod.rs +++ b/crates/nargo/src/ops/mod.rs @@ -9,3 +9,4 @@ mod execute; mod preprocess; mod prove; mod verify; +mod foreign_calls; diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/Nargo.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/Nargo.toml new file mode 100644 index 00000000000..dc0c2f8917c --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/Nargo.toml @@ -0,0 +1,5 @@ +[package] +authors = [""] +compiler_version = "0.8.0" + +[dependencies] \ No newline at end of file diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/Prover.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/Prover.toml new file mode 100644 index 00000000000..f28f2f8cc48 --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/Prover.toml @@ -0,0 +1,2 @@ +x = "5" +y = "10" diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr new file mode 100644 index 00000000000..7394d3d34d3 --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr @@ -0,0 +1,7 @@ +use dep::std; + +fn main(x : Field, y : pub Field) { + + std::println_new(x); + assert(x != y); +} diff --git a/noir_stdlib/src/lib.nr b/noir_stdlib/src/lib.nr index 7629634ac0a..6fae03b6c97 100644 --- a/noir_stdlib/src/lib.nr +++ b/noir_stdlib/src/lib.nr @@ -18,5 +18,19 @@ mod compat; #[builtin(println)] fn println(_input : T) {} +#[oracle(println)] +unconstrained fn println_oracle(_input: T) {} + +#[oracle(println)] +unconstrained fn println_format_oracle(_msg: T, _args: [A]) {} + +unconstrained fn println_new(input: T) { + println_oracle(input); +} + +unconstrained fn println_format(msg: T, args: [A]) { + println_format_oracle(msg, args); +} + #[foreign(recursive_aggregation)] fn verify_proof(_verification_key : [Field], _proof : [Field], _public_inputs : [Field], _key_hash : Field, _input_aggregation_object : [Field]) -> [Field] {} From d75aedb9500414e09c97d498cb0e64b2e397a0de Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 11 Jul 2023 19:42:10 +0000 Subject: [PATCH 02/20] foreign call enum --- crates/nargo/src/ops/execute.rs | 21 ++++++++++++++++++++ crates/nargo/src/ops/foreign_calls.rs | 28 +++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/crates/nargo/src/ops/execute.rs b/crates/nargo/src/ops/execute.rs index 34c6105eb37..b308c34723d 100644 --- a/crates/nargo/src/ops/execute.rs +++ b/crates/nargo/src/ops/execute.rs @@ -5,6 +5,8 @@ use acvm::{acir::circuit::Circuit, acir::native_types::WitnessMap}; use crate::NargoError; +use super::foreign_calls::ForeignCall; + pub fn execute_circuit( _backend: &B, circuit: Circuit, @@ -60,3 +62,22 @@ fn execute_foreign_call(foreign_call: &ForeignCallWaitInfo) -> ForeignCallResult _ => panic!("unexpected foreign call type"), } } + +fn execute_foreign_call_new(foreign_call: &ForeignCallWaitInfo) { + let foreign_call_name = foreign_call.function.as_str(); + match ForeignCall::lookup(foreign_call_name) { + Some(ForeignCall::Println) => { + dbg!("println"); + } + Some(ForeignCall::PrintlnFormat) => { + dbg!("println_format"); + } + Some(ForeignCall::Sequence) => { + dbg!("sequence"); + } + Some(ForeignCall::ReverseSequence) => { + dbg!("ReverseSequence"); + } + None => panic!("unexpected foreign call {:?}", foreign_call_name), + } +} diff --git a/crates/nargo/src/ops/foreign_calls.rs b/crates/nargo/src/ops/foreign_calls.rs index 2912603c626..c9e828d0df1 100644 --- a/crates/nargo/src/ops/foreign_calls.rs +++ b/crates/nargo/src/ops/foreign_calls.rs @@ -5,4 +5,32 @@ pub enum ForeignCall { PrintlnFormat, Sequence, ReverseSequence, +} + +impl std::fmt::Display for ForeignCall { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.name()) + } +} + +impl ForeignCall { + pub fn name(&self) -> &'static str { + match self { + ForeignCall::Println => "println", + ForeignCall::PrintlnFormat => "println_format", + ForeignCall::Sequence => "get_number_sequence", + ForeignCall::ReverseSequence => "get_reverse_number_sequence", + } + } + + pub fn lookup(op_name: &str) -> Option { + match op_name { + "println" => Some(ForeignCall::Println), + "println_format" => Some(ForeignCall::PrintlnFormat), + "get_number_sequence" => Some(ForeignCall::Sequence), + "get_reverse_number_sequence" => Some(ForeignCall::ReverseSequence), + _ => None, + + } + } } \ No newline at end of file From ce082b622d9f40135200a6e483a39214f5d2c627 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 12 Jul 2023 19:56:59 +0000 Subject: [PATCH 03/20] working println and println_format w/ brillig oracles --- Cargo.lock | 3 + Cargo.toml | 3 +- crates/nargo/Cargo.toml | 2 + crates/nargo/src/errors.rs | 18 +++ crates/nargo/src/ops/execute.rs | 63 +--------- crates/nargo/src/ops/foreign_calls.rs | 114 +++++++++++++++++- crates/nargo/src/ops/mod.rs | 2 +- .../debug_logs/src/main.nr | 23 ++++ crates/noirc_abi/src/input_parser/json.rs | 4 +- crates/noirc_abi/src/input_parser/mod.rs | 2 +- crates/noirc_abi/src/lib.rs | 73 ++++++----- crates/noirc_frontend/Cargo.toml | 1 + .../src/monomorphization/mod.rs | 52 +++++++- noir_stdlib/src/lib.nr | 8 +- 14 files changed, 255 insertions(+), 113 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 30bd29bac1a..3f504e0d541 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1973,8 +1973,10 @@ dependencies = [ "iter-extended", "noirc_abi", "noirc_driver", + "regex", "rustc_version", "serde", + "serde_json", "thiserror", "toml", ] @@ -2117,6 +2119,7 @@ dependencies = [ "noirc_errors", "rustc-hash", "serde", + "serde_json", "small-ord-set", "smol_str", "strum", diff --git a/Cargo.toml b/Cargo.toml index 2ceef28d077..8e12729384b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -37,7 +37,6 @@ noirc_errors = { path = "crates/noirc_errors" } noirc_evaluator = { path = "crates/noirc_evaluator" } noirc_frontend = { path = "crates/noirc_frontend" } noir_wasm = { path = "crates/wasm" } - cfg-if = "1.0.0" clap = { version = "4.1.4", features = ["derive"] } codespan = "0.11.1" @@ -57,4 +56,4 @@ wasm-bindgen = { version = "=0.2.86", features = ["serde-serialize"] } wasm-bindgen-test = "0.3.33" [patch.crates-io] -async-lsp = { git = "https://github.com/oxalica/async-lsp", rev = "09dbcc11046f7a188a80137f8d36484d86c78c78" } +async-lsp = { git = "https://github.com/oxalica/async-lsp", rev = "09dbcc11046f7a188a80137f8d36484d86c78c78" } \ No newline at end of file diff --git a/crates/nargo/Cargo.toml b/crates/nargo/Cargo.toml index 8d3c9fbd3cd..5cd49ca7a4a 100644 --- a/crates/nargo/Cargo.toml +++ b/crates/nargo/Cargo.toml @@ -17,4 +17,6 @@ noirc_driver.workspace = true iter-extended.workspace = true toml.workspace = true serde.workspace = true +serde_json.workspace = true thiserror.workspace = true +regex = "1.9.1" diff --git a/crates/nargo/src/errors.rs b/crates/nargo/src/errors.rs index e9561aa15a9..2878d9f2db7 100644 --- a/crates/nargo/src/errors.rs +++ b/crates/nargo/src/errors.rs @@ -1,4 +1,5 @@ use acvm::pwg::OpcodeResolutionError; +use noirc_abi::errors::{AbiError, InputParserError}; use thiserror::Error; #[derive(Debug, Error)] @@ -10,4 +11,21 @@ pub enum NargoError { /// ACIR circuit solving error #[error(transparent)] SolvingError(#[from] OpcodeResolutionError), + + #[error(transparent)] + ForeignCallError(#[from] ForeignCallError), +} + +#[derive(Debug, Error)] +pub enum ForeignCallError { + #[error("Foreign call inputs needed for execution are missing")] + MissingForeignCallInputs, + + /// ABI encoding/decoding error + #[error(transparent)] + AbiError(#[from] AbiError), + + /// Input parsing error + #[error(transparent)] + InputParserError(#[from] InputParserError), } diff --git a/crates/nargo/src/ops/execute.rs b/crates/nargo/src/ops/execute.rs index 087ad58df49..dd68c30af4c 100644 --- a/crates/nargo/src/ops/execute.rs +++ b/crates/nargo/src/ops/execute.rs @@ -1,8 +1,6 @@ -use acvm::acir::brillig_vm::{ForeignCallResult, Value}; -use acvm::pwg::{ACVMStatus, ForeignCallWaitInfo, ACVM}; +use acvm::pwg::{ACVMStatus, ACVM}; use acvm::BlackBoxFunctionSolver; use acvm::{acir::circuit::Circuit, acir::native_types::WitnessMap}; -use iter_extended::vecmap; use crate::NargoError; @@ -26,7 +24,7 @@ pub fn execute_circuit( ACVMStatus::Failure(error) => return Err(error.into()), ACVMStatus::RequiresForeignCall => { while let Some(foreign_call) = acvm.get_pending_foreign_call() { - let foreign_call_result = execute_foreign_call(foreign_call); + let foreign_call_result = ForeignCall::execute(foreign_call)?; acvm.resolve_pending_foreign_call(foreign_call_result); } } @@ -36,60 +34,3 @@ pub fn execute_circuit( let solved_witness = acvm.finalize(); Ok(solved_witness) } - -fn execute_foreign_call(foreign_call: &ForeignCallWaitInfo) -> ForeignCallResult { - // TODO(#1615): Nargo only supports "oracle_print_**_impl" functions that print a singular value or an array and nothing else - // This should be expanded in a general logging refactor - match foreign_call.function.as_str() { - // TODO(#1910): Move to an enum and don't match directly on these strings - "oracle_print_impl" => { - let values = &foreign_call.inputs[0]; - println!("{:?}", values[0].to_field().to_hex()); - values[0].into() - } - "oracle_print_array_impl" => { - let mut outputs_hex = Vec::new(); - for values in &foreign_call.inputs { - for value in values { - outputs_hex.push(value.to_field().to_hex()); - } - } - // Join all of the hex strings using a comma - let comma_separated_elements = outputs_hex.join(", "); - let output_witnesses_string = "[".to_owned() + &comma_separated_elements + "]"; - println!("{output_witnesses_string}"); - - foreign_call.inputs[0][0].into() - } - "get_number_sequence" => { - let sequence_length: u128 = foreign_call.inputs[0][0].to_field().to_u128(); - - vecmap(0..sequence_length, Value::from).into() - } - "get_reverse_number_sequence" => { - let sequence_length: u128 = foreign_call.inputs[0][0].to_field().to_u128(); - - vecmap((0..sequence_length).rev(), Value::from).into() - } - _ => panic!("unexpected foreign call type"), - } -} - -fn execute_foreign_call_new(foreign_call: &ForeignCallWaitInfo) { - let foreign_call_name = foreign_call.function.as_str(); - match ForeignCall::lookup(foreign_call_name) { - Some(ForeignCall::Println) => { - dbg!("println"); - } - Some(ForeignCall::PrintlnFormat) => { - dbg!("println_format"); - } - Some(ForeignCall::Sequence) => { - dbg!("sequence"); - } - Some(ForeignCall::ReverseSequence) => { - dbg!("ReverseSequence"); - } - None => panic!("unexpected foreign call {:?}", foreign_call_name), - } -} diff --git a/crates/nargo/src/ops/foreign_calls.rs b/crates/nargo/src/ops/foreign_calls.rs index c9e828d0df1..f69d0b9dcd0 100644 --- a/crates/nargo/src/ops/foreign_calls.rs +++ b/crates/nargo/src/ops/foreign_calls.rs @@ -1,6 +1,16 @@ +use acvm::{ + acir::brillig_vm::{ForeignCallResult, Value}, + pwg::ForeignCallWaitInfo, +}; +use iter_extended::vecmap; +use noirc_abi::{decode_string_value, decode_value, input_parser::json::JsonTypes, AbiType}; +use regex::{Captures, Regex}; + +use crate::errors::ForeignCallError; + /// This enumeration represents the Brillig foreign calls that are natively supported by nargo. /// After resolution of a foreign call, nargo will restart execution of the ACVM -pub enum ForeignCall { +pub(crate) enum ForeignCall { Println, PrintlnFormat, Sequence, @@ -14,7 +24,7 @@ impl std::fmt::Display for ForeignCall { } impl ForeignCall { - pub fn name(&self) -> &'static str { + pub(crate) fn name(&self) -> &'static str { match self { ForeignCall::Println => "println", ForeignCall::PrintlnFormat => "println_format", @@ -23,14 +33,110 @@ impl ForeignCall { } } - pub fn lookup(op_name: &str) -> Option { + pub(crate) fn lookup(op_name: &str) -> Option { match op_name { "println" => Some(ForeignCall::Println), "println_format" => Some(ForeignCall::PrintlnFormat), "get_number_sequence" => Some(ForeignCall::Sequence), "get_reverse_number_sequence" => Some(ForeignCall::ReverseSequence), _ => None, + } + } + + pub(crate) fn execute( + foreign_call: &ForeignCallWaitInfo, + ) -> Result { + let foreign_call_name = foreign_call.function.as_str(); + match Self::lookup(foreign_call_name) { + Some(ForeignCall::Println) => { + Self::execute_println(&foreign_call.inputs)?; + Ok(foreign_call.inputs[0][0].into()) + } + Some(ForeignCall::PrintlnFormat) => { + Self::execute_println_format(&foreign_call.inputs)?; + Ok(foreign_call.inputs[0][0].into()) + } + Some(ForeignCall::Sequence) => { + let sequence_length: u128 = foreign_call.inputs[0][0].to_field().to_u128(); + + Ok(vecmap(0..sequence_length, Value::from).into()) + } + Some(ForeignCall::ReverseSequence) => { + let sequence_length: u128 = foreign_call.inputs[0][0].to_field().to_u128(); + + Ok(vecmap(0..sequence_length, Value::from).into()) + } + None => panic!("unexpected foreign call {:?}", foreign_call_name), + } + } + + fn execute_println(foreign_call_inputs: &[Vec]) -> Result<(), ForeignCallError> { + // Fetch the abi type from the foreign call input + let (abi_type, input_values) = fetch_abi_type(foreign_call_inputs)?; + + let input_values_as_fields = input_values + .into_iter() + .flat_map(|values| vecmap(values, |value| value.to_field())) + .collect::>(); + let decoded_value = decode_value(&mut input_values_as_fields.into_iter(), &abi_type) + .map_err(|err| ForeignCallError::AbiError(err))?; + + let json_value = JsonTypes::try_from_input_value(&decoded_value, &abi_type) + .map_err(|err| ForeignCallError::InputParserError(err))?; + let output_string = serde_json::to_string_pretty(&json_value) + .map_err(|err| ForeignCallError::InputParserError(err.into()))?; + println!("{output_string}"); + Ok(()) + } + + fn execute_println_format(foreign_call_inputs: &[Vec]) -> Result<(), ForeignCallError> { + // Fetch the message from the first input + let (message_as_values, input_and_abi_values) = + foreign_call_inputs.split_first().ok_or(ForeignCallError::MissingForeignCallInputs)?; + // Fetch the abi type from the foreign call input + let (abi_type, input_values) = fetch_abi_type(input_and_abi_values)?; + + let input_values_as_fields = vecmap(input_values[0].iter(), |value| value.to_field()); + + let mut output_strings = Vec::new(); + + // This currently only works for arrays of single values + for input_value in input_values_as_fields { + let value_to_decode = vec![input_value]; + let decoded_value = decode_value(&mut value_to_decode.into_iter(), &abi_type) + .map_err(|err| ForeignCallError::AbiError(err))?; + let json_value = JsonTypes::try_from_input_value(&decoded_value, &abi_type) + .map_err(|err| ForeignCallError::InputParserError(err))?; + let output_string = serde_json::to_string_pretty(&json_value) + .map_err(|err| ForeignCallError::InputParserError(err.into()))?; + output_strings.push(output_string); } + + let message_as_fields = vecmap(message_as_values, |value| value.to_field()); + let message_as_string = decode_string_value(&message_as_fields); + + let re = Regex::new(r"\{(\d+)\}").unwrap(); + + let formatted_str = re.replace_all(&message_as_string, |caps: &Captures| { + let (_, [target_idx]) = caps.extract(); + &output_strings[target_idx.parse::().unwrap()] + }); + println!("{formatted_str}"); + Ok(()) } -} \ No newline at end of file +} + +fn fetch_abi_type( + foreign_call_inputs: &[Vec], +) -> Result<(AbiType, &[Vec]), ForeignCallError> { + // Fetch the abi from the last input. We will now be left with + let (abi_type_as_values, input_values) = + foreign_call_inputs.split_last().ok_or(ForeignCallError::MissingForeignCallInputs)?; + let abi_type_as_fields = vecmap(abi_type_as_values, |value| value.to_field()); + let abi_type_as_string = decode_string_value(&abi_type_as_fields); + let abi_type: AbiType = serde_json::from_str(&abi_type_as_string) + .map_err(|err| ForeignCallError::InputParserError(err.into()))?; + + Ok((abi_type, input_values)) +} diff --git a/crates/nargo/src/ops/mod.rs b/crates/nargo/src/ops/mod.rs index f82fbffef7d..6d55e5b0dad 100644 --- a/crates/nargo/src/ops/mod.rs +++ b/crates/nargo/src/ops/mod.rs @@ -6,7 +6,7 @@ pub use self::verify::verify_proof; mod codegen_verifier; mod execute; +mod foreign_calls; mod preprocess; mod prove; mod verify; -mod foreign_calls; diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr index 7394d3d34d3..07dd4103218 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr @@ -3,5 +3,28 @@ use dep::std; fn main(x : Field, y : pub Field) { std::println_new(x); + std::println_new([x, y]); + + let s = myStruct { x: x, y: y }; + std::println_new(s); + + let foo = fooStruct { my_struct: s, foo: 15 }; + std::println_new(foo); + + std::println_format("x: {0}, y: {1}", [x, y]); + + // TODO: Not supported until we remove aos_to_soa from monomorphization pass + // std::println_format("s1: {0}, s2: {1}", [s, s]); + assert(x != y); } + +struct myStruct { + x: Field, + y: Field, +} + +struct fooStruct { + my_struct: myStruct, + foo: Field, +} diff --git a/crates/noirc_abi/src/input_parser/json.rs b/crates/noirc_abi/src/input_parser/json.rs index 748c0d09e04..e3a7b455bc8 100644 --- a/crates/noirc_abi/src/input_parser/json.rs +++ b/crates/noirc_abi/src/input_parser/json.rs @@ -59,7 +59,7 @@ pub(crate) fn serialize_to_json( #[derive(Debug, Deserialize, Serialize, Clone)] #[serde(untagged)] -enum JsonTypes { +pub enum JsonTypes { // This is most likely going to be a hex string // But it is possible to support UTF-8 String(String), @@ -78,7 +78,7 @@ enum JsonTypes { } impl JsonTypes { - fn try_from_input_value( + pub fn try_from_input_value( value: &InputValue, abi_type: &AbiType, ) -> Result { diff --git a/crates/noirc_abi/src/input_parser/mod.rs b/crates/noirc_abi/src/input_parser/mod.rs index da9d777d0ab..317cf75d187 100644 --- a/crates/noirc_abi/src/input_parser/mod.rs +++ b/crates/noirc_abi/src/input_parser/mod.rs @@ -1,4 +1,4 @@ -mod json; +pub mod json; mod toml; use std::{collections::BTreeMap, path::Path}; diff --git a/crates/noirc_abi/src/lib.rs b/crates/noirc_abi/src/lib.rs index 856889b3860..79250a8fe7a 100644 --- a/crates/noirc_abi/src/lib.rs +++ b/crates/noirc_abi/src/lib.rs @@ -345,7 +345,7 @@ impl Abi { .copied() })?; - Self::decode_value(&mut param_witness_values.into_iter(), &typ) + decode_value(&mut param_witness_values.into_iter(), &typ) .map(|input_value| (name.clone(), input_value)) })?; @@ -362,7 +362,7 @@ impl Abi { .copied() }) { - Some(Self::decode_value(&mut return_witness_values.into_iter(), return_type)?) + Some(decode_value(&mut return_witness_values.into_iter(), return_type)?) } else { // Unlike for the circuit inputs, we tolerate not being able to find the witness values for the return value. // This is because the user may be decoding a partial witness map for which is hasn't been calculated yet. @@ -375,49 +375,48 @@ impl Abi { Ok((public_inputs_map, return_value)) } +} - fn decode_value( - field_iterator: &mut impl Iterator, - value_type: &AbiType, - ) -> Result { - // This function assumes that `field_iterator` contains enough `FieldElement`s in order to decode a `value_type` - // `Abi.decode` enforces that the encoded inputs matches the expected length defined by the ABI so this is safe. - let value = match value_type { - AbiType::Field | AbiType::Integer { .. } | AbiType::Boolean => { - let field_element = field_iterator.next().unwrap(); - - InputValue::Field(field_element) - } - AbiType::Array { length, typ } => { - let length = *length as usize; - let mut array_elements = Vec::with_capacity(length); - for _ in 0..length { - array_elements.push(Self::decode_value(field_iterator, typ)?); - } - - InputValue::Vec(array_elements) +pub fn decode_value( + field_iterator: &mut impl Iterator, + value_type: &AbiType, +) -> Result { + // This function assumes that `field_iterator` contains enough `FieldElement`s in order to decode a `value_type` + // `Abi.decode` enforces that the encoded inputs matches the expected length defined by the ABI so this is safe. + let value = match value_type { + AbiType::Field | AbiType::Integer { .. } | AbiType::Boolean => { + let field_element = field_iterator.next().unwrap(); + + InputValue::Field(field_element) + } + AbiType::Array { length, typ } => { + let length = *length as usize; + let mut array_elements = Vec::with_capacity(length); + for _ in 0..length { + array_elements.push(decode_value(field_iterator, typ)?); } - AbiType::String { length } => { - let field_elements: Vec = - field_iterator.take(*length as usize).collect(); - InputValue::String(decode_string_value(&field_elements)) - } - AbiType::Struct { fields, .. } => { - let mut struct_map = BTreeMap::new(); + InputValue::Vec(array_elements) + } + AbiType::String { length } => { + let field_elements: Vec = field_iterator.take(*length as usize).collect(); - for (field_key, param_type) in fields { - let field_value = Self::decode_value(field_iterator, param_type)?; + InputValue::String(decode_string_value(&field_elements)) + } + AbiType::Struct { fields, .. } => { + let mut struct_map = BTreeMap::new(); - struct_map.insert(field_key.to_owned(), field_value); - } + for (field_key, param_type) in fields { + let field_value = decode_value(field_iterator, param_type)?; - InputValue::Struct(struct_map) + struct_map.insert(field_key.to_owned(), field_value); } - }; - Ok(value) - } + InputValue::Struct(struct_map) + } + }; + + Ok(value) } pub fn decode_string_value(field_elements: &[FieldElement]) -> String { diff --git a/crates/noirc_frontend/Cargo.toml b/crates/noirc_frontend/Cargo.toml index f3fc1c83758..a9a62673af6 100644 --- a/crates/noirc_frontend/Cargo.toml +++ b/crates/noirc_frontend/Cargo.toml @@ -17,6 +17,7 @@ chumsky.workspace = true thiserror.workspace = true smol_str.workspace = true serde.workspace = true +serde_json.workspace = true rustc-hash = "1.1.0" small-ord-set = "0.1.3" diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index d8dfb1550c0..f3bd51afe2a 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -745,15 +745,65 @@ impl<'interner> Monomorphizer<'interner> { id: node_interner::ExprId, ) -> ast::Expression { let func = Box::new(self.expr(call.func)); - let arguments = vecmap(&call.arguments, |id| self.expr(*id)); + let mut arguments = vecmap(&call.arguments, |id| self.expr(*id)); + let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); let return_type = self.interner.id_type(id); let return_type = Self::convert_type(&return_type); let location = call.location; + if let ast::Expression::Ident(ident) = func.as_ref() { + if let Definition::Oracle(name) = &ident.definition { + match name.as_str() { + "println" => { + self.append_abi_arg(&hir_arguments[0], &mut arguments, false); + } + "println_format" => { + // The first arugment represents the format string while the second argument + // contains an array of arguments to be formatted. + // Arrays can only have elements of the same type, thus we only need the `AbiType` of one element of the second type. + // The caller who executes this foreign call will then be responsible for handling formatting according + // to the string supplied in the first argument. + self.append_abi_arg(&hir_arguments[1], &mut arguments, true); + } + _ => (), + } + } + } + self.try_evaluate_call(&func, &call.arguments, &return_type) .unwrap_or(ast::Expression::Call(ast::Call { func, arguments, return_type, location })) } + fn append_abi_arg( + &self, + hir_argument: &HirExpression, + arguments: &mut Vec, + is_format_call: bool, + ) { + match hir_argument { + HirExpression::Ident(ident) => { + let typ = self.interner.id_type(ident.id); + let typ = if is_format_call { + match typ { + Type::Array(_, element_type) => element_type.follow_bindings(), + _ => { + unreachable!("ICE: argument supplied to a format call must be an array") + } + } + } else { + typ.follow_bindings() + }; + + let abi_type = typ.as_abi_type(); + let abi_as_string = + serde_json::to_string(&abi_type).expect("ICE: expected Abi type to serialize"); + + arguments.push(ast::Expression::Literal(ast::Literal::Str(abi_as_string))); + } + _ => unreachable!("logging expr {:?} is not supported", arguments[0]), + } + } + /// Try to evaluate certain builtin functions (currently only 'array_len' and field modulus methods) /// at their call site. /// NOTE: Evaluating at the call site means we cannot track aliased functions. diff --git a/noir_stdlib/src/lib.nr b/noir_stdlib/src/lib.nr index 6fae03b6c97..7a690448cf5 100644 --- a/noir_stdlib/src/lib.nr +++ b/noir_stdlib/src/lib.nr @@ -19,16 +19,16 @@ mod compat; fn println(_input : T) {} #[oracle(println)] -unconstrained fn println_oracle(_input: T) {} +unconstrained fn println_oracle(_input: T) -> Field {} -#[oracle(println)] -unconstrained fn println_format_oracle(_msg: T, _args: [A]) {} +#[oracle(println_format)] +unconstrained fn println_format_oracle(_msg: T, _args: [A; N]) -> Field {} unconstrained fn println_new(input: T) { println_oracle(input); } -unconstrained fn println_format(msg: T, args: [A]) { +unconstrained fn println_format(msg: T, args: [A; N]) { println_format_oracle(msg, args); } From 44ef83315db53a40d5a1463bef729864f7c95356 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 12 Jul 2023 20:06:30 +0000 Subject: [PATCH 04/20] fix up brillig_oracle test --- crates/nargo/src/ops/foreign_calls.rs | 2 +- .../brillig_oracle/src/main.nr | 29 +++---------------- crates/noirc_frontend/src/parser/parser.rs | 2 +- 3 files changed, 6 insertions(+), 27 deletions(-) diff --git a/crates/nargo/src/ops/foreign_calls.rs b/crates/nargo/src/ops/foreign_calls.rs index 0aaa1647311..db68361c0bc 100644 --- a/crates/nargo/src/ops/foreign_calls.rs +++ b/crates/nargo/src/ops/foreign_calls.rs @@ -64,7 +64,7 @@ impl ForeignCall { Some(ForeignCall::ReverseSequence) => { let sequence_length: u128 = foreign_call.inputs[0][0].to_field().to_u128(); - Ok(vecmap(0..sequence_length, Value::from).into()) + Ok(vecmap((0..sequence_length).rev(), Value::from).into()) } None => panic!("unexpected foreign call {:?}", foreign_call_name), } diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_oracle/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_oracle/src/main.nr index 53356c80ab9..a5dfa8fc5b7 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_oracle/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_oracle/src/main.nr @@ -2,31 +2,9 @@ use dep::std::slice; // Tests oracle usage in brillig/unconstrained functions fn main(x: Field) { - // call through a brillig wrapper - oracle_print_array_wrapper([x, x]); - - // TODO(#1615) Nargo currently only supports resolving one foreign call - // oracle_print_wrapper(x); - get_number_sequence_wrapper(20); } -// TODO(#1911) -#[oracle(oracle_print_impl)] -unconstrained fn oracle_print(_x : Field) -> Field {} - -unconstrained fn oracle_print_wrapper(x: Field) { - oracle_print(x); -} - -// TODO(#1911) -#[oracle(oracle_print_array_impl)] -unconstrained fn oracle_print_array(_arr : [Field; 2]) -> Field {} - -unconstrained fn oracle_print_array_wrapper(arr: [Field; 2]) { - oracle_print_array(arr); -} - // TODO(#1911): This function does not need to be an oracle but acts // as a useful test while we finalize code generation for slices in Brillig #[oracle(get_number_sequence)] @@ -43,9 +21,10 @@ unconstrained fn get_number_sequence_wrapper(size: Field) { } let reversed_slice = get_reverse_number_sequence(size); + assert(reversed_slice[19] == 0); // Regression test that we have not overwritten memory - for i in 0..19 as u32 { - assert(slice[i] == reversed_slice[19 - i]); - } + // for i in 0..19 as u32 { + // assert(slice[i] == reversed_slice[19 - i]); + // } } diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index 6e565ac9207..955c5027686 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -188,7 +188,7 @@ fn function_definition(allow_self: bool) -> impl NoirParser { }) } -/// function_modifiers: 'unconstrained'? 'open'? 'internal'? +/// function_modifiers: 'unconstrained'? 'open'? 'internal'? /// /// returns (is_unconstrained, is_open, is_internal) for whether each keyword was present fn function_modifiers() -> impl NoirParser<(bool, bool, bool)> { From e9f6488204710025412f24b3c5da1e4f75715276 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 12 Jul 2023 20:09:10 +0000 Subject: [PATCH 05/20] uncomment regression test for slice return from foreign calls in brillig --- .../test_data_ssa_refactor/brillig_oracle/src/main.nr | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_oracle/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_oracle/src/main.nr index a5dfa8fc5b7..84dcb1a0915 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_oracle/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_oracle/src/main.nr @@ -21,10 +21,9 @@ unconstrained fn get_number_sequence_wrapper(size: Field) { } let reversed_slice = get_reverse_number_sequence(size); - assert(reversed_slice[19] == 0); // Regression test that we have not overwritten memory - // for i in 0..19 as u32 { - // assert(slice[i] == reversed_slice[19 - i]); - // } + for i in 0..19 as u32 { + assert(slice[i] == reversed_slice[19 - i]); + } } From e457fafcdef5e505c90d2fbbf4c8ff9d713c0ac0 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 12 Jul 2023 20:17:11 +0000 Subject: [PATCH 06/20] cargo clippy --- crates/nargo/src/ops/foreign_calls.rs | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/crates/nargo/src/ops/foreign_calls.rs b/crates/nargo/src/ops/foreign_calls.rs index db68361c0bc..218eccc74f5 100644 --- a/crates/nargo/src/ops/foreign_calls.rs +++ b/crates/nargo/src/ops/foreign_calls.rs @@ -74,15 +74,11 @@ impl ForeignCall { // Fetch the abi type from the foreign call input let (abi_type, input_values) = fetch_abi_type(foreign_call_inputs)?; - let input_values_as_fields = input_values - .into_iter() - .flat_map(|values| vecmap(values, |value| value.to_field())) - .collect::>(); - let decoded_value = decode_value(&mut input_values_as_fields.into_iter(), &abi_type) - .map_err(|err| ForeignCallError::AbiError(err))?; - - let json_value = JsonTypes::try_from_input_value(&decoded_value, &abi_type) - .map_err(|err| ForeignCallError::InputParserError(err))?; + let mut input_values_as_fields = + input_values.iter().flat_map(|values| vecmap(values, |value| value.to_field())); + let decoded_value = decode_value(&mut input_values_as_fields, &abi_type)?; + + let json_value = JsonTypes::try_from_input_value(&decoded_value, &abi_type)?; let output_string = serde_json::to_string_pretty(&json_value) .map_err(|err| ForeignCallError::InputParserError(err.into()))?; @@ -104,10 +100,8 @@ impl ForeignCall { // This currently only works for arrays of single values for input_value in input_values_as_fields { let value_to_decode = vec![input_value]; - let decoded_value = decode_value(&mut value_to_decode.into_iter(), &abi_type) - .map_err(|err| ForeignCallError::AbiError(err))?; - let json_value = JsonTypes::try_from_input_value(&decoded_value, &abi_type) - .map_err(|err| ForeignCallError::InputParserError(err))?; + let decoded_value = decode_value(&mut value_to_decode.into_iter(), &abi_type)?; + let json_value = JsonTypes::try_from_input_value(&decoded_value, &abi_type)?; let output_string = serde_json::to_string_pretty(&json_value) .map_err(|err| ForeignCallError::InputParserError(err.into()))?; output_strings.push(output_string); From 7cb1b109f06990dfa27873f585d8aab8200d0e10 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Thu, 13 Jul 2023 14:36:11 +0000 Subject: [PATCH 07/20] got structs serialized correctly without aos_to_soa --- crates/nargo/src/ops/foreign_calls.rs | 5 +- .../debug_logs/src/main.nr | 14 ++- .../src/brillig/brillig_gen/brillig_block.rs | 6 +- .../src/brillig/brillig_ir/debug_show.rs | 2 +- .../src/ssa_refactor/ssa_gen/mod.rs | 1 + .../src/monomorphization/mod.rs | 103 +++++++++++------- 6 files changed, 80 insertions(+), 51 deletions(-) diff --git a/crates/nargo/src/ops/foreign_calls.rs b/crates/nargo/src/ops/foreign_calls.rs index 218eccc74f5..b0887983e0b 100644 --- a/crates/nargo/src/ops/foreign_calls.rs +++ b/crates/nargo/src/ops/foreign_calls.rs @@ -92,9 +92,10 @@ impl ForeignCall { foreign_call_inputs.split_first().ok_or(ForeignCallError::MissingForeignCallInputs)?; // Fetch the abi type from the foreign call input let (abi_type, input_values) = fetch_abi_type(input_and_abi_values)?; - + dbg!(input_values.clone()); + dbg!(abi_type.clone()); let input_values_as_fields = vecmap(input_values[0].iter(), |value| value.to_field()); - + dbg!(input_values_as_fields.clone()); let mut output_strings = Vec::new(); // This currently only works for arrays of single values diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr index 07dd4103218..c4b40ccb939 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr @@ -2,19 +2,21 @@ use dep::std; fn main(x : Field, y : pub Field) { - std::println_new(x); - std::println_new([x, y]); + // std::println_new(x); + // std::println_new([x, y]); let s = myStruct { x: x, y: y }; - std::println_new(s); + // std::println_new(s); let foo = fooStruct { my_struct: s, foo: 15 }; - std::println_new(foo); + // std::println_new(foo); - std::println_format("x: {0}, y: {1}", [x, y]); + // std::println_format("x: {0}, y: {1}", [x, y]); // TODO: Not supported until we remove aos_to_soa from monomorphization pass - // std::println_format("s1: {0}, s2: {1}", [s, s]); + let s2 = myStruct { x: 20, y: 30 }; + let struct_arr = [s, s2]; + std::println_format("s1: {0}, s2: {1}", [s, s2]); assert(x != y); } diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 7e8865328aa..5de583756ad 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -133,6 +133,7 @@ impl<'block> BrilligBlock<'block> { fn convert_block_params(&mut self, block: &BasicBlock, dfg: &DataFlowGraph) { for param_id in block.parameters() { let value = &dfg[*param_id]; + dbg!(value.clone()); let param_type = match value { Value::Param { typ, .. } => typ, _ => unreachable!("ICE: Only Param type values should appear in block parameters"), @@ -437,6 +438,7 @@ impl<'block> BrilligBlock<'block> { Value::Param { .. } | Value::Instruction { .. } => { // All block parameters and instruction results should have already been // converted to registers so we fetch from the cache. + dbg!(value.clone()); self.function_context.get_or_create_register(self.brillig_context, value_id) } Value::NumericConstant { constant, .. } => { @@ -471,9 +473,7 @@ impl<'block> BrilligBlock<'block> { let typ = dfg[value_id].get_type(); match typ { Type::Numeric(_) => RegisterOrMemory::RegisterIndex(register_index), - Type::Array(_, size) => { - RegisterOrMemory::HeapArray(HeapArray { pointer: register_index, size }) - } + Type::Array(..) => RegisterOrMemory::HeapArray(HeapArray { pointer: register_index, size: compute_size_of_type(&typ) }), _ => { unreachable!("type not supported for conversion into brillig register") } diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs b/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs index 977792ee07b..2762935a683 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs @@ -7,7 +7,7 @@ use acvm::acir::brillig::{ }; /// Controls whether debug traces are enabled -const ENABLE_DEBUG_TRACE: bool = true; +const ENABLE_DEBUG_TRACE: bool = false; /// Trait for converting values into debug-friendly strings. trait DebugToString { diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs index ac89575ecea..b6d8449e66c 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs @@ -362,6 +362,7 @@ impl<'a> FunctionContext<'a> { /// Generate SSA for a function call. Note that calls to built-in functions /// and intrinsics are also represented by the function call instruction. fn codegen_call(&mut self, call: &ast::Call) -> Values { + // dbg!(call.clone()); let function = self.codegen_non_tuple_expression(&call.func); let arguments = call .arguments diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index f3bd51afe2a..a30f686417a 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -188,7 +188,7 @@ impl<'interner> Monomorphizer<'interner> { let meta = self.interner.function_meta(&f); let name = self.interner.function_name(&f).to_owned(); - let return_type = Self::convert_type(meta.return_type()); + let return_type = self.convert_type(meta.return_type()); let parameters = self.parameters(meta.parameters); let body = self.expr(*self.interner.function(&f).as_expr()); let unconstrained = meta.is_unconstrained; @@ -223,7 +223,7 @@ impl<'interner> Monomorphizer<'interner> { let new_id = self.next_local_id(); let definition = self.interner.definition(ident.id); let name = definition.name.clone(); - new_params.push((new_id, definition.mutable, name, Self::convert_type(typ))); + new_params.push((new_id, definition.mutable, name, self.convert_type(typ))); self.define_local(ident.id, new_id); } HirPattern::Mutable(pattern, _) => self.parameter(*pattern, typ, new_params), @@ -262,7 +262,7 @@ impl<'interner> Monomorphizer<'interner> { HirExpression::Literal(HirLiteral::Str(contents)) => Literal(Str(contents)), HirExpression::Literal(HirLiteral::Bool(value)) => Literal(Bool(value)), HirExpression::Literal(HirLiteral::Integer(value)) => { - let typ = Self::convert_type(&self.interner.id_type(expr)); + let typ = self.convert_type(&self.interner.id_type(expr)); Literal(Integer(value, typ)) } HirExpression::Literal(HirLiteral::Array(array)) => match array { @@ -276,7 +276,7 @@ impl<'interner> Monomorphizer<'interner> { HirExpression::Prefix(prefix) => ast::Expression::Unary(ast::Unary { operator: prefix.operator, rhs: Box::new(self.expr(prefix.rhs)), - result_type: Self::convert_type(&self.interner.id_type(expr)), + result_type: self.convert_type(&self.interner.id_type(expr)), }), HirExpression::Infix(infix) => { @@ -299,7 +299,7 @@ impl<'interner> Monomorphizer<'interner> { HirExpression::Cast(cast) => ast::Expression::Cast(ast::Cast { lhs: Box::new(self.expr(cast.lhs)), - r#type: Self::convert_type(&cast.r#type), + r#type: self.convert_type(&cast.r#type), }), HirExpression::For(for_expr) => { @@ -313,7 +313,7 @@ impl<'interner> Monomorphizer<'interner> { ast::Expression::For(ast::For { index_variable, index_name: self.interner.definition_name(for_expr.identifier.id).to_owned(), - index_type: Self::convert_type(&self.interner.id_type(for_expr.start_range)), + index_type: self.convert_type(&self.interner.id_type(for_expr.start_range)), start_range: Box::new(start), end_range: Box::new(end), block, @@ -328,7 +328,7 @@ impl<'interner> Monomorphizer<'interner> { condition: Box::new(cond), consequence: Box::new(then), alternative: else_, - typ: Self::convert_type(&self.interner.id_type(expr)), + typ: self.convert_type(&self.interner.id_type(expr)), }) } @@ -353,9 +353,9 @@ impl<'interner> Monomorphizer<'interner> { array_elements: Vec, ) -> ast::Expression { let element_type = - Self::convert_type(&unwrap_array_element_type(&self.interner.id_type(array))); + self.convert_type(&unwrap_array_element_type(&self.interner.id_type(array))); let contents = vecmap(array_elements, |id| self.expr(id)); - Self::aos_to_soa(contents, element_type) + self.aos_to_soa(contents, element_type) } fn repeated_array( @@ -363,14 +363,14 @@ impl<'interner> Monomorphizer<'interner> { repeated_element: node_interner::ExprId, length: HirType, ) -> ast::Expression { - let element_type = Self::convert_type(&self.interner.id_type(repeated_element)); + let element_type = self.convert_type(&self.interner.id_type(repeated_element)); let contents = self.expr(repeated_element); let length = length .evaluate_to_u64() .expect("Length of array is unknown when evaluating numeric generic"); let contents = vec![contents; length as usize]; - Self::aos_to_soa(contents, element_type) + self.aos_to_soa(contents, element_type) } /// Convert an array in (potentially) array of structs form into struct of arrays form. @@ -379,9 +379,16 @@ impl<'interner> Monomorphizer<'interner> { /// /// TODO Remove side effects from clones fn aos_to_soa( + &self, array_contents: Vec, element_type: ast::Type, ) -> ast::Expression { + if self.interner.enable_slices { + return ast::Expression::Literal(ast::Literal::Array(ast::ArrayLiteral { + contents: array_contents, + element_type, + })) + } match element_type { ast::Type::Field | ast::Type::Integer(_, _) @@ -402,7 +409,7 @@ impl<'interner> Monomorphizer<'interner> { ast::Expression::ExtractTupleField(Box::new(element.clone()), i) }); - Self::aos_to_soa(contents, element_type) + self.aos_to_soa(contents, element_type) }, )), @@ -413,22 +420,26 @@ impl<'interner> Monomorphizer<'interner> { } fn index(&mut self, id: node_interner::ExprId, index: HirIndexExpression) -> ast::Expression { - let element_type = Self::convert_type(&self.interner.id_type(id)); + let element_type = self.convert_type(&self.interner.id_type(id)); let collection = Box::new(self.expr(index.collection)); let index = Box::new(self.expr(index.index)); let location = self.interner.expr_location(&id); - Self::aos_to_soa_index(collection, index, element_type, location) + self.aos_to_soa_index(collection, index, element_type, location) } /// Unpack an array index into an array of structs into a struct of arrays index if needed. /// E.g. transforms my_pair_array[i] into (my_pair1_array[i], my_pair2_array[i]) fn aos_to_soa_index( + &self, collection: Box, index: Box, element_type: ast::Type, location: Location, ) -> ast::Expression { + if self.interner.enable_slices { + return ast::Expression::Index(ast::Index { collection, index, element_type, location }) + } match element_type { ast::Type::Field | ast::Type::Integer(_, _) @@ -446,7 +457,7 @@ impl<'interner> Monomorphizer<'interner> { let collection = Box::new(ast::Expression::ExtractTupleField(collection.clone(), i)); - Self::aos_to_soa_index(collection, index.clone(), element_type, location) + self.aos_to_soa_index(collection, index.clone(), element_type, location) })) } @@ -495,7 +506,7 @@ impl<'interner> Monomorphizer<'interner> { for (field_name, expr_id) in constructor.fields { let new_id = self.next_local_id(); let field_type = field_type_map.get(&field_name.0.contents).unwrap(); - let typ = Self::convert_type(field_type); + let typ = self.convert_type(field_type); field_vars.insert(field_name.0.contents.clone(), (new_id, typ)); let expression = Box::new(self.expr(expr_id)); @@ -591,7 +602,7 @@ impl<'interner> Monomorphizer<'interner> { let mutable = false; let definition = Definition::Local(fresh_id); let name = i.to_string(); - let typ = Self::convert_type(&field_type); + let typ = self.convert_type(&field_type); let new_rhs = ast::Expression::Ident(ast::Ident { location, mutable, definition, name, typ }); @@ -611,7 +622,7 @@ impl<'interner> Monomorphizer<'interner> { let mutable = definition.mutable; let definition = self.lookup_local(ident.id)?; - let typ = Self::convert_type(&self.interner.id_type(ident.id)); + let typ = self.convert_type(&self.interner.id_type(ident.id)); Some(ast::Ident { location: Some(ident.location), mutable, definition, name, typ }) } @@ -626,7 +637,7 @@ impl<'interner> Monomorphizer<'interner> { let typ = self.interner.id_type(expr_id); let definition = self.lookup_function(*func_id, expr_id, &typ); - let typ = Self::convert_type(&typ); + let typ = self.convert_type(&typ); let ident = ast::Ident { location, mutable, definition, name, typ }; ast::Expression::Ident(ident) } @@ -652,7 +663,7 @@ impl<'interner> Monomorphizer<'interner> { } /// Convert a non-tuple/struct type to a monomorphized type - fn convert_type(typ: &HirType) -> ast::Type { + fn convert_type(&self, typ: &HirType) -> ast::Type { match typ { HirType::FieldElement(_) => ast::Type::Field, HirType::Integer(_, sign, bits) => ast::Type::Integer(*sign, *bits), @@ -662,12 +673,15 @@ impl<'interner> Monomorphizer<'interner> { HirType::Array(length, element) => { let length = length.evaluate_to_u64().unwrap_or(0); - let element = Self::convert_type(element.as_ref()); - Self::aos_to_soa_type(length, element) + let element = self.convert_type(element.as_ref()); + if self.interner.enable_slices { + return ast::Type::Array(length, Box::new(element)) + } + self.aos_to_soa_type(length, element) } HirType::Slice(element) => { - let element = Self::convert_type(element.as_ref()); + let element = self.convert_type(element.as_ref()); ast::Type::Slice(Box::new(element)) } @@ -675,7 +689,7 @@ impl<'interner> Monomorphizer<'interner> { | HirType::TypeVariable(binding) | HirType::NamedGeneric(binding, _) => { if let TypeBinding::Bound(binding) = &*binding.borrow() { - return Self::convert_type(binding); + return self.convert_type(binding); } // Default any remaining unbound type variables to Field. @@ -692,23 +706,23 @@ impl<'interner> Monomorphizer<'interner> { HirType::Struct(def, args) => { let fields = def.borrow().get_fields(args); - let fields = vecmap(fields, |(_, field)| Self::convert_type(&field)); + let fields = vecmap(fields, |(_, field)| self.convert_type(&field)); ast::Type::Tuple(fields) } HirType::Tuple(fields) => { - let fields = vecmap(fields, Self::convert_type); + let fields = vecmap(fields, |typ| self.convert_type(typ)); ast::Type::Tuple(fields) } HirType::Function(args, ret) => { - let args = vecmap(args, Self::convert_type); - let ret = Box::new(Self::convert_type(ret)); + let args = vecmap(args, |typ| self.convert_type(typ)); + let ret = Box::new(self.convert_type(ret)); ast::Type::Function(args, ret) } HirType::MutableReference(element) => { - let element = Self::convert_type(element); + let element = self.convert_type(element); ast::Type::MutableReference(Box::new(element)) } @@ -720,7 +734,10 @@ impl<'interner> Monomorphizer<'interner> { /// Converts arrays of structs (AOS) into structs of arrays (SOA). /// This is required since our SSA pass does not support arrays of structs. - fn aos_to_soa_type(length: u64, element: ast::Type) -> ast::Type { + fn aos_to_soa_type(&self, length: u64, element: ast::Type) -> ast::Type { + // if self.interner.enable_slices { + // return ast::Type::Array(length, Box::new(element)) + // } match element { ast::Type::Field | ast::Type::Integer(_, _) @@ -730,7 +747,7 @@ impl<'interner> Monomorphizer<'interner> { | ast::Type::MutableReference(_) => ast::Type::Array(length, Box::new(element)), ast::Type::Tuple(elements) => { - ast::Type::Tuple(vecmap(elements, |typ| Self::aos_to_soa_type(length, typ))) + ast::Type::Tuple(vecmap(elements, |typ| self.aos_to_soa_type(length, typ))) } ast::Type::Array(_, _) | ast::Type::String(_) | ast::Type::Slice(_) => { @@ -748,7 +765,7 @@ impl<'interner> Monomorphizer<'interner> { let mut arguments = vecmap(&call.arguments, |id| self.expr(*id)); let hir_arguments = vecmap(&call.arguments, |id| self.interner.expression(id)); let return_type = self.interner.id_type(id); - let return_type = Self::convert_type(&return_type); + let return_type = self.convert_type(&return_type); let location = call.location; if let ast::Expression::Ident(ident) = func.as_ref() { @@ -785,7 +802,10 @@ impl<'interner> Monomorphizer<'interner> { let typ = self.interner.id_type(ident.id); let typ = if is_format_call { match typ { - Type::Array(_, element_type) => element_type.follow_bindings(), + Type::Array(_, element_type) => { + let typ = element_type.follow_bindings(); + typ + } _ => { unreachable!("ICE: argument supplied to a format call must be an array") } @@ -913,7 +933,7 @@ impl<'interner> Monomorphizer<'interner> { match index_lvalue { Some((index, element_type, location)) => { - Self::aos_to_soa_assign(expression, Box::new(lvalue), index, element_type, location) + self.aos_to_soa_assign(expression, Box::new(lvalue), index, element_type, location) } None => ast::Expression::Assign(ast::Assign { expression, lvalue }), } @@ -948,13 +968,13 @@ impl<'interner> Monomorphizer<'interner> { ); let index = Box::new(self.expr(index)); - let element_type = Self::convert_type(&typ); + let element_type = self.convert_type(&typ); (array, Some((index, element_type, location))) } HirLValue::Dereference { lvalue, element_type } => { let (reference, index) = self.lvalue(*lvalue); let reference = Box::new(reference); - let element_type = Self::convert_type(&element_type); + let element_type = self.convert_type(&element_type); let lvalue = ast::LValue::Dereference { reference, element_type }; (lvalue, index) } @@ -962,12 +982,17 @@ impl<'interner> Monomorphizer<'interner> { } fn aos_to_soa_assign( + &self, expression: Box, lvalue: Box, index: Box, typ: ast::Type, location: Location, ) -> ast::Expression { + if self.interner.enable_slices { + let lvalue = ast::LValue::Index { array: lvalue, index, element_type: typ, location }; + return ast::Expression::Assign(ast::Assign { lvalue, expression }) + } match typ { ast::Type::Tuple(fields) => { let fields = fields.into_iter().enumerate(); @@ -975,7 +1000,7 @@ impl<'interner> Monomorphizer<'interner> { let expression = ast::Expression::ExtractTupleField(expression.clone(), i); let lvalue = ast::LValue::MemberAccess { object: lvalue.clone(), field_index: i }; - Self::aos_to_soa_assign( + self.aos_to_soa_assign( Box::new(expression), Box::new(lvalue), index.clone(), @@ -994,9 +1019,9 @@ impl<'interner> Monomorphizer<'interner> { } fn lambda(&mut self, lambda: HirLambda) -> ast::Expression { - let ret_type = Self::convert_type(&lambda.return_type); + let ret_type = self.convert_type(&lambda.return_type); let lambda_name = "lambda"; - let parameter_types = vecmap(&lambda.parameters, |(_, typ)| Self::convert_type(typ)); + let parameter_types = vecmap(&lambda.parameters, |(_, typ)| self.convert_type(typ)); // Manually convert to Parameters type so we can reuse the self.parameters method let parameters = Parameters(vecmap(lambda.parameters, |(pattern, typ)| { From c7e4f0a27fffb4396d2064a9f827501bc3f10174 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Thu, 13 Jul 2023 14:36:18 +0000 Subject: [PATCH 08/20] remove dbg --- crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 5de583756ad..daf6f4b872a 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -438,7 +438,6 @@ impl<'block> BrilligBlock<'block> { Value::Param { .. } | Value::Instruction { .. } => { // All block parameters and instruction results should have already been // converted to registers so we fetch from the cache. - dbg!(value.clone()); self.function_context.get_or_create_register(self.brillig_context, value_id) } Value::NumericConstant { constant, .. } => { From 1c64affd89516abc41c07514726a3daa845d6ac7 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Thu, 13 Jul 2023 15:46:40 +0000 Subject: [PATCH 09/20] working println_format --- crates/nargo/src/ops/foreign_calls.rs | 22 ++++++++++--------- .../debug_logs/src/main.nr | 21 +++++++++--------- crates/noirc_abi/src/input_parser/json.rs | 15 +++++++++++-- crates/noirc_abi/src/lib.rs | 9 -------- .../src/brillig/brillig_gen/brillig_block.rs | 6 +++-- .../src/monomorphization/mod.rs | 8 +++---- 6 files changed, 44 insertions(+), 37 deletions(-) diff --git a/crates/nargo/src/ops/foreign_calls.rs b/crates/nargo/src/ops/foreign_calls.rs index b0887983e0b..012ba47a7a1 100644 --- a/crates/nargo/src/ops/foreign_calls.rs +++ b/crates/nargo/src/ops/foreign_calls.rs @@ -72,8 +72,10 @@ impl ForeignCall { fn execute_println(foreign_call_inputs: &[Vec]) -> Result<(), ForeignCallError> { // Fetch the abi type from the foreign call input + // The remaining input values should hold what is to be printed let (abi_type, input_values) = fetch_abi_type(foreign_call_inputs)?; + // We must use a flat map here as each value in a struct will be in a separate input value let mut input_values_as_fields = input_values.iter().flat_map(|values| vecmap(values, |value| value.to_field())); let decoded_value = decode_value(&mut input_values_as_fields, &abi_type)?; @@ -91,19 +93,19 @@ impl ForeignCall { let (message_as_values, input_and_abi_values) = foreign_call_inputs.split_first().ok_or(ForeignCallError::MissingForeignCallInputs)?; // Fetch the abi type from the foreign call input + // The remaining input values should hold what is to be printed let (abi_type, input_values) = fetch_abi_type(input_and_abi_values)?; - dbg!(input_values.clone()); - dbg!(abi_type.clone()); - let input_values_as_fields = vecmap(input_values[0].iter(), |value| value.to_field()); - dbg!(input_values_as_fields.clone()); - let mut output_strings = Vec::new(); - // This currently only works for arrays of single values - for input_value in input_values_as_fields { - let value_to_decode = vec![input_value]; - let decoded_value = decode_value(&mut value_to_decode.into_iter(), &abi_type)?; + // Fetch the abi field count from the type to account for nested structs + let type_size = abi_type.field_count(); + let input_values_chunks = input_values[0].chunks(type_size as usize); + + let mut output_strings = Vec::new(); + for input_values in input_values_chunks { + let input_values_as_fields = vecmap(input_values, |value| value.to_field()); + let decoded_value = decode_value(&mut input_values_as_fields.into_iter(), &abi_type)?; let json_value = JsonTypes::try_from_input_value(&decoded_value, &abi_type)?; - let output_string = serde_json::to_string_pretty(&json_value) + let output_string = serde_json::to_string(&json_value) .map_err(|err| ForeignCallError::InputParserError(err.into()))?; output_strings.push(output_string); } diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr index c4b40ccb939..872cbf1eeff 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr @@ -2,28 +2,29 @@ use dep::std; fn main(x : Field, y : pub Field) { - // std::println_new(x); - // std::println_new([x, y]); + std::println_new("*** println_new ***"); + std::println_new(x); + std::println_new([x, y]); - let s = myStruct { x: x, y: y }; - // std::println_new(s); - + let s = myStruct { y: x, x: y }; let foo = fooStruct { my_struct: s, foo: 15 }; - // std::println_new(foo); + std::println_new(foo); - // std::println_format("x: {0}, y: {1}", [x, y]); + std::println_new("*** println_format ***"); + std::println_format("x: {0}, y: {1}", [x, y]); - // TODO: Not supported until we remove aos_to_soa from monomorphization pass let s2 = myStruct { x: 20, y: 30 }; - let struct_arr = [s, s2]; std::println_format("s1: {0}, s2: {1}", [s, s2]); + let foo2 = fooStruct { my_struct: s2, foo: 20 }; + std::println_format("foo1: {0}, foo2: {1}", [foo, foo2]); + assert(x != y); } struct myStruct { - x: Field, y: Field, + x: Field, } struct fooStruct { diff --git a/crates/noirc_abi/src/input_parser/json.rs b/crates/noirc_abi/src/input_parser/json.rs index e3a7b455bc8..a009d03af7b 100644 --- a/crates/noirc_abi/src/input_parser/json.rs +++ b/crates/noirc_abi/src/input_parser/json.rs @@ -84,8 +84,7 @@ impl JsonTypes { ) -> Result { let json_value = match (value, abi_type) { (InputValue::Field(f), AbiType::Field | AbiType::Integer { .. }) => { - let f_str = format!("0x{}", f.to_hex()); - JsonTypes::String(f_str) + JsonTypes::String(Self::format_field_string(*f)) } (InputValue::Field(f), AbiType::Boolean) => JsonTypes::Bool(f.is_one()), @@ -109,6 +108,18 @@ impl JsonTypes { }; Ok(json_value) } + + /// This trims any leading zeroes. + /// A singular '0' will be prepended as well if the trimmed string has an odd length. + /// A hex string's length needs to be even to decode into bytes, as two digits correspond to + /// one byte. + fn format_field_string(field: FieldElement) -> String { + let mut trimmed_field = field.to_hex().trim_start_matches('0').to_owned(); + if trimmed_field.len() % 2 != 0 { + trimmed_field = "0".to_owned() + &trimmed_field + } + "0x".to_owned() + &trimmed_field + } } impl InputValue { diff --git a/crates/noirc_abi/src/lib.rs b/crates/noirc_abi/src/lib.rs index 79250a8fe7a..86f9edc73bd 100644 --- a/crates/noirc_abi/src/lib.rs +++ b/crates/noirc_abi/src/lib.rs @@ -117,15 +117,6 @@ pub enum Sign { } impl AbiType { - pub fn num_elements(&self) -> usize { - match self { - AbiType::Field | AbiType::Integer { .. } | AbiType::Boolean => 1, - AbiType::Array { length, typ: _ } => *length as usize, - AbiType::Struct { fields, .. } => fields.len(), - AbiType::String { length } => *length as usize, - } - } - /// Returns the number of field elements required to represent the type once encoded. pub fn field_count(&self) -> u32 { match self { diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index daf6f4b872a..a5193a0369a 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -133,7 +133,6 @@ impl<'block> BrilligBlock<'block> { fn convert_block_params(&mut self, block: &BasicBlock, dfg: &DataFlowGraph) { for param_id in block.parameters() { let value = &dfg[*param_id]; - dbg!(value.clone()); let param_type = match value { Value::Param { typ, .. } => typ, _ => unreachable!("ICE: Only Param type values should appear in block parameters"), @@ -472,7 +471,10 @@ impl<'block> BrilligBlock<'block> { let typ = dfg[value_id].get_type(); match typ { Type::Numeric(_) => RegisterOrMemory::RegisterIndex(register_index), - Type::Array(..) => RegisterOrMemory::HeapArray(HeapArray { pointer: register_index, size: compute_size_of_type(&typ) }), + Type::Array(..) => RegisterOrMemory::HeapArray(HeapArray { + pointer: register_index, + size: compute_size_of_type(&typ), + }), _ => { unreachable!("type not supported for conversion into brillig register") } diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index a30f686417a..8c62b8d0fd7 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -387,7 +387,7 @@ impl<'interner> Monomorphizer<'interner> { return ast::Expression::Literal(ast::Literal::Array(ast::ArrayLiteral { contents: array_contents, element_type, - })) + })); } match element_type { ast::Type::Field @@ -438,7 +438,7 @@ impl<'interner> Monomorphizer<'interner> { location: Location, ) -> ast::Expression { if self.interner.enable_slices { - return ast::Expression::Index(ast::Index { collection, index, element_type, location }) + return ast::Expression::Index(ast::Index { collection, index, element_type, location }); } match element_type { ast::Type::Field @@ -675,7 +675,7 @@ impl<'interner> Monomorphizer<'interner> { let length = length.evaluate_to_u64().unwrap_or(0); let element = self.convert_type(element.as_ref()); if self.interner.enable_slices { - return ast::Type::Array(length, Box::new(element)) + return ast::Type::Array(length, Box::new(element)); } self.aos_to_soa_type(length, element) } @@ -991,7 +991,7 @@ impl<'interner> Monomorphizer<'interner> { ) -> ast::Expression { if self.interner.enable_slices { let lvalue = ast::LValue::Index { array: lvalue, index, element_type: typ, location }; - return ast::Expression::Assign(ast::Assign { lvalue, expression }) + return ast::Expression::Assign(ast::Assign { lvalue, expression }); } match typ { ast::Type::Tuple(fields) => { From 5c24bf7a9984465d8f7cb7bca332dc1fd1a4ed9e Mon Sep 17 00:00:00 2001 From: vezenovm Date: Thu, 13 Jul 2023 16:09:46 +0000 Subject: [PATCH 10/20] cargo clippy --- crates/noirc_abi/src/input_parser/json.rs | 2 +- crates/noirc_frontend/src/monomorphization/mod.rs | 11 ++++------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/crates/noirc_abi/src/input_parser/json.rs b/crates/noirc_abi/src/input_parser/json.rs index a009d03af7b..7640dd29961 100644 --- a/crates/noirc_abi/src/input_parser/json.rs +++ b/crates/noirc_abi/src/input_parser/json.rs @@ -116,7 +116,7 @@ impl JsonTypes { fn format_field_string(field: FieldElement) -> String { let mut trimmed_field = field.to_hex().trim_start_matches('0').to_owned(); if trimmed_field.len() % 2 != 0 { - trimmed_field = "0".to_owned() + &trimmed_field + trimmed_field = "0".to_owned() + &trimmed_field; } "0x".to_owned() + &trimmed_field } diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index 8c62b8d0fd7..913523f4b8c 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -735,9 +735,9 @@ impl<'interner> Monomorphizer<'interner> { /// Converts arrays of structs (AOS) into structs of arrays (SOA). /// This is required since our SSA pass does not support arrays of structs. fn aos_to_soa_type(&self, length: u64, element: ast::Type) -> ast::Type { - // if self.interner.enable_slices { - // return ast::Type::Array(length, Box::new(element)) - // } + if self.interner.enable_slices { + return ast::Type::Array(length, Box::new(element)) + } match element { ast::Type::Field | ast::Type::Integer(_, _) @@ -802,10 +802,7 @@ impl<'interner> Monomorphizer<'interner> { let typ = self.interner.id_type(ident.id); let typ = if is_format_call { match typ { - Type::Array(_, element_type) => { - let typ = element_type.follow_bindings(); - typ - } + Type::Array(_, element_type) => element_type.follow_bindings(), _ => { unreachable!("ICE: argument supplied to a format call must be an array") } From f4b17d47e1b3f211098cf0a58e6417bdfe6f3ab8 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Thu, 13 Jul 2023 16:16:44 +0000 Subject: [PATCH 11/20] rename enable_slices to experimental_ssa --- crates/nargo_cli/src/cli/check_cmd.rs | 5 +++-- crates/noirc_driver/src/lib.rs | 4 ++-- crates/noirc_frontend/src/hir/def_map/mod.rs | 2 +- .../src/hir/resolution/resolver.rs | 2 +- .../src/monomorphization/mod.rs | 19 ++++++++++++------- crates/noirc_frontend/src/node_interner.rs | 7 ++++--- 6 files changed, 23 insertions(+), 16 deletions(-) diff --git a/crates/nargo_cli/src/cli/check_cmd.rs b/crates/nargo_cli/src/cli/check_cmd.rs index 5abef7bfb2a..f7121f8984b 100644 --- a/crates/nargo_cli/src/cli/check_cmd.rs +++ b/crates/nargo_cli/src/cli/check_cmd.rs @@ -218,8 +218,9 @@ d2 = ["", "", ""] pub(crate) fn check_crate_and_report_errors( context: &mut Context, deny_warnings: bool, - enable_slices: bool, + experimental_ssa: bool, ) -> Result<(), ReportedErrors> { - let result = check_crate(context, deny_warnings, enable_slices).map(|warnings| ((), warnings)); + let result = + check_crate(context, deny_warnings, experimental_ssa).map(|warnings| ((), warnings)); super::compile_cmd::report_errors(result, context, deny_warnings) } diff --git a/crates/noirc_driver/src/lib.rs b/crates/noirc_driver/src/lib.rs index b4d0e636e20..ec8a81536a9 100644 --- a/crates/noirc_driver/src/lib.rs +++ b/crates/noirc_driver/src/lib.rs @@ -152,7 +152,7 @@ pub fn propagate_dep( pub fn check_crate( context: &mut Context, deny_warnings: bool, - enable_slices: bool, + experimental_ssa: bool, ) -> Result { // Add the stdlib before we check the crate // TODO: This should actually be done when constructing the driver and then propagated to each dependency when added; @@ -163,7 +163,7 @@ pub fn check_crate( let std_crate = create_non_local_crate(context, path_to_std_lib_file, CrateType::Library); propagate_dep(context, std_crate, &CrateName::new(std_crate_name).unwrap()); - context.def_interner.enable_slices = enable_slices; + context.def_interner.experimental_ssa = experimental_ssa; let mut errors = vec![]; match context.crate_graph.crate_type(LOCAL_CRATE) { diff --git a/crates/noirc_frontend/src/hir/def_map/mod.rs b/crates/noirc_frontend/src/hir/def_map/mod.rs index 3997903a43e..6afe4f8c234 100644 --- a/crates/noirc_frontend/src/hir/def_map/mod.rs +++ b/crates/noirc_frontend/src/hir/def_map/mod.rs @@ -86,7 +86,7 @@ impl CrateDefMap { // The last crate represents the stdlib crate. // After resolving the manifest of the local crate the stdlib is added to the manifest and propagated to all crates // thus being the last crate. - if !context.def_interner.enable_slices && context.crate_graph.is_last_crate(crate_id) { + if !context.def_interner.experimental_ssa && context.crate_graph.is_last_crate(crate_id) { let path_as_str = context .file_manager .path(root_file_id) diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index ea0f341e983..e0631aaea55 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -326,7 +326,7 @@ impl<'a> Resolver<'a> { UnresolvedType::FieldElement(comp_time) => Type::FieldElement(comp_time), UnresolvedType::Array(size, elem) => { let elem = Box::new(self.resolve_type_inner(*elem, new_variables)); - if self.interner.enable_slices && size.is_none() { + if self.interner.experimental_ssa && size.is_none() { return Type::Slice(elem); } let resolved_size = self.resolve_array_size(size, new_variables); diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index 913523f4b8c..a18fad49bf0 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -383,7 +383,7 @@ impl<'interner> Monomorphizer<'interner> { array_contents: Vec, element_type: ast::Type, ) -> ast::Expression { - if self.interner.enable_slices { + if self.interner.experimental_ssa { return ast::Expression::Literal(ast::Literal::Array(ast::ArrayLiteral { contents: array_contents, element_type, @@ -437,8 +437,13 @@ impl<'interner> Monomorphizer<'interner> { element_type: ast::Type, location: Location, ) -> ast::Expression { - if self.interner.enable_slices { - return ast::Expression::Index(ast::Index { collection, index, element_type, location }); + if self.interner.experimental_ssa { + return ast::Expression::Index(ast::Index { + collection, + index, + element_type, + location, + }); } match element_type { ast::Type::Field @@ -674,7 +679,7 @@ impl<'interner> Monomorphizer<'interner> { HirType::Array(length, element) => { let length = length.evaluate_to_u64().unwrap_or(0); let element = self.convert_type(element.as_ref()); - if self.interner.enable_slices { + if self.interner.experimental_ssa { return ast::Type::Array(length, Box::new(element)); } self.aos_to_soa_type(length, element) @@ -735,8 +740,8 @@ impl<'interner> Monomorphizer<'interner> { /// Converts arrays of structs (AOS) into structs of arrays (SOA). /// This is required since our SSA pass does not support arrays of structs. fn aos_to_soa_type(&self, length: u64, element: ast::Type) -> ast::Type { - if self.interner.enable_slices { - return ast::Type::Array(length, Box::new(element)) + if self.interner.experimental_ssa { + return ast::Type::Array(length, Box::new(element)); } match element { ast::Type::Field @@ -986,7 +991,7 @@ impl<'interner> Monomorphizer<'interner> { typ: ast::Type, location: Location, ) -> ast::Expression { - if self.interner.enable_slices { + if self.interner.experimental_ssa { let lvalue = ast::LValue::Index { array: lvalue, index, element_type: typ, location }; return ast::Expression::Assign(ast::Assign { lvalue, expression }); } diff --git a/crates/noirc_frontend/src/node_interner.rs b/crates/noirc_frontend/src/node_interner.rs index f37daf45136..6953eaa1806 100644 --- a/crates/noirc_frontend/src/node_interner.rs +++ b/crates/noirc_frontend/src/node_interner.rs @@ -72,8 +72,9 @@ pub struct NodeInterner { primitive_methods: HashMap<(TypeMethodKey, String), FuncId>, /// TODO(#1850): This is technical debt that should be removed once we fully move over - /// to the new SSA pass which does have slices enabled - pub enable_slices: bool, + /// to the new SSA pass which has certain frontend features enabled + /// such as slices and the removal of aos_to_soa + pub experimental_ssa: bool, } #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)] @@ -253,7 +254,7 @@ impl Default for NodeInterner { globals: HashMap::new(), struct_methods: HashMap::new(), primitive_methods: HashMap::new(), - enable_slices: false, + experimental_ssa: false, }; // An empty block expression is used often, we add this into the `node` on startup From d2065ea68d7cfabe17b842c8776dd99476c7fd65 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Thu, 13 Jul 2023 16:58:25 +0000 Subject: [PATCH 12/20] remove dbg and fix format_field_string --- crates/noirc_abi/src/input_parser/json.rs | 3 +++ crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs | 2 +- crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs | 1 - 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/noirc_abi/src/input_parser/json.rs b/crates/noirc_abi/src/input_parser/json.rs index 7640dd29961..782d93c698f 100644 --- a/crates/noirc_abi/src/input_parser/json.rs +++ b/crates/noirc_abi/src/input_parser/json.rs @@ -114,6 +114,9 @@ impl JsonTypes { /// A hex string's length needs to be even to decode into bytes, as two digits correspond to /// one byte. fn format_field_string(field: FieldElement) -> String { + if field.is_zero() { + return "0x00".to_owned(); + } let mut trimmed_field = field.to_hex().trim_start_matches('0').to_owned(); if trimmed_field.len() % 2 != 0 { trimmed_field = "0".to_owned() + &trimmed_field; diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs b/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs index 2762935a683..977792ee07b 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs @@ -7,7 +7,7 @@ use acvm::acir::brillig::{ }; /// Controls whether debug traces are enabled -const ENABLE_DEBUG_TRACE: bool = false; +const ENABLE_DEBUG_TRACE: bool = true; /// Trait for converting values into debug-friendly strings. trait DebugToString { diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs index a7b95b3f381..13e67f26cc5 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs @@ -364,7 +364,6 @@ impl<'a> FunctionContext<'a> { /// Generate SSA for a function call. Note that calls to built-in functions /// and intrinsics are also represented by the function call instruction. fn codegen_call(&mut self, call: &ast::Call) -> Values { - // dbg!(call.clone()); let function = self.codegen_non_tuple_expression(&call.func); let arguments = call .arguments From e6f5815e8c6d53a4d0e5cf120549e37c3e056a7d Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 19 Jul 2023 16:05:51 +0000 Subject: [PATCH 13/20] pr comments, and remove format work to move into separate PR --- Cargo.lock | 1 - crates/nargo/Cargo.toml | 3 +- crates/nargo/src/ops/foreign_calls.rs | 56 ++----------------- crates/nargo_cli/src/cli/check_cmd.rs | 4 +- .../debug_logs/src/main.nr | 11 ---- crates/noirc_abi/src/input_parser/json.rs | 10 ++++ .../src/monomorphization/mod.rs | 44 +++++---------- noir_stdlib/src/lib.nr | 7 --- 8 files changed, 34 insertions(+), 102 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 060768a397b..bf2d45c50da 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1987,7 +1987,6 @@ dependencies = [ "noirc_abi", "noirc_driver", "noirc_errors", - "regex", "rustc_version", "serde", "serde_json", diff --git a/crates/nargo/Cargo.toml b/crates/nargo/Cargo.toml index afbafdff931..6c053cba931 100644 --- a/crates/nargo/Cargo.toml +++ b/crates/nargo/Cargo.toml @@ -20,5 +20,4 @@ serde.workspace = true serde_json.workspace = true thiserror.workspace = true noirc_errors.workspace = true -base64.workspace = true -regex = "1.9.1" +base64.workspace = true \ No newline at end of file diff --git a/crates/nargo/src/ops/foreign_calls.rs b/crates/nargo/src/ops/foreign_calls.rs index 012ba47a7a1..5dacbe2965d 100644 --- a/crates/nargo/src/ops/foreign_calls.rs +++ b/crates/nargo/src/ops/foreign_calls.rs @@ -4,7 +4,6 @@ use acvm::{ }; use iter_extended::vecmap; use noirc_abi::{decode_string_value, decode_value, input_parser::json::JsonTypes, AbiType}; -use regex::{Captures, Regex}; use crate::errors::ForeignCallError; @@ -12,7 +11,6 @@ use crate::errors::ForeignCallError; /// After resolution of a foreign call, nargo will restart execution of the ACVM pub(crate) enum ForeignCall { Println, - PrintlnFormat, Sequence, ReverseSequence, } @@ -27,7 +25,6 @@ impl ForeignCall { pub(crate) fn name(&self) -> &'static str { match self { ForeignCall::Println => "println", - ForeignCall::PrintlnFormat => "println_format", ForeignCall::Sequence => "get_number_sequence", ForeignCall::ReverseSequence => "get_reverse_number_sequence", } @@ -36,7 +33,6 @@ impl ForeignCall { pub(crate) fn lookup(op_name: &str) -> Option { match op_name { "println" => Some(ForeignCall::Println), - "println_format" => Some(ForeignCall::PrintlnFormat), "get_number_sequence" => Some(ForeignCall::Sequence), "get_reverse_number_sequence" => Some(ForeignCall::ReverseSequence), _ => None, @@ -52,10 +48,6 @@ impl ForeignCall { Self::execute_println(&foreign_call.inputs)?; Ok(foreign_call.inputs[0][0].into()) } - Some(ForeignCall::PrintlnFormat) => { - Self::execute_println_format(&foreign_call.inputs)?; - Ok(foreign_call.inputs[0][0].into()) - } Some(ForeignCall::Sequence) => { let sequence_length: u128 = foreign_call.inputs[0][0].to_field().to_u128(); @@ -71,63 +63,27 @@ impl ForeignCall { } fn execute_println(foreign_call_inputs: &[Vec]) -> Result<(), ForeignCallError> { - // Fetch the abi type from the foreign call input - // The remaining input values should hold what is to be printed let (abi_type, input_values) = fetch_abi_type(foreign_call_inputs)?; // We must use a flat map here as each value in a struct will be in a separate input value + // let mut input_values_as_fields = + // input_values.iter().flat_map(|values| vecmap(values, |value| value.to_field())); let mut input_values_as_fields = - input_values.iter().flat_map(|values| vecmap(values, |value| value.to_field())); + input_values.iter().flat_map(|values| values.iter().map(|value| value.to_field())); let decoded_value = decode_value(&mut input_values_as_fields, &abi_type)?; let json_value = JsonTypes::try_from_input_value(&decoded_value, &abi_type)?; - let output_string = serde_json::to_string_pretty(&json_value) - .map_err(|err| ForeignCallError::InputParserError(err.into()))?; - - println!("{output_string}"); - Ok(()) - } - - fn execute_println_format(foreign_call_inputs: &[Vec]) -> Result<(), ForeignCallError> { - // Fetch the message from the first input - let (message_as_values, input_and_abi_values) = - foreign_call_inputs.split_first().ok_or(ForeignCallError::MissingForeignCallInputs)?; - // Fetch the abi type from the foreign call input - // The remaining input values should hold what is to be printed - let (abi_type, input_values) = fetch_abi_type(input_and_abi_values)?; - - // Fetch the abi field count from the type to account for nested structs - let type_size = abi_type.field_count(); - let input_values_chunks = input_values[0].chunks(type_size as usize); - - let mut output_strings = Vec::new(); - for input_values in input_values_chunks { - let input_values_as_fields = vecmap(input_values, |value| value.to_field()); - let decoded_value = decode_value(&mut input_values_as_fields.into_iter(), &abi_type)?; - let json_value = JsonTypes::try_from_input_value(&decoded_value, &abi_type)?; - let output_string = serde_json::to_string(&json_value) - .map_err(|err| ForeignCallError::InputParserError(err.into()))?; - output_strings.push(output_string); - } - - let message_as_fields = vecmap(message_as_values, |value| value.to_field()); - let message_as_string = decode_string_value(&message_as_fields); - - let re = Regex::new(r"\{(\d+)\}").unwrap(); - let formatted_str = re.replace_all(&message_as_string, |caps: &Captures| { - let (_, [target_idx]) = caps.extract(); - &output_strings[target_idx.parse::().unwrap()] - }); - println!("{formatted_str}"); + println!("{json_value}"); Ok(()) } } +/// Fetch the abi type from the foreign call input +/// The remaining input values should hold the values to be printed fn fetch_abi_type( foreign_call_inputs: &[Vec], ) -> Result<(AbiType, &[Vec]), ForeignCallError> { - // Fetch the abi from the last input. We will now be left with let (abi_type_as_values, input_values) = foreign_call_inputs.split_last().ok_or(ForeignCallError::MissingForeignCallInputs)?; let abi_type_as_fields = vecmap(abi_type_as_values, |value| value.to_field()); diff --git a/crates/nargo_cli/src/cli/check_cmd.rs b/crates/nargo_cli/src/cli/check_cmd.rs index 9cb023a0e8f..61a4f79593c 100644 --- a/crates/nargo_cli/src/cli/check_cmd.rs +++ b/crates/nargo_cli/src/cli/check_cmd.rs @@ -222,7 +222,7 @@ pub(crate) fn check_crate_and_report_errors( deny_warnings: bool, experimental_ssa: bool, ) -> Result<(), ReportedErrors> { - let result = - check_crate(context, crate_id, deny_warnings, experimental_ssa).map(|warnings| ((), warnings)); + let result = check_crate(context, crate_id, deny_warnings, experimental_ssa) + .map(|warnings| ((), warnings)); super::compile_cmd::report_errors(result, context, deny_warnings) } diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr index 872cbf1eeff..7c4252bc71a 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr @@ -9,17 +9,6 @@ fn main(x : Field, y : pub Field) { let s = myStruct { y: x, x: y }; let foo = fooStruct { my_struct: s, foo: 15 }; std::println_new(foo); - - std::println_new("*** println_format ***"); - std::println_format("x: {0}, y: {1}", [x, y]); - - let s2 = myStruct { x: 20, y: 30 }; - std::println_format("s1: {0}, s2: {1}", [s, s2]); - - let foo2 = fooStruct { my_struct: s2, foo: 20 }; - std::println_format("foo1: {0}, foo2: {1}", [foo, foo2]); - - assert(x != y); } struct myStruct { diff --git a/crates/noirc_abi/src/input_parser/json.rs b/crates/noirc_abi/src/input_parser/json.rs index 782d93c698f..7a0cd76698d 100644 --- a/crates/noirc_abi/src/input_parser/json.rs +++ b/crates/noirc_abi/src/input_parser/json.rs @@ -175,3 +175,13 @@ impl InputValue { Ok(input_value) } } + +impl std::fmt::Display for JsonTypes { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + // From the docs: https://doc.rust-lang.org/std/fmt/struct.Error.html + // This type does not support transmission of an error other than that an error + // occurred. Any extra information must be arranged to be transmitted through + // some other means. + write!(f, "{}", serde_json::to_string(&self).map_err(|_| std::fmt::Error)?) + } +} diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index a42f74ae97b..df9be34bc41 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -776,19 +776,8 @@ impl<'interner> Monomorphizer<'interner> { if let ast::Expression::Ident(ident) = func.as_ref() { if let Definition::Oracle(name) = &ident.definition { - match name.as_str() { - "println" => { - self.append_abi_arg(&hir_arguments[0], &mut arguments, false); - } - "println_format" => { - // The first arugment represents the format string while the second argument - // contains an array of arguments to be formatted. - // Arrays can only have elements of the same type, thus we only need the `AbiType` of one element of the second type. - // The caller who executes this foreign call will then be responsible for handling formatting according - // to the string supplied in the first argument. - self.append_abi_arg(&hir_arguments[1], &mut arguments, true); - } - _ => (), + if name.as_str() == "println" { + self.append_abi_arg(&hir_arguments[0], &mut arguments); } } } @@ -797,25 +786,22 @@ impl<'interner> Monomorphizer<'interner> { .unwrap_or(ast::Expression::Call(ast::Call { func, arguments, return_type, location })) } - fn append_abi_arg( - &self, - hir_argument: &HirExpression, - arguments: &mut Vec, - is_format_call: bool, - ) { + /// Adds a function argument that contains type metadata that is required to tell + /// a caller (such as nargo) how to convert values passed to an foreign call + /// back to a human-readable string. + /// The values passed to an foreign call will be a simple list of field elements, + /// thus requiring extra metadata to correctly decode this list of elements. + /// + /// The Noir compiler has an `AbiType` that handles encoding/decoding a list + /// of field elements to/from JSON. The type metadata attached in this method + /// is the serialized `AbiType` for the argument passed to the function. + /// The caller that is running a Noir program should then deserialize the `AbiType`, + /// and accurately decode the list of field elements passed to the foreign call. + fn append_abi_arg(&self, hir_argument: &HirExpression, arguments: &mut Vec) { match hir_argument { HirExpression::Ident(ident) => { let typ = self.interner.id_type(ident.id); - let typ = if is_format_call { - match typ { - Type::Array(_, element_type) => element_type.follow_bindings(), - _ => { - unreachable!("ICE: argument supplied to a format call must be an array") - } - } - } else { - typ.follow_bindings() - }; + let typ = typ.follow_bindings(); let abi_type = typ.as_abi_type(); let abi_as_string = diff --git a/noir_stdlib/src/lib.nr b/noir_stdlib/src/lib.nr index 7a690448cf5..9a06b915aaa 100644 --- a/noir_stdlib/src/lib.nr +++ b/noir_stdlib/src/lib.nr @@ -21,16 +21,9 @@ fn println(_input : T) {} #[oracle(println)] unconstrained fn println_oracle(_input: T) -> Field {} -#[oracle(println_format)] -unconstrained fn println_format_oracle(_msg: T, _args: [A; N]) -> Field {} - unconstrained fn println_new(input: T) { println_oracle(input); } -unconstrained fn println_format(msg: T, args: [A; N]) { - println_format_oracle(msg, args); -} - #[foreign(recursive_aggregation)] fn verify_proof(_verification_key : [Field], _proof : [Field], _public_inputs : [Field], _key_hash : Field, _input_aggregation_object : [Field]) -> [Field] {} From 13f6de6f5c19727b86590bbf1c7d6890672a7c17 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 19 Jul 2023 16:48:11 +0000 Subject: [PATCH 14/20] add comment about removing old println --- .../debug_logs/src/main.nr | 8 ++--- crates/noirc_frontend/src/graph/mod.rs | 10 ------- crates/noirc_frontend/src/hir/def_map/mod.rs | 29 ++++++++++++++++--- noir_stdlib/src/lib.nr | 3 +- 4 files changed, 31 insertions(+), 19 deletions(-) diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr index 7c4252bc71a..29386feb98c 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr @@ -2,13 +2,13 @@ use dep::std; fn main(x : Field, y : pub Field) { - std::println_new("*** println_new ***"); - std::println_new(x); - std::println_new([x, y]); + std::println("*** println ***"); + std::println(x); + std::println([x, y]); let s = myStruct { y: x, x: y }; let foo = fooStruct { my_struct: s, foo: 15 }; - std::println_new(foo); + std::println(foo); } struct myStruct { diff --git a/crates/noirc_frontend/src/graph/mod.rs b/crates/noirc_frontend/src/graph/mod.rs index 7a95ff324de..e6dd226997f 100644 --- a/crates/noirc_frontend/src/graph/mod.rs +++ b/crates/noirc_frontend/src/graph/mod.rs @@ -51,16 +51,6 @@ pub struct CrateGraph { arena: FxHashMap, } -impl CrateGraph { - pub fn is_last_crate(&self, crate_id: CrateId) -> bool { - match crate_id { - CrateId::Crate(crate_id) | CrateId::Stdlib(crate_id) => { - (self.arena.len() - 1) == crate_id - } - } - } -} - /// List of characters that are not allowed in a crate name /// For example, Hyphen(-) is disallowed as it is similar to underscore(_) /// and we do not want names that differ by a hyphen diff --git a/crates/noirc_frontend/src/hir/def_map/mod.rs b/crates/noirc_frontend/src/hir/def_map/mod.rs index 6afe4f8c234..7ab0faab995 100644 --- a/crates/noirc_frontend/src/hir/def_map/mod.rs +++ b/crates/noirc_frontend/src/hir/def_map/mod.rs @@ -80,21 +80,42 @@ impl CrateDefMap { let mut ast = parse_file(&mut context.file_manager, root_file_id, errors); // TODO(#1850): This check should be removed once we fully move over to the new SSA pass - // Compiling with the old SSA pass will lead to duplicate method definitions between + // There are some features that use the new SSA pass that also affect the stdlib. + // 1. Compiling with the old SSA pass will lead to duplicate method definitions between // the `slice` and `array` modules of the stdlib. + // 2. The `println` method is a builtin with the old SSA but is a normal function that calls + // an oracle in the new SSA. // // The last crate represents the stdlib crate. // After resolving the manifest of the local crate the stdlib is added to the manifest and propagated to all crates // thus being the last crate. - if !context.def_interner.experimental_ssa && context.crate_graph.is_last_crate(crate_id) { + if crate_id.is_stdlib() { let path_as_str = context .file_manager .path(root_file_id) .to_str() .expect("expected std path to be convertible to str"); assert_eq!(path_as_str, "std/lib"); - ast.module_decls - .retain(|ident| ident.0.contents != "slice" && ident.0.contents != "collections"); + if context.def_interner.experimental_ssa { + ast.functions.retain(|func| { + if func.def.name.0.contents.as_str() == "println" { + func.def.is_unconstrained + } else { + true + } + }); + } else { + ast.functions.retain(|func| { + if func.def.name.0.contents.as_str() == "println" { + !func.def.is_unconstrained + } else { + true + } + }); + ast.module_decls.retain(|ident| { + ident.0.contents != "slice" && ident.0.contents != "collections" + }); + } } // Allocate a default Module for the root, giving it a ModuleId diff --git a/noir_stdlib/src/lib.nr b/noir_stdlib/src/lib.nr index 9a06b915aaa..09e4323fbc7 100644 --- a/noir_stdlib/src/lib.nr +++ b/noir_stdlib/src/lib.nr @@ -15,13 +15,14 @@ mod unsafe; mod collections; mod compat; +// TODO(#1850): Remove this builtin to use the foreign call based println #[builtin(println)] fn println(_input : T) {} #[oracle(println)] unconstrained fn println_oracle(_input: T) -> Field {} -unconstrained fn println_new(input: T) { +unconstrained fn println(input: T) { println_oracle(input); } From 2c0a8ba84b16b856669eec13d5fb1d73a5d61811 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 19 Jul 2023 16:50:42 +0000 Subject: [PATCH 15/20] remove old comment --- crates/nargo/src/ops/foreign_calls.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/nargo/src/ops/foreign_calls.rs b/crates/nargo/src/ops/foreign_calls.rs index 5dacbe2965d..0f18de91195 100644 --- a/crates/nargo/src/ops/foreign_calls.rs +++ b/crates/nargo/src/ops/foreign_calls.rs @@ -66,8 +66,6 @@ impl ForeignCall { let (abi_type, input_values) = fetch_abi_type(foreign_call_inputs)?; // We must use a flat map here as each value in a struct will be in a separate input value - // let mut input_values_as_fields = - // input_values.iter().flat_map(|values| vecmap(values, |value| value.to_field())); let mut input_values_as_fields = input_values.iter().flat_map(|values| values.iter().map(|value| value.to_field())); let decoded_value = decode_value(&mut input_values_as_fields, &abi_type)?; From 20f060f7566f324a3eb474085423516098a7b90d Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 19 Jul 2023 19:22:53 +0000 Subject: [PATCH 16/20] have println return nothing --- crates/nargo/src/ops/foreign_calls.rs | 2 +- noir_stdlib/src/lib.nr | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/nargo/src/ops/foreign_calls.rs b/crates/nargo/src/ops/foreign_calls.rs index 0f18de91195..ea7f9be21b4 100644 --- a/crates/nargo/src/ops/foreign_calls.rs +++ b/crates/nargo/src/ops/foreign_calls.rs @@ -46,7 +46,7 @@ impl ForeignCall { match Self::lookup(foreign_call_name) { Some(ForeignCall::Println) => { Self::execute_println(&foreign_call.inputs)?; - Ok(foreign_call.inputs[0][0].into()) + Ok(ForeignCallResult { values: vec![] }) } Some(ForeignCall::Sequence) => { let sequence_length: u128 = foreign_call.inputs[0][0].to_field().to_u128(); diff --git a/noir_stdlib/src/lib.nr b/noir_stdlib/src/lib.nr index 09e4323fbc7..35844c5d005 100644 --- a/noir_stdlib/src/lib.nr +++ b/noir_stdlib/src/lib.nr @@ -20,7 +20,7 @@ mod compat; fn println(_input : T) {} #[oracle(println)] -unconstrained fn println_oracle(_input: T) -> Field {} +unconstrained fn println_oracle(_input: T) {} unconstrained fn println(input: T) { println_oracle(input); From b28e5987bf6dab6b3d43c72315008d0d6904996e Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 19 Jul 2023 20:31:12 +0100 Subject: [PATCH 17/20] Update crates/noirc_frontend/src/hir/def_map/mod.rs Co-authored-by: jfecher --- crates/noirc_frontend/src/hir/def_map/mod.rs | 24 +++++++------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/crates/noirc_frontend/src/hir/def_map/mod.rs b/crates/noirc_frontend/src/hir/def_map/mod.rs index 7ab0faab995..9e1d9bf3af0 100644 --- a/crates/noirc_frontend/src/hir/def_map/mod.rs +++ b/crates/noirc_frontend/src/hir/def_map/mod.rs @@ -96,22 +96,14 @@ impl CrateDefMap { .to_str() .expect("expected std path to be convertible to str"); assert_eq!(path_as_str, "std/lib"); - if context.def_interner.experimental_ssa { - ast.functions.retain(|func| { - if func.def.name.0.contents.as_str() == "println" { - func.def.is_unconstrained - } else { - true - } - }); - } else { - ast.functions.retain(|func| { - if func.def.name.0.contents.as_str() == "println" { - !func.def.is_unconstrained - } else { - true - } - }); + // There are 2 printlns in the stdlib. If we are using the experimental SSA, we want to keep + // only the unconstrained one. Otherwise we want to keep only the constrained one. + ast.functions.retain(|func| { + func.def.name.0.contents.as_str() != "println" + || func.def.is_unconstrained == context.def_interner.experimental_ssa + }); + + if !context.def_interner.experimental_ssa { ast.module_decls.retain(|ident| { ident.0.contents != "slice" && ident.0.contents != "collections" }); From 46daccca929a93cd78797adbdb4166d9020a7d75 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 20 Jul 2023 15:25:25 +0100 Subject: [PATCH 18/20] Update crates/noirc_frontend/src/hir/def_map/mod.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- crates/noirc_frontend/src/hir/def_map/mod.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/crates/noirc_frontend/src/hir/def_map/mod.rs b/crates/noirc_frontend/src/hir/def_map/mod.rs index 9e1d9bf3af0..8bb88abd1e9 100644 --- a/crates/noirc_frontend/src/hir/def_map/mod.rs +++ b/crates/noirc_frontend/src/hir/def_map/mod.rs @@ -86,9 +86,6 @@ impl CrateDefMap { // 2. The `println` method is a builtin with the old SSA but is a normal function that calls // an oracle in the new SSA. // - // The last crate represents the stdlib crate. - // After resolving the manifest of the local crate the stdlib is added to the manifest and propagated to all crates - // thus being the last crate. if crate_id.is_stdlib() { let path_as_str = context .file_manager From dece38511a3b99dcb995fef3c8e1e2c9379f2585 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Thu, 20 Jul 2023 14:52:54 +0000 Subject: [PATCH 19/20] add comment to append_abi_arg call --- crates/noirc_frontend/src/monomorphization/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index df9be34bc41..a3188eaa14a 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -777,6 +777,8 @@ impl<'interner> Monomorphizer<'interner> { if let ast::Expression::Ident(ident) = func.as_ref() { if let Definition::Oracle(name) = &ident.definition { if name.as_str() == "println" { + // Oracle calls are required to be wrapped in an unconstrained function + // Thus, the only argument to the `println` oracle is expected to always be an ident self.append_abi_arg(&hir_arguments[0], &mut arguments); } } From 269696f8f3b3c25cd54697a6b797087a10d0bbd7 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Thu, 20 Jul 2023 15:44:05 +0000 Subject: [PATCH 20/20] include println oracle comment in stdlib --- noir_stdlib/src/lib.nr | 2 ++ 1 file changed, 2 insertions(+) diff --git a/noir_stdlib/src/lib.nr b/noir_stdlib/src/lib.nr index 35844c5d005..80bf6e8bae5 100644 --- a/noir_stdlib/src/lib.nr +++ b/noir_stdlib/src/lib.nr @@ -19,6 +19,8 @@ mod compat; #[builtin(println)] fn println(_input : T) {} +// Oracle calls are required to be wrapped in an unconstrained function +// Thus, the only argument to the `println` oracle is expected to always be an ident #[oracle(println)] unconstrained fn println_oracle(_input: T) {}