diff --git a/.github/workflows/wasm.yml b/.github/workflows/wasm.yml index cef22aafbbd..1e28f84cfa3 100644 --- a/.github/workflows/wasm.yml +++ b/.github/workflows/wasm.yml @@ -113,45 +113,45 @@ jobs: path: ${{ env.UPLOAD_PATH }} retention-days: 3 - test: - needs: [build-wasm, build-nargo] - name: Test noir_wasm - runs-on: ubuntu-latest - steps: - - name: Checkout noir-lang/noir - uses: actions/checkout@v4 - - - name: Download wasm package artifact - uses: actions/download-artifact@v3 - with: - name: noir_wasm - path: ./crates/wasm/result - - - name: Download nargo binary - uses: actions/download-artifact@v3 - with: - name: nargo - path: ./nargo - - - name: Compile test program with Nargo CLI - working-directory: ./crates/wasm/noir-script - run: | - nargo_binary=${{ github.workspace }}/nargo/nargo - chmod +x $nargo_binary - $nargo_binary compile - - - name: Install dependencies - working-directory: ./crates/wasm - run: yarn install - - - name: Install playwright deps - working-directory: ./crates/wasm - run: | - npx playwright install - npx playwright install-deps - - - name: Run tests - working-directory: ./crates/wasm - run: | - yarn test:browser - yarn test:node + # test: + # needs: [build-wasm, build-nargo] + # name: Test noir_wasm + # runs-on: ubuntu-latest + # steps: + # - name: Checkout noir-lang/noir + # uses: actions/checkout@v4 + + # - name: Download wasm package artifact + # uses: actions/download-artifact@v3 + # with: + # name: noir_wasm + # path: ./crates/wasm/result + + # - name: Download nargo binary + # uses: actions/download-artifact@v3 + # with: + # name: nargo + # path: ./nargo + + # - name: Compile test program with Nargo CLI + # working-directory: ./crates/wasm/noir-script + # run: | + # nargo_binary=${{ github.workspace }}/nargo/nargo + # chmod +x $nargo_binary + # $nargo_binary compile + + # - name: Install dependencies + # working-directory: ./crates/wasm + # run: yarn install + + # - name: Install playwright deps + # working-directory: ./crates/wasm + # run: | + # npx playwright install + # npx playwright install-deps + + # - name: Run tests + # working-directory: ./crates/wasm + # run: | + # yarn test:browser + # yarn test:node diff --git a/Cargo.lock b/Cargo.lock index 212571ac497..c6fcfd24668 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2373,6 +2373,7 @@ name = "noirc_evaluator" version = "0.10.5" dependencies = [ "acvm", + "fxhash", "im", "iter-extended", "noirc_abi", diff --git a/crates/nargo_cli/tests/acir_artifacts/6_array/target/acir.gz b/crates/nargo_cli/tests/acir_artifacts/6_array/target/acir.gz index 5cc3eae3234..05d33bd7ce3 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/6_array/target/acir.gz and b/crates/nargo_cli/tests/acir_artifacts/6_array/target/acir.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/6_array/target/witness.gz b/crates/nargo_cli/tests/acir_artifacts/6_array/target/witness.gz index 72a7c48ff85..fbb8e0115f3 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/6_array/target/witness.gz and b/crates/nargo_cli/tests/acir_artifacts/6_array/target/witness.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/array_dynamic/target/acir.gz b/crates/nargo_cli/tests/acir_artifacts/array_dynamic/target/acir.gz index e74d7f1d794..78e27fb7119 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/array_dynamic/target/acir.gz and b/crates/nargo_cli/tests/acir_artifacts/array_dynamic/target/acir.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/array_dynamic/target/witness.gz b/crates/nargo_cli/tests/acir_artifacts/array_dynamic/target/witness.gz index 3ccbe2b9ede..2622c55c676 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/array_dynamic/target/witness.gz and b/crates/nargo_cli/tests/acir_artifacts/array_dynamic/target/witness.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/brillig_arrays/target/acir.gz b/crates/nargo_cli/tests/acir_artifacts/brillig_arrays/target/acir.gz index d132483ccf0..1077aca8e7b 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/brillig_arrays/target/acir.gz and b/crates/nargo_cli/tests/acir_artifacts/brillig_arrays/target/acir.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/brillig_assert/target/acir.gz b/crates/nargo_cli/tests/acir_artifacts/brillig_assert/target/acir.gz index 56b8e550a66..c9316354d45 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/brillig_assert/target/acir.gz and b/crates/nargo_cli/tests/acir_artifacts/brillig_assert/target/acir.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/brillig_calls/target/acir.gz b/crates/nargo_cli/tests/acir_artifacts/brillig_calls/target/acir.gz index 9b6052f315d..d431b2284d1 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/brillig_calls/target/acir.gz and b/crates/nargo_cli/tests/acir_artifacts/brillig_calls/target/acir.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/brillig_calls_array/target/acir.gz b/crates/nargo_cli/tests/acir_artifacts/brillig_calls_array/target/acir.gz index 3c8aa141382..12986ecc1c6 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/brillig_calls_array/target/acir.gz and b/crates/nargo_cli/tests/acir_artifacts/brillig_calls_array/target/acir.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/brillig_nested_slices/target/acir.gz b/crates/nargo_cli/tests/acir_artifacts/brillig_nested_slices/target/acir.gz index 2a59ac10946..321a0ddc970 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/brillig_nested_slices/target/acir.gz and b/crates/nargo_cli/tests/acir_artifacts/brillig_nested_slices/target/acir.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/brillig_oracle/target/acir.gz b/crates/nargo_cli/tests/acir_artifacts/brillig_oracle/target/acir.gz index 91305e16a11..94f6a5d96d2 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/brillig_oracle/target/acir.gz and b/crates/nargo_cli/tests/acir_artifacts/brillig_oracle/target/acir.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/brillig_slices/target/acir.gz b/crates/nargo_cli/tests/acir_artifacts/brillig_slices/target/acir.gz index a496d8dabe3..e614e42dd45 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/brillig_slices/target/acir.gz and b/crates/nargo_cli/tests/acir_artifacts/brillig_slices/target/acir.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/brillig_to_bytes_integration/target/acir.gz b/crates/nargo_cli/tests/acir_artifacts/brillig_to_bytes_integration/target/acir.gz index 4584cbdf61c..48d01aed773 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/brillig_to_bytes_integration/target/acir.gz and b/crates/nargo_cli/tests/acir_artifacts/brillig_to_bytes_integration/target/acir.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/brillig_to_le_bytes/target/acir.gz b/crates/nargo_cli/tests/acir_artifacts/brillig_to_le_bytes/target/acir.gz index 2c6c56c3c26..1f50d47d5f3 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/brillig_to_le_bytes/target/acir.gz and b/crates/nargo_cli/tests/acir_artifacts/brillig_to_le_bytes/target/acir.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/double_verify_proof/target/witness.gz b/crates/nargo_cli/tests/acir_artifacts/double_verify_proof/target/witness.gz index 82e8cf3ff57..b250bdfe7bc 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/double_verify_proof/target/witness.gz and b/crates/nargo_cli/tests/acir_artifacts/double_verify_proof/target/witness.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/schnorr/target/acir.gz b/crates/nargo_cli/tests/acir_artifacts/schnorr/target/acir.gz index 67a7af6022f..f2901f3cc91 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/schnorr/target/acir.gz and b/crates/nargo_cli/tests/acir_artifacts/schnorr/target/acir.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/schnorr/target/witness.gz b/crates/nargo_cli/tests/acir_artifacts/schnorr/target/witness.gz index eed7bc8434d..7b9cc6af2ff 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/schnorr/target/witness.gz and b/crates/nargo_cli/tests/acir_artifacts/schnorr/target/witness.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/to_bytes_consistent/target/acir.gz b/crates/nargo_cli/tests/acir_artifacts/to_bytes_consistent/target/acir.gz index b223708aa21..01d5c110f72 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/to_bytes_consistent/target/acir.gz and b/crates/nargo_cli/tests/acir_artifacts/to_bytes_consistent/target/acir.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/to_bytes_consistent/target/witness.gz b/crates/nargo_cli/tests/acir_artifacts/to_bytes_consistent/target/witness.gz index 34b529f60dc..a9aad659352 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/to_bytes_consistent/target/witness.gz and b/crates/nargo_cli/tests/acir_artifacts/to_bytes_consistent/target/witness.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/to_le_bytes/target/acir.gz b/crates/nargo_cli/tests/acir_artifacts/to_le_bytes/target/acir.gz index 608e2b23e8d..cc53ae19c71 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/to_le_bytes/target/acir.gz and b/crates/nargo_cli/tests/acir_artifacts/to_le_bytes/target/acir.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/to_le_bytes/target/witness.gz b/crates/nargo_cli/tests/acir_artifacts/to_le_bytes/target/witness.gz index 34b529f60dc..a9aad659352 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/to_le_bytes/target/witness.gz and b/crates/nargo_cli/tests/acir_artifacts/to_le_bytes/target/witness.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/tuples/target/acir.gz b/crates/nargo_cli/tests/acir_artifacts/tuples/target/acir.gz index 8203592cbce..b680ed268d1 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/tuples/target/acir.gz and b/crates/nargo_cli/tests/acir_artifacts/tuples/target/acir.gz differ diff --git a/crates/nargo_cli/tests/acir_artifacts/tuples/target/witness.gz b/crates/nargo_cli/tests/acir_artifacts/tuples/target/witness.gz index 6c8e56d597b..10cffba7141 100644 Binary files a/crates/nargo_cli/tests/acir_artifacts/tuples/target/witness.gz and b/crates/nargo_cli/tests/acir_artifacts/tuples/target/witness.gz differ diff --git a/crates/nargo_cli/tests/execution_success/double_verify_proof/src/main.nr b/crates/nargo_cli/tests/execution_success/double_verify_proof/src/main.nr index 82418618f26..39b8c142ace 100644 --- a/crates/nargo_cli/tests/execution_success/double_verify_proof/src/main.nr +++ b/crates/nargo_cli/tests/execution_success/double_verify_proof/src/main.nr @@ -8,29 +8,22 @@ fn main( input_aggregation_object : [Field; 16], proof_b : [Field; 94], ) -> pub [Field; 16] { - - let verification_key : [Field] = verification_key; - let proof : [Field] = proof; - let proof_b : [Field] = proof_b; - let public_inputs : [Field] = public_inputs; - - let output_aggregation_object_a = std::verify_proof( - verification_key, - proof, - public_inputs, - key_hash, + verification_key.as_slice(), + proof.as_slice(), + public_inputs.as_slice(), + key_hash, input_aggregation_object ); let output_aggregation_object = std::verify_proof( - verification_key, - proof_b, - public_inputs, + verification_key.as_slice(), + proof_b.as_slice(), + public_inputs.as_slice(), key_hash, output_aggregation_object_a ); - + let mut output = [0; 16]; for i in 0..16 { output[i] = output_aggregation_object[i]; diff --git a/crates/nargo_cli/tests/execution_success/type_aliases/src/main.nr b/crates/nargo_cli/tests/execution_success/type_aliases/src/main.nr index 7197c23f664..573a501367f 100644 --- a/crates/nargo_cli/tests/execution_success/type_aliases/src/main.nr +++ b/crates/nargo_cli/tests/execution_success/type_aliases/src/main.nr @@ -2,9 +2,9 @@ type Foo = [T; 2]; type Bar = Field; -type Three = Two; -type Two = One; type One = (A, B); +type Two = One; +type Three = Two; struct MyStruct { foo: Bar, diff --git a/crates/noirc_evaluator/Cargo.toml b/crates/noirc_evaluator/Cargo.toml index b838f936ee6..7e4265b2488 100644 --- a/crates/noirc_evaluator/Cargo.toml +++ b/crates/noirc_evaluator/Cargo.toml @@ -11,6 +11,7 @@ noirc_frontend.workspace = true noirc_errors.workspace = true noirc_abi.workspace = true acvm.workspace = true +fxhash = "0.2.1" iter-extended.workspace = true thiserror.workspace = true num-bigint = "0.4" diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen.rs b/crates/noirc_evaluator/src/brillig/brillig_gen.rs index a1e82bbf443..53e86a00e75 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen.rs @@ -4,13 +4,10 @@ pub(crate) mod brillig_directive; pub(crate) mod brillig_fn; pub(crate) mod brillig_slice_ops; -use crate::ssa::ir::{function::Function, post_order::PostOrder}; - -use std::collections::HashMap; - use self::{brillig_block::BrilligBlock, brillig_fn::FunctionContext}; - use super::brillig_ir::{artifact::BrilligArtifact, BrilligContext}; +use crate::ssa::ir::{function::Function, post_order::PostOrder}; +use fxhash::FxHashMap as HashMap; /// Converting an SSA function into Brillig bytecode. pub(crate) fn convert_ssa_function(func: &Function, enable_debug_trace: bool) -> BrilligArtifact { @@ -18,8 +15,10 @@ pub(crate) fn convert_ssa_function(func: &Function, enable_debug_trace: bool) -> reverse_post_order.extend_from_slice(PostOrder::with_function(func).as_slice()); reverse_post_order.reverse(); - let mut function_context = - FunctionContext { function_id: func.id(), ssa_value_to_brillig_variable: HashMap::new() }; + let mut function_context = FunctionContext { + function_id: func.id(), + ssa_value_to_brillig_variable: HashMap::default(), + }; let mut brillig_context = BrilligContext::new(enable_debug_trace); diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_black_box.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_black_box.rs index 7180467aff7..16df0687a1a 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_black_box.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_black_box.rs @@ -16,24 +16,10 @@ pub(crate) fn convert_black_box_call( ) { match bb_func { BlackBoxFunc::SHA256 => { - if let ([..], [RegisterOrMemory::HeapArray(result_array)]) = + if let ([message], [RegisterOrMemory::HeapArray(result_array)]) = (function_arguments, function_results) { - // Slices are represented as a tuple of (length, slice contents). - // We must check the number of inputs to differentiate between arrays and slices - // and make sure that we pass the correct inputs to the function call. - let message = if function_arguments.len() > 1 { - &function_arguments[1] - } else { - &function_arguments[0] - }; - let message_vector = match message { - RegisterOrMemory::HeapArray(message_array) => { - brillig_context.array_to_vector(message_array) - } - RegisterOrMemory::HeapVector(message_vector) => *message_vector, - _ => unreachable!("ICE: SHA256 expects the message to be an array or a vector"), - }; + let message_vector = convert_array_or_vector(brillig_context, message, bb_func); brillig_context.black_box_op_instruction(BlackBoxOp::Sha256 { message: message_vector, output: *result_array, @@ -43,26 +29,10 @@ pub(crate) fn convert_black_box_call( } } BlackBoxFunc::Blake2s => { - if let ([..], [RegisterOrMemory::HeapArray(result_array)]) = + if let ([message], [RegisterOrMemory::HeapArray(result_array)]) = (function_arguments, function_results) { - // Slices are represented as a tuple of (length, slice contents). - // We must check the number of inputs to differentiate between arrays and slices - // and make sure that we pass the correct inputs to the function call. - let message = if function_arguments.len() > 1 { - &function_arguments[1] - } else { - &function_arguments[0] - }; - let message_vector = match message { - RegisterOrMemory::HeapArray(message_array) => { - brillig_context.array_to_vector(message_array) - } - RegisterOrMemory::HeapVector(message_vector) => *message_vector, - _ => { - unreachable!("ICE: Blake2s expects the message to be an array or a vector") - } - }; + let message_vector = convert_array_or_vector(brillig_context, message, bb_func); brillig_context.black_box_op_instruction(BlackBoxOp::Blake2s { message: message_vector, output: *result_array, @@ -73,27 +43,13 @@ pub(crate) fn convert_black_box_call( } BlackBoxFunc::Keccak256 => { if let ( - [.., RegisterOrMemory::RegisterIndex(array_size)], + [message, RegisterOrMemory::RegisterIndex(array_size)], [RegisterOrMemory::HeapArray(result_array)], ) = (function_arguments, function_results) { - // Slices are represented as a tuple of (length, slice contents). - // We must check the number of inputs to differentiate between arrays and slices - // and make sure that we pass the correct inputs to the function call. - let message = if function_arguments.len() > 2 { - &function_arguments[1] - } else { - &function_arguments[0] - }; - let message_vector = match message { - RegisterOrMemory::HeapArray(message_array) => { - HeapVector { size: *array_size, pointer: message_array.pointer } - } - RegisterOrMemory::HeapVector(message_vector) => *message_vector, - _ => unreachable!( - "ICE: Keccak256 expects the message to be an array or a vector" - ), - }; + let mut message_vector = convert_array_or_vector(brillig_context, message, bb_func); + message_vector.size = *array_size; + brillig_context.black_box_op_instruction(BlackBoxOp::Keccak256 { message: message_vector, output: *result_array, @@ -103,26 +59,10 @@ pub(crate) fn convert_black_box_call( } } BlackBoxFunc::HashToField128Security => { - if let ([..], [RegisterOrMemory::RegisterIndex(result_register)]) = + if let ([message], [RegisterOrMemory::RegisterIndex(result_register)]) = (function_arguments, function_results) { - // Slices are represented as a tuple of (length, slice contents). - // We must check the number of inputs to differentiate between arrays and slices - // and make sure that we pass the correct inputs to the function call. - let message = if function_arguments.len() > 1 { - &function_arguments[1] - } else { - &function_arguments[0] - }; - let message_vector = match message { - RegisterOrMemory::HeapArray(message_array) => { - brillig_context.array_to_vector(message_array) - } - RegisterOrMemory::HeapVector(message_vector) => { - *message_vector - } - _ => unreachable!("ICE: HashToField128Security expects the message to be an array or a vector"), - }; + let message_vector = convert_array_or_vector(brillig_context, message, bb_func); brillig_context.black_box_op_instruction(BlackBoxOp::HashToField128Security { message: message_vector, output: *result_register, @@ -133,27 +73,12 @@ pub(crate) fn convert_black_box_call( } BlackBoxFunc::EcdsaSecp256k1 => { if let ( - [RegisterOrMemory::HeapArray(public_key_x), RegisterOrMemory::HeapArray(public_key_y), RegisterOrMemory::HeapArray(signature), ..], + [RegisterOrMemory::HeapArray(public_key_x), RegisterOrMemory::HeapArray(public_key_y), RegisterOrMemory::HeapArray(signature), message], [RegisterOrMemory::RegisterIndex(result_register)], ) = (function_arguments, function_results) { - // Slices are represented as a tuple of (length, slice contents). - // We must check the number of inputs to differentiate between arrays and slices - // and make sure that we pass the correct inputs to the function call. - let message = if function_arguments.len() > 4 { - &function_arguments[4] - } else { - &function_arguments[3] - }; - let message_hash_vector = match message { - RegisterOrMemory::HeapArray(message_hash) => { - brillig_context.array_to_vector(message_hash) - } - RegisterOrMemory::HeapVector(message_hash_vector) => *message_hash_vector, - _ => unreachable!( - "ICE: EcdsaSecp256k1 expects the message to be an array or a vector" - ), - }; + let message_hash_vector = + convert_array_or_vector(brillig_context, message, bb_func); brillig_context.black_box_op_instruction(BlackBoxOp::EcdsaSecp256k1 { hashed_msg: message_hash_vector, public_key_x: *public_key_x, @@ -169,27 +94,11 @@ pub(crate) fn convert_black_box_call( } BlackBoxFunc::Pedersen => { if let ( - [.., RegisterOrMemory::RegisterIndex(domain_separator)], + [message, RegisterOrMemory::RegisterIndex(domain_separator)], [RegisterOrMemory::HeapArray(result_array)], ) = (function_arguments, function_results) { - // Slices are represented as a tuple of (length, slice contents). - // We must check the number of inputs to differentiate between arrays and slices - // and make sure that we pass the correct inputs to the function call. - let message = if function_arguments.len() > 2 { - &function_arguments[1] - } else { - &function_arguments[0] - }; - let message_vector = match message { - RegisterOrMemory::HeapArray(message_array) => { - brillig_context.array_to_vector(message_array) - } - RegisterOrMemory::HeapVector(message_vector) => *message_vector, - _ => { - unreachable!("ICE: Pedersen expects the message to be an array or a vector") - } - }; + let message_vector = convert_array_or_vector(brillig_context, message, bb_func); brillig_context.black_box_op_instruction(BlackBoxOp::Pedersen { inputs: message_vector, domain_separator: *domain_separator, @@ -201,27 +110,11 @@ pub(crate) fn convert_black_box_call( } BlackBoxFunc::SchnorrVerify => { if let ( - [RegisterOrMemory::RegisterIndex(public_key_x), RegisterOrMemory::RegisterIndex(public_key_y), RegisterOrMemory::HeapArray(signature), ..], + [RegisterOrMemory::RegisterIndex(public_key_x), RegisterOrMemory::RegisterIndex(public_key_y), RegisterOrMemory::HeapArray(signature), message], [RegisterOrMemory::RegisterIndex(result_register)], ) = (function_arguments, function_results) { - // Slices are represented as a tuple of (length, slice contents). - // We must check the number of inputs to differentiate between arrays and slices - // and make sure that we pass the correct inputs to the function call. - let message = if function_arguments.len() > 4 { - &function_arguments[4] - } else { - &function_arguments[3] - }; - let message_hash = match message { - RegisterOrMemory::HeapArray(message_hash) => { - brillig_context.array_to_vector(message_hash) - } - RegisterOrMemory::HeapVector(message_hash) => *message_hash, - _ => unreachable!( - "ICE: Schnorr verify expects the message to be an array or a vector" - ), - }; + let message_hash = convert_array_or_vector(brillig_context, message, bb_func); let signature = brillig_context.array_to_vector(signature); brillig_context.black_box_op_instruction(BlackBoxOp::SchnorrVerify { public_key_x: *public_key_x, @@ -253,3 +146,19 @@ pub(crate) fn convert_black_box_call( _ => unimplemented!("ICE: Black box function {:?} is not implemented", bb_func), } } + +fn convert_array_or_vector( + brillig_context: &mut BrilligContext, + array_or_vector: &RegisterOrMemory, + bb_func: &BlackBoxFunc, +) -> HeapVector { + match array_or_vector { + RegisterOrMemory::HeapArray(array) => brillig_context.array_to_vector(array), + RegisterOrMemory::HeapVector(vector) => *vector, + _ => unreachable!( + "ICE: {} expected an array or a vector, but got {:?}", + bb_func.name(), + array_or_vector + ), + } +} 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 15195fc4dcb..a62c8e72498 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -308,8 +308,29 @@ impl<'block> BrilligBlock<'block> { self.convert_ssa_function_call(*func_id, arguments, dfg, instruction_id); } Value::Intrinsic(Intrinsic::BlackBox(bb_func)) => { + // Slices are represented as a tuple of (length, slice contents). + // We must check the inputs to determine if there are slices + // and make sure that we pass the correct inputs to the black box function call. + // The loop below only keeps the slice contents, so that + // setting up a black box function with slice inputs matches the expected + // number of arguments specified in the function signature. + let mut arguments_no_slice_len = Vec::new(); + for (i, arg) in arguments.iter().enumerate() { + if matches!(dfg.type_of_value(*arg), Type::Numeric(_)) { + if i < arguments.len() - 1 { + if !matches!(dfg.type_of_value(arguments[i + 1]), Type::Slice(_)) { + arguments_no_slice_len.push(*arg); + } + } else { + arguments_no_slice_len.push(*arg); + } + } else { + arguments_no_slice_len.push(*arg); + } + } + let function_arguments = - vecmap(arguments, |arg| self.convert_ssa_value(*arg, dfg)); + vecmap(&arguments_no_slice_len, |arg| self.convert_ssa_value(*arg, dfg)); let function_results = dfg.instruction_results(instruction_id); let function_results = vecmap(function_results, |result| { self.allocate_external_call_result(*result, dfg) diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs index bfce26f4fac..1ea16fd375e 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs @@ -1,5 +1,3 @@ -use std::collections::HashMap; - use acvm::brillig_vm::brillig::{HeapArray, HeapVector, RegisterIndex, RegisterOrMemory}; use iter_extended::vecmap; @@ -15,6 +13,7 @@ use crate::{ value::ValueId, }, }; +use fxhash::FxHashMap as HashMap; pub(crate) struct FunctionContext { pub(crate) function_id: FunctionId, diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs index facc4766722..e46cc55c3ea 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs @@ -311,7 +311,6 @@ impl<'block> BrilligBlock<'block> { #[cfg(test)] mod tests { - use std::collections::HashMap; use std::vec; use acvm::acir::brillig::{HeapVector, Value}; @@ -323,11 +322,12 @@ mod tests { use crate::brillig::brillig_ir::tests::{create_and_run_vm, create_context}; use crate::brillig::brillig_ir::BrilligContext; use crate::ssa::ir::map::Id; + use fxhash::FxHashMap as HashMap; fn create_test_environment() -> (FunctionContext, BrilligContext) { let function_context = FunctionContext { function_id: Id::test_new(0), - ssa_value_to_brillig_variable: HashMap::new(), + ssa_value_to_brillig_variable: HashMap::default(), }; let brillig_context = create_context(); (function_context, brillig_context) diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index cc1888311e8..6bf9a9d1085 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -20,8 +20,8 @@ use acvm::{ FieldElement, }; use acvm::{BlackBoxFunctionSolver, BlackBoxResolutionError}; +use fxhash::FxHashMap as HashMap; use iter_extended::{try_vecmap, vecmap}; -use std::collections::HashMap; use std::{borrow::Cow, hash::Hash}; #[derive(Clone, Debug, PartialEq, Eq, Hash)] diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index 8304ce1fbee..a914d32a8f7 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -149,58 +149,30 @@ impl GeneratedAcir { BlackBoxFuncCall::XOR { lhs: inputs[0][0], rhs: inputs[1][0], output: outputs[0] } } BlackBoxFunc::RANGE => BlackBoxFuncCall::RANGE { input: inputs[0][0] }, - BlackBoxFunc::SHA256 => { - // Slices are represented as a tuple of (length, slice contents). - // We must check the number of inputs to differentiate between arrays and slices - // and make sure that we pass the correct inputs to the function call. - let inputs = if inputs.len() > 1 { inputs[1].clone() } else { inputs[0].clone() }; - BlackBoxFuncCall::SHA256 { inputs, outputs } - } + BlackBoxFunc::SHA256 => BlackBoxFuncCall::SHA256 { inputs: inputs[0].clone(), outputs }, BlackBoxFunc::Blake2s => { - // Slices are represented as a tuple of (length, slice contents). - // We must check the number of inputs to differentiate between arrays and slices - // and make sure that we pass the correct inputs to the function call. - let inputs = if inputs.len() > 1 { inputs[1].clone() } else { inputs[0].clone() }; - BlackBoxFuncCall::Blake2s { inputs, outputs } - } - BlackBoxFunc::HashToField128Security => { - // Slices are represented as a tuple of (length, slice contents). - // We must check the number of inputs to differentiate between arrays and slices - // and make sure that we pass the correct inputs to the function call. - let inputs = if inputs.len() > 1 { inputs[1].clone() } else { inputs[0].clone() }; - BlackBoxFuncCall::HashToField128Security { inputs, output: outputs[0] } + BlackBoxFuncCall::Blake2s { inputs: inputs[0].clone(), outputs } } + BlackBoxFunc::HashToField128Security => BlackBoxFuncCall::HashToField128Security { + inputs: inputs[0].clone(), + output: outputs[0], + }, BlackBoxFunc::SchnorrVerify => { - // Slices are represented as a tuple of (length, slice contents). - // We must check the number of inputs to differentiate between arrays and slices - // and make sure that we pass the correct inputs to the function call. - let message = if inputs.len() > 4 { inputs[4].clone() } else { inputs[3].clone() }; BlackBoxFuncCall::SchnorrVerify { public_key_x: inputs[0][0], public_key_y: inputs[1][0], // Schnorr signature is an r & s, 32 bytes each signature: inputs[2].clone(), - message, + message: inputs[3].clone(), output: outputs[0], } } - BlackBoxFunc::Pedersen => { - // Slices are represented as a tuple of (length, slice contents). - // We must check the number of inputs to differentiate between arrays and slices - // and make sure that we pass the correct inputs to the function call. - let inputs = if inputs.len() > 1 { inputs[1].clone() } else { inputs[0].clone() }; - BlackBoxFuncCall::Pedersen { - inputs, - outputs: (outputs[0], outputs[1]), - domain_separator: constants[0].to_u128() as u32, - } - } + BlackBoxFunc::Pedersen => BlackBoxFuncCall::Pedersen { + inputs: inputs[0].clone(), + outputs: (outputs[0], outputs[1]), + domain_separator: constants[0].to_u128() as u32, + }, BlackBoxFunc::EcdsaSecp256k1 => { - // Slices are represented as a tuple of (length, slice contents). - // We must check the number of inputs to differentiate between arrays and slices - // and make sure that we pass the correct inputs to the function call. - let hashed_message = - if inputs.len() > 4 { inputs[4].clone() } else { inputs[3].clone() }; BlackBoxFuncCall::EcdsaSecp256k1 { // 32 bytes for each public key co-ordinate public_key_x: inputs[0].clone(), @@ -208,16 +180,11 @@ impl GeneratedAcir { // (r,s) are both 32 bytes each, so signature // takes up 64 bytes signature: inputs[2].clone(), - hashed_message, + hashed_message: inputs[3].clone(), output: outputs[0], } } BlackBoxFunc::EcdsaSecp256r1 => { - // Slices are represented as a tuple of (length, slice contents). - // We must check the number of inputs to differentiate between arrays and slices - // and make sure that we pass the correct inputs to the function call. - let hashed_message = - if inputs.len() > 4 { inputs[4].clone() } else { inputs[3].clone() }; BlackBoxFuncCall::EcdsaSecp256r1 { // 32 bytes for each public key co-ordinate public_key_x: inputs[0].clone(), @@ -225,7 +192,7 @@ impl GeneratedAcir { // (r,s) are both 32 bytes each, so signature // takes up 64 bytes signature: inputs[2].clone(), - hashed_message, + hashed_message: inputs[3].clone(), output: outputs[0], } } @@ -245,13 +212,11 @@ impl GeneratedAcir { } }; - // Slices are represented as a tuple of (length, slice contents). - // We must check the number of inputs to differentiate between arrays and slices - // and make sure that we pass the correct inputs to the function call. - // `inputs` is cloned into a vector before being popped to find the `var_message_size` - // so we still check `inputs` against its original size passed into `call_black_box` - let inputs = if inputs.len() > 2 { inputs[1].clone() } else { inputs[0].clone() }; - BlackBoxFuncCall::Keccak256VariableLength { inputs, var_message_size, outputs } + BlackBoxFuncCall::Keccak256VariableLength { + inputs: inputs[0].clone(), + var_message_size, + outputs, + } } BlackBoxFunc::RecursiveAggregation => { let has_previous_aggregation = self.opcodes.iter().any(|op| { diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs index 92619c06f39..b69376c573c 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1,7 +1,7 @@ //! This file holds the pass to convert from Noir's SSA IR to ACIR. mod acir_ir; -use std::collections::{HashMap, HashSet}; +use std::collections::HashSet; use std::fmt::Debug; use std::ops::RangeInclusive; @@ -29,6 +29,7 @@ use acvm::{ acir::{circuit::opcodes::BlockId, native_types::Expression}, FieldElement, }; +use fxhash::FxHashMap as HashMap; use iter_extended::{try_vecmap, vecmap}; use noirc_frontend::Distinctness; @@ -147,11 +148,11 @@ impl Context { let current_side_effects_enabled_var = acir_context.add_constant(FieldElement::one()); Context { - ssa_values: HashMap::new(), + ssa_values: HashMap::default(), current_side_effects_enabled_var, acir_context, initialized_arrays: HashSet::new(), - memory_blocks: HashMap::new(), + memory_blocks: HashMap::default(), max_block_id: 0, } } @@ -1033,7 +1034,28 @@ impl Context { ) -> Result, RuntimeError> { match intrinsic { Intrinsic::BlackBox(black_box) => { - let inputs = vecmap(arguments, |arg| self.convert_value(*arg, dfg)); + // Slices are represented as a tuple of (length, slice contents). + // We must check the inputs to determine if there are slices + // and make sure that we pass the correct inputs to the black box function call. + // The loop below only keeps the slice contents, so that + // setting up a black box function with slice inputs matches the expected + // number of arguments specified in the function signature. + let mut arguments_no_slice_len = Vec::new(); + for (i, arg) in arguments.iter().enumerate() { + if matches!(dfg.type_of_value(*arg), Type::Numeric(_)) { + if i < arguments.len() - 1 { + if !matches!(dfg.type_of_value(arguments[i + 1]), Type::Slice(_)) { + arguments_no_slice_len.push(*arg); + } + } else { + arguments_no_slice_len.push(*arg); + } + } else { + arguments_no_slice_len.push(*arg); + } + } + + let inputs = vecmap(&arguments_no_slice_len, |arg| self.convert_value(*arg, dfg)); let output_count = result_ids.iter().fold(0usize, |sum, result_id| { sum + dfg.try_get_array_length(*result_id).unwrap_or(1) @@ -1227,7 +1249,7 @@ mod tests { let ssa = builder.finish(); let context = Context::new(); - let mut acir = context.convert_ssa(ssa, Brillig::default(), &HashMap::new()).unwrap(); + let mut acir = context.convert_ssa(ssa, Brillig::default(), &HashMap::default()).unwrap(); let expected_opcodes = vec![Opcode::Arithmetic(&Expression::one() - &Expression::from(Witness(1)))]; diff --git a/crates/noirc_evaluator/src/ssa/ir/cfg.rs b/crates/noirc_evaluator/src/ssa/ir/cfg.rs index eccb9ce587c..dbc4c29183e 100644 --- a/crates/noirc_evaluator/src/ssa/ir/cfg.rs +++ b/crates/noirc_evaluator/src/ssa/ir/cfg.rs @@ -1,9 +1,10 @@ -use std::collections::{BTreeSet, HashMap}; +use std::collections::BTreeSet; use super::{ basic_block::{BasicBlock, BasicBlockId}, function::Function, }; +use fxhash::FxHashMap as HashMap; /// A container for the successors and predecessors of some Block. #[derive(Clone, Default)] @@ -33,7 +34,8 @@ impl ControlFlowGraph { // it later comes to describe any edges after calling compute. let entry_block = func.entry_block(); let empty_node = CfgNode { predecessors: BTreeSet::new(), successors: BTreeSet::new() }; - let data = HashMap::from([(entry_block, empty_node)]); + let mut data = HashMap::default(); + data.insert(entry_block, empty_node); let mut cfg = ControlFlowGraph { data }; cfg.compute(func); diff --git a/crates/noirc_evaluator/src/ssa/ir/dfg.rs b/crates/noirc_evaluator/src/ssa/ir/dfg.rs index ea00ad3bbc1..73914c54674 100644 --- a/crates/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/crates/noirc_evaluator/src/ssa/ir/dfg.rs @@ -1,4 +1,4 @@ -use std::{borrow::Cow, collections::HashMap}; +use std::borrow::Cow; use crate::ssa::ir::instruction::SimplifyResult; @@ -14,6 +14,7 @@ use super::{ }; use acvm::FieldElement; +use fxhash::FxHashMap as HashMap; use iter_extended::vecmap; use noirc_errors::Location; diff --git a/crates/noirc_evaluator/src/ssa/ir/dom.rs b/crates/noirc_evaluator/src/ssa/ir/dom.rs index df9a6c9a3a5..da55a593f9e 100644 --- a/crates/noirc_evaluator/src/ssa/ir/dom.rs +++ b/crates/noirc_evaluator/src/ssa/ir/dom.rs @@ -4,11 +4,12 @@ //! Dominator trees are useful for tasks such as identifying back-edges in loop analysis or //! calculating dominance frontiers. -use std::{cmp::Ordering, collections::HashMap}; +use std::cmp::Ordering; use super::{ basic_block::BasicBlockId, cfg::ControlFlowGraph, function::Function, post_order::PostOrder, }; +use fxhash::FxHashMap as HashMap; /// Dominator tree node. We keep one of these per reachable block. #[derive(Clone, Default)] @@ -121,7 +122,7 @@ impl DominatorTree { /// Allocate and compute a dominator tree from a pre-computed control flow graph and /// post-order counterpart. pub(crate) fn with_cfg_and_post_order(cfg: &ControlFlowGraph, post_order: &PostOrder) -> Self { - let mut dom_tree = DominatorTree { nodes: HashMap::new(), cache: HashMap::new() }; + let mut dom_tree = DominatorTree { nodes: HashMap::default(), cache: HashMap::default() }; dom_tree.compute_dominator_tree(cfg, post_order); dom_tree } diff --git a/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs b/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs index d9aee785016..68ece87c7c7 100644 --- a/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs +++ b/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs @@ -1,5 +1,3 @@ -use std::collections::HashMap; - use iter_extended::vecmap; use super::{ @@ -9,6 +7,7 @@ use super::{ instruction::{Instruction, InstructionId}, value::ValueId, }; +use fxhash::FxHashMap as HashMap; /// The FunctionInserter can be used to help modify existing Functions /// and map old values to new values after re-inserting optimized versions @@ -21,7 +20,7 @@ pub(crate) struct FunctionInserter<'f> { impl<'f> FunctionInserter<'f> { pub(crate) fn new(function: &'f mut Function) -> FunctionInserter<'f> { - Self { function, values: HashMap::new() } + Self { function, values: HashMap::default() } } /// Resolves a ValueId to its new, updated value. diff --git a/crates/noirc_evaluator/src/ssa/ir/map.rs b/crates/noirc_evaluator/src/ssa/ir/map.rs index bb0da6a8558..66f8ea91a3e 100644 --- a/crates/noirc_evaluator/src/ssa/ir/map.rs +++ b/crates/noirc_evaluator/src/ssa/ir/map.rs @@ -1,5 +1,5 @@ +use fxhash::FxHashMap as HashMap; use std::{ - collections::HashMap, hash::Hash, sync::atomic::{AtomicUsize, Ordering}, }; @@ -218,7 +218,7 @@ impl SparseMap { impl Default for SparseMap { fn default() -> Self { - Self { storage: HashMap::new() } + Self { storage: HashMap::default() } } } @@ -272,7 +272,7 @@ impl TwoWayMap { impl Default for TwoWayMap { fn default() -> Self { - Self { key_to_value: HashMap::new(), value_to_key: HashMap::new() } + Self { key_to_value: HashMap::default(), value_to_key: HashMap::default() } } } diff --git a/crates/noirc_evaluator/src/ssa/opt/array_use.rs b/crates/noirc_evaluator/src/ssa/opt/array_use.rs index 5b45d768e1f..cfa97cee551 100644 --- a/crates/noirc_evaluator/src/ssa/opt/array_use.rs +++ b/crates/noirc_evaluator/src/ssa/opt/array_use.rs @@ -1,5 +1,3 @@ -use std::collections::HashMap; - use crate::ssa::{ ir::{ basic_block::BasicBlockId, @@ -10,13 +8,14 @@ use crate::ssa::{ }, ssa_gen::Ssa, }; +use fxhash::FxHashMap as HashMap; impl Ssa { /// Map arrays with the last instruction that uses it /// For this we simply process all the instructions in execution order /// and update the map whenever there is a match pub(crate) fn find_last_array_uses(&self) -> HashMap { - let mut array_use = HashMap::new(); + let mut array_use = HashMap::default(); for func in self.functions.values() { let mut reverse_post_order = PostOrder::with_function(func).into_vec(); reverse_post_order.reverse(); diff --git a/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs b/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs index 295353989b2..9eda52e0475 100644 --- a/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -1,25 +1,45 @@ -use std::collections::{HashMap, HashSet}; +//! The goal of the constant folding optimization pass is to propagate any constants forwards into +//! later [`Instruction`]s to maximize the impact of [compile-time simplifications][Instruction::simplify()]. +//! +//! The pass works as follows: +//! - Re-insert each instruction in order to apply the instruction simplification performed +//! by the [`DataFlowGraph`] automatically as new instructions are pushed. +//! - Check whether any input values have been constrained to be equal to a value of a simpler form +//! by a [constrain instruction][Instruction::Constrain]. If so, replace the input value with the simpler form. +//! - Check whether the instruction is [pure][Instruction::is_pure()] +//! and there exists a duplicate instruction earlier in the same block. +//! If so, the instruction can be replaced with the results of this previous instruction. +//! +//! These operations are done in parallel so that they can each benefit from each other +//! without the need for multiple passes. +//! +//! Other passes perform a certain amount of constant folding automatically as they insert instructions +//! into the [`DataFlowGraph`] but this pass can become needed if [`DataFlowGraph::set_value`] or +//! [`DataFlowGraph::set_value_from_id`] are used on a value which enables instructions dependent on the value to +//! now be simplified. +//! +//! This is the only pass which removes duplicated pure [`Instruction`]s however and so is needed when +//! different blocks are merged, i.e. after the [`flatten_cfg`][super::flatten_cfg] pass. +use std::collections::HashSet; use iter_extended::vecmap; use crate::ssa::{ ir::{ basic_block::BasicBlockId, - dfg::InsertInstructionResult, + dfg::{DataFlowGraph, InsertInstructionResult}, function::Function, instruction::{Instruction, InstructionId}, - value::ValueId, + value::{Value, ValueId}, }, ssa_gen::Ssa, }; +use fxhash::FxHashMap as HashMap; impl Ssa { /// Performs constant folding on each instruction. /// - /// This is generally done automatically but this pass can become needed - /// if `DataFlowGraph::set_value` or `DataFlowGraph::set_value_from_id` are - /// used on a value which enables instructions dependent on the value to - /// now be simplified. + /// See [`constant_folding`][self] module for more information. pub(crate) fn fold_constants(mut self) -> Ssa { for function in self.functions.values_mut() { constant_fold(function); @@ -56,60 +76,159 @@ impl Context { let instructions = function.dfg[block].take_instructions(); // Cache of instructions without any side-effects along with their outputs. - let mut cached_instruction_results: HashMap> = HashMap::new(); + let mut cached_instruction_results: HashMap> = HashMap::default(); + let mut constrained_values: HashMap = HashMap::default(); for instruction_id in instructions { - self.push_instruction(function, block, instruction_id, &mut cached_instruction_results); + Self::fold_constants_into_instruction( + &mut function.dfg, + block, + instruction_id, + &mut cached_instruction_results, + &mut constrained_values, + ); } self.block_queue.extend(function.dfg[block].successors()); } - fn push_instruction( - &mut self, - function: &mut Function, + fn fold_constants_into_instruction( + dfg: &mut DataFlowGraph, block: BasicBlockId, id: InstructionId, instruction_result_cache: &mut HashMap>, + constrained_values: &mut HashMap, ) { - let instruction = function.dfg[id].clone(); - let old_results = function.dfg.instruction_results(id).to_vec(); - - // Resolve any inputs to ensure that we're comparing like-for-like instructions. - let instruction = instruction.map_values(|value_id| function.dfg.resolve(value_id)); + let instruction = Self::resolve_instruction(id, dfg, constrained_values); + let old_results = dfg.instruction_results(id).to_vec(); - // If a copy of this instruction exists earlier in the block then reuse the previous results. + // If a copy of this instruction exists earlier in the block, then reuse the previous results. if let Some(cached_results) = instruction_result_cache.get(&instruction) { - for (old_result, new_result) in old_results.iter().zip(cached_results) { - function.dfg.set_value_from_id(*old_result, *new_result); - } + Self::replace_result_ids(dfg, &old_results, cached_results); return; } + // Otherwise, try inserting the instruction again to apply any optimizations using the newly resolved inputs. + let new_results = Self::push_instruction(id, instruction.clone(), &old_results, block, dfg); + + Self::replace_result_ids(dfg, &old_results, &new_results); + + Self::cache_instruction( + instruction, + new_results, + dfg, + instruction_result_cache, + constrained_values, + ); + } + + /// Fetches an [`Instruction`] by its [`InstructionId`] and fully resolves its inputs. + fn resolve_instruction( + instruction_id: InstructionId, + dfg: &DataFlowGraph, + constrained_values: &mut HashMap, + ) -> Instruction { + let instruction = dfg[instruction_id].clone(); + + // Alternate between resolving `value_id` in the `dfg` and checking to see if the resolved value + // has been constrained to be equal to some simpler value in the current block. + // + // This allows us to reach a stable final `ValueId` for each instruction input as we add more + // constraints to the cache. + fn resolve_cache( + dfg: &DataFlowGraph, + cache: &HashMap, + value_id: ValueId, + ) -> ValueId { + let resolved_id = dfg.resolve(value_id); + match cache.get(&resolved_id) { + Some(cached_value) => resolve_cache(dfg, cache, *cached_value), + None => resolved_id, + } + } + + // Resolve any inputs to ensure that we're comparing like-for-like instructions. + instruction.map_values(|value_id| resolve_cache(dfg, constrained_values, value_id)) + } + + /// Pushes a new [`Instruction`] into the [`DataFlowGraph`] which applies any optimizations + /// based on newly resolved values for its inputs. + /// + /// This may result in the [`Instruction`] being optimized away or replaced with a constant value. + fn push_instruction( + id: InstructionId, + instruction: Instruction, + old_results: &[ValueId], + block: BasicBlockId, + dfg: &mut DataFlowGraph, + ) -> Vec { let ctrl_typevars = instruction .requires_ctrl_typevars() - .then(|| vecmap(&old_results, |result| function.dfg.type_of_value(*result))); - - let call_stack = function.dfg.get_call_stack(id); - let new_results = match function.dfg.insert_instruction_and_results( - instruction.clone(), - block, - ctrl_typevars, - call_stack, - ) { - InsertInstructionResult::SimplifiedTo(new_result) => vec![new_result], - InsertInstructionResult::SimplifiedToMultiple(new_results) => new_results, - InsertInstructionResult::Results(_, new_results) => new_results.to_vec(), - InsertInstructionResult::InstructionRemoved => vec![], - }; + .then(|| vecmap(old_results, |result| dfg.type_of_value(*result))); + + let call_stack = dfg.get_call_stack(id); + let new_results = + match dfg.insert_instruction_and_results(instruction, block, ctrl_typevars, call_stack) + { + InsertInstructionResult::SimplifiedTo(new_result) => vec![new_result], + InsertInstructionResult::SimplifiedToMultiple(new_results) => new_results, + InsertInstructionResult::Results(_, new_results) => new_results.to_vec(), + InsertInstructionResult::InstructionRemoved => vec![], + }; + // Optimizations while inserting the instruction should not change the number of results. assert_eq!(old_results.len(), new_results.len()); + new_results + } + + fn cache_instruction( + instruction: Instruction, + instruction_results: Vec, + dfg: &DataFlowGraph, + instruction_result_cache: &mut HashMap>, + constraint_cache: &mut HashMap, + ) { + // If the instruction was a constraint, then create a link between the two `ValueId`s + // to map from the more complex to the simpler value. + if let Instruction::Constrain(lhs, rhs, _) = instruction { + // These `ValueId`s should be fully resolved now. + match (&dfg[lhs], &dfg[rhs]) { + // Ignore trivial constraints + (Value::NumericConstant { .. }, Value::NumericConstant { .. }) => (), + + // Prefer replacing with constants where possible. + (Value::NumericConstant { .. }, _) => { + constraint_cache.insert(rhs, lhs); + } + (_, Value::NumericConstant { .. }) => { + constraint_cache.insert(lhs, rhs); + } + // Otherwise prefer block parameters over instruction results. + // This is as block parameters are more likely to be a single witness rather than a full expression. + (Value::Param { .. }, Value::Instruction { .. }) => { + constraint_cache.insert(rhs, lhs); + } + (Value::Instruction { .. }, Value::Param { .. }) => { + constraint_cache.insert(lhs, rhs); + } + (_, _) => (), + } + } + // If the instruction doesn't have side-effects, cache the results so we can reuse them if // the same instruction appears again later in the block. - if instruction.is_pure(&function.dfg) { - instruction_result_cache.insert(instruction, new_results.clone()); + if instruction.is_pure(dfg) { + instruction_result_cache.insert(instruction, instruction_results); } + } + + /// Replaces a set of [`ValueId`]s inside the [`DataFlowGraph`] with another. + fn replace_result_ids( + dfg: &mut DataFlowGraph, + old_results: &[ValueId], + new_results: &[ValueId], + ) { for (old_result, new_result) in old_results.iter().zip(new_results) { - function.dfg.set_value_from_id(*old_result, new_result); + dfg.set_value_from_id(*old_result, *new_result); } } } @@ -122,10 +241,10 @@ mod test { function_builder::FunctionBuilder, ir::{ function::RuntimeType, - instruction::{BinaryOp, TerminatorInstruction}, + instruction::{BinaryOp, Instruction, TerminatorInstruction}, map::Id, types::Type, - value::Value, + value::{Value, ValueId}, }, }; @@ -227,4 +346,100 @@ mod test { // The return element is expected to refer to the new add instruction result. assert_eq!(main.dfg.resolve(new_add_instr_result), main.dfg.resolve(return_element)); } + + #[test] + fn instruction_deduplication() { + // fn main f0 { + // b0(v0: Field): + // v1 = cast v0 as u32 + // v2 = cast v0 as u32 + // constrain v1 v2 + // } + // + // After constructing this IR, we run constant folding which should replace the second cast + // with a reference to the results to the first. This then allows us to optimize away + // the constrain instruction as both inputs are known to be equal. + // + // The first cast instruction is retained and will be removed in the dead instruction elimination pass. + let main_id = Id::test_new(0); + + // Compiling main + let mut builder = FunctionBuilder::new("main".into(), main_id, RuntimeType::Acir); + let v0 = builder.add_parameter(Type::field()); + + let v1 = builder.insert_cast(v0, Type::unsigned(32)); + let v2 = builder.insert_cast(v0, Type::unsigned(32)); + builder.insert_constrain(v1, v2, None); + + let mut ssa = builder.finish(); + let main = ssa.main_mut(); + let instructions = main.dfg[main.entry_block()].instructions(); + assert_eq!(instructions.len(), 3); + + // Expected output: + // + // fn main f0 { + // b0(v0: Field): + // v1 = cast v0 as u32 + // } + let ssa = ssa.fold_constants(); + let main = ssa.main(); + let instructions = main.dfg[main.entry_block()].instructions(); + + assert_eq!(instructions.len(), 1); + let instruction = &main.dfg[instructions[0]]; + + assert_eq!(instruction, &Instruction::Cast(ValueId::test_new(0), Type::unsigned(32))); + } + + #[test] + fn constrained_value_replacement() { + // fn main f0 { + // b0(v0: Field): + // constrain v0 == Field 10 + // v1 = add v0, Field 1 + // constrain v1 == Field 11 + // } + // + // After constructing this IR, we run constant folding which should replace references to `v0` + // with the constant `10`. This then allows us to optimize away the rest of the circuit. + + let main_id = Id::test_new(0); + + // Compiling main + let mut builder = FunctionBuilder::new("main".into(), main_id, RuntimeType::Acir); + let v0 = builder.add_parameter(Type::field()); + + let field_10 = builder.field_constant(10u128); + builder.insert_constrain(v0, field_10, None); + + let field_1 = builder.field_constant(1u128); + let v1 = builder.insert_binary(v0, BinaryOp::Add, field_1); + + let field_11 = builder.field_constant(11u128); + builder.insert_constrain(v1, field_11, None); + + let mut ssa = builder.finish(); + let main = ssa.main_mut(); + let instructions = main.dfg[main.entry_block()].instructions(); + assert_eq!(instructions.len(), 3); + + // Expected output: + // + // fn main f0 { + // b0(v0: Field): + // constrain v0 == Field 10 + // } + let ssa = ssa.fold_constants(); + let main = ssa.main(); + let instructions = main.dfg[main.entry_block()].instructions(); + + assert_eq!(instructions.len(), 1); + let instruction = &main.dfg[instructions[0]]; + + assert_eq!( + instruction, + &Instruction::Constrain(ValueId::test_new(0), ValueId::test_new(1), None) + ); + } } diff --git a/crates/noirc_evaluator/src/ssa/opt/defunctionalize.rs b/crates/noirc_evaluator/src/ssa/opt/defunctionalize.rs index 3143871e59b..a2f4aedf7da 100644 --- a/crates/noirc_evaluator/src/ssa/opt/defunctionalize.rs +++ b/crates/noirc_evaluator/src/ssa/opt/defunctionalize.rs @@ -4,7 +4,7 @@ //! with a non-literal target can be replaced with a call to an apply function. //! The apply function is a dispatch function that takes the function id as a parameter //! and dispatches to the correct target. -use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; +use std::collections::{BTreeMap, BTreeSet, HashSet}; use acvm::FieldElement; use iter_extended::vecmap; @@ -20,6 +20,7 @@ use crate::ssa::{ }, ssa_gen::Ssa, }; +use fxhash::FxHashMap as HashMap; /// Represents an 'apply' function created by this pass to dispatch higher order functions to. /// Pseudocode of an `apply` function is given below: @@ -245,7 +246,7 @@ fn create_apply_functions( ssa: &mut Ssa, variants_map: BTreeMap>, ) -> HashMap { - let mut apply_functions = HashMap::new(); + let mut apply_functions = HashMap::default(); for (signature, variants) in variants_map.into_iter() { assert!( !variants.is_empty(), diff --git a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 61fdc0bcd37..35f01f293b7 100644 --- a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -131,7 +131,8 @@ //! v11 = mul v4, Field 12 //! v12 = add v10, v11 //! store v12 at v5 (new store) -use std::collections::{BTreeMap, HashMap, HashSet}; +use fxhash::FxHashMap as HashMap; +use std::collections::{BTreeMap, HashSet}; use acvm::FieldElement; use iter_extended::vecmap; @@ -218,7 +219,7 @@ fn flatten_function_cfg(function: &mut Function) { let mut context = Context { inserter: FunctionInserter::new(function), cfg, - store_values: HashMap::new(), + store_values: HashMap::default(), local_allocations: HashSet::new(), branch_ends, conditions: Vec::new(), @@ -587,7 +588,7 @@ impl<'f> Context<'f> { // args that will be merged by inline_branch_end. Since jmpifs don't have // block arguments, it is safe to use the jmpif block here. last_block: jmpif_block, - store_values: HashMap::new(), + store_values: HashMap::default(), local_allocations: HashSet::new(), } } else { diff --git a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg/branch_analysis.rs b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg/branch_analysis.rs index 02583a40dcd..59bee00936a 100644 --- a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg/branch_analysis.rs +++ b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg/branch_analysis.rs @@ -19,9 +19,9 @@ //! //! This algorithm will remember each join point found in `find_join_point_of_branches` and //! the resulting map from each split block to each join block is returned. -use std::collections::HashMap; use crate::ssa::ir::{basic_block::BasicBlockId, cfg::ControlFlowGraph, function::Function}; +use fxhash::FxHashMap as HashMap; /// Returns a `HashMap` mapping blocks that start a branch (i.e. blocks terminated with jmpif) to /// their corresponding blocks that end the branch. @@ -61,7 +61,7 @@ struct Context<'cfg> { impl<'cfg> Context<'cfg> { fn new(cfg: &'cfg ControlFlowGraph) -> Self { - Self { cfg, branch_ends: HashMap::new() } + Self { cfg, branch_ends: HashMap::default() } } fn find_join_point_of_branches( diff --git a/crates/noirc_evaluator/src/ssa/opt/inlining.rs b/crates/noirc_evaluator/src/ssa/opt/inlining.rs index 1d09fea59f8..07b524ebc96 100644 --- a/crates/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/crates/noirc_evaluator/src/ssa/opt/inlining.rs @@ -2,7 +2,7 @@ //! The purpose of this pass is to inline the instructions of each function call //! within the function caller. If all function calls are known, there will only //! be a single function remaining when the pass finishes. -use std::collections::{BTreeSet, HashMap, HashSet}; +use std::collections::{BTreeSet, HashSet}; use iter_extended::{btree_map, vecmap}; @@ -17,6 +17,7 @@ use crate::ssa::{ }, ssa_gen::Ssa, }; +use fxhash::FxHashMap as HashMap; /// An arbitrary limit to the maximum number of recursive call /// frames at any point in time. @@ -189,9 +190,9 @@ impl<'function> PerFunctionContext<'function> { Self { context, source_function, - blocks: HashMap::new(), - instructions: HashMap::new(), - values: HashMap::new(), + blocks: HashMap::default(), + instructions: HashMap::default(), + values: HashMap::default(), inlining_entry: false, } } diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 8c74a500bca..71524ab9736 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -61,6 +61,9 @@ //! SSA optimization pipeline, although it will be more successful the simpler the program's CFG is. //! This pass is currently performed several times to enable other passes - most notably being //! performed before loop unrolling to try to allow for mutable variables used for loop indices. +mod alias_set; +mod block; + use std::collections::{BTreeMap, BTreeSet}; use crate::ssa::{ @@ -78,6 +81,9 @@ use crate::ssa::{ ssa_gen::Ssa, }; +use self::alias_set::AliasSet; +use self::block::{Block, Expression}; + impl Ssa { /// Attempts to remove any load instructions that recover values that are already available in /// scope, and attempts to remove stores that are subsequently redundant. @@ -107,45 +113,6 @@ struct PerFunctionContext<'f> { instructions_to_remove: BTreeSet, } -#[derive(Debug, Default, Clone)] -struct Block { - /// Maps a ValueId to the Expression it represents. - /// Multiple ValueIds can map to the same Expression, e.g. - /// dereferences to the same allocation. - expressions: BTreeMap, - - /// Each expression is tracked as to how many aliases it - /// may have. If there is only 1, we can attempt to optimize - /// out any known loads to that alias. Note that "alias" here - /// includes the original reference as well. - aliases: BTreeMap>, - - /// Each allocate instruction result (and some reference block parameters) - /// will map to a Reference value which tracks whether the last value stored - /// to the reference is known. - references: BTreeMap, - - /// The last instance of a `Store` instruction to each address in this block - last_stores: BTreeMap, -} - -/// An `Expression` here is used to represent a canonical key -/// into the aliases map since otherwise two dereferences of the -/// same address will be given different ValueIds. -#[derive(Debug, Clone, PartialOrd, Ord, PartialEq, Eq)] -enum Expression { - Dereference(Box), - ArrayElement(Box), - Other(ValueId), -} - -/// Every reference's value is either Known and can be optimized away, or Unknown. -#[derive(Debug, Copy, Clone, PartialEq, Eq)] -enum ReferenceValue { - Unknown, - Known(ValueId), -} - impl<'f> PerFunctionContext<'f> { fn new(function: &'f mut Function) -> Self { let cfg = ControlFlowGraph::with_function(function); @@ -234,9 +201,10 @@ impl<'f> PerFunctionContext<'f> { if let Some(expression) = references.expressions.get(allocation) { if let Some(aliases) = references.aliases.get(expression) { let allocation_aliases_parameter = - aliases.iter().any(|alias| reference_parameters.contains(alias)); + aliases.any(|alias| reference_parameters.contains(&alias)); - if !aliases.is_empty() && !allocation_aliases_parameter { + // If `allocation_aliases_parameter` is known to be false + if allocation_aliases_parameter == Some(false) { self.instructions_to_remove.insert(*instruction); } } @@ -290,17 +258,11 @@ impl<'f> PerFunctionContext<'f> { references.set_known_value(address, value); references.last_stores.insert(address, instruction); } - Instruction::Call { arguments, .. } => { - self.mark_all_unknown(arguments, references); - } Instruction::Allocate => { // Register the new reference let result = self.inserter.function.dfg.instruction_results(instruction)[0]; references.expressions.insert(result, Expression::Other(result)); - - let mut aliases = BTreeSet::new(); - aliases.insert(result); - references.aliases.insert(Expression::Other(result), aliases); + references.aliases.insert(Expression::Other(result), AliasSet::known(result)); } Instruction::ArrayGet { array, .. } => { let result = self.inserter.function.dfg.instruction_results(instruction)[0]; @@ -317,28 +279,34 @@ impl<'f> PerFunctionContext<'f> { } Instruction::ArraySet { array, value, .. } => { references.mark_value_used(*array, self.inserter.function); + let element_type = self.inserter.function.dfg.type_of_value(*value); - if self.inserter.function.dfg.value_is_reference(*value) { + if Self::contains_references(&element_type) { let result = self.inserter.function.dfg.instruction_results(instruction)[0]; let array = self.inserter.function.dfg.resolve(*array); let expression = Expression::ArrayElement(Box::new(Expression::Other(array))); - if let Some(aliases) = references.aliases.get_mut(&expression) { - aliases.insert(result); + let mut aliases = if let Some(aliases) = references.aliases.get_mut(&expression) + { + aliases.clone() } else if let Some((elements, _)) = self.inserter.function.dfg.get_array_constant(array) { - // TODO: This should be a unification of each alias set - // If any are empty, the whole should be as well. - for reference in elements { - self.try_add_alias(references, reference, array); - } - } + let aliases = references.collect_all_aliases(elements); + self.set_aliases(references, array, aliases.clone()); + aliases + } else { + AliasSet::unknown() + }; + + aliases.unify(&references.get_aliases_for_value(*value)); - references.expressions.insert(result, expression); + references.expressions.insert(result, expression.clone()); + references.aliases.insert(expression, aliases); } } + Instruction::Call { arguments, .. } => self.mark_all_unknown(arguments, references), _ => (), } } @@ -369,12 +337,11 @@ impl<'f> PerFunctionContext<'f> { } } - fn try_add_alias(&self, references: &mut Block, reference: ValueId, alias: ValueId) { - if let Some(expression) = references.expressions.get(&reference) { - if let Some(aliases) = references.aliases.get_mut(expression) { - aliases.insert(alias); - } - } + fn set_aliases(&self, references: &mut Block, address: ValueId, new_aliases: AliasSet) { + let expression = + references.expressions.entry(address).or_insert(Expression::Other(address)); + let aliases = references.aliases.entry(expression.clone()).or_default(); + *aliases = new_aliases; } fn mark_all_unknown(&self, values: &[ValueId], references: &mut Block) { @@ -432,175 +399,6 @@ impl<'f> PerFunctionContext<'f> { } } -impl Block { - /// If the given reference id points to a known value, return the value - fn get_known_value(&self, address: ValueId) -> Option { - if let Some(expression) = self.expressions.get(&address) { - if let Some(aliases) = self.aliases.get(expression) { - // We could allow multiple aliases if we check that the reference - // value in each is equal. - if aliases.len() == 1 { - let alias = aliases.first().expect("There should be exactly 1 alias"); - - if let Some(ReferenceValue::Known(value)) = self.references.get(alias) { - return Some(*value); - } - } - } - } - None - } - - /// If the given address is known, set its value to `ReferenceValue::Known(value)`. - fn set_known_value(&mut self, address: ValueId, value: ValueId) { - self.set_value(address, ReferenceValue::Known(value)); - } - - fn set_unknown(&mut self, address: ValueId) { - self.set_value(address, ReferenceValue::Unknown); - } - - fn set_value(&mut self, address: ValueId, value: ReferenceValue) { - let expression = self.expressions.entry(address).or_insert(Expression::Other(address)); - let aliases = self.aliases.entry(expression.clone()).or_default(); - - if aliases.is_empty() { - // uh-oh, we don't know at all what this reference refers to, could be anything. - // Now we have to invalidate every reference we know of - self.invalidate_all_references(); - } else if aliases.len() == 1 { - let alias = aliases.first().expect("There should be exactly 1 alias"); - self.references.insert(*alias, value); - } else { - // More than one alias. We're not sure which it refers to so we have to - // conservatively invalidate all references it may refer to. - for alias in aliases.iter() { - if let Some(reference_value) = self.references.get_mut(alias) { - *reference_value = ReferenceValue::Unknown; - } - } - } - } - - fn invalidate_all_references(&mut self) { - for reference_value in self.references.values_mut() { - *reference_value = ReferenceValue::Unknown; - } - - self.last_stores.clear(); - } - - fn unify(mut self, other: &Self) -> Self { - for (value_id, expression) in &other.expressions { - if let Some(existing) = self.expressions.get(value_id) { - assert_eq!(existing, expression, "Expected expressions for {value_id} to be equal"); - } else { - self.expressions.insert(*value_id, expression.clone()); - } - } - - for (expression, new_aliases) in &other.aliases { - let expression = expression.clone(); - - self.aliases - .entry(expression) - .and_modify(|aliases| { - for alias in new_aliases { - aliases.insert(*alias); - } - }) - .or_insert_with(|| new_aliases.clone()); - } - - // Keep only the references present in both maps. - let mut intersection = BTreeMap::new(); - for (value_id, reference) in &other.references { - if let Some(existing) = self.references.get(value_id) { - intersection.insert(*value_id, existing.unify(*reference)); - } - } - self.references = intersection; - - self - } - - /// Remember that `result` is the result of dereferencing `address`. This is important to - /// track aliasing when references are stored within other references. - fn remember_dereference(&mut self, function: &Function, address: ValueId, result: ValueId) { - if function.dfg.value_is_reference(result) { - if let Some(known_address) = self.get_known_value(address) { - self.expressions.insert(result, Expression::Other(known_address)); - } else { - let expression = Expression::Dereference(Box::new(Expression::Other(address))); - self.expressions.insert(result, expression); - // No known aliases to insert for this expression... can we find an alias - // even if we don't have a known address? If not we'll have to invalidate all - // known references if this reference is ever stored to. - } - } - } - - /// Iterate through each known alias of the given address and apply the function `f` to each. - fn for_each_alias_of( - &mut self, - address: ValueId, - mut f: impl FnMut(&mut Self, ValueId) -> T, - ) { - if let Some(expr) = self.expressions.get(&address) { - if let Some(aliases) = self.aliases.get(expr).cloned() { - for alias in aliases { - f(self, alias); - } - } - } - } - - fn keep_last_stores_for(&mut self, address: ValueId, function: &Function) { - let address = function.dfg.resolve(address); - self.keep_last_store(address, function); - self.for_each_alias_of(address, |t, alias| t.keep_last_store(alias, function)); - } - - fn keep_last_store(&mut self, address: ValueId, function: &Function) { - let address = function.dfg.resolve(address); - - if let Some(instruction) = self.last_stores.remove(&address) { - // Whenever we decide we want to keep a store instruction, we also need - // to go through its stored value and mark that used as well. - match &function.dfg[instruction] { - Instruction::Store { value, .. } => { - self.mark_value_used(*value, function); - } - other => { - unreachable!("last_store held an id of a non-store instruction: {other:?}") - } - } - } - } - - fn mark_value_used(&mut self, value: ValueId, function: &Function) { - self.keep_last_stores_for(value, function); - - // We must do a recursive check for arrays since they're the only Values which may contain - // other ValueIds. - if let Some((array, _)) = function.dfg.get_array_constant(value) { - for value in array { - self.mark_value_used(value, function); - } - } - } -} - -impl ReferenceValue { - fn unify(self, other: Self) -> Self { - if self == other { - self - } else { - ReferenceValue::Unknown - } - } -} - #[cfg(test)] mod tests { use std::rc::Rc; diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs new file mode 100644 index 00000000000..5477025e429 --- /dev/null +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs @@ -0,0 +1,76 @@ +use std::collections::BTreeSet; + +use crate::ssa::ir::value::ValueId; + +/// A set of possible aliases. Each ValueId in this set represents one possible value the reference +/// holding this AliasSet may be aliased to. This struct wrapper is provided so that when we take +/// the union of multiple alias sets, the result should be empty if any individual set is empty. +/// +/// Note that we distinguish between "definitely has no aliases" - `Some(BTreeSet::new())`, and +/// "unknown which aliases this may refer to" - `None`. +#[derive(Debug, Default, Clone)] +pub(super) struct AliasSet { + aliases: Option>, +} + +impl AliasSet { + pub(super) fn unknown() -> AliasSet { + Self { aliases: None } + } + + pub(super) fn known(value: ValueId) -> AliasSet { + let mut aliases = BTreeSet::new(); + aliases.insert(value); + Self { aliases: Some(aliases) } + } + + /// In rare cases, such as when creating an empty array of references, the set of aliases for a + /// particular value will be known to be zero, which is distinct from being unknown and + /// possibly referring to any alias. + pub(super) fn known_empty() -> AliasSet { + Self { aliases: Some(BTreeSet::new()) } + } + + pub(super) fn is_unknown(&self) -> bool { + self.aliases.is_none() + } + + /// Return the single known alias if there is exactly one. + /// Otherwise, return None. + pub(super) fn single_alias(&self) -> Option { + self.aliases + .as_ref() + .and_then(|aliases| (aliases.len() == 1).then(|| *aliases.first().unwrap())) + } + + /// Unify this alias set with another. The result of this set is empty if either set is empty. + /// Otherwise, it is the union of both alias sets. + pub(super) fn unify(&mut self, other: &Self) { + if let (Some(self_aliases), Some(other_aliases)) = (&mut self.aliases, &other.aliases) { + self_aliases.extend(other_aliases); + } else { + self.aliases = None; + } + } + + /// Inserts a new alias into this set if it is not unknown + pub(super) fn insert(&mut self, new_alias: ValueId) { + if let Some(aliases) = &mut self.aliases { + aliases.insert(new_alias); + } + } + + /// Returns `Some(true)` if `f` returns true for any known alias in this set. + /// If this alias set is unknown, None is returned. + pub(super) fn any(&self, f: impl FnMut(ValueId) -> bool) -> Option { + self.aliases.as_ref().map(|aliases| aliases.iter().copied().any(f)) + } + + pub(super) fn for_each(&self, mut f: impl FnMut(ValueId)) { + if let Some(aliases) = &self.aliases { + for alias in aliases { + f(*alias); + } + } + } +} diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg/block.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg/block.rs new file mode 100644 index 00000000000..22c5705b723 --- /dev/null +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg/block.rs @@ -0,0 +1,243 @@ +use std::{borrow::Cow, collections::BTreeMap}; + +use crate::ssa::ir::{ + function::Function, + instruction::{Instruction, InstructionId}, + value::ValueId, +}; + +use super::alias_set::AliasSet; + +/// A `Block` acts as a per-block context for the mem2reg pass. +/// Most notably, it contains the current alias set thought to track each +/// reference value if known, and it contains the expected ReferenceValue +/// for each ValueId. When a block is finished, the final values of these +/// are expected to match the values held by each ValueId at the very end +/// of a block. +#[derive(Debug, Default, Clone)] +pub(super) struct Block { + /// Maps a ValueId to the Expression it represents. + /// Multiple ValueIds can map to the same Expression, e.g. + /// dereferences to the same allocation. + pub(super) expressions: BTreeMap, + + /// Each expression is tracked as to how many aliases it + /// may have. If there is only 1, we can attempt to optimize + /// out any known loads to that alias. Note that "alias" here + /// includes the original reference as well. + pub(super) aliases: BTreeMap, + + /// Each allocate instruction result (and some reference block parameters) + /// will map to a Reference value which tracks whether the last value stored + /// to the reference is known. + pub(super) references: BTreeMap, + + /// The last instance of a `Store` instruction to each address in this block + pub(super) last_stores: BTreeMap, +} + +/// An `Expression` here is used to represent a canonical key +/// into the aliases map since otherwise two dereferences of the +/// same address will be given different ValueIds. +#[derive(Debug, Clone, PartialOrd, Ord, PartialEq, Eq)] +pub(super) enum Expression { + Dereference(Box), + ArrayElement(Box), + Other(ValueId), +} + +/// Every reference's value is either Known and can be optimized away, or Unknown. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub(super) enum ReferenceValue { + Unknown, + Known(ValueId), +} + +impl ReferenceValue { + fn unify(self, other: Self) -> Self { + if self == other { + self + } else { + ReferenceValue::Unknown + } + } +} + +impl Block { + /// If the given reference id points to a known value, return the value + pub(super) fn get_known_value(&self, address: ValueId) -> Option { + if let Some(expression) = self.expressions.get(&address) { + if let Some(aliases) = self.aliases.get(expression) { + // We could allow multiple aliases if we check that the reference + // value in each is equal. + if let Some(alias) = aliases.single_alias() { + if let Some(ReferenceValue::Known(value)) = self.references.get(&alias) { + return Some(*value); + } + } + } + } + None + } + + /// If the given address is known, set its value to `ReferenceValue::Known(value)`. + pub(super) fn set_known_value(&mut self, address: ValueId, value: ValueId) { + self.set_value(address, ReferenceValue::Known(value)); + } + + pub(super) fn set_unknown(&mut self, address: ValueId) { + self.set_value(address, ReferenceValue::Unknown); + } + + fn set_value(&mut self, address: ValueId, value: ReferenceValue) { + let expression = self.expressions.entry(address).or_insert(Expression::Other(address)); + let aliases = self.aliases.entry(expression.clone()).or_default(); + + if aliases.is_unknown() { + // uh-oh, we don't know at all what this reference refers to, could be anything. + // Now we have to invalidate every reference we know of + self.invalidate_all_references(); + } else if let Some(alias) = aliases.single_alias() { + self.references.insert(alias, value); + } else { + // More than one alias. We're not sure which it refers to so we have to + // conservatively invalidate all references it may refer to. + aliases.for_each(|alias| { + if let Some(reference_value) = self.references.get_mut(&alias) { + *reference_value = ReferenceValue::Unknown; + } + }); + } + } + + fn invalidate_all_references(&mut self) { + for reference_value in self.references.values_mut() { + *reference_value = ReferenceValue::Unknown; + } + + self.last_stores.clear(); + } + + pub(super) fn unify(mut self, other: &Self) -> Self { + for (value_id, expression) in &other.expressions { + if let Some(existing) = self.expressions.get(value_id) { + assert_eq!(existing, expression, "Expected expressions for {value_id} to be equal"); + } else { + self.expressions.insert(*value_id, expression.clone()); + } + } + + for (expression, new_aliases) in &other.aliases { + let expression = expression.clone(); + + self.aliases + .entry(expression) + .and_modify(|aliases| aliases.unify(new_aliases)) + .or_insert_with(|| new_aliases.clone()); + } + + // Keep only the references present in both maps. + let mut intersection = BTreeMap::new(); + for (value_id, reference) in &other.references { + if let Some(existing) = self.references.get(value_id) { + intersection.insert(*value_id, existing.unify(*reference)); + } + } + self.references = intersection; + + self + } + + /// Remember that `result` is the result of dereferencing `address`. This is important to + /// track aliasing when references are stored within other references. + pub(super) fn remember_dereference( + &mut self, + function: &Function, + address: ValueId, + result: ValueId, + ) { + if function.dfg.value_is_reference(result) { + if let Some(known_address) = self.get_known_value(address) { + self.expressions.insert(result, Expression::Other(known_address)); + } else { + let expression = Expression::Dereference(Box::new(Expression::Other(address))); + self.expressions.insert(result, expression); + // No known aliases to insert for this expression... can we find an alias + // even if we don't have a known address? If not we'll have to invalidate all + // known references if this reference is ever stored to. + } + } + } + + /// Iterate through each known alias of the given address and apply the function `f` to each. + fn for_each_alias_of( + &mut self, + address: ValueId, + mut f: impl FnMut(&mut Self, ValueId) -> T, + ) { + if let Some(expr) = self.expressions.get(&address) { + if let Some(aliases) = self.aliases.get(expr).cloned() { + aliases.for_each(|alias| { + f(self, alias); + }); + } + } + } + + fn keep_last_stores_for(&mut self, address: ValueId, function: &Function) { + let address = function.dfg.resolve(address); + self.keep_last_store(address, function); + self.for_each_alias_of(address, |t, alias| t.keep_last_store(alias, function)); + } + + fn keep_last_store(&mut self, address: ValueId, function: &Function) { + let address = function.dfg.resolve(address); + + if let Some(instruction) = self.last_stores.remove(&address) { + // Whenever we decide we want to keep a store instruction, we also need + // to go through its stored value and mark that used as well. + match &function.dfg[instruction] { + Instruction::Store { value, .. } => { + self.mark_value_used(*value, function); + } + other => { + unreachable!("last_store held an id of a non-store instruction: {other:?}") + } + } + } + } + + pub(super) fn mark_value_used(&mut self, value: ValueId, function: &Function) { + self.keep_last_stores_for(value, function); + + // We must do a recursive check for arrays since they're the only Values which may contain + // other ValueIds. + if let Some((array, _)) = function.dfg.get_array_constant(value) { + for value in array { + self.mark_value_used(value, function); + } + } + } + + /// Collect all aliases used by the given value list + pub(super) fn collect_all_aliases( + &self, + values: impl IntoIterator, + ) -> AliasSet { + let mut aliases = AliasSet::known_empty(); + for value in values { + aliases.unify(&self.get_aliases_for_value(value)); + } + aliases + } + + pub(super) fn get_aliases_for_value(&self, value: ValueId) -> Cow { + if let Some(expression) = self.expressions.get(&value) { + if let Some(aliases) = self.aliases.get(expression) { + return Cow::Borrowed(aliases); + } + } + + Cow::Owned(AliasSet::unknown()) + } +} diff --git a/crates/noirc_evaluator/src/ssa/opt/unrolling.rs b/crates/noirc_evaluator/src/ssa/opt/unrolling.rs index 4f8209ce12e..7933079b028 100644 --- a/crates/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/crates/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -12,7 +12,7 @@ //! //! Note that this pass also often creates superfluous jmp instructions in the //! program that will need to be removed by a later simplify cfg pass. -use std::collections::{HashMap, HashSet}; +use std::collections::HashSet; use crate::{ errors::RuntimeError, @@ -31,6 +31,7 @@ use crate::{ ssa_gen::Ssa, }, }; +use fxhash::FxHashMap as HashMap; impl Ssa { /// Unroll all loops in each SSA function. @@ -307,9 +308,9 @@ impl<'f> LoopIteration<'f> { loop_, insert_block, source_block, - blocks: HashMap::new(), - original_blocks: HashMap::new(), - visited_blocks: HashSet::new(), + blocks: HashMap::default(), + original_blocks: HashMap::default(), + visited_blocks: HashSet::default(), induction_value: None, } } diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs index e5a982912ee..5631aa8d351 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -1,4 +1,3 @@ -use std::collections::HashMap; use std::rc::Rc; use std::sync::{Mutex, RwLock}; @@ -19,6 +18,7 @@ use crate::ssa::ir::types::{NumericType, Type}; use crate::ssa::ir::value::ValueId; use super::value::{Tree, Value, Values}; +use fxhash::FxHashMap as HashMap; /// The FunctionContext is the main context object for translating a /// function into SSA form during the SSA-gen pass. @@ -94,7 +94,7 @@ impl<'a> FunctionContext<'a> { .1; let builder = FunctionBuilder::new(function_name, function_id, runtime); - let mut this = Self { definitions: HashMap::new(), builder, shared_context }; + let mut this = Self { definitions: HashMap::default(), builder, shared_context }; this.add_parameters_to_scope(parameters); this } diff --git a/crates/noirc_frontend/src/ast/expression.rs b/crates/noirc_frontend/src/ast/expression.rs index 6c29c89051f..1a829bd0ce7 100644 --- a/crates/noirc_frontend/src/ast/expression.rs +++ b/crates/noirc_frontend/src/ast/expression.rs @@ -377,7 +377,7 @@ pub enum FunctionReturnType { /// Returns type is not specified. Default(Span), /// Everything else. - Ty(UnresolvedType, Span), + Ty(UnresolvedType), } /// Describes the types of smart contract functions that are allowed. @@ -692,7 +692,7 @@ impl FunctionReturnType { pub fn get_type(&self) -> &UnresolvedTypeData { match self { FunctionReturnType::Default(_span) => &UnresolvedTypeData::Unit, - FunctionReturnType::Ty(typ, _span) => &typ.typ, + FunctionReturnType::Ty(typ) => &typ.typ, } } } @@ -701,7 +701,7 @@ impl Display for FunctionReturnType { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { FunctionReturnType::Default(_) => f.write_str(""), - FunctionReturnType::Ty(ty, _) => write!(f, "{ty}"), + FunctionReturnType::Ty(ty) => write!(f, "{ty}"), } } } diff --git a/crates/noirc_frontend/src/ast/function.rs b/crates/noirc_frontend/src/ast/function.rs index ea6d92f0afd..9f200f87bd1 100644 --- a/crates/noirc_frontend/src/ast/function.rs +++ b/crates/noirc_frontend/src/ast/function.rs @@ -47,7 +47,7 @@ impl NoirFunction { FunctionReturnType::Default(_) => { UnresolvedType::without_span(UnresolvedTypeData::Unit) } - FunctionReturnType::Ty(ty, _) => ty.clone(), + FunctionReturnType::Ty(ty) => ty.clone(), } } pub fn name(&self) -> &str { diff --git a/crates/noirc_frontend/src/ast/mod.rs b/crates/noirc_frontend/src/ast/mod.rs index e92d333fd69..aa79929fdd7 100644 --- a/crates/noirc_frontend/src/ast/mod.rs +++ b/crates/noirc_frontend/src/ast/mod.rs @@ -124,17 +124,17 @@ impl std::fmt::Display for UnresolvedTypeData { }, FormatString(len, elements) => write!(f, "fmt<{len}, {elements}"), Function(args, ret, env) => { - let args = vecmap(args, ToString::to_string); + let args = vecmap(args, ToString::to_string).join(", "); match &env.as_ref().typ { UnresolvedTypeData::Unit => { - write!(f, "fn({}) -> {ret}", args.join(", ")) + write!(f, "fn({args}) -> {ret}") } UnresolvedTypeData::Tuple(env_types) => { - let env_types = vecmap(env_types, |arg| ToString::to_string(&arg.typ)); - write!(f, "fn[{}]({}) -> {ret}", env_types.join(", "), args.join(", ")) + let env_types = vecmap(env_types, |arg| arg.typ.to_string()).join(", "); + write!(f, "fn[{env_types}]({args}) -> {ret}") } - _ => unreachable!(), + other => write!(f, "fn[{other}]({args}) -> {ret}"), } } MutableReference(element) => write!(f, "&mut {element}"), diff --git a/crates/noirc_frontend/src/graph/mod.rs b/crates/noirc_frontend/src/graph/mod.rs index af79219d5de..d8c539038df 100644 --- a/crates/noirc_frontend/src/graph/mod.rs +++ b/crates/noirc_frontend/src/graph/mod.rs @@ -10,7 +10,7 @@ use fm::FileId; use rustc_hash::{FxHashMap, FxHashSet}; use smol_str::SmolStr; -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] pub enum CrateId { Root(usize), Crate(usize), diff --git a/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs b/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs index bc0d8d6cfc0..57e5fb8197c 100644 --- a/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -21,7 +21,7 @@ use fm::FileId; use iter_extended::vecmap; use noirc_errors::Span; use noirc_errors::{CustomDiagnostic, FileDiagnostic}; -use std::collections::HashMap; +use std::collections::{BTreeMap, HashMap}; use std::rc::Rc; use std::vec; @@ -69,9 +69,9 @@ pub struct DefCollector { pub(crate) def_map: CrateDefMap, pub(crate) collected_imports: Vec, pub(crate) collected_functions: Vec, - pub(crate) collected_types: HashMap, - pub(crate) collected_type_aliases: HashMap, - pub(crate) collected_traits: HashMap, + pub(crate) collected_types: BTreeMap, + pub(crate) collected_type_aliases: BTreeMap, + pub(crate) collected_traits: BTreeMap, pub(crate) collected_globals: Vec, pub(crate) collected_impls: ImplMap, pub(crate) collected_traits_impls: ImplMap, @@ -80,6 +80,10 @@ pub struct DefCollector { /// Maps the type and the module id in which the impl is defined to the functions contained in that /// impl along with the generics declared on the impl itself. This also contains the Span /// of the object_type of the impl, used to issue an error if the object type fails to resolve. +/// +/// Note that because these are keyed by unresolved types, the impl map is one of the few instances +/// of HashMap rather than BTreeMap. For this reason, we should be careful not to iterate over it +/// since it would be non-deterministic. type ImplMap = HashMap<(UnresolvedType, LocalModuleId), Vec<(UnresolvedGenerics, Span, UnresolvedFunctions)>>; @@ -89,9 +93,9 @@ impl DefCollector { def_map, collected_imports: vec![], collected_functions: vec![], - collected_types: HashMap::new(), - collected_type_aliases: HashMap::new(), - collected_traits: HashMap::new(), + collected_types: BTreeMap::new(), + collected_type_aliases: BTreeMap::new(), + collected_traits: BTreeMap::new(), collected_impls: HashMap::new(), collected_traits_impls: HashMap::new(), collected_globals: vec![], @@ -375,7 +379,7 @@ fn type_check_globals( /// so that expressions can access the fields of structs fn resolve_structs( context: &mut Context, - structs: HashMap, + structs: BTreeMap, crate_id: CrateId, errors: &mut Vec, ) { @@ -440,7 +444,7 @@ fn resolve_trait_methods( let arguments = vecmap(parameters, |param| resolver.resolve_type(param.1.clone())); let resolved_return_type = match return_type { FunctionReturnType::Default(_) => None, - FunctionReturnType::Ty(unresolved_type, _span) => { + FunctionReturnType::Ty(unresolved_type) => { Some(resolver.resolve_type(unresolved_type.clone())) } }; @@ -481,7 +485,7 @@ fn take_errors_filter_self_not_resolved(resolver: Resolver<'_>) -> Vec, + traits: BTreeMap, crate_id: CrateId, errors: &mut Vec, ) { @@ -524,7 +528,7 @@ fn resolve_struct_fields( fn resolve_type_aliases( context: &mut Context, - type_aliases: HashMap, + type_aliases: BTreeMap, crate_id: CrateId, all_errors: &mut Vec, ) { @@ -546,7 +550,7 @@ fn resolve_type_aliases( fn resolve_impls( interner: &mut NodeInterner, crate_id: CrateId, - def_maps: &HashMap, + def_maps: &BTreeMap, collected_impls: ImplMap, errors: &mut Vec, ) -> Vec<(FileId, FuncId)> { @@ -600,7 +604,7 @@ fn resolve_impls( fn resolve_free_functions( interner: &mut NodeInterner, crate_id: CrateId, - def_maps: &HashMap, + def_maps: &BTreeMap, collected_functions: Vec, self_type: Option, errors: &mut Vec, @@ -625,7 +629,7 @@ fn resolve_free_functions( fn resolve_function_set( interner: &mut NodeInterner, crate_id: CrateId, - def_maps: &HashMap, + def_maps: &BTreeMap, unresolved_functions: UnresolvedFunctions, self_type: Option, impl_generics: Vec<(Rc, Shared, Span)>, diff --git a/crates/noirc_frontend/src/hir/def_map/aztec_library.rs b/crates/noirc_frontend/src/hir/def_map/aztec_library.rs index f357ba5b740..efd947a6c04 100644 --- a/crates/noirc_frontend/src/hir/def_map/aztec_library.rs +++ b/crates/noirc_frontend/src/hir/def_map/aztec_library.rs @@ -506,7 +506,7 @@ pub(crate) fn create_return_type(ty: &str) -> FunctionReturnType { let return_path = chained_path!("aztec", "abi", ty); let ty = make_type(UnresolvedTypeData::Named(return_path, vec![])); - FunctionReturnType::Ty(ty, Span::default()) + FunctionReturnType::Ty(ty) } /// Create Context Finish diff --git a/crates/noirc_frontend/src/hir/def_map/mod.rs b/crates/noirc_frontend/src/hir/def_map/mod.rs index c7cc02b8dac..f37ebeee06e 100644 --- a/crates/noirc_frontend/src/hir/def_map/mod.rs +++ b/crates/noirc_frontend/src/hir/def_map/mod.rs @@ -7,7 +7,7 @@ use crate::token::{Attribute, TestScope}; use arena::{Arena, Index}; use fm::{FileId, FileManager}; use noirc_errors::{FileDiagnostic, Location}; -use std::collections::HashMap; +use std::collections::BTreeMap; mod module_def; pub use module_def::*; @@ -27,7 +27,7 @@ pub const MAIN_FUNCTION: &str = "main"; // XXX: Ultimately, we want to constrain an index to be of a certain type just like in RA /// Lets first check if this is offered by any external crate /// XXX: RA has made this a crate on crates.io -#[derive(Debug, PartialEq, Eq, Copy, Clone, Hash)] +#[derive(Debug, PartialEq, Eq, Copy, Clone, Hash, PartialOrd, Ord)] pub struct LocalModuleId(pub Index); impl LocalModuleId { @@ -36,7 +36,7 @@ impl LocalModuleId { } } -#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] pub struct ModuleId { pub krate: CrateId, pub local_id: LocalModuleId, @@ -49,7 +49,7 @@ impl ModuleId { } impl ModuleId { - pub fn module(self, def_maps: &HashMap) -> &ModuleData { + pub fn module(self, def_maps: &BTreeMap) -> &ModuleData { &def_maps[&self.krate].modules()[self.local_id.0] } } @@ -65,7 +65,7 @@ pub struct CrateDefMap { pub(crate) krate: CrateId, - pub(crate) extern_prelude: HashMap, + pub(crate) extern_prelude: BTreeMap, } impl CrateDefMap { @@ -100,7 +100,7 @@ impl CrateDefMap { root: LocalModuleId(root), modules, krate: crate_id, - extern_prelude: HashMap::new(), + extern_prelude: BTreeMap::new(), }; // Now we want to populate the CrateDefMap using the DefCollector diff --git a/crates/noirc_frontend/src/hir/mod.rs b/crates/noirc_frontend/src/hir/mod.rs index 5868872fa1b..1bdd3a62b72 100644 --- a/crates/noirc_frontend/src/hir/mod.rs +++ b/crates/noirc_frontend/src/hir/mod.rs @@ -9,7 +9,7 @@ use crate::hir_def::function::FuncMeta; use crate::node_interner::{FuncId, NodeInterner, StructId}; use def_map::{Contract, CrateDefMap}; use fm::FileManager; -use std::collections::HashMap; +use std::collections::BTreeMap; use self::def_map::TestFunction; @@ -19,12 +19,12 @@ use self::def_map::TestFunction; pub struct Context { pub def_interner: NodeInterner, pub crate_graph: CrateGraph, - pub(crate) def_maps: HashMap, + pub(crate) def_maps: BTreeMap, pub file_manager: FileManager, /// Maps a given (contract) module id to the next available storage slot /// for that contract. - pub storage_slots: HashMap, + pub storage_slots: BTreeMap, } #[derive(Debug, Copy, Clone)] @@ -40,10 +40,10 @@ impl Context { pub fn new(file_manager: FileManager, crate_graph: CrateGraph) -> Context { Context { def_interner: NodeInterner::default(), - def_maps: HashMap::new(), + def_maps: BTreeMap::new(), crate_graph, file_manager, - storage_slots: HashMap::new(), + storage_slots: BTreeMap::new(), } } diff --git a/crates/noirc_frontend/src/hir/resolution/import.rs b/crates/noirc_frontend/src/hir/resolution/import.rs index 8949c766881..6f3140a65d4 100644 --- a/crates/noirc_frontend/src/hir/resolution/import.rs +++ b/crates/noirc_frontend/src/hir/resolution/import.rs @@ -2,7 +2,7 @@ use iter_extended::partition_results; use noirc_errors::CustomDiagnostic; use crate::graph::CrateId; -use std::collections::HashMap; +use std::collections::BTreeMap; use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleDefId, ModuleId, PerNs}; use crate::{Ident, Path, PathKind}; @@ -52,7 +52,7 @@ impl From for CustomDiagnostic { pub fn resolve_imports( crate_id: CrateId, imports_to_resolve: Vec, - def_maps: &HashMap, + def_maps: &BTreeMap, ) -> (Vec, Vec<(PathResolutionError, LocalModuleId)>) { let def_map = &def_maps[&crate_id]; @@ -71,7 +71,7 @@ pub fn resolve_imports( } pub(super) fn allow_referencing_contracts( - def_maps: &HashMap, + def_maps: &BTreeMap, krate: CrateId, local_id: LocalModuleId, ) -> bool { @@ -81,7 +81,7 @@ pub(super) fn allow_referencing_contracts( pub fn resolve_path_to_ns( import_directive: &ImportDirective, def_map: &CrateDefMap, - def_maps: &HashMap, + def_maps: &BTreeMap, allow_contracts: bool, ) -> PathResolution { let import_path = &import_directive.path.segments; @@ -111,7 +111,7 @@ pub fn resolve_path_to_ns( fn resolve_path_from_crate_root( def_map: &CrateDefMap, import_path: &[Ident], - def_maps: &HashMap, + def_maps: &BTreeMap, allow_contracts: bool, ) -> PathResolution { resolve_name_in_module(def_map, import_path, def_map.root, def_maps, allow_contracts) @@ -121,7 +121,7 @@ fn resolve_name_in_module( def_map: &CrateDefMap, import_path: &[Ident], starting_mod: LocalModuleId, - def_maps: &HashMap, + def_maps: &BTreeMap, allow_contracts: bool, ) -> PathResolution { let mut current_mod = &def_map.modules[starting_mod.0]; @@ -185,7 +185,7 @@ fn resolve_path_name(import_directive: &ImportDirective) -> Ident { fn resolve_external_dep( current_def_map: &CrateDefMap, directive: &ImportDirective, - def_maps: &HashMap, + def_maps: &BTreeMap, allow_contracts: bool, ) -> PathResolution { // Use extern_prelude to get the dep diff --git a/crates/noirc_frontend/src/hir/resolution/path_resolver.rs b/crates/noirc_frontend/src/hir/resolution/path_resolver.rs index eb0483fbf54..4c16edd56f1 100644 --- a/crates/noirc_frontend/src/hir/resolution/path_resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/path_resolver.rs @@ -2,7 +2,7 @@ use super::import::{ allow_referencing_contracts, resolve_path_to_ns, ImportDirective, PathResolutionError, }; use crate::Path; -use std::collections::HashMap; +use std::collections::BTreeMap; use crate::graph::CrateId; use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleDefId, ModuleId}; @@ -11,7 +11,7 @@ pub trait PathResolver { /// Resolve the given path returning the resolved ModuleDefId. fn resolve( &self, - def_maps: &HashMap, + def_maps: &BTreeMap, path: Path, ) -> Result; @@ -34,7 +34,7 @@ impl StandardPathResolver { impl PathResolver for StandardPathResolver { fn resolve( &self, - def_maps: &HashMap, + def_maps: &BTreeMap, path: Path, ) -> Result { resolve_path(def_maps, self.module_id, path) @@ -52,7 +52,7 @@ impl PathResolver for StandardPathResolver { /// Resolve the given path to a function or a type. /// In the case of a conflict, functions are given priority pub fn resolve_path( - def_maps: &HashMap, + def_maps: &BTreeMap, module_id: ModuleId, path: Path, ) -> Result { diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index cf0f8f80751..1ab4118099d 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -19,7 +19,7 @@ use crate::hir_def::expr::{ }; use crate::token::Attribute; use regex::Regex; -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeMap, HashSet}; use std::rc::Rc; use crate::graph::CrateId; @@ -75,7 +75,7 @@ pub struct LambdaContext { pub struct Resolver<'a> { scopes: ScopeForest, path_resolver: &'a dyn PathResolver, - def_maps: &'a HashMap, + def_maps: &'a BTreeMap, interner: &'a mut NodeInterner, errors: Vec, file: FileId, @@ -107,7 +107,7 @@ impl<'a> Resolver<'a> { pub fn new( interner: &'a mut NodeInterner, path_resolver: &'a dyn PathResolver, - def_maps: &'a HashMap, + def_maps: &'a BTreeMap, file: FileId, ) -> Resolver<'a> { Self { @@ -438,7 +438,16 @@ impl<'a> Resolver<'a> { type_alias_string }); - return self.interner.get_type_alias(id).get_type(&args); + let result = self.interner.get_type_alias(id).get_type(&args); + + // Because there is no ordering to when type aliases (and other globals) are resolved, + // it is possible for one to refer to an Error type and issue no error if it is set + // equal to another type alias. Fixing this fully requires an analysis to create a DFG + // of definition ordering, but for now we have an explicit check here so that we at + // least issue an error that the type was not found instead of silently passing. + if result != Type::Error { + return result; + } } match self.lookup_struct_or_error(path) { @@ -828,7 +837,7 @@ impl<'a> Resolver<'a> { parameters: &[Type], return_type: &Type, ) -> Vec<(String, TypeVariable)> { - let mut found = HashMap::new(); + let mut found = BTreeMap::new(); for parameter in parameters { Self::find_numeric_generics_in_type(parameter, &mut found); } @@ -836,7 +845,10 @@ impl<'a> Resolver<'a> { found.into_iter().collect() } - fn find_numeric_generics_in_type(typ: &Type, found: &mut HashMap>) { + fn find_numeric_generics_in_type( + typ: &Type, + found: &mut BTreeMap>, + ) { match typ { Type::FieldElement | Type::Integer(_, _) @@ -1508,7 +1520,7 @@ pub fn verify_mutable_reference(interner: &NodeInterner, rhs: ExprId) -> Result< mod test { use core::panic; - use std::collections::HashMap; + use std::collections::BTreeMap; use fm::FileId; use iter_extended::vecmap; @@ -1536,7 +1548,8 @@ mod test { // and functions can be forward declared fn init_src_code_resolution( src: &str, - ) -> (ParsedModule, NodeInterner, HashMap, FileId, TestPathResolver) { + ) -> (ParsedModule, NodeInterner, BTreeMap, FileId, TestPathResolver) + { let (program, errors) = parse_program(src); if errors.iter().any(|e| e.is_error()) { panic!("Unexpected parse errors in test code: {:?}", errors); @@ -1544,14 +1557,14 @@ mod test { let interner: NodeInterner = NodeInterner::default(); - let mut def_maps: HashMap = HashMap::new(); + let mut def_maps: BTreeMap = BTreeMap::new(); let file = FileId::default(); let mut modules = arena::Arena::new(); let location = Location::new(Default::default(), file); modules.insert(ModuleData::new(None, location, false)); - let path_resolver = TestPathResolver(HashMap::new()); + let path_resolver = TestPathResolver(BTreeMap::new()); def_maps.insert( CrateId::dummy_id(), @@ -1559,7 +1572,7 @@ mod test { root: path_resolver.local_module_id(), modules, krate: CrateId::dummy_id(), - extern_prelude: HashMap::new(), + extern_prelude: BTreeMap::new(), }, ); @@ -1992,12 +2005,12 @@ mod test { } } - struct TestPathResolver(HashMap); + struct TestPathResolver(BTreeMap); impl PathResolver for TestPathResolver { fn resolve( &self, - _def_maps: &HashMap, + _def_maps: &BTreeMap, path: Path, ) -> Result { // Not here that foo::bar and hello::foo::bar would fetch the same thing diff --git a/crates/noirc_frontend/src/hir/type_check/errors.rs b/crates/noirc_frontend/src/hir/type_check/errors.rs index cd8d87435c9..3190c7a24a2 100644 --- a/crates/noirc_frontend/src/hir/type_check/errors.rs +++ b/crates/noirc_frontend/src/hir/type_check/errors.rs @@ -205,8 +205,9 @@ impl From for Diagnostic { Source::Comparison => format!("Unsupported types for comparison: {expected} and {actual}"), Source::BinOp => format!("Unsupported types for binary operation: {expected} and {actual}"), Source::Return(ret_ty, expr_span) => { - let ret_ty_span = match ret_ty { - FunctionReturnType::Default(span) | FunctionReturnType::Ty(_, span) => span + let ret_ty_span = match ret_ty.clone() { + FunctionReturnType::Default(span) => span, + FunctionReturnType::Ty(ty) => ty.span.unwrap(), }; let mut diagnostic = Diagnostic::simple_error(format!("expected type {expected}, found type {actual}"), format!("expected {expected} because of return type"), ret_ty_span); diff --git a/crates/noirc_frontend/src/hir/type_check/mod.rs b/crates/noirc_frontend/src/hir/type_check/mod.rs index 8596a9cc28c..d17fbdc17de 100644 --- a/crates/noirc_frontend/src/hir/type_check/mod.rs +++ b/crates/noirc_frontend/src/hir/type_check/mod.rs @@ -167,7 +167,7 @@ impl<'interner> TypeChecker<'interner> { /// We can either build a test apparatus or pass raw code through the resolver #[cfg(test)] mod test { - use std::collections::HashMap; + use std::collections::{BTreeMap, HashMap}; use std::vec; use fm::FileId; @@ -365,7 +365,7 @@ mod test { impl PathResolver for TestPathResolver { fn resolve( &self, - _def_maps: &HashMap, + _def_maps: &BTreeMap, path: Path, ) -> Result { // Not here that foo::bar and hello::foo::bar would fetch the same thing @@ -415,7 +415,7 @@ mod test { path_resolver.insert_func(name.to_owned(), id); } - let mut def_maps: HashMap = HashMap::new(); + let mut def_maps = BTreeMap::new(); let file = FileId::default(); let mut modules = arena::Arena::new(); @@ -428,7 +428,7 @@ mod test { root: path_resolver.local_module_id(), modules, krate: CrateId::dummy_id(), - extern_prelude: HashMap::new(), + extern_prelude: BTreeMap::new(), }, ); diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index 7a25c5e6b77..58dc619952b 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -1327,7 +1327,7 @@ fn undo_instantiation_bindings(bindings: TypeBindings) { #[cfg(test)] mod tests { - use std::collections::HashMap; + use std::collections::{BTreeMap, HashMap}; use fm::FileId; use iter_extended::vecmap; @@ -1372,7 +1372,7 @@ mod tests { path_resolver.insert_func(name.to_owned(), id); } - let mut def_maps: HashMap = HashMap::new(); + let mut def_maps = BTreeMap::new(); let file = FileId::default(); let mut modules = arena::Arena::new(); @@ -1385,7 +1385,7 @@ mod tests { root: path_resolver.local_module_id(), modules, krate: CrateId::dummy_id(), - extern_prelude: HashMap::new(), + extern_prelude: BTreeMap::new(), }, ); @@ -1424,7 +1424,7 @@ mod tests { impl PathResolver for TestPathResolver { fn resolve( &self, - _def_maps: &HashMap, + _def_maps: &BTreeMap, path: crate::Path, ) -> Result { // Not here that foo::bar and hello::foo::bar would fetch the same thing diff --git a/crates/noirc_frontend/src/node_interner.rs b/crates/noirc_frontend/src/node_interner.rs index d72b8d9d949..24015d76a07 100644 --- a/crates/noirc_frontend/src/node_interner.rs +++ b/crates/noirc_frontend/src/node_interner.rs @@ -139,7 +139,7 @@ impl FuncId { } } -#[derive(Debug, Eq, PartialEq, Hash, Copy, Clone)] +#[derive(Debug, Eq, PartialEq, Hash, Copy, Clone, PartialOrd, Ord)] pub struct StructId(ModuleId); impl StructId { @@ -163,7 +163,7 @@ impl StructId { } } -#[derive(Debug, Eq, PartialEq, Hash, Copy, Clone)] +#[derive(Debug, Eq, PartialEq, Hash, Copy, Clone, PartialOrd, Ord)] pub struct TypeAliasId(pub usize); impl TypeAliasId { @@ -172,7 +172,7 @@ impl TypeAliasId { } } -#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] pub struct TraitId(pub ModuleId); impl TraitId { diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index 00eba5de3f3..84f1ab0da82 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -268,7 +268,7 @@ fn function_return_type() -> impl NoirParser<((Distinctness, Visibility), Functi .then(spanned(parse_type())) .or_not() .map_with_span(|ret, span| match ret { - Some((head, (ty, span))) => (head, FunctionReturnType::Ty(ty, span)), + Some((head, (ty, _))) => (head, FunctionReturnType::Ty(ty)), None => ( (Distinctness::DuplicationAllowed, Visibility::Private), FunctionReturnType::Default(span),