From dcb55dffbb59849ae7e0472df3d9dd560667c578 Mon Sep 17 00:00:00 2001 From: jfecher Date: Fri, 25 Aug 2023 13:25:45 -0500 Subject: [PATCH 01/17] fix: Implement new mem2reg pass (#2420) Co-authored-by: Maxim Vezenov --- .../references_aliasing/Nargo.toml | 7 +++++++ .../references_aliasing/Prover.toml | 0 .../references_aliasing/src/main.nr | 10 ++++++++++ .../target/references.bytecode | 1 + .../references_aliasing/target/witness.tr | Bin 0 -> 788 bytes 5 files changed, 18 insertions(+) create mode 100644 crates/nargo_cli/tests/execution_success/references_aliasing/Nargo.toml create mode 100644 crates/nargo_cli/tests/execution_success/references_aliasing/Prover.toml create mode 100644 crates/nargo_cli/tests/execution_success/references_aliasing/src/main.nr create mode 100644 crates/nargo_cli/tests/execution_success/references_aliasing/target/references.bytecode create mode 100644 crates/nargo_cli/tests/execution_success/references_aliasing/target/witness.tr diff --git a/crates/nargo_cli/tests/execution_success/references_aliasing/Nargo.toml b/crates/nargo_cli/tests/execution_success/references_aliasing/Nargo.toml new file mode 100644 index 00000000000..b52fdcf77f0 --- /dev/null +++ b/crates/nargo_cli/tests/execution_success/references_aliasing/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "references" +type = "bin" +authors = [""] +compiler_version = "0.5.1" + +[dependencies] diff --git a/crates/nargo_cli/tests/execution_success/references_aliasing/Prover.toml b/crates/nargo_cli/tests/execution_success/references_aliasing/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/crates/nargo_cli/tests/execution_success/references_aliasing/src/main.nr b/crates/nargo_cli/tests/execution_success/references_aliasing/src/main.nr new file mode 100644 index 00000000000..4582444c8f7 --- /dev/null +++ b/crates/nargo_cli/tests/execution_success/references_aliasing/src/main.nr @@ -0,0 +1,10 @@ +fn increment(mut r: &mut Field) { + *r = *r + 1; +} + +fn main() { + let mut x = 100; + let mut xref = &mut x; + increment(xref); + assert(*xref == 101); +} diff --git a/crates/nargo_cli/tests/execution_success/references_aliasing/target/references.bytecode b/crates/nargo_cli/tests/execution_success/references_aliasing/target/references.bytecode new file mode 100644 index 00000000000..3e521923327 --- /dev/null +++ b/crates/nargo_cli/tests/execution_success/references_aliasing/target/references.bytecode @@ -0,0 +1 @@ +H4sIAAAAAAAA/+1c8ZONVRh+WCKykYqIiFRE37f37u5dkY2IEEJEtLv2bkJKQkpCSkJKIjLGNE3TmJqpmZqpmZrRH9L/Uu907szZzfjlPO+Z98yc88u+w8yz3/O853ue93zf3fsXgL/x/zXY/ex0P4uwVQ7ysCpFW7Vab2+pl5Wyu2jp6Km1FtXWnrZaWStba629LbVKpV6r1to7ejrai46yWqmXfa0dlT4HNpiAVe/7bzX9izHoJvwHkfkP5mEV/vU2efWQAb3z//82BU4Y8HsG6th8k3+j/nKNJjUp4A4Bb/Nr8R7C71HhQZrXtLEsG99QpGd8Q6FjfLd5dTa+QMyhSkINg23jE97D+D1SNT62po1l2fiGIz3jGw4d47vdq7PxBWIOd4KycUfAtvEJ7xH8HqkaH1vTxrJsfCORnvGNhI7x3eHV2fgCMUc6Qdm4o2Db+IT3KH6PVI2PrWljWTa+ZtCMrxbL+JqhY3x3enU2vkDMZicoG3c0bBuf8B7N71EyhjIGLEOp12MZyhjoGMpdXp0NJRBzjBOUjTsWtg1FeI/l9+iWhhKqA9Ok74bOTcHmzNxH9yTCmdnneyNxLsJWL7PP43jX1SIYbO+Rnoy7CW4o76vQ8bEmKv+yytzf44l9uQrWkBXvcRWRf78h6z6vzkNWIOZ4JygbdwJsD1nCewK/R6qPq9iaNhZ7SGCeLifS9CsrsYyPd839je9+r87GF4g50QnKxp0E28YnvCfxe6T5uKqVqelkIudYhsK8Zv96H/DqbCiBmJOdoGzcKbBtKMJ7Cr9HqpPUFMSZpIqwRX1OP5WAFfsIORU6xvegV2fjC8Sc6gRl406DbeMT3tP4PUrGUKaDZSjxXvxNh46hPOTV2VACMac7Qdm4M2DbUIT3DH6PVJ/1ME36YejcFGzOzH30SCKcmX1+NBLnImz1Mvs8k3ddKi/+pCczwX/xdw06PsZ+8cfc37OIfbkG2pDVG2vIIvLvN2Q95tV5yArEnOUEZePOhu0hS3jP5vdI9XEVW9PGsny6nAOW8cX7nPoc6Bjf416djS8Qc44TlI1bwLbxCe+C3yNFQylbmJqWRM6xDIV5zf71tnh1NpRAzNIJysatwLahCO8Kv0eqkxRb08ayPElVkd4kVYWO8bV6dTa+QMyqE5SN2wbbxie82/g9SsZQ2pGeobRDx1BqXp0NJRCz3QnKxu2AbUMR3h38HiVjKHORnqHMhY6hPOHV2VACMec6Qdm482DbUIT3PH6PkjGU+UjPUOZDx1Ce9OpsKIGY852gbNwFsG0ownsBv0fJGEon0jOUTugYylNenQ0lELPTCcrGXQjbhiI4C/k9SsZQFiE9Q1kEHUN52quzoQRiLnKCsnEXw7ahCO/F/B4lYyhLwDKUeH+NsQQ6hvKMV2dDCcRc4gRl4y6FbUMR3kv5PUrGUJYhPUNZBh1Dedars6EEYi5zgrJxl8O2oQjv5fweqf55F9OkV/A4t9yKcxG2qCa6EumZ6EromOhzXp1NNBBzpROUjbsKtk1UeK/i90jlWsXsV4D/N3XfQCc8mkjX2fiWEGYgryb2hahfMkG0BukF0RroBNHzXp2DKBBzjROUjbsWtoNIeK/l90jlWiUwV4MfRN8ijSBiDjXriH0h6pdMEK1HekG0HjpB9IJX5yAKxFzvBGXjboDtIBLeG/g9UrlWCcx14AfRd0gjiJhDzUZiX4j63TKIQjkzX0K/CNt+Jvf0RoV75Xukca8wfXcTsS9E/VS+yUn29SaFfXM90r4pglZZZXrEZmJfrhPvjViDL5F/v8H3Ja/Og28g5mYnKBt3C2wPvsJ7C79Hqt8/wNa0sSyf+LeCZnzRPuq6FTrG97JXZ+MLxNzqBGXjdsG28QnvLn6PkjGUbqRnKN3QMZQer86GEojZ7QRl426DbUMR3tv4PVL9ZFoXEasXOjcFmzNzH9UjcS7CFjU4+sAKjnjvnvqgExyveHUOjkDMPicoG3c7bAeH8N7O75FqcDBN9FWkERzMfbSDx7nfuyLLz4F3Evus9RzdcvDuAit4453YdkEneF/z6hy8gZi7nKBs3N2wHbzCeze/R8mc2F5HGsHL3EdvROJchC1qcOwBKzjindj2QCc43vTqHByBmHucoGzcvbAdHMJ7L79HqsHBNNG3kEZwMPfRPh5nlU8syYl8J/ifWPoB3P3N5i1PD3Yo8P4ROvd1E/k69xO1JPa61NLP8qB1AKxBK94J/QB0Bq23vToPWoGYB5ygbNyDsD1oCe+D/B4lc0J/Bzo3BZszcx+9G4lzEbaowXEIrOCId0I/BJ3geM+rc3AEYh5ygrJxD8N2cAjvw/weqQYH00TfRxrBwdxHR3icVU7o8gRmP/gn1Z/A3d9s3vK0aJ8C75+hc1+zT+hHiVoSe11q6Wd50DoG1qAV74R+DDqD1gdenQetQMxjTlA27nHYHrSE93F+j5I5oX8InZuCzZm5jz6KxLkIW9TgOAFWcMQ7oZ+ATnB87NU5OAIxTzhB2bgnYTs4hPdJfo9Ug4Npop8gjeBg7qNTPM4qJ3R5AnMU/JPqL+DubzZveVp0RIH3r9C5r9kn9NNELYm9LrX0szxonQFr0Ip3Qj8DnUHrU6/Og1Yg5hknKBv3LGwPWsL7LL9HyZzQP4POTcHmzNxHn0fiXIQtanCcAys44p3Qz0EnOL7w6hwcgZjnnKBs3POwHRzC+zy/R6rBwTTRL5FGcDD30QUeZ5UTujyBOQ3+SfU3cPc3m7c8LTqlwPt36NzX7BP6RaKWxF6XWvpZHqi/gu28lgy4qHCv/AHbHiF5dUGB959IwyMuEbUk9rpk68feN3I/X1LYNzcQZ98UQausdhE5Xyb25QbhumJ/1zWRf78D7ddenQ+0gZiXnaBs3CuwfaAV3lf4PVL9rmumpoMGXKO//gGokXOuaucAAA== diff --git a/crates/nargo_cli/tests/execution_success/references_aliasing/target/witness.tr b/crates/nargo_cli/tests/execution_success/references_aliasing/target/witness.tr new file mode 100644 index 0000000000000000000000000000000000000000..22a1c7fe1098dfe81652877dc26d99dc3aae67ec GIT binary patch literal 788 zcmV+v1MB=BiwFP!00002|E-o)&l5oyg>iRxciq{U9YH_b-JPB84DRmk?ixY}A%p;d z1QJLf;iu7laMAnk*|te9nkVO+_dV13T<_mgzx=8B|2?VOlQ!U3Nkfp;h+~Pypspq$ zt10NQ8K|o{$Z7$4YzgXW1+rR$9@|v0WTKS{CJjQ0JV~9mS)wwpt*|1qS&>>PgH_fz z?@Z!^l0n4G$q;LwyloXLbtbnZof;E_R$eFGr&fjXubniBiiOLBNGbp+qz}2w)2LO+ z#8kQRTebu9YR`F0I#jVlR55C+o$%VHp(unavRv7a>$H@3KXx_(IY`!qd?YZF#9p!{c9|E|4P8T1jtH3mIUXhz&U2%95tAi z0Xj0p_(5%xe{x*J?1Y zHQ+sZEvRc9nAducwE@g)BX|ec1l|EQgL&nkM-O@|fgTHRCj!Wd;90l@JPWsi*>3~$ z+76yyJ3!V>utU4R4($f#xCfl$UXZm9?9hI&LkGa@4}y6e0{8wf$T|Xc=qT8sW8fT* zgL6CqvQB~>It6y}^6cIW}vp@(4hkHEYhgM0r3WIY8vJ_B_<2j}<# zWW5CQdIje78ua)EWW5Dh??Bdjko5ta<3~`}Cs5ZHP}f&b*EdktcTm?4P}fgT*Dp}l SZ&24Cko6Z+MM~zdGXMaV18<%H literal 0 HcmV?d00001 From c4a14f2982a227f8f737cb320253542ddce75371 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 25 Aug 2023 13:35:27 -0500 Subject: [PATCH 02/17] Support aliasing arrays in mem2reg --- .../src/brillig/brillig_gen/brillig_block.rs | 6 +- crates/noirc_evaluator/src/ssa/ir/dfg.rs | 10 +- crates/noirc_evaluator/src/ssa/ir/value.rs | 16 +-- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 123 ++++++++++++++++-- 4 files changed, 134 insertions(+), 21 deletions(-) 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 831ad3d5d2a..c50019dd622 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1081,7 +1081,7 @@ impl<'block> BrilligBlock<'block> { /// Returns the type of the operation considering the types of the operands /// TODO: SSA issues binary operations between fields and integers. /// This probably should be explicitly casted in SSA to avoid having to coerce at this level. -pub(crate) fn type_of_binary_operation(lhs_type: Type, rhs_type: Type) -> Type { +pub(crate) fn type_of_binary_operation(lhs_type: &Type, rhs_type: &Type) -> Type { match (lhs_type, rhs_type) { (_, Type::Function) | (Type::Function, _) => { unreachable!("Functions are invalid in binary operations") @@ -1098,7 +1098,7 @@ pub(crate) fn type_of_binary_operation(lhs_type: Type, rhs_type: Type) -> Type { // If either side is a Field constant then, we coerce into the type // of the other operand (Type::Numeric(NumericType::NativeField), typ) - | (typ, Type::Numeric(NumericType::NativeField)) => typ, + | (typ, Type::Numeric(NumericType::NativeField)) => typ.clone(), // If both sides are numeric type, then we expect their types to be // the same. (Type::Numeric(lhs_type), Type::Numeric(rhs_type)) => { @@ -1106,7 +1106,7 @@ pub(crate) fn type_of_binary_operation(lhs_type: Type, rhs_type: Type) -> Type { lhs_type, rhs_type, "lhs and rhs types in a binary operation are always the same" ); - Type::Numeric(lhs_type) + Type::Numeric(lhs_type.clone()) } } } diff --git a/crates/noirc_evaluator/src/ssa/ir/dfg.rs b/crates/noirc_evaluator/src/ssa/ir/dfg.rs index 7dcf652c6e6..a9523afb2a4 100644 --- a/crates/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/crates/noirc_evaluator/src/ssa/ir/dfg.rs @@ -106,7 +106,7 @@ impl DataFlowGraph { let parameters = self.blocks[block].parameters(); let parameters = vecmap(parameters.iter().enumerate(), |(position, param)| { - let typ = self.values[*param].get_type(); + let typ = self.values[*param].get_type().clone(); self.values.insert(Value::Param { block: new_block, position, typ }) }); @@ -314,7 +314,13 @@ impl DataFlowGraph { /// Returns the type of a given value pub(crate) fn type_of_value(&self, value: ValueId) -> Type { - self.values[value].get_type() + self.values[value].get_type().clone() + } + + /// True if the type of this value is Type::Reference. + /// Using this method over type_of_value avoids cloning the value's type. + pub(crate) fn value_is_reference(&self, value: ValueId) -> bool { + matches!(self.values[value].get_type(), Type::Reference) } /// Appends a result type to the instruction. diff --git a/crates/noirc_evaluator/src/ssa/ir/value.rs b/crates/noirc_evaluator/src/ssa/ir/value.rs index 54831eb4a07..8c2a4e88ef3 100644 --- a/crates/noirc_evaluator/src/ssa/ir/value.rs +++ b/crates/noirc_evaluator/src/ssa/ir/value.rs @@ -57,15 +57,15 @@ pub(crate) enum Value { impl Value { /// Retrieves the type of this Value - pub(crate) fn get_type(&self) -> Type { + pub(crate) fn get_type(&self) -> &Type { match self { - Value::Instruction { typ, .. } => typ.clone(), - Value::Param { typ, .. } => typ.clone(), - Value::NumericConstant { typ, .. } => typ.clone(), - Value::Array { typ, .. } => typ.clone(), - Value::Function { .. } => Type::Function, - Value::Intrinsic { .. } => Type::Function, - Value::ForeignFunction { .. } => Type::Function, + Value::Instruction { typ, .. } => typ, + Value::Param { typ, .. } => typ, + Value::NumericConstant { typ, .. } => typ, + Value::Array { typ, .. } => typ, + Value::Function { .. } => &Type::Function, + Value::Intrinsic { .. } => &Type::Function, + Value::ForeignFunction { .. } => &Type::Function, } } } diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 3e63aec63b4..e2bae6de300 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -129,6 +129,7 @@ struct Block { #[derive(Debug, Clone, PartialOrd, Ord, PartialEq, Eq)] enum Expression { Dereference(Box), + ArrayElement(Box), Other(ValueId), } @@ -230,7 +231,7 @@ impl PerFunctionContext { let reference_parameters = function .parameters() .iter() - .filter(|param| function.dfg.type_of_value(**param) == Type::Reference) + .filter(|param| function.dfg.value_is_reference(**param)) .collect::>(); for (allocation, instruction) in last_stores { @@ -243,8 +244,6 @@ impl PerFunctionContext { self.instructions_to_remove.insert(instruction); } } - } else { - self.instructions_to_remove.insert(instruction); } } } @@ -270,13 +269,15 @@ impl PerFunctionContext { self.instructions_to_remove.insert(instruction); } else { - last_stores.remove(&address); + references.mark_value_used(address, function, last_stores); } } Instruction::Store { address, value } => { let address = function.dfg.resolve(*address); let value = function.dfg.resolve(*value); + self.check_array_aliasing(function, references, value); + // If there was another store to this instruction without any (unremoved) loads or // function calls in-between, we can remove the previous store. if let Some(last_store) = last_stores.get(&address) { @@ -298,10 +299,79 @@ impl PerFunctionContext { aliases.insert(result); references.aliases.insert(Expression::Other(result), aliases); } + Instruction::ArrayGet { array, .. } => { + let result = function.dfg.instruction_results(instruction)[0]; + references.mark_value_used(*array, function, last_stores); + + if function.dfg.value_is_reference(result) { + let array = 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); + } + } + } + Instruction::ArraySet { array, value, .. } => { + references.mark_value_used(*array, function, last_stores); + + if function.dfg.value_is_reference(*value) { + let result = function.dfg.instruction_results(instruction)[0]; + let array = 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); + } else if let Some((elements, _)) = 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); + } + } + + references.expressions.insert(result, expression); + } + } _ => (), } } + fn check_array_aliasing(&self, function: &Function, references: &mut Block, array: ValueId) { + if let Some((elements, typ)) = function.dfg.get_array_constant(array) { + if Self::contains_references(&typ) { + // TODO: Check if type directly holds references or holds arrays that hold references + let expr = Expression::ArrayElement(Box::new(Expression::Other(array))); + references.expressions.insert(array, expr.clone()); + let aliases = references.aliases.entry(expr).or_default(); + + for element in elements { + aliases.insert(element); + } + } + } + } + + fn contains_references(typ: &Type) -> bool { + match typ { + Type::Numeric(_) => false, + Type::Function => false, + Type::Reference => true, + Type::Array(elements, _) | Type::Slice(elements) => { + elements.iter().any(|element| Self::contains_references(element)) + } + } + } + + 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 mark_all_unknown( &self, values: &[ValueId], @@ -310,10 +380,10 @@ impl PerFunctionContext { last_stores: &mut BTreeMap, ) { for value in values { - if function.dfg.type_of_value(*value) == Type::Reference { + if function.dfg.value_is_reference(*value) { let value = function.dfg.resolve(*value); references.set_unknown(value, last_stores); - last_stores.remove(&value); + references.mark_value_used(value, function, last_stores); } } } @@ -345,7 +415,7 @@ impl PerFunctionContext { // Add an alias for each reference parameter for (parameter, argument) in destination_parameters.iter().zip(arguments) { - if function.dfg.type_of_value(*parameter) == Type::Reference { + if function.dfg.value_is_reference(*parameter) { let argument = function.dfg.resolve(*argument); if let Some(expression) = references.expressions.get(&argument) { @@ -477,7 +547,7 @@ impl Block { /// 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.type_of_value(result) == Type::Reference { + 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 { @@ -489,6 +559,43 @@ impl Block { } } } + + /// 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(ValueId) -> T) { + if let Some(expr) = self.expressions.get(&address) { + if let Some(aliases) = self.aliases.get(expr) { + for alias in aliases { + f(*alias); + } + } + } + } + + fn keep_last_stores_for( + &mut self, + address: ValueId, + last_stores: &mut BTreeMap, + ) { + last_stores.remove(&address); + self.for_each_alias_of(address, |alias| last_stores.remove(&alias)); + } + + fn mark_value_used( + &mut self, + value: ValueId, + function: &Function, + last_stores: &mut BTreeMap, + ) { + self.keep_last_stores_for(value, last_stores); + + // 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, last_stores); + } + } + } } impl ReferenceValue { From 5467e56475153bfb0707b53fadad1252c87b33c7 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 25 Aug 2023 14:10:25 -0500 Subject: [PATCH 03/17] Move last_stores in Block struct --- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 115 ++++++++++-------- 1 file changed, 62 insertions(+), 53 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index e2bae6de300..9682081a0f2 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -121,6 +121,9 @@ struct Block { /// 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 @@ -202,19 +205,18 @@ impl PerFunctionContext { mut references: Block, ) { let instructions = function.dfg[block].instructions().to_vec(); - let mut last_stores = BTreeMap::new(); for instruction in instructions { - self.analyze_instruction(function, &mut references, instruction, &mut last_stores); + self.analyze_instruction(function, &mut references, instruction); } - self.handle_terminator(function, block, &mut references, &mut last_stores); + self.handle_terminator(function, block, &mut references); // If there's only 1 block in the function total, we can remove any remaining last stores // as well. We can't do this if there are multiple blocks since subsequent blocks may // reference these stores. if self.post_order.as_slice().len() == 1 { - self.remove_stores_that_do_not_alias_parameters(function, &references, last_stores); + self.remove_stores_that_do_not_alias_parameters(function, &references); } self.blocks.insert(block, references); @@ -226,7 +228,6 @@ impl PerFunctionContext { &mut self, function: &Function, references: &Block, - last_stores: BTreeMap, ) { let reference_parameters = function .parameters() @@ -234,14 +235,14 @@ impl PerFunctionContext { .filter(|param| function.dfg.value_is_reference(**param)) .collect::>(); - for (allocation, instruction) in last_stores { + for (allocation, instruction) in &references.last_stores { 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)); if !aliases.is_empty() && !allocation_aliases_parameter { - self.instructions_to_remove.insert(instruction); + self.instructions_to_remove.insert(*instruction); } } } @@ -253,7 +254,6 @@ impl PerFunctionContext { function: &mut Function, references: &mut Block, instruction: InstructionId, - last_stores: &mut BTreeMap, ) { match &function.dfg[instruction] { Instruction::Load { address } => { @@ -269,7 +269,7 @@ impl PerFunctionContext { self.instructions_to_remove.insert(instruction); } else { - references.mark_value_used(address, function, last_stores); + references.mark_value_used(address, function); } } Instruction::Store { address, value } => { @@ -280,15 +280,15 @@ impl PerFunctionContext { // If there was another store to this instruction without any (unremoved) loads or // function calls in-between, we can remove the previous store. - if let Some(last_store) = last_stores.get(&address) { + if let Some(last_store) = references.last_stores.get(&address) { self.instructions_to_remove.insert(*last_store); } - references.set_known_value(address, value, last_stores); - last_stores.insert(address, instruction); + references.set_known_value(address, value); + references.last_stores.insert(address, instruction); } Instruction::Call { arguments, .. } => { - self.mark_all_unknown(arguments, function, references, last_stores); + self.mark_all_unknown(arguments, function, references); } Instruction::Allocate => { // Register the new reference @@ -301,7 +301,7 @@ impl PerFunctionContext { } Instruction::ArrayGet { array, .. } => { let result = function.dfg.instruction_results(instruction)[0]; - references.mark_value_used(*array, function, last_stores); + references.mark_value_used(*array, function); if function.dfg.value_is_reference(result) { let array = function.dfg.resolve(*array); @@ -313,7 +313,7 @@ impl PerFunctionContext { } } Instruction::ArraySet { array, value, .. } => { - references.mark_value_used(*array, function, last_stores); + references.mark_value_used(*array, function); if function.dfg.value_is_reference(*value) { let result = function.dfg.instruction_results(instruction)[0]; @@ -377,13 +377,12 @@ impl PerFunctionContext { values: &[ValueId], function: &Function, references: &mut Block, - last_stores: &mut BTreeMap, ) { for value in values { if function.dfg.value_is_reference(*value) { let value = function.dfg.resolve(*value); - references.set_unknown(value, last_stores); - references.mark_value_used(value, function, last_stores); + references.set_unknown(value); + references.mark_value_used(value, function); } } } @@ -405,7 +404,6 @@ impl PerFunctionContext { function: &mut Function, block: BasicBlockId, references: &mut Block, - last_stores: &mut BTreeMap, ) { match function.dfg[block].unwrap_terminator() { TerminatorInstruction::JmpIf { .. } => (), // Nothing to do @@ -431,7 +429,7 @@ impl PerFunctionContext { // Removing all `last_stores` for each returned reference is more important here // than setting them all to ReferenceValue::Unknown since no other block should // have a block with a Return terminator as a predecessor anyway. - self.mark_all_unknown(return_values, function, references, last_stores); + self.mark_all_unknown(return_values, function, references); } } } @@ -461,53 +459,49 @@ impl Block { &mut self, address: ValueId, value: ValueId, - last_stores: &mut BTreeMap, ) { - self.set_value(address, ReferenceValue::Known(value), last_stores); + self.set_value(address, ReferenceValue::Known(value)); } fn set_unknown( &mut self, address: ValueId, - last_stores: &mut BTreeMap, ) { - self.set_value(address, ReferenceValue::Unknown, last_stores); + self.set_value(address, ReferenceValue::Unknown); } fn set_value( &mut self, address: ValueId, value: ReferenceValue, - last_stores: &mut BTreeMap, ) { - if let Some(expression) = self.expressions.get(&address) { - if let Some(aliases) = self.aliases.get(expression) { - 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(last_stores); - } 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; - } - } + 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, last_stores: &mut BTreeMap) { + fn invalidate_all_references(&mut self) { for reference_value in self.references.values_mut() { *reference_value = ReferenceValue::Unknown; } - last_stores.clear(); + self.last_stores.clear(); } fn unify(mut self, other: &Self) -> Self { @@ -561,11 +555,11 @@ impl Block { } /// 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(ValueId) -> T) { + 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) { + if let Some(aliases) = self.aliases.get(expr).cloned() { for alias in aliases { - f(*alias); + f(self, alias); } } } @@ -574,25 +568,40 @@ impl Block { fn keep_last_stores_for( &mut self, address: ValueId, - last_stores: &mut BTreeMap, + function: &Function, ) { - last_stores.remove(&address); - self.for_each_alias_of(address, |alias| last_stores.remove(&alias)); + 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, - last_stores: &mut BTreeMap, ) { - self.keep_last_stores_for(value, last_stores); + 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, last_stores); + self.mark_value_used(value, function); } } } From f7e937462f2d4b9fb9ee2d775bf7e90e12928fb0 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 25 Aug 2023 14:28:07 -0500 Subject: [PATCH 04/17] Satisfy clippy --- .../noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs | 2 +- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) 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 c50019dd622..567c0510588 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1106,7 +1106,7 @@ pub(crate) fn type_of_binary_operation(lhs_type: &Type, rhs_type: &Type) -> Type lhs_type, rhs_type, "lhs and rhs types in a binary operation are always the same" ); - Type::Numeric(lhs_type.clone()) + Type::Numeric(*lhs_type) } } } diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 9682081a0f2..878c249d09a 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -236,7 +236,7 @@ impl PerFunctionContext { .collect::>(); for (allocation, instruction) in &references.last_stores { - if let Some(expression) = references.expressions.get(&allocation) { + 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)); @@ -359,7 +359,7 @@ impl PerFunctionContext { Type::Function => false, Type::Reference => true, Type::Array(elements, _) | Type::Slice(elements) => { - elements.iter().any(|element| Self::contains_references(element)) + elements.iter().any(Self::contains_references) } } } From 58f1187d9ca298f762bbf8cd5a82985065246b62 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 28 Aug 2023 13:54:08 -0500 Subject: [PATCH 05/17] Constant fold during mem2reg pass --- crates/noirc_evaluator/src/ssa/ir/dfg.rs | 11 +- .../src/ssa/ir/function_inserter.rs | 26 +++- .../noirc_evaluator/src/ssa/ir/instruction.rs | 22 ++- .../src/ssa/opt/constant_folding.rs | 2 +- .../noirc_evaluator/src/ssa/opt/inlining.rs | 4 +- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 145 +++++++++--------- 6 files changed, 124 insertions(+), 86 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/ir/dfg.rs b/crates/noirc_evaluator/src/ssa/ir/dfg.rs index a9523afb2a4..ea00ad3bbc1 100644 --- a/crates/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/crates/noirc_evaluator/src/ssa/ir/dfg.rs @@ -171,7 +171,7 @@ impl DataFlowGraph { let id = self.make_instruction(instruction, ctrl_typevars); self.blocks[block].insert_instruction(id); self.locations.insert(id, call_stack); - InsertInstructionResult::Results(self.instruction_results(id)) + InsertInstructionResult::Results(id, self.instruction_results(id)) } } } @@ -478,7 +478,8 @@ impl std::ops::IndexMut for DataFlowGraph { // be a list of results or a single ValueId if the instruction was simplified // to an existing value. pub(crate) enum InsertInstructionResult<'dfg> { - Results(&'dfg [ValueId]), + /// Results is the standard case containing the instruction id and the results of that instruction. + Results(InstructionId, &'dfg [ValueId]), SimplifiedTo(ValueId), SimplifiedToMultiple(Vec), InstructionRemoved, @@ -490,7 +491,7 @@ impl<'dfg> InsertInstructionResult<'dfg> { match self { InsertInstructionResult::SimplifiedTo(value) => *value, InsertInstructionResult::SimplifiedToMultiple(values) => values[0], - InsertInstructionResult::Results(results) => results[0], + InsertInstructionResult::Results(_, results) => results[0], InsertInstructionResult::InstructionRemoved => { panic!("Instruction was removed, no results") } @@ -501,7 +502,7 @@ impl<'dfg> InsertInstructionResult<'dfg> { /// This is used for instructions returning multiple results like function calls. pub(crate) fn results(self) -> Cow<'dfg, [ValueId]> { match self { - InsertInstructionResult::Results(results) => Cow::Borrowed(results), + InsertInstructionResult::Results(_, results) => Cow::Borrowed(results), InsertInstructionResult::SimplifiedTo(result) => Cow::Owned(vec![result]), InsertInstructionResult::SimplifiedToMultiple(results) => Cow::Owned(results), InsertInstructionResult::InstructionRemoved => Cow::Owned(vec![]), @@ -513,7 +514,7 @@ impl<'dfg> InsertInstructionResult<'dfg> { match self { InsertInstructionResult::SimplifiedTo(_) => 1, InsertInstructionResult::SimplifiedToMultiple(results) => results.len(), - InsertInstructionResult::Results(results) => results.len(), + InsertInstructionResult::Results(_, results) => results.len(), InsertInstructionResult::InstructionRemoved => 0, } } diff --git a/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs b/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs index bc0084b6d4e..43d3917700d 100644 --- a/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs +++ b/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs @@ -30,7 +30,7 @@ impl<'f> FunctionInserter<'f> { pub(crate) fn resolve(&mut self, mut value: ValueId) -> ValueId { value = self.function.dfg.resolve(value); match self.values.get(&value) { - Some(value) => *value, + Some(value) => self.resolve(*value), None => match &self.function.dfg[value] { super::value::Value::Array { array, typ } => { let array = array.clone(); @@ -62,9 +62,27 @@ impl<'f> FunctionInserter<'f> { ) } - pub(crate) fn push_instruction(&mut self, id: InstructionId, block: BasicBlockId) { + /// Maps a terminator in place, replacing any ValueId in the terminator with the + /// resolved version of that value id from this FunctionInserter's internal value mapping. + pub(crate) fn map_terminator_in_place(&mut self, block: BasicBlockId) { + let mut terminator = self.function.dfg[block].take_terminator(); + terminator.mutate_values(|value| self.resolve(value)); + self.function.dfg[block].set_terminator(terminator); + } + + /// Push a new instruction to the given block and return its new InstructionId. + /// If the instruction was simplified out of the program, None is returned. + pub(crate) fn push_instruction( + &mut self, + id: InstructionId, + block: BasicBlockId, + ) -> Option { let (instruction, location) = self.map_instruction(id); - self.push_instruction_value(instruction, id, block, location); + + match self.push_instruction_value(instruction, id, block, location) { + InsertInstructionResult::Results(new_id, _) => Some(new_id), + _ => None, + } } pub(crate) fn push_instruction_value( @@ -110,7 +128,7 @@ impl<'f> FunctionInserter<'f> { values.insert(*old_result, *new_result); } } - InsertInstructionResult::Results(new_results) => { + InsertInstructionResult::Results(_, new_results) => { for (old_result, new_result) in old_results.iter().zip(*new_results) { values.insert(*old_result, *new_result); } diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction.rs b/crates/noirc_evaluator/src/ssa/ir/instruction.rs index 6b68b0f85a4..4826e373de1 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction.rs @@ -517,6 +517,26 @@ impl TerminatorInstruction { } } + /// Mutate each ValueId to a new ValueId using the given mapping function + pub(crate) fn mutate_values(&mut self, mut f: impl FnMut(ValueId) -> ValueId) { + use TerminatorInstruction::*; + match self { + JmpIf { condition, .. } => { + *condition = f(*condition); + } + Jmp { arguments, .. } => { + for argument in arguments { + *argument = f(*argument); + } + } + Return { return_values } => { + for return_value in return_values { + *return_value = f(*return_value); + } + } + } + } + /// Apply a function to each value pub(crate) fn for_each_value(&self, mut f: impl FnMut(ValueId) -> T) { use TerminatorInstruction::*; @@ -837,7 +857,7 @@ pub(crate) enum SimplifyResult { /// a function such as a tuple SimplifiedToMultiple(Vec), - /// Replace this function with an simpler but equivalent function. + /// Replace this function with an simpler but equivalent instruction. SimplifiedToInstruction(Instruction), /// Remove the instruction, it is unnecessary diff --git a/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs b/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs index e53bfac8c03..3159e360799 100644 --- a/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -95,7 +95,7 @@ impl Context { ) { InsertInstructionResult::SimplifiedTo(new_result) => vec![new_result], InsertInstructionResult::SimplifiedToMultiple(new_results) => new_results, - InsertInstructionResult::Results(new_results) => new_results.to_vec(), + InsertInstructionResult::Results(_, new_results) => new_results.to_vec(), InsertInstructionResult::InstructionRemoved => vec![], }; assert_eq!(old_results.len(), new_results.len()); diff --git a/crates/noirc_evaluator/src/ssa/opt/inlining.rs b/crates/noirc_evaluator/src/ssa/opt/inlining.rs index 55424f8f32f..ba247631e61 100644 --- a/crates/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/crates/noirc_evaluator/src/ssa/opt/inlining.rs @@ -392,7 +392,7 @@ impl<'function> PerFunctionContext<'function> { self.context.call_stack.pop_back(); } - let new_results = InsertInstructionResult::Results(&new_results); + let new_results = InsertInstructionResult::Results(call_id, &new_results); Self::insert_new_instruction_results(&mut self.values, old_results, new_results); } @@ -435,7 +435,7 @@ impl<'function> PerFunctionContext<'function> { values.insert(*old_result, new_result); } } - InsertInstructionResult::Results(new_results) => { + InsertInstructionResult::Results(_, new_results) => { for (old_result, new_result) in old_results.iter().zip(new_results) { values.insert(*old_result, *new_result); } diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 878c249d09a..1ad505e7fa0 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -69,6 +69,7 @@ use crate::ssa::{ cfg::ControlFlowGraph, dom::DominatorTree, function::Function, + function_inserter::FunctionInserter, instruction::{Instruction, InstructionId, TerminatorInstruction}, post_order::PostOrder, types::Type, @@ -83,20 +84,22 @@ impl Ssa { pub(crate) fn mem2reg(mut self) -> Ssa { for function in self.functions.values_mut() { let mut context = PerFunctionContext::new(function); - context.mem2reg(function); - context.remove_instructions(function); + context.mem2reg(); + context.remove_instructions(); } self } } -struct PerFunctionContext { +struct PerFunctionContext<'f> { cfg: ControlFlowGraph, post_order: PostOrder, dom_tree: DominatorTree, blocks: BTreeMap, + inserter: FunctionInserter<'f>, + /// Load and Store instructions that should be removed at the end of the pass. /// /// We avoid removing individual instructions as we go since removing elements @@ -143,8 +146,8 @@ enum ReferenceValue { Known(ValueId), } -impl PerFunctionContext { - fn new(function: &Function) -> Self { +impl<'f> PerFunctionContext<'f> { + fn new(function: &'f mut Function) -> Self { let cfg = ControlFlowGraph::with_function(function); let post_order = PostOrder::with_function(function); let dom_tree = DominatorTree::with_cfg_and_post_order(&cfg, &post_order); @@ -153,6 +156,7 @@ impl PerFunctionContext { cfg, post_order, dom_tree, + inserter: FunctionInserter::new(function), blocks: BTreeMap::new(), instructions_to_remove: BTreeSet::new(), } @@ -162,14 +166,14 @@ impl PerFunctionContext { /// /// This function is expected to be the same one that the internal cfg, post_order, and /// dom_tree were created from. - fn mem2reg(&mut self, function: &mut Function) { + fn mem2reg(&mut self) { // Iterate each block in reverse post order = forward order - let mut block_order = PostOrder::with_function(function).into_vec(); + let mut block_order = PostOrder::with_function(self.inserter.function).into_vec(); block_order.reverse(); for block in block_order { let references = self.find_starting_references(block); - self.analyze_block(function, block, references); + self.analyze_block(block, references); } } @@ -198,25 +202,20 @@ impl PerFunctionContext { /// This will remove any known loads in the block and track the value of references /// as they are stored to. When this function is finished, the value of each reference /// at the end of this block will be remembered in `self.blocks`. - fn analyze_block( - &mut self, - function: &mut Function, - block: BasicBlockId, - mut references: Block, - ) { - let instructions = function.dfg[block].instructions().to_vec(); + fn analyze_block(&mut self, block: BasicBlockId, mut references: Block) { + let instructions = self.inserter.function.dfg[block].take_instructions(); for instruction in instructions { - self.analyze_instruction(function, &mut references, instruction); + self.analyze_instruction(block, &mut references, instruction); } - self.handle_terminator(function, block, &mut references); + self.handle_terminator(block, &mut references); // If there's only 1 block in the function total, we can remove any remaining last stores // as well. We can't do this if there are multiple blocks since subsequent blocks may // reference these stores. if self.post_order.as_slice().len() == 1 { - self.remove_stores_that_do_not_alias_parameters(function, &references); + self.remove_stores_that_do_not_alias_parameters(&references); } self.blocks.insert(block, references); @@ -224,15 +223,13 @@ impl PerFunctionContext { /// Add all instructions in `last_stores` to `self.instructions_to_remove` which do not /// possibly alias any parameters of the given function. - fn remove_stores_that_do_not_alias_parameters( - &mut self, - function: &Function, - references: &Block, - ) { - let reference_parameters = function + fn remove_stores_that_do_not_alias_parameters(&mut self, references: &Block) { + let reference_parameters = self + .inserter + .function .parameters() .iter() - .filter(|param| function.dfg.value_is_reference(**param)) + .filter(|param| self.inserter.function.dfg.value_is_reference(**param)) .collect::>(); for (allocation, instruction) in &references.last_stores { @@ -251,32 +248,40 @@ impl PerFunctionContext { fn analyze_instruction( &mut self, - function: &mut Function, + block_id: BasicBlockId, references: &mut Block, - instruction: InstructionId, + mut instruction: InstructionId, ) { - match &function.dfg[instruction] { + // If the instruction was simplified and optimized out of the program we shouldn't analyze + // it. Analyzing it could make tracking aliases less accurate if it is e.g. an ArrayGet + // call that used to hold references but has since been optimized out to a known result. + if let Some(new_id) = self.inserter.push_instruction(instruction, block_id) { + instruction = new_id; + } else { + return; + } + + match &self.inserter.function.dfg[instruction] { Instruction::Load { address } => { - let address = function.dfg.resolve(*address); + let address = self.inserter.function.dfg.resolve(*address); - let result = function.dfg.instruction_results(instruction)[0]; - references.remember_dereference(function, address, result); + let result = self.inserter.function.dfg.instruction_results(instruction)[0]; + references.remember_dereference(self.inserter.function, address, result); // If the load is known, replace it with the known value and remove the load if let Some(value) = references.get_known_value(address) { - let result = function.dfg.instruction_results(instruction)[0]; - function.dfg.set_value_from_id(result, value); - + let result = self.inserter.function.dfg.instruction_results(instruction)[0]; + self.inserter.map_value(result, value); self.instructions_to_remove.insert(instruction); } else { - references.mark_value_used(address, function); + references.mark_value_used(address, self.inserter.function); } } Instruction::Store { address, value } => { - let address = function.dfg.resolve(*address); - let value = function.dfg.resolve(*value); + let address = self.inserter.function.dfg.resolve(*address); + let value = self.inserter.function.dfg.resolve(*value); - self.check_array_aliasing(function, references, value); + self.check_array_aliasing(references, value); // If there was another store to this instruction without any (unremoved) loads or // function calls in-between, we can remove the previous store. @@ -288,11 +293,11 @@ impl PerFunctionContext { references.last_stores.insert(address, instruction); } Instruction::Call { arguments, .. } => { - self.mark_all_unknown(arguments, function, references); + self.mark_all_unknown(arguments, references); } Instruction::Allocate => { // Register the new reference - let result = function.dfg.instruction_results(instruction)[0]; + let result = self.inserter.function.dfg.instruction_results(instruction)[0]; references.expressions.insert(result, Expression::Other(result)); let mut aliases = BTreeSet::new(); @@ -300,11 +305,11 @@ impl PerFunctionContext { references.aliases.insert(Expression::Other(result), aliases); } Instruction::ArrayGet { array, .. } => { - let result = function.dfg.instruction_results(instruction)[0]; - references.mark_value_used(*array, function); + let result = self.inserter.function.dfg.instruction_results(instruction)[0]; + references.mark_value_used(*array, self.inserter.function); - if function.dfg.value_is_reference(result) { - let array = function.dfg.resolve(*array); + if self.inserter.function.dfg.value_is_reference(result) { + 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) { @@ -313,17 +318,19 @@ impl PerFunctionContext { } } Instruction::ArraySet { array, value, .. } => { - references.mark_value_used(*array, function); + references.mark_value_used(*array, self.inserter.function); - if function.dfg.value_is_reference(*value) { - let result = function.dfg.instruction_results(instruction)[0]; - let array = function.dfg.resolve(*array); + if self.inserter.function.dfg.value_is_reference(*value) { + 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); - } else if let Some((elements, _)) = function.dfg.get_array_constant(array) { + } 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 { @@ -338,8 +345,8 @@ impl PerFunctionContext { } } - fn check_array_aliasing(&self, function: &Function, references: &mut Block, array: ValueId) { - if let Some((elements, typ)) = function.dfg.get_array_constant(array) { + fn check_array_aliasing(&self, references: &mut Block, array: ValueId) { + if let Some((elements, typ)) = self.inserter.function.dfg.get_array_constant(array) { if Self::contains_references(&typ) { // TODO: Check if type directly holds references or holds arrays that hold references let expr = Expression::ArrayElement(Box::new(Expression::Other(array))); @@ -372,17 +379,12 @@ impl PerFunctionContext { } } - fn mark_all_unknown( - &self, - values: &[ValueId], - function: &Function, - references: &mut Block, - ) { + fn mark_all_unknown(&self, values: &[ValueId], references: &mut Block) { for value in values { - if function.dfg.value_is_reference(*value) { - let value = function.dfg.resolve(*value); + if self.inserter.function.dfg.value_is_reference(*value) { + let value = self.inserter.function.dfg.resolve(*value); references.set_unknown(value); - references.mark_value_used(value, function); + references.mark_value_used(value, self.inserter.function); } } } @@ -390,31 +392,28 @@ impl PerFunctionContext { /// Remove any instructions in `self.instructions_to_remove` from the current function. /// This is expected to contain any loads which were replaced and any stores which are /// no longer needed. - fn remove_instructions(&self, function: &mut Function) { + fn remove_instructions(&mut self) { // The order we iterate blocks in is not important for block in self.post_order.as_slice() { - function.dfg[*block] + self.inserter.function.dfg[*block] .instructions_mut() .retain(|instruction| !self.instructions_to_remove.contains(instruction)); } } - fn handle_terminator( - &self, - function: &mut Function, - block: BasicBlockId, - references: &mut Block, - ) { - match function.dfg[block].unwrap_terminator() { + fn handle_terminator(&mut self, block: BasicBlockId, references: &mut Block) { + self.inserter.map_terminator_in_place(block); + + match self.inserter.function.dfg[block].unwrap_terminator() { TerminatorInstruction::JmpIf { .. } => (), // Nothing to do TerminatorInstruction::Jmp { destination, arguments, .. } => { - let destination_parameters = function.dfg[*destination].parameters(); + let destination_parameters = self.inserter.function.dfg[*destination].parameters(); assert_eq!(destination_parameters.len(), arguments.len()); // Add an alias for each reference parameter for (parameter, argument) in destination_parameters.iter().zip(arguments) { - if function.dfg.value_is_reference(*parameter) { - let argument = function.dfg.resolve(*argument); + if self.inserter.function.dfg.value_is_reference(*parameter) { + let argument = self.inserter.function.dfg.resolve(*argument); if let Some(expression) = references.expressions.get(&argument) { if let Some(aliases) = references.aliases.get_mut(expression) { @@ -429,7 +428,7 @@ impl PerFunctionContext { // Removing all `last_stores` for each returned reference is more important here // than setting them all to ReferenceValue::Unknown since no other block should // have a block with a Return terminator as a predecessor anyway. - self.mark_all_unknown(return_values, function, references); + self.mark_all_unknown(return_values, references); } } } From fcc6df92549bbff6a828e46f1308f568dcc064f9 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 28 Aug 2023 14:03:04 -0500 Subject: [PATCH 06/17] Formatting --- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 41 +++++++------------ .../src/hir/resolution/resolver.rs | 3 +- crates/noirc_frontend/src/parser/parser.rs | 22 ++++------ 3 files changed, 25 insertions(+), 41 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 1ad505e7fa0..5ad0f44ad2f 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -454,26 +454,15 @@ impl Block { } /// If the given address is known, set its value to `ReferenceValue::Known(value)`. - fn set_known_value( - &mut self, - address: ValueId, - value: ValueId, - ) { + fn set_known_value(&mut self, address: ValueId, value: ValueId) { self.set_value(address, ReferenceValue::Known(value)); } - fn set_unknown( - &mut self, - address: ValueId, - ) { + fn set_unknown(&mut self, address: ValueId) { self.set_value(address, ReferenceValue::Unknown); } - fn set_value( - &mut self, - address: ValueId, - value: ReferenceValue, - ) { + 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(); @@ -554,7 +543,11 @@ impl Block { } /// 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) { + 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 { @@ -564,11 +557,7 @@ impl Block { } } - fn keep_last_stores_for( - &mut self, - address: ValueId, - function: &Function, - ) { + 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)); @@ -583,17 +572,15 @@ impl Block { 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:?}"), + } + other => { + unreachable!("last_store held an id of a non-store instruction: {other:?}") + } } } } - fn mark_value_used( - &mut self, - value: ValueId, - function: &Function, - ) { + 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 diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index d0159474606..7cde9ae231d 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -371,7 +371,8 @@ impl<'a> Resolver<'a> { // expect() here is valid, because the only places we don't have a span are omitted types // e.g. a function without return type implicitly has a spanless UnresolvedType::Unit return type // To get an invalid env type, the user must explicitly specify the type, which will have a span - let env_span = env.span.expect("Unexpected missing span for closure environment type"); + let env_span = + env.span.expect("Unexpected missing span for closure environment type"); let env = Box::new(self.resolve_type_inner(*env, new_variables)); diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index 5f4b5f5484a..1da476a42ee 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -40,8 +40,8 @@ use crate::{ BinaryOp, BinaryOpKind, BlockExpression, ConstrainStatement, Distinctness, FunctionDefinition, FunctionReturnType, Ident, IfExpression, InfixExpression, LValue, Lambda, Literal, NoirFunction, NoirStruct, NoirTrait, NoirTypeAlias, Path, PathKind, Pattern, Recoverable, - TraitConstraint, TraitImpl, TraitImplItem, TraitItem, TypeImpl, UnaryOp, - UnresolvedTypeExpression, UseTree, UseTreeKind, Visibility, TraitBound, + TraitBound, TraitConstraint, TraitImpl, TraitImplItem, TraitItem, TypeImpl, UnaryOp, + UnresolvedTypeExpression, UseTree, UseTreeKind, Visibility, }; use chumsky::prelude::*; @@ -527,19 +527,17 @@ fn trait_implementation_body() -> impl NoirParser> { } fn where_clause() -> impl NoirParser> { - struct MultiTraitConstraint { typ: UnresolvedType, trait_bounds: Vec, } - let constraints = parse_type() - .then_ignore(just(Token::Colon)) - .then(trait_bounds()) - .validate(|(typ, trait_bounds), span, emit| { + let constraints = parse_type().then_ignore(just(Token::Colon)).then(trait_bounds()).validate( + |(typ, trait_bounds), span, emit| { emit(ParserError::with_reason(ParserErrorReason::ExperimentalFeature("Traits"), span)); MultiTraitConstraint { typ, trait_bounds } - }); + }, + ); keyword(Keyword::Where) .ignore_then(constraints.separated_by(just(Token::Comma))) @@ -549,7 +547,8 @@ fn where_clause() -> impl NoirParser> { let mut result: Vec = Vec::new(); for constraint in x { for bound in constraint.trait_bounds { - result.push(TraitConstraint { typ:constraint.typ.clone(), trait_bound:bound } ); + result + .push(TraitConstraint { typ: constraint.typ.clone(), trait_bound: bound }); } } result @@ -557,10 +556,7 @@ fn where_clause() -> impl NoirParser> { } fn trait_bounds() -> impl NoirParser> { - trait_bound() - .separated_by(just(Token::Plus)) - .at_least(1) - .allow_trailing() + trait_bound().separated_by(just(Token::Plus)).at_least(1).allow_trailing() } fn trait_bound() -> impl NoirParser { From a34319c710df63d1868a8f01b9adf23c8aa59e24 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 28 Aug 2023 14:05:41 -0500 Subject: [PATCH 07/17] Add regression test --- .../references_aliasing/src/main.nr | 27 ++++++++++++++++--- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/crates/nargo_cli/tests/execution_success/references_aliasing/src/main.nr b/crates/nargo_cli/tests/execution_success/references_aliasing/src/main.nr index 4582444c8f7..02057732f35 100644 --- a/crates/nargo_cli/tests/execution_success/references_aliasing/src/main.nr +++ b/crates/nargo_cli/tests/execution_success/references_aliasing/src/main.nr @@ -1,10 +1,29 @@ -fn increment(mut r: &mut Field) { - *r = *r + 1; -} - fn main() { let mut x = 100; let mut xref = &mut x; increment(xref); assert(*xref == 101); + + regression_2445(); +} + +fn increment(mut r: &mut Field) { + *r = *r + 1; +} + +// If aliasing within arrays and constant folding within the mem2reg pass aren't +// handled, we'll fail to optimize out all the references in this function. +fn regression_2445() { + let mut var = 0; + let ref = &mut &mut var; + + let mut array = [ref, ref]; + + **array[0] = 1; + **array[1] = 2; + + assert(var == 2); + assert(**ref == 2); + assert(**array[0] == 2); + assert(**array[1] == 2); } From 51762c4d48f6fdb5a434c46c3951749674507edd Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 28 Aug 2023 14:07:48 -0500 Subject: [PATCH 08/17] Formatting --- .../src/brillig/brillig_gen/brillig_block.rs | 2 +- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- .../src/hir/resolution/resolver.rs | 3 ++- crates/noirc_frontend/src/parser/parser.rs | 22 ++++++++----------- 4 files changed, 13 insertions(+), 16 deletions(-) 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 c50019dd622..567c0510588 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1106,7 +1106,7 @@ pub(crate) fn type_of_binary_operation(lhs_type: &Type, rhs_type: &Type) -> Type lhs_type, rhs_type, "lhs and rhs types in a binary operation are always the same" ); - Type::Numeric(lhs_type.clone()) + Type::Numeric(*lhs_type) } } } diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index e2bae6de300..8156a946e34 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -359,7 +359,7 @@ impl PerFunctionContext { Type::Function => false, Type::Reference => true, Type::Array(elements, _) | Type::Slice(elements) => { - elements.iter().any(|element| Self::contains_references(element)) + elements.iter().any(Self::contains_references) } } } diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index d0159474606..7cde9ae231d 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -371,7 +371,8 @@ impl<'a> Resolver<'a> { // expect() here is valid, because the only places we don't have a span are omitted types // e.g. a function without return type implicitly has a spanless UnresolvedType::Unit return type // To get an invalid env type, the user must explicitly specify the type, which will have a span - let env_span = env.span.expect("Unexpected missing span for closure environment type"); + let env_span = + env.span.expect("Unexpected missing span for closure environment type"); let env = Box::new(self.resolve_type_inner(*env, new_variables)); diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index 5f4b5f5484a..1da476a42ee 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -40,8 +40,8 @@ use crate::{ BinaryOp, BinaryOpKind, BlockExpression, ConstrainStatement, Distinctness, FunctionDefinition, FunctionReturnType, Ident, IfExpression, InfixExpression, LValue, Lambda, Literal, NoirFunction, NoirStruct, NoirTrait, NoirTypeAlias, Path, PathKind, Pattern, Recoverable, - TraitConstraint, TraitImpl, TraitImplItem, TraitItem, TypeImpl, UnaryOp, - UnresolvedTypeExpression, UseTree, UseTreeKind, Visibility, TraitBound, + TraitBound, TraitConstraint, TraitImpl, TraitImplItem, TraitItem, TypeImpl, UnaryOp, + UnresolvedTypeExpression, UseTree, UseTreeKind, Visibility, }; use chumsky::prelude::*; @@ -527,19 +527,17 @@ fn trait_implementation_body() -> impl NoirParser> { } fn where_clause() -> impl NoirParser> { - struct MultiTraitConstraint { typ: UnresolvedType, trait_bounds: Vec, } - let constraints = parse_type() - .then_ignore(just(Token::Colon)) - .then(trait_bounds()) - .validate(|(typ, trait_bounds), span, emit| { + let constraints = parse_type().then_ignore(just(Token::Colon)).then(trait_bounds()).validate( + |(typ, trait_bounds), span, emit| { emit(ParserError::with_reason(ParserErrorReason::ExperimentalFeature("Traits"), span)); MultiTraitConstraint { typ, trait_bounds } - }); + }, + ); keyword(Keyword::Where) .ignore_then(constraints.separated_by(just(Token::Comma))) @@ -549,7 +547,8 @@ fn where_clause() -> impl NoirParser> { let mut result: Vec = Vec::new(); for constraint in x { for bound in constraint.trait_bounds { - result.push(TraitConstraint { typ:constraint.typ.clone(), trait_bound:bound } ); + result + .push(TraitConstraint { typ: constraint.typ.clone(), trait_bound: bound }); } } result @@ -557,10 +556,7 @@ fn where_clause() -> impl NoirParser> { } fn trait_bounds() -> impl NoirParser> { - trait_bound() - .separated_by(just(Token::Plus)) - .at_least(1) - .allow_trailing() + trait_bound().separated_by(just(Token::Plus)).at_least(1).allow_trailing() } fn trait_bound() -> impl NoirParser { From 410bc266956ec6ec46bfb2689173862efcae5352 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 25 Aug 2023 14:10:25 -0500 Subject: [PATCH 09/17] Move last_stores in Block struct --- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 115 ++++++++++-------- 1 file changed, 62 insertions(+), 53 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 8156a946e34..b3451ba05ff 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -121,6 +121,9 @@ struct Block { /// 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 @@ -202,19 +205,18 @@ impl PerFunctionContext { mut references: Block, ) { let instructions = function.dfg[block].instructions().to_vec(); - let mut last_stores = BTreeMap::new(); for instruction in instructions { - self.analyze_instruction(function, &mut references, instruction, &mut last_stores); + self.analyze_instruction(function, &mut references, instruction); } - self.handle_terminator(function, block, &mut references, &mut last_stores); + self.handle_terminator(function, block, &mut references); // If there's only 1 block in the function total, we can remove any remaining last stores // as well. We can't do this if there are multiple blocks since subsequent blocks may // reference these stores. if self.post_order.as_slice().len() == 1 { - self.remove_stores_that_do_not_alias_parameters(function, &references, last_stores); + self.remove_stores_that_do_not_alias_parameters(function, &references); } self.blocks.insert(block, references); @@ -226,7 +228,6 @@ impl PerFunctionContext { &mut self, function: &Function, references: &Block, - last_stores: BTreeMap, ) { let reference_parameters = function .parameters() @@ -234,14 +235,14 @@ impl PerFunctionContext { .filter(|param| function.dfg.value_is_reference(**param)) .collect::>(); - for (allocation, instruction) in last_stores { + for (allocation, instruction) in &references.last_stores { 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)); if !aliases.is_empty() && !allocation_aliases_parameter { - self.instructions_to_remove.insert(instruction); + self.instructions_to_remove.insert(*instruction); } } } @@ -253,7 +254,6 @@ impl PerFunctionContext { function: &mut Function, references: &mut Block, instruction: InstructionId, - last_stores: &mut BTreeMap, ) { match &function.dfg[instruction] { Instruction::Load { address } => { @@ -269,7 +269,7 @@ impl PerFunctionContext { self.instructions_to_remove.insert(instruction); } else { - references.mark_value_used(address, function, last_stores); + references.mark_value_used(address, function); } } Instruction::Store { address, value } => { @@ -280,15 +280,15 @@ impl PerFunctionContext { // If there was another store to this instruction without any (unremoved) loads or // function calls in-between, we can remove the previous store. - if let Some(last_store) = last_stores.get(&address) { + if let Some(last_store) = references.last_stores.get(&address) { self.instructions_to_remove.insert(*last_store); } - references.set_known_value(address, value, last_stores); - last_stores.insert(address, instruction); + references.set_known_value(address, value); + references.last_stores.insert(address, instruction); } Instruction::Call { arguments, .. } => { - self.mark_all_unknown(arguments, function, references, last_stores); + self.mark_all_unknown(arguments, function, references); } Instruction::Allocate => { // Register the new reference @@ -301,7 +301,7 @@ impl PerFunctionContext { } Instruction::ArrayGet { array, .. } => { let result = function.dfg.instruction_results(instruction)[0]; - references.mark_value_used(*array, function, last_stores); + references.mark_value_used(*array, function); if function.dfg.value_is_reference(result) { let array = function.dfg.resolve(*array); @@ -313,7 +313,7 @@ impl PerFunctionContext { } } Instruction::ArraySet { array, value, .. } => { - references.mark_value_used(*array, function, last_stores); + references.mark_value_used(*array, function); if function.dfg.value_is_reference(*value) { let result = function.dfg.instruction_results(instruction)[0]; @@ -377,13 +377,12 @@ impl PerFunctionContext { values: &[ValueId], function: &Function, references: &mut Block, - last_stores: &mut BTreeMap, ) { for value in values { if function.dfg.value_is_reference(*value) { let value = function.dfg.resolve(*value); - references.set_unknown(value, last_stores); - references.mark_value_used(value, function, last_stores); + references.set_unknown(value); + references.mark_value_used(value, function); } } } @@ -405,7 +404,6 @@ impl PerFunctionContext { function: &mut Function, block: BasicBlockId, references: &mut Block, - last_stores: &mut BTreeMap, ) { match function.dfg[block].unwrap_terminator() { TerminatorInstruction::JmpIf { .. } => (), // Nothing to do @@ -431,7 +429,7 @@ impl PerFunctionContext { // Removing all `last_stores` for each returned reference is more important here // than setting them all to ReferenceValue::Unknown since no other block should // have a block with a Return terminator as a predecessor anyway. - self.mark_all_unknown(return_values, function, references, last_stores); + self.mark_all_unknown(return_values, function, references); } } } @@ -461,53 +459,49 @@ impl Block { &mut self, address: ValueId, value: ValueId, - last_stores: &mut BTreeMap, ) { - self.set_value(address, ReferenceValue::Known(value), last_stores); + self.set_value(address, ReferenceValue::Known(value)); } fn set_unknown( &mut self, address: ValueId, - last_stores: &mut BTreeMap, ) { - self.set_value(address, ReferenceValue::Unknown, last_stores); + self.set_value(address, ReferenceValue::Unknown); } fn set_value( &mut self, address: ValueId, value: ReferenceValue, - last_stores: &mut BTreeMap, ) { - if let Some(expression) = self.expressions.get(&address) { - if let Some(aliases) = self.aliases.get(expression) { - 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(last_stores); - } 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; - } - } + 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, last_stores: &mut BTreeMap) { + fn invalidate_all_references(&mut self) { for reference_value in self.references.values_mut() { *reference_value = ReferenceValue::Unknown; } - last_stores.clear(); + self.last_stores.clear(); } fn unify(mut self, other: &Self) -> Self { @@ -561,11 +555,11 @@ impl Block { } /// 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(ValueId) -> T) { + 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) { + if let Some(aliases) = self.aliases.get(expr).cloned() { for alias in aliases { - f(*alias); + f(self, alias); } } } @@ -574,25 +568,40 @@ impl Block { fn keep_last_stores_for( &mut self, address: ValueId, - last_stores: &mut BTreeMap, + function: &Function, ) { - last_stores.remove(&address); - self.for_each_alias_of(address, |alias| last_stores.remove(&alias)); + 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, - last_stores: &mut BTreeMap, ) { - self.keep_last_stores_for(value, last_stores); + 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, last_stores); + self.mark_value_used(value, function); } } } From 1fc27a30d904d9cd50c1cf89ccae931fa4eef5a4 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 28 Aug 2023 16:05:59 -0500 Subject: [PATCH 10/17] Apply fix from #2466 --- crates/noirc_evaluator/src/ssa.rs | 1 + .../src/ssa/opt/constant_folding.rs | 4 +- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 48 ++++++------------- 3 files changed, 19 insertions(+), 34 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa.rs b/crates/noirc_evaluator/src/ssa.rs index 7dbd627a949..dd8fae50ade 100644 --- a/crates/noirc_evaluator/src/ssa.rs +++ b/crates/noirc_evaluator/src/ssa.rs @@ -63,6 +63,7 @@ pub(crate) fn optimize_into_acir( // and this pass is missed, slice merging will fail inside of flattening. .mem2reg() .print(print_ssa_passes, "After Mem2Reg:") + .fold_constants() .flatten_cfg() .print(print_ssa_passes, "After Flattening:") // Run mem2reg once more with the flattened CFG to catch any remaining loads/stores diff --git a/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs b/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs index e53bfac8c03..a3f96161626 100644 --- a/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -102,7 +102,9 @@ impl Context { // 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.has_side_effects(&function.dfg) { + if !instruction.has_side_effects(&function.dfg) + && !matches!(instruction, Instruction::Allocate) + { instruction_result_cache.insert(instruction, new_results.clone()); } for (old_result, new_result) in old_results.iter().zip(new_results) { diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index b3451ba05ff..152320f2206 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -372,12 +372,7 @@ impl PerFunctionContext { } } - fn mark_all_unknown( - &self, - values: &[ValueId], - function: &Function, - references: &mut Block, - ) { + fn mark_all_unknown(&self, values: &[ValueId], function: &Function, references: &mut Block) { for value in values { if function.dfg.value_is_reference(*value) { let value = function.dfg.resolve(*value); @@ -455,26 +450,15 @@ impl Block { } /// If the given address is known, set its value to `ReferenceValue::Known(value)`. - fn set_known_value( - &mut self, - address: ValueId, - value: ValueId, - ) { + fn set_known_value(&mut self, address: ValueId, value: ValueId) { self.set_value(address, ReferenceValue::Known(value)); } - fn set_unknown( - &mut self, - address: ValueId, - ) { + fn set_unknown(&mut self, address: ValueId) { self.set_value(address, ReferenceValue::Unknown); } - fn set_value( - &mut self, - address: ValueId, - value: ReferenceValue, - ) { + 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(); @@ -555,7 +539,11 @@ impl Block { } /// 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) { + 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 { @@ -565,11 +553,7 @@ impl Block { } } - fn keep_last_stores_for( - &mut self, - address: ValueId, - function: &Function, - ) { + 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)); @@ -584,17 +568,15 @@ impl Block { 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:?}"), + } + other => { + unreachable!("last_store held an id of a non-store instruction: {other:?}") + } } } } - fn mark_value_used( - &mut self, - value: ValueId, - function: &Function, - ) { + 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 From 7e323837a9d366c5c25ff1cf5f5145c001d77045 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 28 Aug 2023 16:05:59 -0500 Subject: [PATCH 11/17] Apply fix from #2466 --- crates/noirc_evaluator/src/ssa.rs | 1 + crates/noirc_evaluator/src/ssa/opt/constant_folding.rs | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa.rs b/crates/noirc_evaluator/src/ssa.rs index 7dbd627a949..dd8fae50ade 100644 --- a/crates/noirc_evaluator/src/ssa.rs +++ b/crates/noirc_evaluator/src/ssa.rs @@ -63,6 +63,7 @@ pub(crate) fn optimize_into_acir( // and this pass is missed, slice merging will fail inside of flattening. .mem2reg() .print(print_ssa_passes, "After Mem2Reg:") + .fold_constants() .flatten_cfg() .print(print_ssa_passes, "After Flattening:") // Run mem2reg once more with the flattened CFG to catch any remaining loads/stores diff --git a/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs b/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs index 3159e360799..4cccd101949 100644 --- a/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -102,7 +102,9 @@ impl Context { // 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.has_side_effects(&function.dfg) { + if !instruction.has_side_effects(&function.dfg) + && !matches!(instruction, Instruction::Allocate) + { instruction_result_cache.insert(instruction, new_results.clone()); } for (old_result, new_result) in old_results.iter().zip(new_results) { From be82feca023022752e3d3e8037fb88f42823fae0 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 28 Aug 2023 16:20:17 -0500 Subject: [PATCH 12/17] Avoid mapping ids to the same id --- crates/noirc_evaluator/src/ssa/ir/function_inserter.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs b/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs index 43d3917700d..cd039079c73 100644 --- a/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs +++ b/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs @@ -47,12 +47,16 @@ impl<'f> FunctionInserter<'f> { /// Insert a key, value pair if the key isn't already present in the map pub(crate) fn try_map_value(&mut self, key: ValueId, value: ValueId) { - self.values.entry(key).or_insert(value); + if key != value { + self.values.entry(key).or_insert(value); + } } /// Insert a key, value pair in the map pub(crate) fn map_value(&mut self, key: ValueId, value: ValueId) { - self.values.insert(key, value); + if key != value { + self.values.insert(key, value); + } } pub(crate) fn map_instruction(&mut self, id: InstructionId) -> (Instruction, CallStack) { From 21cdfd0b39b0dd412b999bee0af6622af722620b Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 28 Aug 2023 16:49:34 -0500 Subject: [PATCH 13/17] Clear last stores after cloning predecessor block --- crates/noirc_evaluator/src/ssa/ir/function_inserter.rs | 4 +++- crates/noirc_evaluator/src/ssa/opt/constant_folding.rs | 1 + crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 3 ++- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs b/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs index cd039079c73..c969f9e9e4b 100644 --- a/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs +++ b/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs @@ -54,7 +54,9 @@ impl<'f> FunctionInserter<'f> { /// Insert a key, value pair in the map pub(crate) fn map_value(&mut self, key: ValueId, value: ValueId) { - if key != value { + if key == value { + self.values.remove(&key); + } else { self.values.insert(key, value); } } diff --git a/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs b/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs index 4cccd101949..2440cf23f85 100644 --- a/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -104,6 +104,7 @@ impl Context { // the same instruction appears again later in the block. if !instruction.has_side_effects(&function.dfg) && !matches!(instruction, Instruction::Allocate) + && !matches!(instruction, Instruction::Load { .. }) { instruction_result_cache.insert(instruction, new_results.clone()); } diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 5ad0f44ad2f..ed2717616d1 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -183,7 +183,8 @@ impl<'f> PerFunctionContext<'f> { let mut predecessors = self.cfg.predecessors(block); if let Some(first_predecessor) = predecessors.next() { - let first = self.blocks.get(&first_predecessor).cloned().unwrap_or_default(); + let mut first = self.blocks.get(&first_predecessor).cloned().unwrap_or_default(); + first.last_stores.clear(); // Note that we have to start folding with the first block as the accumulator. // If we started with an empty block, an empty block union'd with any other block From 61be2a927b5481d91f720a93a78463c87b86db40 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 28 Aug 2023 16:55:56 -0500 Subject: [PATCH 14/17] Add last stores fix --- crates/noirc_evaluator/src/ssa/opt/constant_folding.rs | 1 + crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs b/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs index a3f96161626..9ad6750bd66 100644 --- a/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -104,6 +104,7 @@ impl Context { // the same instruction appears again later in the block. if !instruction.has_side_effects(&function.dfg) && !matches!(instruction, Instruction::Allocate) + && !matches!(instruction, Instruction::Load { .. }) { instruction_result_cache.insert(instruction, new_results.clone()); } diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 152320f2206..974c5e7b470 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -179,7 +179,8 @@ impl PerFunctionContext { let mut predecessors = self.cfg.predecessors(block); if let Some(first_predecessor) = predecessors.next() { - let first = self.blocks.get(&first_predecessor).cloned().unwrap_or_default(); + let mut first = self.blocks.get(&first_predecessor).cloned().unwrap_or_default(); + first.last_stores.clear(); // Note that we have to start folding with the first block as the accumulator. // If we started with an empty block, an empty block union'd with any other block From 0bccde7de9f921f02c18a06dfbac4feb8e705eff Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 28 Aug 2023 16:57:14 -0500 Subject: [PATCH 15/17] Clippy --- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 974c5e7b470..4d2531fa27c 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -237,7 +237,7 @@ impl PerFunctionContext { .collect::>(); for (allocation, instruction) in &references.last_stores { - if let Some(expression) = references.expressions.get(&allocation) { + 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)); From ea39628a6b646f03d6a5a81f14af7576d9a8d685 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 29 Aug 2023 12:47:44 -0500 Subject: [PATCH 16/17] Update tests --- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 25 ++++++++----------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 96fb91c0e97..85e8196f951 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -720,12 +720,18 @@ mod tests { // Store is needed by the return value, and can't be removed assert_eq!(count_stores(block_id, &func.dfg), 1); + let instructions = func.dfg[block_id].instructions(); + assert_eq!(instructions.len(), 2); let ret_val_id = match func.dfg[block_id].terminator().unwrap() { - TerminatorInstruction::Return { return_values } => return_values.first().unwrap(), + TerminatorInstruction::Return { return_values } => *return_values.first().unwrap(), _ => unreachable!(), }; - assert_eq!(func.dfg[*ret_val_id], func.dfg[v0]); + + // Since the mem2reg pass simplifies as it goes, the id of the allocate instruction result + // is most likely no longer v0. We have to retrieve the new id here. + let alloca_id = func.dfg.instruction_results(instructions[0])[0]; + assert_eq!(ret_val_id, alloca_id); } fn count_stores(block: BasicBlockId, dfg: &DataFlowGraph) -> usize { @@ -900,18 +906,7 @@ mod tests { let b1_instructions = main.dfg[b1].instructions(); - // The last instruction in b1 should be a binary operation - match &main.dfg[*b1_instructions.last().unwrap()] { - Instruction::Binary(binary) => { - let lhs = - main.dfg.get_numeric_constant(binary.lhs).expect("Expected constant value"); - let rhs = - main.dfg.get_numeric_constant(binary.rhs).expect("Expected constant value"); - - assert_eq!(lhs, rhs); - assert_eq!(lhs, FieldElement::from(2u128)); - } - _ => unreachable!(), - } + // We expect the last eq to be optimized out + assert_eq!(b1_instructions.len(), 1); } } From 82c2d923e39ff6a5f318c7d0f573b27bc2541e28 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 29 Aug 2023 15:48:53 -0500 Subject: [PATCH 17/17] PR comments --- .../src/ssa/ir/function_inserter.rs | 6 +++++- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 19 +++++++++---------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs b/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs index c969f9e9e4b..d9aee785016 100644 --- a/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs +++ b/crates/noirc_evaluator/src/ssa/ir/function_inserter.rs @@ -47,7 +47,11 @@ impl<'f> FunctionInserter<'f> { /// Insert a key, value pair if the key isn't already present in the map pub(crate) fn try_map_value(&mut self, key: ValueId, value: ValueId) { - if key != value { + if key == value { + // This case is technically not needed since try_map_value isn't meant to change + // existing entries, but we should never have a value in the map referring to itself anyway. + self.values.remove(&key); + } else { self.values.entry(key).or_insert(value); } } diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 85e8196f951..84af1053ed7 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -793,12 +793,12 @@ mod tests { // Expected result: // acir fn main f0 { // b0(): - // v0 = allocate - // store Field 5 at v0 + // v7 = allocate + // store Field 5 at v7 // jmp b1(Field 5) // b1(v3: Field): - // store Field 6 at v0 - // return v3, Field 5, Field 6 // Optimized to constants 5 and 6 + // store Field 6 at v7 + // return v3, Field 5, Field 6 // } let ssa = ssa.mem2reg(); @@ -880,14 +880,13 @@ mod tests { // Expected result: // acir fn main f0 { // b0(): - // v0 = allocate - // store Field 0 at v0 - // v2 = allocate - // store v0 at v2 + // v9 = allocate + // store Field 0 at v9 + // v10 = allocate + // store v9 at v10 // jmp b1() // b1(): - // store Field 2 at v0 - // v8 = eq Field 1, Field 2 + // store Field 2 at v9 // return // } let ssa = ssa.mem2reg();