From 0330ced124d5455cc584694255a3ceed9c35b69f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Rodr=C3=ADguez?= Date: Wed, 4 Sep 2024 13:47:59 +0200 Subject: [PATCH] feat: Liveness analysis for constants (#8294) Bytecode size change:
Changes in bytecode size ``` contracts: Transpiling AppSubscription::assert_block_number with size 40 => 40 contracts: Transpiling AppSubscription::assert_not_expired with size 37 => 37 contracts: Transpiling AppSubscription::constructor with size 1311 => 1209 contracts: Transpiling Auth::constructor with size 812 => 753 contracts: Transpiling Auth::get_authorized with size 143 => 143 contracts: Transpiling Auth::get_authorized_delay with size 191 => 184 contracts: Transpiling Auth::get_scheduled_authorized with size 115 => 116 contracts: Transpiling Auth::set_authorized with size 1093 => 1040 contracts: Transpiling Auth::set_authorized_delay with size 1047 => 994 contracts: Transpiling AuthRegistry::_set_authorized with size 78 => 78 contracts: Transpiling AuthRegistry::consume with size 731 => 683 contracts: Transpiling AuthRegistry::is_consumable with size 131 => 130 contracts: Transpiling AuthRegistry::is_reject_all with size 116 => 115 contracts: Transpiling AuthRegistry::set_authorized with size 73 => 73 contracts: Transpiling AuthRegistry::set_reject_all with size 58 => 58 contracts: Transpiling AuthWitTest::consume_public with size 1038 => 989 contracts: Transpiling AvmInitializerTest::constructor with size 806 => 749 contracts: Transpiling AvmInitializerTest::read_storage_immutable with size 100 => 99 contracts: Transpiling AvmTest::add_args_return with size 15 => 15 contracts: Transpiling AvmTest::add_storage_map with size 182 => 176 contracts: Transpiling AvmTest::add_u128 with size 32 => 32 contracts: Transpiling AvmTest::assert_nullifier_exists with size 16 => 16 contracts: Transpiling AvmTest::assert_same with size 18 => 18 contracts: Transpiling AvmTest::assert_timestamp with size 15 => 15 contracts: Transpiling AvmTest::assertion_failure with size 14 => 14 contracts: Transpiling AvmTest::check_selector with size 966 => 924 contracts: Transpiling AvmTest::create_different_nullifier_in_nested_call with size 135 => 126 contracts: Transpiling AvmTest::create_same_nullifier_in_nested_call with size 133 => 124 contracts: Transpiling AvmTest::debug_logging with size 205 => 236 contracts: Transpiling AvmTest::elliptic_curve_add_and_double with size 33 => 33 contracts: Transpiling AvmTest::emit_nullifier_and_check with size 17 => 17 contracts: Transpiling AvmTest::emit_unencrypted_log with size 502 => 508 contracts: Transpiling AvmTest::get_address with size 13 => 13 contracts: Transpiling AvmTest::get_args_hash with size 18 => 18 contracts: Transpiling AvmTest::get_block_number with size 13 => 13 contracts: Transpiling AvmTest::get_chain_id with size 13 => 13 contracts: Transpiling AvmTest::get_da_gas_left with size 13 => 13 contracts: Transpiling AvmTest::get_fee_per_da_gas with size 13 => 13 contracts: Transpiling AvmTest::get_fee_per_l2_gas with size 13 => 13 contracts: Transpiling AvmTest::get_function_selector with size 13 => 13 contracts: Transpiling AvmTest::get_l2_gas_left with size 13 => 13 contracts: Transpiling AvmTest::get_sender with size 13 => 13 contracts: Transpiling AvmTest::get_storage_address with size 13 => 13 contracts: Transpiling AvmTest::get_timestamp with size 13 => 13 contracts: Transpiling AvmTest::get_transaction_fee with size 13 => 13 contracts: Transpiling AvmTest::get_version with size 13 => 13 contracts: Transpiling AvmTest::keccak_f1600 with size 61 => 61 contracts: Transpiling AvmTest::keccak_hash with size 840 => 796 contracts: Transpiling AvmTest::l1_to_l2_msg_exists with size 17 => 17 contracts: Transpiling AvmTest::modulo2 with size 16 => 16 contracts: Transpiling AvmTest::nested_call_to_add with size 220 => 206 contracts: Transpiling AvmTest::nested_call_to_add_with_gas with size 213 => 206 contracts: Transpiling AvmTest::nested_static_call_to_add with size 220 => 206 contracts: Transpiling AvmTest::nested_static_call_to_set_storage with size 132 => 123 contracts: Transpiling AvmTest::new_note_hash with size 11 => 11 contracts: Transpiling AvmTest::new_nullifier with size 11 => 11 contracts: Transpiling AvmTest::note_hash_exists with size 17 => 17 contracts: Transpiling AvmTest::nullifier_collision with size 12 => 12 contracts: Transpiling AvmTest::nullifier_exists with size 17 => 17 contracts: Transpiling AvmTest::pedersen_commit with size 81 => 78 contracts: Transpiling AvmTest::pedersen_hash with size 19 => 19 contracts: Transpiling AvmTest::pedersen_hash_with_index with size 19 => 19 contracts: Transpiling AvmTest::poseidon2_hash with size 444 => 432 contracts: Transpiling AvmTest::read_storage_list with size 124 => 123 contracts: Transpiling AvmTest::read_storage_map with size 112 => 111 contracts: Transpiling AvmTest::read_storage_single with size 93 => 92 contracts: Transpiling AvmTest::send_l2_to_l1_msg with size 12 => 12 contracts: Transpiling AvmTest::set_opcode_big_field with size 19 => 19 contracts: Transpiling AvmTest::set_opcode_small_field with size 13 => 13 contracts: Transpiling AvmTest::set_opcode_u32 with size 13 => 13 contracts: Transpiling AvmTest::set_opcode_u64 with size 13 => 13 contracts: Transpiling AvmTest::set_opcode_u8 with size 13 => 13 contracts: Transpiling AvmTest::set_read_storage_single with size 132 => 127 contracts: Transpiling AvmTest::set_storage_list with size 43 => 43 contracts: Transpiling AvmTest::set_storage_map with size 72 => 71 contracts: Transpiling AvmTest::set_storage_single with size 40 => 39 contracts: Transpiling AvmTest::sha256_hash with size 46 => 46 contracts: Transpiling AvmTest::test_get_contract_instance with size 215 => 208 contracts: Transpiling AvmTest::test_get_contract_instance_raw with size 82 => 82 contracts: Transpiling AvmTest::to_radix_le with size 136 => 137 contracts: Transpiling AvmTest::u128_addition_overflow with size 355 => 270 contracts: Transpiling AvmTest::u128_from_integer_overflow with size 140 => 139 contracts: Transpiling AvmTest::variable_base_msm with size 92 => 90 contracts: Transpiling Benchmarking::broadcast with size 114 => 112 contracts: Transpiling Benchmarking::increment_balance with size 282 => 266 contracts: Transpiling CardGame::on_card_played with size 1162 => 1122 contracts: Transpiling CardGame::on_cards_claimed with size 961 => 917 contracts: Transpiling CardGame::on_game_joined with size 893 => 874 contracts: Transpiling CardGame::start_game with size 1396 => 1353 contracts: Transpiling Child::pub_get_value with size 22 => 22 contracts: Transpiling Child::pub_inc_value with size 140 => 135 contracts: Transpiling Child::pub_inc_value_internal with size 145 => 140 contracts: Transpiling Child::pub_set_value with size 51 => 49 contracts: Transpiling Child::set_value_twice_with_nested_first with size 178 => 166 contracts: Transpiling Child::set_value_twice_with_nested_last with size 178 => 166 contracts: Transpiling Child::set_value_with_two_nested_calls with size 120 => 98 contracts: Transpiling Claim::constructor with size 933 => 864 contracts: Transpiling Crowdfunding::_check_deadline with size 123 => 121 contracts: Transpiling Crowdfunding::_publish_donation_receipts with size 1106 => 1066 contracts: Transpiling Crowdfunding::init with size 1061 => 981 contracts: Transpiling DelegatedOn::public_set_value with size 43 => 42 contracts: Transpiling Delegator::public_delegate_set_value with size 97 => 95 contracts: Transpiling DocsExample::get_shared_immutable_constrained_public with size 112 => 111 contracts: Transpiling DocsExample::get_shared_immutable_constrained_public_indirect with size 79 => 72 contracts: Transpiling DocsExample::get_shared_immutable_constrained_public_multiple with size 143 => 142 contracts: Transpiling DocsExample::initialize_public_immutable with size 167 => 161 contracts: Transpiling DocsExample::initialize_shared_immutable with size 167 => 161 contracts: Transpiling DocsExample::spend_public_authwit with size 14 => 14 contracts: Transpiling DocsExample::update_leader with size 50 => 50 contracts: Transpiling EasyPrivateVoting::add_to_tally_public with size 239 => 231 contracts: Transpiling EasyPrivateVoting::constructor with size 870 => 806 contracts: Transpiling EasyPrivateVoting::end_vote with size 138 => 135 contracts: Transpiling FeeJuice::_increase_public_balance with size 197 => 192 contracts: Transpiling FeeJuice::balance_of_public with size 125 => 124 contracts: Transpiling FeeJuice::check_balance with size 187 => 177 contracts: Transpiling FeeJuice::claim_public with size 1811 => 1699 contracts: Transpiling FeeJuice::set_portal with size 238 => 226 contracts: Transpiling FPC::constructor with size 807 => 749 contracts: Transpiling FPC::pay_refund with size 452 => 427 contracts: Transpiling FPC::pay_refund_with_shielded_rebate with size 452 => 427 contracts: Transpiling FPC::prepare_fee with size 360 => 338 contracts: Transpiling ImportTest::pub_call_public_fn with size 132 => 123 contracts: Transpiling InclusionProofs::constructor with size 712 => 661 contracts: Transpiling InclusionProofs::push_nullifier_public with size 18 => 18 contracts: Transpiling InclusionProofs::test_nullifier_inclusion_from_public with size 22 => 22 contracts: Transpiling KeyRegistry::register_initial_keys with size 1660 => 1533 contracts: Transpiling KeyRegistry::rotate_npk_m with size 2361 => 2211 contracts: Transpiling Lending::_borrow with size 2246 => 2307 contracts: Transpiling Lending::_deposit with size 249 => 236 contracts: Transpiling Lending::_repay with size 1331 => 1365 contracts: Transpiling Lending::_withdraw with size 2340 => 2360 contracts: Transpiling Lending::borrow_public with size 283 => 265 contracts: Transpiling Lending::deposit_public with size 616 => 567 contracts: Transpiling Lending::get_asset with size 183 => 182 contracts: Transpiling Lending::get_assets with size 195 => 191 contracts: Transpiling Lending::get_position with size 909 => 883 contracts: Transpiling Lending::init with size 320 => 300 contracts: Transpiling Lending::repay_public with size 544 => 499 contracts: Transpiling Lending::update_accumulator with size 2210 => 2409 contracts: Transpiling Lending::withdraw_public with size 283 => 265 contracts: Transpiling Parent::pub_entry_point with size 67 => 60 contracts: Transpiling Parent::pub_entry_point_twice with size 114 => 93 contracts: Transpiling Parent::public_nested_static_call with size 1081 => 1037 contracts: Transpiling Parent::public_static_call with size 91 => 84 contracts: Transpiling PriceFeed::get_price with size 125 => 124 contracts: Transpiling PriceFeed::set_price with size 68 => 69 contracts: Transpiling PrivateFPC::constructor with size 810 => 752 contracts: Transpiling StatefulTest::get_public_value with size 113 => 112 contracts: Transpiling StatefulTest::increment_public_value with size 143 => 140 contracts: Transpiling StatefulTest::increment_public_value_no_init_check with size 136 => 133 contracts: Transpiling StatefulTest::public_constructor with size 877 => 814 contracts: Transpiling StaticChild::pub_get_value with size 26 => 26 contracts: Transpiling StaticChild::pub_illegal_inc_value with size 144 => 139 contracts: Transpiling StaticChild::pub_inc_value with size 140 => 135 contracts: Transpiling StaticChild::pub_set_value with size 51 => 49 contracts: Transpiling StaticParent::public_call with size 67 => 60 contracts: Transpiling StaticParent::public_get_value_from_child with size 148 => 138 contracts: Transpiling StaticParent::public_nested_static_call with size 303 => 285 contracts: Transpiling StaticParent::public_static_call with size 91 => 84 contracts: Transpiling Test::assert_public_global_vars with size 40 => 40 contracts: Transpiling Test::consume_message_from_arbitrary_sender_public with size 1312 => 1230 contracts: Transpiling Test::consume_mint_public_message with size 1582 => 1483 contracts: Transpiling Test::create_l2_to_l1_message_arbitrary_recipient_public with size 12 => 12 contracts: Transpiling Test::create_l2_to_l1_message_public with size 24 => 24 contracts: Transpiling Test::dummy_public_call with size 14 => 14 contracts: Transpiling Test::emit_nullifier_public with size 11 => 11 contracts: Transpiling Test::emit_unencrypted with size 210 => 217 contracts: Transpiling Test::is_time_equal with size 18 => 18 contracts: Transpiling TestLog::emit_unencrypted_events with size 2147 => 2029 contracts: Transpiling Token::_increase_public_balance with size 196 => 191 contracts: Transpiling Token::_reduce_total_supply with size 174 => 170 contracts: Transpiling Token::admin with size 104 => 103 contracts: Transpiling Token::assert_minter_and_mint with size 257 => 250 contracts: Transpiling Token::balance_of_public with size 132 => 131 contracts: Transpiling Token::burn_public with size 1786 => 1679 contracts: Transpiling Token::complete_refund with size 519 => 478 contracts: Transpiling Token::constructor with size 1310 => 1210 contracts: Transpiling Token::is_minter with size 123 => 122 contracts: Transpiling Token::mint_private with size 579 => 556 contracts: Transpiling Token::mint_public with size 400 => 381 contracts: Transpiling Token::public_get_decimals with size 107 => 106 contracts: Transpiling Token::public_get_name with size 104 => 103 contracts: Transpiling Token::public_get_symbol with size 104 => 103 contracts: Transpiling Token::set_admin with size 139 => 135 contracts: Transpiling Token::set_minter with size 157 => 154 contracts: Transpiling Token::shield with size 1998 => 1880 contracts: Transpiling Token::total_supply with size 116 => 115 contracts: Transpiling Token::transfer_public with size 1808 => 1698 contracts: Transpiling TokenBlacklist::_increase_public_balance with size 196 => 191 contracts: Transpiling TokenBlacklist::_reduce_total_supply with size 174 => 170 contracts: Transpiling TokenBlacklist::balance_of_public with size 132 => 131 contracts: Transpiling TokenBlacklist::burn_public with size 1935 => 1820 contracts: Transpiling TokenBlacklist::constructor with size 1791 => 1674 contracts: Transpiling TokenBlacklist::get_roles with size 193 => 192 contracts: Transpiling TokenBlacklist::mint_private with size 644 => 619 contracts: Transpiling TokenBlacklist::mint_public with size 612 => 581 contracts: Transpiling TokenBlacklist::shield with size 2147 => 2021 contracts: Transpiling TokenBlacklist::total_supply with size 116 => 115 contracts: Transpiling TokenBlacklist::transfer_public with size 2112 => 1983 contracts: Transpiling TokenBlacklist::update_roles with size 1347 => 1266 contracts: Transpiling TokenBridge::_assert_token_is_same with size 106 => 105 contracts: Transpiling TokenBridge::_call_mint_on_token with size 295 => 279 contracts: Transpiling TokenBridge::claim_public with size 1913 => 1790 contracts: Transpiling TokenBridge::constructor with size 838 => 777 contracts: Transpiling TokenBridge::exit_to_l1_public with size 900 => 876 contracts: Transpiling TokenBridge::get_portal_address_public with size 111 => 110 contracts: Transpiling TokenBridge::get_token with size 104 => 103 contracts: Transpiling Uniswap::_approve_bridge_and_exit_input_asset_to_L1 with size 4804 => 4439 contracts: Transpiling Uniswap::_assert_token_is_same with size 78 => 71 contracts: Transpiling Uniswap::constructor with size 807 => 749 contracts: Transpiling Uniswap::swap_public with size 3294 => 3092 ```
--------- Co-authored-by: Maxim Vezenov --- .../src/brillig/brillig_gen.rs | 1 + .../src/brillig/brillig_gen/brillig_block.rs | 72 ++++++--- .../brillig_gen/brillig_block_variables.rs | 62 +++----- .../src/brillig/brillig_gen/brillig_fn.rs | 10 +- .../brillig_gen/constant_allocation.rs | 142 ++++++++++++++++++ .../brillig/brillig_gen/variable_liveness.rs | 86 +++++++---- .../noirc_evaluator/src/ssa/ir/dom.rs | 2 +- 7 files changed, 271 insertions(+), 104 deletions(-) create mode 100644 noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen.rs index b256c2b85ab..628ec9657f2 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen.rs @@ -4,6 +4,7 @@ pub(crate) mod brillig_block_variables; pub(crate) mod brillig_directive; pub(crate) mod brillig_fn; pub(crate) mod brillig_slice_ops; +mod constant_allocation; mod variable_liveness; use acvm::FieldElement; diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 26abafe177f..55794c2b7dd 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -29,6 +29,7 @@ use std::sync::Arc; use super::brillig_black_box::convert_black_box_call; use super::brillig_block_variables::BlockVariables; use super::brillig_fn::FunctionContext; +use super::constant_allocation::InstructionLocation; /// Generate the compilation artifacts for compiling a function into brillig bytecode. pub(crate) struct BrilligBlock<'block> { @@ -117,6 +118,13 @@ impl<'block> BrilligBlock<'block> { terminator_instruction: &TerminatorInstruction, dfg: &DataFlowGraph, ) { + self.initialize_constants( + &self + .function_context + .constant_allocation + .allocated_at_location(self.block_id, InstructionLocation::Terminator), + dfg, + ); match terminator_instruction { TerminatorInstruction::JmpIf { condition, @@ -244,6 +252,13 @@ impl<'block> BrilligBlock<'block> { let instruction = &dfg[instruction_id]; self.brillig_context.set_call_stack(dfg.get_call_stack(instruction_id)); + self.initialize_constants( + &self.function_context.constant_allocation.allocated_at_location( + self.block_id, + InstructionLocation::Instruction(instruction_id), + ), + dfg, + ); match instruction { Instruction::Binary(binary) => { let result_var = self.variables.define_single_addr_variable( @@ -734,9 +749,6 @@ impl<'block> BrilligBlock<'block> { .brillig_context .codegen_pre_call_save_registers_prep_args(&argument_registers, &variables_to_save); - // We don't save and restore constants, so we dump them before a external call since the callee might use the registers where they are allocated. - self.variables.dump_constants(); - // Call instruction, which will interpret above registers 0..num args self.brillig_context.add_external_call_instruction(func_id); @@ -1478,6 +1490,12 @@ impl<'block> BrilligBlock<'block> { } } + fn initialize_constants(&mut self, constants: &[ValueId], dfg: &DataFlowGraph) { + for &constant_id in constants { + self.convert_ssa_value(constant_id, dfg); + } + } + /// Converts an SSA `ValueId` into a `RegisterOrMemory`. Initializes if necessary. fn convert_ssa_value(&mut self, value_id: ValueId, dfg: &DataFlowGraph) -> BrilligVariable { let value_id = dfg.resolve(value_id); @@ -1493,11 +1511,15 @@ impl<'block> BrilligBlock<'block> { Value::NumericConstant { constant, .. } => { // Constants might have been converted previously or not, so we get or create and // (re)initialize the value inside. - if let Some(variable) = self.variables.get_constant(value_id, dfg) { - variable + if self.variables.is_allocated(&value_id) { + self.variables.get_allocation(self.function_context, value_id, dfg) } else { - let new_variable = - self.variables.allocate_constant(self.brillig_context, value_id, dfg); + let new_variable = self.variables.define_variable( + self.function_context, + self.brillig_context, + value_id, + dfg, + ); self.brillig_context .const_instruction(new_variable.extract_single_addr(), *constant); @@ -1505,11 +1527,15 @@ impl<'block> BrilligBlock<'block> { } } Value::Array { array, typ } => { - if let Some(variable) = self.variables.get_constant(value_id, dfg) { - variable + if self.variables.is_allocated(&value_id) { + self.variables.get_allocation(self.function_context, value_id, dfg) } else { - let new_variable = - self.variables.allocate_constant(self.brillig_context, value_id, dfg); + let new_variable = self.variables.define_variable( + self.function_context, + self.brillig_context, + value_id, + dfg, + ); // Initialize the variable let pointer = match new_variable { @@ -1549,8 +1575,12 @@ impl<'block> BrilligBlock<'block> { // around values representing function pointers, even though // there is no interaction with the function possible given that // value. - let new_variable = - self.variables.allocate_constant(self.brillig_context, value_id, dfg); + let new_variable = self.variables.define_variable( + self.function_context, + self.brillig_context, + value_id, + dfg, + ); self.brillig_context.const_instruction( new_variable.extract_single_addr(), @@ -1698,18 +1728,10 @@ impl<'block> BrilligBlock<'block> { self.brillig_context.mov_instruction(write_pointer_register, pointer); for (element_idx, element_id) in data.iter().enumerate() { - if let Some((constant, typ)) = dfg.get_numeric_constant_with_type(*element_id) { - self.brillig_context.indirect_const_instruction( - write_pointer_register, - typ.bit_size(), - constant, - ); - } else { - let element_variable = self.convert_ssa_value(*element_id, dfg); - // Store the item in memory - self.brillig_context - .codegen_store_variable_in_pointer(write_pointer_register, element_variable); - } + let element_variable = self.convert_ssa_value(*element_id, dfg); + // Store the item in memory + self.brillig_context + .codegen_store_variable_in_pointer(write_pointer_register, element_variable); if element_idx != data.len() - 1 { // Increment the write_pointer_register diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs index 63b2073c654..90af2e42211 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs @@ -1,5 +1,5 @@ use acvm::FieldElement; -use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; +use fxhash::FxHashSet as HashSet; use crate::{ brillig::brillig_ir::{ @@ -22,16 +22,15 @@ use super::brillig_fn::FunctionContext; #[derive(Debug, Default)] pub(crate) struct BlockVariables { available_variables: HashSet, - available_constants: HashMap, } impl BlockVariables { /// Creates a BlockVariables instance. It uses the variables that are live in to the block and the global available variables (block parameters) pub(crate) fn new(live_in: HashSet) -> Self { - BlockVariables { available_variables: live_in, ..Default::default() } + BlockVariables { available_variables: live_in } } - /// Returns all non-constant variables that have not been removed at this point. + /// Returns all variables that have not been removed at this point. pub(crate) fn get_available_variables( &self, function_context: &FunctionContext, @@ -48,7 +47,7 @@ impl BlockVariables { .collect() } - /// For a given SSA non constant value id, define the variable and return the corresponding cached allocation. + /// For a given SSA value id, define the variable and return the corresponding cached allocation. pub(crate) fn define_variable( &mut self, function_context: &mut FunctionContext, @@ -97,6 +96,11 @@ impl BlockVariables { }); } + /// Checks if a variable is allocated. + pub(crate) fn is_allocated(&self, value_id: &ValueId) -> bool { + self.available_variables.contains(value_id) + } + /// For a given SSA value id, return the corresponding cached allocation. pub(crate) fn get_allocation( &mut self, @@ -105,48 +109,16 @@ impl BlockVariables { dfg: &DataFlowGraph, ) -> BrilligVariable { let value_id = dfg.resolve(value_id); - if let Some(constant) = self.available_constants.get(&value_id) { - *constant - } else { - assert!( - self.available_variables.contains(&value_id), - "ICE: ValueId {value_id:?} is not available" - ); - - *function_context - .ssa_value_allocations - .get(&value_id) - .unwrap_or_else(|| panic!("ICE: Value not found in cache {value_id}")) - } - } - - /// Creates a constant. Constants are a special case in SSA, since they are "defined" every time they are used. - /// We keep constants block-local. - pub(crate) fn allocate_constant( - &mut self, - brillig_context: &mut BrilligContext, - value_id: ValueId, - dfg: &DataFlowGraph, - ) -> BrilligVariable { - let value_id = dfg.resolve(value_id); - let constant = allocate_value(value_id, brillig_context, dfg); - self.available_constants.insert(value_id, constant); - constant - } - /// Gets a constant. - pub(crate) fn get_constant( - &mut self, - value_id: ValueId, - dfg: &DataFlowGraph, - ) -> Option { - let value_id = dfg.resolve(value_id); - self.available_constants.get(&value_id).cloned() - } + assert!( + self.available_variables.contains(&value_id), + "ICE: ValueId {value_id:?} is not available" + ); - /// Removes the allocations of all constants. Constants will need to be reallocated and reinitialized after this. - pub(crate) fn dump_constants(&mut self) { - self.available_constants.clear(); + *function_context + .ssa_value_allocations + .get(&value_id) + .unwrap_or_else(|| panic!("ICE: Value not found in cache {value_id}")) } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs index c1abad17a8f..2779be103cd 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs @@ -15,7 +15,7 @@ use crate::{ }; use fxhash::FxHashMap as HashMap; -use super::variable_liveness::VariableLiveness; +use super::{constant_allocation::ConstantAllocation, variable_liveness::VariableLiveness}; pub(crate) struct FunctionContext { pub(crate) function_id: FunctionId, @@ -25,6 +25,8 @@ pub(crate) struct FunctionContext { pub(crate) blocks: Vec, /// Liveness information for each variable in the function. pub(crate) liveness: VariableLiveness, + /// Information on where to allocate constants + pub(crate) constant_allocation: ConstantAllocation, } impl FunctionContext { @@ -36,11 +38,15 @@ impl FunctionContext { reverse_post_order.extend_from_slice(PostOrder::with_function(function).as_slice()); reverse_post_order.reverse(); + let constants = ConstantAllocation::from_function(function); + let liveness = VariableLiveness::from_function(function, &constants); + Self { function_id: id, ssa_value_allocations: HashMap::default(), blocks: reverse_post_order, - liveness: VariableLiveness::from_function(function), + liveness, + constant_allocation: constants, } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs new file mode 100644 index 00000000000..cf484fa5038 --- /dev/null +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs @@ -0,0 +1,142 @@ +//! This module analyzes the usage of constants in a given function and decides an allocation point for them. +//! The allocation point will be the common dominator of all the places where the constant is used. +//! By allocating in the common dominator, we can cache the constants for all subsequent uses. +use fxhash::FxHashMap as HashMap; + +use crate::ssa::ir::{ + basic_block::BasicBlockId, + cfg::ControlFlowGraph, + dfg::DataFlowGraph, + dom::DominatorTree, + function::Function, + instruction::InstructionId, + post_order::PostOrder, + value::{Value, ValueId}, +}; + +use super::variable_liveness::{collect_variables_of_value, variables_used_in_instruction}; + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub(crate) enum InstructionLocation { + Instruction(InstructionId), + Terminator, +} + +pub(crate) struct ConstantAllocation { + constant_usage: HashMap>>, + allocation_points: HashMap>>, + dominator_tree: DominatorTree, +} + +impl ConstantAllocation { + pub(crate) fn from_function(func: &Function) -> Self { + let cfg = ControlFlowGraph::with_function(func); + let post_order = PostOrder::with_function(func); + let dominator_tree = DominatorTree::with_cfg_and_post_order(&cfg, &post_order); + let mut instance = ConstantAllocation { + constant_usage: HashMap::default(), + allocation_points: HashMap::default(), + dominator_tree, + }; + instance.collect_constant_usage(func); + instance.decide_allocation_points(); + + instance + } + + pub(crate) fn allocated_in_block(&self, block_id: BasicBlockId) -> Vec { + self.allocation_points.get(&block_id).map_or(Vec::default(), |allocations| { + allocations.iter().flat_map(|(_, constants)| constants.iter()).copied().collect() + }) + } + + pub(crate) fn allocated_at_location( + &self, + block_id: BasicBlockId, + location: InstructionLocation, + ) -> Vec { + self.allocation_points.get(&block_id).map_or(Vec::default(), |allocations| { + allocations.get(&location).map_or(Vec::default(), |constants| constants.clone()) + }) + } + + fn collect_constant_usage(&mut self, func: &Function) { + let mut record_if_constant = + |block_id: BasicBlockId, value_id: ValueId, location: InstructionLocation| { + if is_constant_value(value_id, &func.dfg) { + self.constant_usage + .entry(value_id) + .or_default() + .entry(block_id) + .or_default() + .push(location); + } + }; + for block_id in func.reachable_blocks() { + let block = &func.dfg[block_id]; + for &inst_id in block.instructions() { + let variables = variables_used_in_instruction(&func.dfg[inst_id], &func.dfg); + for variable in variables { + record_if_constant( + block_id, + variable, + InstructionLocation::Instruction(inst_id), + ); + } + } + if let Some(terminator_instruction) = block.terminator() { + terminator_instruction.for_each_value(|value_id| { + let variables = collect_variables_of_value(value_id, &func.dfg); + for variable in variables { + record_if_constant(block_id, variable, InstructionLocation::Terminator); + } + }); + } + } + } + + fn decide_allocation_points(&mut self) { + for (constant_id, usage_in_blocks) in self.constant_usage.iter() { + let block_ids: Vec<_> = usage_in_blocks.iter().map(|(block_id, _)| *block_id).collect(); + + let common_dominator = self.common_dominator(&block_ids); + + // If the common dominator is one of the places where it's used, we take the first usage in the common dominator. + // Otherwise, we allocate it at the terminator of the common dominator. + let location = if let Some(locations_in_common_dominator) = + usage_in_blocks.get(&common_dominator) + { + *locations_in_common_dominator + .first() + .expect("At least one location must have been found") + } else { + InstructionLocation::Terminator + }; + + self.allocation_points + .entry(common_dominator) + .or_default() + .entry(location) + .or_default() + .push(*constant_id); + } + } + + fn common_dominator(&self, block_ids: &[BasicBlockId]) -> BasicBlockId { + if block_ids.len() == 1 { + return block_ids[0]; + } + + let mut common_dominator = block_ids[0]; + + for block_id in block_ids.iter().skip(1) { + common_dominator = self.dominator_tree.common_dominator(common_dominator, *block_id); + } + + common_dominator + } +} + +pub(crate) fn is_constant_value(id: ValueId, dfg: &DataFlowGraph) -> bool { + matches!(&dfg[dfg.resolve(id)], Value::NumericConstant { .. } | Value::Array { .. }) +} diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs index 52eded81919..73e88cee676 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs @@ -1,5 +1,6 @@ //! This module analyzes the liveness of variables (non-constant values) throughout a function. //! It uses the approach detailed in the section 4.2 of this paper https://inria.hal.science/inria-00558509v2/document + use crate::ssa::ir::{ basic_block::{BasicBlock, BasicBlockId}, cfg::ControlFlowGraph, @@ -13,6 +14,8 @@ use crate::ssa::ir::{ use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; +use super::constant_allocation::ConstantAllocation; + /// A back edge is an edge from a node to one of its ancestors. It denotes a loop in the CFG. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] struct BackEdge { @@ -42,7 +45,7 @@ fn find_back_edges( } /// Collects the underlying variables inside a value id. It might be more than one, for example in constant arrays that are constructed with multiple vars. -fn collect_variables_of_value(value_id: ValueId, dfg: &DataFlowGraph) -> Vec { +pub(crate) fn collect_variables_of_value(value_id: ValueId, dfg: &DataFlowGraph) -> Vec { let value_id = dfg.resolve(value_id); let value = &dfg[value_id]; @@ -52,7 +55,7 @@ fn collect_variables_of_value(value_id: ValueId, dfg: &DataFlowGraph) -> Vec { - let mut value_ids = Vec::new(); + let mut value_ids = vec![value_id]; array.iter().for_each(|item_id| { let underlying_ids = collect_variables_of_value(*item_id, dfg); @@ -61,19 +64,21 @@ fn collect_variables_of_value(value_id: ValueId, dfg: &DataFlowGraph) -> Vec { + vec![value_id] + } // Functions are not variables in a defunctionalized SSA. Only constant function values should appear. - Value::ForeignFunction(_) - | Value::Function(_) - | Value::Intrinsic(..) - // Constants are not treated as variables for the variable liveness analysis, since they are defined every time they are used. - | Value::NumericConstant { .. } => { + Value::ForeignFunction(_) | Value::Function(_) | Value::Intrinsic(..) => { vec![] } } } -fn variables_used_in_instruction(instruction: &Instruction, dfg: &DataFlowGraph) -> Vec { - let mut used = Vec::new(); +pub(crate) fn variables_used_in_instruction( + instruction: &Instruction, + dfg: &DataFlowGraph, +) -> Variables { + let mut used = HashSet::default(); instruction.for_each_value(|value_id| { let underlying_ids = collect_variables_of_value(value_id, dfg); @@ -83,8 +88,8 @@ fn variables_used_in_instruction(instruction: &Instruction, dfg: &DataFlowGraph) used } -fn variables_used_in_block(block: &BasicBlock, dfg: &DataFlowGraph) -> Vec { - let mut used: Vec = block +fn variables_used_in_block(block: &BasicBlock, dfg: &DataFlowGraph) -> Variables { + let mut used: Variables = block .instructions() .iter() .flat_map(|instruction_id| { @@ -124,6 +129,7 @@ type LastUses = HashMap; pub(crate) struct VariableLiveness { cfg: ControlFlowGraph, post_order: PostOrder, + dominator_tree: DominatorTree, /// The variables that are alive before the block starts executing live_in: HashMap, /// The variables that stop being alive after each specific instruction @@ -134,13 +140,15 @@ pub(crate) struct VariableLiveness { impl VariableLiveness { /// Computes the liveness of variables throughout a function. - pub(crate) fn from_function(func: &Function) -> Self { + pub(crate) fn from_function(func: &Function, constants: &ConstantAllocation) -> Self { let cfg = ControlFlowGraph::with_function(func); let post_order = PostOrder::with_function(func); + let dominator_tree = DominatorTree::with_cfg_and_post_order(&cfg, &post_order); let mut instance = Self { cfg, post_order, + dominator_tree, live_in: HashMap::default(), last_uses: HashMap::default(), param_definitions: HashMap::default(), @@ -148,7 +156,7 @@ impl VariableLiveness { instance.compute_block_param_definitions(func); - instance.compute_live_in_of_blocks(func); + instance.compute_live_in_of_blocks(func, constants); instance.compute_last_uses(func); @@ -182,8 +190,6 @@ impl VariableLiveness { } fn compute_block_param_definitions(&mut self, func: &Function) { - let tree = DominatorTree::with_cfg_and_post_order(&self.cfg, &self.post_order); - // Going in reverse post order to process the entry block first let mut reverse_post_order = Vec::new(); reverse_post_order.extend_from_slice(self.post_order.as_slice()); @@ -191,18 +197,19 @@ impl VariableLiveness { for block in reverse_post_order { let params = func.dfg[block].parameters(); // If it has no dominator, it's the entry block - let dominator_block = tree.immediate_dominator(block).unwrap_or(func.entry_block()); + let dominator_block = + self.dominator_tree.immediate_dominator(block).unwrap_or(func.entry_block()); let definitions_for_the_dominator = self.param_definitions.entry(dominator_block).or_default(); definitions_for_the_dominator.extend(params.iter()); } } - fn compute_live_in_of_blocks(&mut self, func: &Function) { + fn compute_live_in_of_blocks(&mut self, func: &Function, constants: &ConstantAllocation) { let back_edges = find_back_edges(func, &self.cfg, &self.post_order); // First pass, propagate up the live_ins skipping back edges - self.compute_live_in_recursive(func, func.entry_block(), &back_edges); + self.compute_live_in_recursive(func, func.entry_block(), &back_edges, constants); // Second pass, propagate header live_ins to the loop bodies for back_edge in back_edges { @@ -215,8 +222,11 @@ impl VariableLiveness { func: &Function, block_id: BasicBlockId, back_edges: &HashSet, + constants: &ConstantAllocation, ) { - let defined = self.compute_defined_variables(block_id, &func.dfg); + let mut defined = self.compute_defined_variables(block_id, &func.dfg); + + defined.extend(constants.allocated_in_block(block_id)); let block: &BasicBlock = &func.dfg[block_id]; @@ -227,7 +237,7 @@ impl VariableLiveness { for successor_id in block.successors() { if !back_edges.contains(&BackEdge { start: block_id, header: successor_id }) { if !self.live_in.contains_key(&successor_id) { - self.compute_live_in_recursive(func, successor_id, back_edges); + self.compute_live_in_recursive(func, successor_id, back_edges, constants); } live_out.extend( self.live_in @@ -332,6 +342,7 @@ impl VariableLiveness { mod test { use fxhash::FxHashSet; + use crate::brillig::brillig_gen::constant_allocation::ConstantAllocation; use crate::brillig::brillig_gen::variable_liveness::VariableLiveness; use crate::ssa::function_builder::FunctionBuilder; use crate::ssa::ir::function::RuntimeType; @@ -403,11 +414,18 @@ mod test { let ssa = builder.finish(); let func = ssa.main(); - let liveness = VariableLiveness::from_function(func); + let constants = ConstantAllocation::from_function(func); + let liveness = VariableLiveness::from_function(func, &constants); assert!(liveness.get_live_in(&func.entry_block()).is_empty()); - assert_eq!(liveness.get_live_in(&b2), &FxHashSet::from_iter([v3, v0].into_iter())); - assert_eq!(liveness.get_live_in(&b1), &FxHashSet::from_iter([v3, v1].into_iter())); + assert_eq!( + liveness.get_live_in(&b2), + &FxHashSet::from_iter([v3, v0, twenty_seven].into_iter()) + ); + assert_eq!( + liveness.get_live_in(&b1), + &FxHashSet::from_iter([v3, v1, twenty_seven].into_iter()) + ); assert_eq!(liveness.get_live_in(&b3), &FxHashSet::from_iter([v3].into_iter())); let block_1 = &func.dfg[b1]; @@ -415,11 +433,11 @@ mod test { let block_3 = &func.dfg[b3]; assert_eq!( liveness.get_last_uses(&b1).get(&block_1.instructions()[0]), - Some(&FxHashSet::from_iter([v1].into_iter())) + Some(&FxHashSet::from_iter([v1, twenty_seven].into_iter())) ); assert_eq!( liveness.get_last_uses(&b2).get(&block_2.instructions()[0]), - Some(&FxHashSet::from_iter([v0].into_iter())) + Some(&FxHashSet::from_iter([v0, twenty_seven].into_iter())) ); assert_eq!( liveness.get_last_uses(&b3).get(&block_3.instructions()[0]), @@ -548,7 +566,8 @@ mod test { let ssa = builder.finish(); let func = ssa.main(); - let liveness = VariableLiveness::from_function(func); + let constants = ConstantAllocation::from_function(func); + let liveness = VariableLiveness::from_function(func, &constants); assert!(liveness.get_live_in(&func.entry_block()).is_empty()); assert_eq!(liveness.get_live_in(&b1), &FxHashSet::from_iter([v0, v1, v3, v4].into_iter())); @@ -558,18 +577,21 @@ mod test { liveness.get_live_in(&b4), &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7].into_iter()) ); - assert_eq!(liveness.get_live_in(&b6), &FxHashSet::from_iter([v0, v1, v3, v4].into_iter())); + assert_eq!( + liveness.get_live_in(&b6), + &FxHashSet::from_iter([v0, v1, v3, v4, one].into_iter()) + ); assert_eq!( liveness.get_live_in(&b5), - &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7].into_iter()) + &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7, one].into_iter()) ); assert_eq!( liveness.get_live_in(&b7), - &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7].into_iter()) + &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7, one].into_iter()) ); assert_eq!( liveness.get_live_in(&b8), - &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7].into_iter()) + &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7, one].into_iter()) ); let block_3 = &func.dfg[b3]; @@ -621,7 +643,9 @@ mod test { let ssa = builder.finish(); let func = ssa.main(); - let liveness = VariableLiveness::from_function(func); + + let constants = ConstantAllocation::from_function(func); + let liveness = VariableLiveness::from_function(func, &constants); // Entry point defines its own params and also b3's params. assert_eq!(liveness.defined_block_params(&func.entry_block()), vec![v0, v1, v2]); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dom.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dom.rs index 15fa3bad38d..94f7a405c05 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dom.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dom.rs @@ -214,7 +214,7 @@ impl DominatorTree { /// Compute the common dominator of two basic blocks. /// /// Both basic blocks are assumed to be reachable. - fn common_dominator( + pub(crate) fn common_dominator( &self, mut block_a_id: BasicBlockId, mut block_b_id: BasicBlockId,