From b7ff73b62dacfa1893b5c7c6e9bf031bf96dabdd Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 2 Jun 2023 12:38:11 -0500 Subject: [PATCH 01/21] Represent SSA arrays with im::Vector --- Cargo.lock | 45 ++++++++++- crates/noirc_evaluator/Cargo.toml | 1 + .../src/ssa_refactor/acir_gen/mod.rs | 3 +- .../src/ssa_refactor/ir/dfg.rs | 28 +++++-- .../src/ssa_refactor/ir/instruction.rs | 73 ++++++++++++------ .../src/ssa_refactor/ir/printer.rs | 14 +++- .../src/ssa_refactor/ir/types.rs | 4 + .../src/ssa_refactor/ir/value.rs | 6 +- .../src/ssa_refactor/opt/mem2reg.rs | 75 +++++++++---------- .../src/ssa_refactor/ssa_builder/mod.rs | 38 +++++++--- .../src/ssa_refactor/ssa_gen/context.rs | 2 +- .../src/ssa_refactor/ssa_gen/mod.rs | 25 +++---- .../src/ssa_refactor/ssa_gen/value.rs | 5 +- 13 files changed, 218 insertions(+), 101 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 66dffa03c9b..79ccf9be4e4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -381,6 +381,15 @@ version = "1.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "bef38d45163c2f1dde094a7dfd33ccf595c92905c8f8f4fdc18d06fb1037718a" +[[package]] +name = "bitmaps" +version = "2.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "031043d04099746d8db04daf1fa424b2bc8bd69d92b25962dcde24da39ab64a2" +dependencies = [ + "typenum", +] + [[package]] name = "bitvec" version = "0.18.5" @@ -1538,6 +1547,20 @@ dependencies = [ "winapi-util", ] +[[package]] +name = "im" +version = "15.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d0acd33ff0285af998aaf9b57342af478078f53492322fafc47450e09397e0e9" +dependencies = [ + "bitmaps", + "rand_core 0.6.4", + "rand_xoshiro", + "sized-chunks", + "typenum", + "version_check", +] + [[package]] name = "indenter" version = "0.3.3" @@ -1908,6 +1931,7 @@ version = "0.6.0" dependencies = [ "acvm", "arena", + "im", "iter-extended", "noirc_abi", "noirc_errors", @@ -2273,6 +2297,15 @@ dependencies = [ "getrandom", ] +[[package]] +name = "rand_xoshiro" +version = "0.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6f97cdb2a36ed4183de61b2f824cc45c9f1037f28afe0a322e9fff4c108b5aaa" +dependencies = [ + "rand_core 0.6.4", +] + [[package]] name = "rayon" version = "1.7.0" @@ -2833,6 +2866,16 @@ version = "0.1.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f27f6278552951f1f2b8cf9da965d10969b2efdea95a6ec47987ab46edfe263a" +[[package]] +name = "sized-chunks" +version = "0.6.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "16d69225bde7a69b235da73377861095455d298f2b970996eec25ddbb42b3d1e" +dependencies = [ + "bitmaps", + "typenum", +] + [[package]] name = "slab" version = "0.4.8" @@ -3902,4 +3945,4 @@ dependencies = [ "quote", "syn 1.0.109", "synstructure", -] \ No newline at end of file +] diff --git a/crates/noirc_evaluator/Cargo.toml b/crates/noirc_evaluator/Cargo.toml index 2e82d34d2dd..b2a01b142d0 100644 --- a/crates/noirc_evaluator/Cargo.toml +++ b/crates/noirc_evaluator/Cargo.toml @@ -16,6 +16,7 @@ iter-extended.workspace = true thiserror.workspace = true num-bigint = "0.4" num-traits = "0.2.8" +im = "15.1" [dev-dependencies] rand="0.8.5" diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs index 717884dc996..a4bdd19dfa7 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs @@ -86,7 +86,7 @@ impl Context { ) { let mut param_array_lengths_iter = param_array_lengths.iter(); for param_id in params { - let value = dfg[*param_id]; + let value = &dfg[*param_id]; let param_type = match value { Value::Param { typ, .. } => typ, _ => unreachable!("ICE: Only Param type values should appear in block parameters"), @@ -220,6 +220,7 @@ impl Context { let acir_var = match value { Value::NumericConstant { constant, .. } => self.acir_context.add_constant(*constant), Value::Intrinsic(..) => todo!(), + Value::Array(..) => todo!(), Value::Function(..) => unreachable!("ICE: All functions should have been inlined"), Value::Instruction { .. } | Value::Param { .. } => { unreachable!("ICE: Should have been in cache {value:?}") diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs index 42835c1946d..a15422b8b80 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs @@ -8,7 +8,7 @@ use super::{ instruction::{ Instruction, InstructionId, InstructionResultType, Intrinsic, TerminatorInstruction, }, - map::{DenseMap, Id}, + map::DenseMap, types::Type, value::{Value, ValueId}, }; @@ -157,7 +157,7 @@ impl DataFlowGraph { /// Set the value of value_to_replace to refer to the value referred to by new_value. pub(crate) fn set_value_from_id(&mut self, value_to_replace: ValueId, new_value: ValueId) { - let new_value = self.values[new_value]; + let new_value = self.values[new_value].clone(); self.values[value_to_replace] = new_value; } @@ -172,6 +172,12 @@ impl DataFlowGraph { id } + /// Returns the field element represented by this value if it is a numeric constant. + /// Returns None if the given value is not a numeric constant. + pub(crate) fn make_array(&mut self, elements: im::Vector) -> ValueId { + self.make_value(Value::Array(elements)) + } + /// Gets or creates a ValueId for the given FunctionId. pub(crate) fn import_function(&mut self, function: FunctionId) -> ValueId { if let Some(existing) = self.functions.get(&function) { @@ -265,7 +271,7 @@ impl DataFlowGraph { } /// Add a parameter to the given block - pub(crate) fn add_block_parameter(&mut self, block_id: BasicBlockId, typ: Type) -> Id { + pub(crate) fn add_block_parameter(&mut self, block_id: BasicBlockId, typ: Type) -> ValueId { let block = &mut self.blocks[block_id]; let position = block.parameters().len(); let parameter = self.values.insert(Value::Param { block: block_id, position, typ }); @@ -275,7 +281,7 @@ impl DataFlowGraph { /// Returns the field element represented by this value if it is a numeric constant. /// Returns None if the given value is not a numeric constant. - pub(crate) fn get_numeric_constant(&self, value: Id) -> Option { + pub(crate) fn get_numeric_constant(&self, value: ValueId) -> Option { self.get_numeric_constant_with_type(value).map(|(value, _typ)| value) } @@ -283,7 +289,7 @@ impl DataFlowGraph { /// Returns None if the given value is not a numeric constant. pub(crate) fn get_numeric_constant_with_type( &self, - value: Id, + value: ValueId, ) -> Option<(FieldElement, Type)> { match self.values[value] { Value::NumericConstant { constant, typ } => Some((constant, typ)), @@ -291,6 +297,16 @@ impl DataFlowGraph { } } + /// Returns the Value::Array associated with this ValueId if it refers to an array constant. + /// Otherwise, this returns None. + pub(crate) fn get_array_constant(&self, value: ValueId) -> Option> { + match &self.values[value] { + // Vectors are shared, so cloning them is cheap + Value::Array(array) => Some(array.clone()), + _ => None, + } + } + /// Sets the terminator instruction for the given basic block pub(crate) fn set_block_terminator( &mut self, @@ -397,7 +413,7 @@ mod tests { #[test] fn make_instruction() { let mut dfg = DataFlowGraph::default(); - let ins = Instruction::Allocate { size: 20 }; + let ins = Instruction::Allocate; let ins_id = dfg.make_instruction(ins, None); let results = dfg.instruction_results(ins_id); diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs index 98e277c0fa2..9664cf56a8e 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs @@ -94,17 +94,21 @@ pub(crate) enum Instruction { /// Allocates a region of memory. Note that this is not concerned with /// the type of memory, the type of element is determined when loading this memory. - /// - /// `size` is the size of the region to be allocated by the number of FieldElements it - /// contains. Note that non-numeric types like Functions and References are counted as 1 field - /// each. - Allocate { size: u32 }, + /// This is used for representing mutable variables and references. + Allocate, /// Loads a value from memory. Load { address: ValueId }, /// Writes a value to memory. Store { address: ValueId, value: ValueId }, + + /// Retrieve a value from an array at the given index + ArrayGet { array: ValueId, index: ValueId }, + + /// Creates a new array with the new value at the given index. All other elements are identical + /// to those in the given array. This will not modify the original array. + ArraySet { array: ValueId, index: ValueId, value: ValueId }, } impl Instruction { @@ -122,8 +126,11 @@ impl Instruction { Instruction::Not(value) | Instruction::Truncate { value, .. } => { InstructionResultType::Operand(*value) } + Instruction::ArraySet { array, .. } => InstructionResultType::Operand(*array), Instruction::Constrain(_) | Instruction::Store { .. } => InstructionResultType::None, - Instruction::Load { .. } | Instruction::Call { .. } => InstructionResultType::Unknown, + Instruction::Load { .. } | Instruction::ArrayGet { .. } | Instruction::Call { .. } => { + InstructionResultType::Unknown + } } } @@ -155,11 +162,17 @@ impl Instruction { func: f(*func), arguments: vecmap(arguments.iter().copied(), f), }, - Instruction::Allocate { size } => Instruction::Allocate { size: *size }, + Instruction::Allocate => Instruction::Allocate, Instruction::Load { address } => Instruction::Load { address: f(*address) }, Instruction::Store { address, value } => { Instruction::Store { address: f(*address), value: f(*value) } } + Instruction::ArrayGet { array, index } => { + Instruction::ArrayGet { array: f(*array), index: f(*index) } + } + Instruction::ArraySet { array, index, value } => { + Instruction::ArraySet { array: f(*array), index: f(*index), value: f(*value) } + } } } @@ -168,11 +181,10 @@ impl Instruction { pub(crate) fn simplify(&self, dfg: &mut DataFlowGraph) -> SimplifyResult { use SimplifyResult::*; match self { - Instruction::Binary(binary) => binary.simplify(dfg), + Instruction::Binary(binary) => return binary.simplify(dfg), Instruction::Cast(value, typ) => { - match (*typ == dfg.type_of_value(*value)).then_some(*value) { - Some(value) => SimplifiedTo(value), - _ => None, + if let Some(value) = (*typ == dfg.type_of_value(*value)).then_some(*value) { + return SimplifiedTo(value); } } Instruction::Not(value) => { @@ -182,16 +194,15 @@ impl Instruction { // would be incorrect however since the extra bits on the field would not be flipped. Value::NumericConstant { constant, typ } if *typ == Type::bool() => { let value = constant.is_zero() as u128; - SimplifiedTo(dfg.make_constant(value.into(), Type::bool())) + return SimplifiedTo(dfg.make_constant(value.into(), Type::bool())); } Value::Instruction { instruction, .. } => { // !!v => v - match &dfg[*instruction] { - Instruction::Not(value) => SimplifiedTo(*value), - _ => None, + if let Instruction::Not(value) = &dfg[*instruction] { + return SimplifiedTo(*value); } } - _ => None, + _ => (), } } Instruction::Constrain(value) => { @@ -200,14 +211,32 @@ impl Instruction { return Remove; } } - None } - Instruction::Truncate { .. } => None, - Instruction::Call { .. } => None, - Instruction::Allocate { .. } => None, - Instruction::Load { .. } => None, - Instruction::Store { .. } => None, + Instruction::ArrayGet { array, index } => { + let array = dfg.get_array_constant(*array); + if let (Some(array), Some(index)) = (array, dfg.get_numeric_constant(*index)) { + let index = + index.try_to_u64().expect("Expected array index to fit in u64") as usize; + assert!(index < array.len()); + return SimplifiedTo(array[index]); + } + } + Instruction::ArraySet { array, index, value } => { + let array = dfg.get_array_constant(*array); + if let (Some(array), Some(index)) = (array, dfg.get_numeric_constant(*index)) { + let index = + index.try_to_u64().expect("Expected array index to fit in u64") as usize; + assert!(index < array.len()); + return SimplifiedTo(dfg.make_array(array.update(index, *value))); + } + } + Instruction::Truncate { .. } => (), + Instruction::Call { .. } => (), + Instruction::Allocate { .. } => (), + Instruction::Load { .. } => (), + Instruction::Store { .. } => (), } + None } } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs index 7a4651056d1..3fa37e0712c 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs @@ -144,10 +144,22 @@ pub(crate) fn display_instruction( Instruction::Call { func, arguments } => { writeln!(f, "call {}({})", show(*func), value_list(function, arguments)) } - Instruction::Allocate { size } => writeln!(f, "alloc {size} fields"), + Instruction::Allocate => writeln!(f, "allocate"), Instruction::Load { address } => writeln!(f, "load {}", show(*address)), Instruction::Store { address, value } => { writeln!(f, "store {} at {}", show(*value), show(*address)) } + Instruction::ArrayGet { array, index } => { + writeln!(f, "array_get {}, index {}", show(*array), show(*index)) + } + Instruction::ArraySet { array, index, value } => { + writeln!( + f, + "array_set {}, index {}, value {}", + show(*array), + show(*index), + show(*value) + ) + } } } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/types.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/types.rs index e00c25a257c..2d911e9000e 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/types.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/types.rs @@ -22,6 +22,9 @@ pub(crate) enum Type { /// A reference to some value, such as an array Reference, + /// An immutable array value + Array, + /// A function that may be called directly Function, @@ -56,6 +59,7 @@ impl std::fmt::Display for Type { match self { Type::Numeric(numeric) => numeric.fmt(f), Type::Reference => write!(f, "reference"), + Type::Array => write!(f, "array"), Type::Function => write!(f, "function"), Type::Unit => write!(f, "unit"), } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs index a77916ee841..b83e761dc89 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs @@ -13,7 +13,7 @@ pub(crate) type ValueId = Id; /// Value is the most basic type allowed in the IR. /// Transition Note: A Id is similar to `NodeId` in our previous IR. -#[derive(Debug, PartialEq, Eq, Hash, Copy, Clone)] +#[derive(Debug, PartialEq, Eq, Hash, Clone)] pub(crate) enum Value { /// This value was created due to an instruction /// @@ -35,6 +35,9 @@ pub(crate) enum Value { /// This Value originates from a numeric constant NumericConstant { constant: FieldElement, typ: Type }, + /// Represents a constant array value + Array(im::Vector), + /// This Value refers to a function in the IR. /// Functions always have the type Type::Function. /// If the argument or return types are needed, users should retrieve @@ -54,6 +57,7 @@ impl Value { Value::Instruction { typ, .. } => *typ, Value::Param { typ, .. } => *typ, Value::NumericConstant { typ, .. } => *typ, + Value::Array(_) => Type::Reference, Value::Function { .. } => Type::Function, Value::Intrinsic { .. } => Type::Function, } diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs index b924391e03d..002594ad588 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs @@ -102,7 +102,7 @@ impl PerBlockContext { Instruction::Load { address } => { if let Some(address) = self.try_const_address(*address, dfg) { if let Some(last_value) = self.last_stores.get(&address) { - let last_value = dfg[*last_value]; + let last_value = dfg[*last_value].clone(); loads_to_substitute.push((*instruction_id, last_value)); } else { self.failed_substitutes.insert(address); @@ -212,12 +212,13 @@ impl PerBlockContext { #[cfg(test)] mod tests { use acvm::FieldElement; + use im::vector; use crate::ssa_refactor::{ ir::{ basic_block::BasicBlockId, dfg::DataFlowGraph, - instruction::{BinaryOp, Instruction, Intrinsic, TerminatorInstruction}, + instruction::{Instruction, Intrinsic, TerminatorInstruction}, map::Id, types::Type, }, @@ -228,23 +229,24 @@ mod tests { fn test_simple() { // fn func() { // b0(): - // v0 = alloc 2 - // v1 = add v0, Field 1 - // store v1, Field 1 - // v2 = add v0, Field 1 - // v3 = load v1 - // return v3 + // v0 = allocate + // store [Field 1, Field 2] in v0 + // v1 = load v0 + // v2 = array_get v1, index 1 + // return v2 // } let func_id = Id::test_new(0); let mut builder = FunctionBuilder::new("func".into(), func_id); - let v0 = builder.insert_allocate(2); - let const_one = builder.field_constant(FieldElement::one()); - let v1 = builder.insert_binary(v0, BinaryOp::Add, const_one); - builder.insert_store(v1, const_one); - // v2 is created internally by builder.insert_load - let v3 = builder.insert_load(v0, const_one, Type::field()); - builder.terminate_with_return(vec![v3]); + let v0 = builder.insert_allocate(); + let one = builder.field_constant(FieldElement::one()); + let two = builder.field_constant(FieldElement::one()); + let array = builder.array_constant(vector![one, two]); + + builder.insert_store(v0, array); + let v1 = builder.insert_load(v0, Type::Array); + let v2 = builder.insert_array_get(v1, one); + builder.terminate_with_return(vec![v2]); let ssa = builder.finish().mem2reg(); @@ -258,33 +260,29 @@ mod tests { TerminatorInstruction::Return { return_values } => return_values.first().unwrap(), _ => unreachable!(), }; - assert_eq!(func.dfg[*ret_val_id], func.dfg[const_one]); + assert_eq!(func.dfg[*ret_val_id], func.dfg[two]); } #[test] fn test_simple_with_call() { // fn func { // b0(): - // v0 = alloc 2 - // v1 = add v0, Field 1 - // store v1, Field 1 - // v2 = add v0, Field 1 - // v3 = load v1 - // v4 = call f0, v0 - // return v3 + // v0 = allocate + // store v0, Field 1 + // v1 = load v0 + // v2 = call f0(v0) + // return v1 // } let func_id = Id::test_new(0); let mut builder = FunctionBuilder::new("func".into(), func_id); - let v0 = builder.insert_allocate(2); - let const_one = builder.field_constant(FieldElement::one()); - let v1 = builder.insert_binary(v0, BinaryOp::Add, const_one); - builder.insert_store(v1, const_one); - // v2 is created internally by builder.insert_load - let v3 = builder.insert_load(v0, const_one, Type::field()); + let v0 = builder.insert_allocate(); + let one = builder.field_constant(FieldElement::one()); + builder.insert_store(v0, one); + let v1 = builder.insert_load(v0, Type::field()); let f0 = builder.import_intrinsic_id(Intrinsic::Println); builder.insert_call(f0, vec![v0], vec![Type::Unit]); - builder.terminate_with_return(vec![v3]); + builder.terminate_with_return(vec![v1]); let ssa = builder.finish().mem2reg(); @@ -298,7 +296,7 @@ mod tests { TerminatorInstruction::Return { return_values } => return_values.first().unwrap(), _ => unreachable!(), }; - assert_eq!(func.dfg[*ret_val_id], func.dfg[const_one]); + assert_eq!(func.dfg[*ret_val_id], func.dfg[one]); } fn count_stores(block: BasicBlockId, dfg: &DataFlowGraph) -> usize { @@ -322,36 +320,35 @@ mod tests { fn multiple_blocks() { // fn main { // b0(): - // v0 = alloc 1 - // store v0, Field 5 + // v0 = allocate + // store Field 5 in v0 // v1 = load v0 // jmp b1(v1): // b1(v2: Field): // v3 = load v0 - // store v0, Field 6 + // store Field 6 in v0 // v4 = load v0 // return v2, v3, v4 // } let main_id = Id::test_new(0); let mut builder = FunctionBuilder::new("main".into(), main_id); - let v0 = builder.insert_allocate(1); + let v0 = builder.insert_allocate(); let five = builder.field_constant(5u128); builder.insert_store(v0, five); - let zero = builder.field_constant(0u128); - let v1 = builder.insert_load(v0, zero, Type::field()); + let v1 = builder.insert_load(v0, Type::field()); let b1 = builder.insert_block(); builder.terminate_with_jmp(b1, vec![v1]); builder.switch_to_block(b1); let v2 = builder.add_block_parameter(b1, Type::field()); - let v3 = builder.insert_load(v0, zero, Type::field()); + let v3 = builder.insert_load(v0, Type::field()); let six = builder.field_constant(6u128); builder.insert_store(v0, six); - let v4 = builder.insert_load(v0, zero, Type::field()); + let v4 = builder.insert_load(v0, Type::field()); builder.terminate_with_return(vec![v2, v3, v4]); diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_builder/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_builder/mod.rs index 3fe0b885bde..f4222c2c6db 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_builder/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_builder/mod.rs @@ -82,6 +82,11 @@ impl FunctionBuilder { self.numeric_constant(value.into(), Type::field()) } + /// Insert an array constant into the current function with the given element values + pub(crate) fn array_constant(&mut self, elements: im::Vector) -> ValueId { + self.current_function.dfg.make_array(elements) + } + /// Returns the type of the given value. pub(crate) fn type_of_value(&self, value: ValueId) -> Type { self.current_function.dfg.type_of_value(value) @@ -132,8 +137,8 @@ impl FunctionBuilder { /// Insert an allocate instruction at the end of the current block, allocating the /// given amount of field elements. Returns the result of the allocate instruction, /// which is always a Reference to the allocated data. - pub(crate) fn insert_allocate(&mut self, size_to_allocate: u32) -> ValueId { - self.insert_instruction(Instruction::Allocate { size: size_to_allocate }, None).first() + pub(crate) fn insert_allocate(&mut self) -> ValueId { + self.insert_instruction(Instruction::Allocate, None).first() } /// Insert a Load instruction at the end of the current block, loading from the given offset @@ -143,13 +148,7 @@ impl FunctionBuilder { /// 'offset' is in units of FieldElements here. So loading the fourth FieldElement stored in /// an array will have an offset of 3. /// Returns the element that was loaded. - pub(crate) fn insert_load( - &mut self, - mut address: ValueId, - offset: ValueId, - type_to_load: Type, - ) -> ValueId { - address = self.insert_binary(address, BinaryOp::Add, offset); + pub(crate) fn insert_load(&mut self, address: ValueId, type_to_load: Type) -> ValueId { self.insert_instruction(Instruction::Load { address }, Some(vec![type_to_load])).first() } @@ -200,6 +199,27 @@ impl FunctionBuilder { self.insert_instruction(Instruction::Call { func, arguments }, Some(result_types)).results() } + /// Insert an instruction to extract an element from an array + pub(crate) fn insert_array_get( + &mut self, + array: ValueId, + index: ValueId, + element_type: Type, + ) -> ValueId { + let element_type = Some(vec![element_type]); + self.insert_instruction(Instruction::ArrayGet { array, index }, element_type).first() + } + + /// Insert an instruction to create a new array with the given index replaced with a new value + pub(crate) fn insert_array_set( + &mut self, + array: ValueId, + index: ValueId, + value: ValueId, + ) -> ValueId { + self.insert_instruction(Instruction::ArraySet { array, index, value }, None).first() + } + /// Terminates the current block with the given terminator instruction fn terminate_block_with(&mut self, terminator: TerminatorInstruction) { self.current_function.dfg.set_block_terminator(self.current_block, terminator); diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs index 78c64f9fad8..cb1dc2eb855 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs @@ -141,7 +141,7 @@ impl<'a> FunctionContext<'a> { /// Allocate a single slot of memory and store into it the given initial value of the variable. /// Always returns a Value::Mutable wrapping the allocate instruction. pub(super) fn new_mutable_variable(&mut self, value_to_store: ValueId) -> Value { - let alloc = self.builder.insert_allocate(1); + let alloc = self.builder.insert_allocate(); self.builder.insert_store(alloc, value_to_store); let typ = self.builder.type_of_value(value_to_store); Value::Mutable(alloc, typ) diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs index 4368d926caf..466151ff88f 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs @@ -101,8 +101,7 @@ impl<'a> FunctionContext<'a> { match literal { ast::Literal::Array(array) => { let elements = vecmap(&array.contents, |element| self.codegen_expression(element)); - let element_type = Self::convert_type(&array.element_type); - self.codegen_array(elements, element_type) + self.codegen_array(elements) } ast::Literal::Integer(value, typ) => { let typ = Self::convert_non_tuple_type(typ); @@ -115,7 +114,7 @@ impl<'a> FunctionContext<'a> { let elements = vecmap(string.as_bytes(), |byte| { self.builder.numeric_constant(*byte as u128, Type::field()).into() }); - self.codegen_array(elements, Tree::Leaf(Type::field())) + self.codegen_array(elements) } } } @@ -129,24 +128,17 @@ impl<'a> FunctionContext<'a> { /// stored the same as the array [1, 2, 3, 4]. /// /// The value returned from this function is always that of the allocate instruction. - fn codegen_array(&mut self, elements: Vec, element_type: Tree) -> Values { - let size = element_type.size_of_type() * elements.len(); - let array = self.builder.insert_allocate(size.try_into().unwrap_or_else(|_| { - panic!("Cannot allocate {size} bytes for array, it does not fit into a u32") - })); - - // Now we must manually store all the elements into the array - let mut i = 0u128; + fn codegen_array(&mut self, elements: Vec) -> Values { + let mut array = im::Vector::new(); + for element in elements { element.for_each(|element| { - let address = self.make_offset(array, i); let element = element.eval(self); - self.builder.insert_store(address, element); - i += 1; + array.push_back(element); }); } - array.into() + self.builder.array_constant(array).into() } fn codegen_block(&mut self, block: &[Expression]) -> Values { @@ -205,8 +197,9 @@ impl<'a> FunctionContext<'a> { let offset = self.make_offset(base_index, field_index); field_index += 1; if load_result { - self.builder.insert_load(array, offset, typ) + self.builder.insert_array_get(array, offset, typ) } else { + // TODO self.builder.insert_binary(array, BinaryOp::Add, offset) } .into() diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs index 02011adbaa8..8904c84594b 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs @@ -41,10 +41,7 @@ impl Value { pub(super) fn eval(self, ctx: &mut FunctionContext) -> IrValueId { match self { Value::Normal(value) => value, - Value::Mutable(address, typ) => { - let offset = ctx.builder.field_constant(0u128); - ctx.builder.insert_load(address, offset, typ) - } + Value::Mutable(address, typ) => ctx.builder.insert_load(address, typ), } } From 31dd36f3df2824d4ae0bbba355c97c168d26cc7a Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 2 Jun 2023 15:38:15 -0500 Subject: [PATCH 02/21] Get tests passing --- .../src/ssa_refactor/ir/printer.rs | 6 +- .../src/ssa_refactor/opt/constant_folding.rs | 105 ++++++++++++++++++ .../src/ssa_refactor/opt/flatten_cfg.rs | 10 +- .../src/ssa_refactor/opt/inlining.rs | 4 + .../src/ssa_refactor/opt/mem2reg.rs | 8 +- .../src/ssa_refactor/opt/mod.rs | 1 + 6 files changed, 123 insertions(+), 11 deletions(-) create mode 100644 crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs index 3fa37e0712c..37c4177e7dd 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs @@ -67,7 +67,11 @@ fn value(function: &Function, id: ValueId) -> String { } Value::Function(id) => id.to_string(), Value::Intrinsic(intrinsic) => intrinsic.to_string(), - _ => id.to_string(), + Value::Array(elements) => { + let elements = vecmap(elements, |element| value(function, *element)); + format!("[{}]", elements.join(", ")) + } + Value::Param { .. } | Value::Instruction { .. } => id.to_string(), } } diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs new file mode 100644 index 00000000000..3a8faa44dd7 --- /dev/null +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs @@ -0,0 +1,105 @@ +use std::collections::{HashMap, HashSet}; + +use iter_extended::vecmap; + +use crate::ssa_refactor::{ + ir::{ + basic_block::BasicBlockId, dfg::InsertInstructionResult, function::Function, + instruction::InstructionId, value::ValueId, + }, + ssa_gen::Ssa, +}; + +impl Ssa { + pub(crate) fn fold_constants(mut self) -> Ssa { + for function in self.functions.values_mut() { + constant_fold(function); + } + self + } +} + +fn constant_fold(function: &mut Function) { + let mut context = Context::default(); + context.block_queue.push(function.entry_block()); + + while let Some(block) = context.block_queue.pop() { + if context.visited_blocks.contains(&block) { + continue; + } + + context.fold_constants_in_block(function, block); + } +} + +#[derive(Default)] +struct Context { + /// Maps pre-unrolled ValueIds to unrolled ValueIds. + /// These will often be the exact same as before, unless the ValueId was + /// dependent on the loop induction variable which is changing on each iteration. + values: HashMap, + + visited_blocks: HashSet, + block_queue: Vec, +} + +impl Context { + fn fold_constants_in_block(&mut self, function: &mut Function, block: BasicBlockId) { + let instructions = std::mem::take(function.dfg[block].instructions_mut()); + + for instruction in instructions { + self.push_instruction(function, block, instruction); + } + + let terminator = + function.dfg[block].unwrap_terminator().map_values(|value| self.get_value(value)); + + function.dfg.set_block_terminator(block, terminator); + self.block_queue.extend(function.dfg[block].successors()); + } + + fn get_value(&self, value: ValueId) -> ValueId { + self.values.get(&value).copied().unwrap_or(value) + } + + fn push_instruction( + &mut self, + function: &mut Function, + block: BasicBlockId, + id: InstructionId, + ) { + let instruction = function.dfg[id].map_values(|id| self.get_value(id)); + let results = function.dfg.instruction_results(id).to_vec(); + + let ctrl_typevars = instruction + .requires_ctrl_typevars() + .then(|| vecmap(&results, |result| function.dfg.type_of_value(*result))); + + let new_results = + function.dfg.insert_instruction_and_results(instruction, block, ctrl_typevars); + + Self::insert_new_instruction_results(&mut self.values, &results, new_results); + } + + /// Modify the values HashMap to remember the mapping between an instruction result's previous + /// ValueId (from the source_function) and its new ValueId in the destination function. + fn insert_new_instruction_results( + values: &mut HashMap, + old_results: &[ValueId], + new_results: InsertInstructionResult, + ) { + assert_eq!(old_results.len(), new_results.len()); + + match new_results { + InsertInstructionResult::SimplifiedTo(new_result) => { + values.insert(old_results[0], new_result); + } + InsertInstructionResult::Results(new_results) => { + for (old_result, new_result) in old_results.iter().zip(new_results) { + values.insert(*old_result, *new_result); + } + } + InsertInstructionResult::InstructionRemoved => (), + } + } +} diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs index c286df4730e..74b43f9f5ec 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs @@ -946,7 +946,7 @@ mod test { let c1 = builder.add_parameter(Type::bool()); let c4 = builder.add_parameter(Type::bool()); - let r1 = builder.insert_allocate(1); + let r1 = builder.insert_allocate(); let store_value = |builder: &mut FunctionBuilder, value: u128| { let value = builder.field_constant(value); @@ -956,9 +956,8 @@ mod test { let println = builder.import_intrinsic_id(Intrinsic::Println); let call_println = |builder: &mut FunctionBuilder, block: u128| { - let zero = builder.field_constant(0u128); let block = builder.field_constant(block); - let load = builder.insert_load(r1, zero, Type::field()); + let load = builder.insert_load(r1, Type::field()); builder.insert_call(println, vec![block, load], Vec::new()); }; @@ -1002,14 +1001,11 @@ mod test { builder.terminate_with_jmp(b9, vec![]); switch_and_print(&mut builder, b9, 9); - let zero = builder.field_constant(0u128); - let load = builder.insert_load(r1, zero, Type::field()); + let load = builder.insert_load(r1, Type::field()); builder.terminate_with_return(vec![load]); let ssa = builder.finish().flatten_cfg().mem2reg(); - println!("{ssa}"); - // Expected results after mem2reg removes the allocation and each load and store: // // fn main f0 { diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs index 5275a37b95c..9bc6728c975 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs @@ -207,6 +207,10 @@ impl<'function> PerFunctionContext<'function> { } Value::Function(function) => self.context.builder.import_function(*function), Value::Intrinsic(intrinsic) => self.context.builder.import_intrinsic_id(*intrinsic), + Value::Array(array) => { + let elements = array.iter().map(|value| self.translate_value(*value)).collect(); + self.context.builder.array_constant(elements) + } }; self.values.insert(id, new_value); diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs index 002594ad588..63390130c43 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs @@ -128,7 +128,7 @@ impl PerBlockContext { .instruction_results(*instruction_id) .first() .expect("ICE: Load instructions should have single result"); - dfg.set_value(result_value, *new_value); + dfg.set_value(result_value, new_value.clone()); } // Delete load instructions @@ -245,10 +245,12 @@ mod tests { builder.insert_store(v0, array); let v1 = builder.insert_load(v0, Type::Array); - let v2 = builder.insert_array_get(v1, one); + let v2 = builder.insert_array_get(v1, one, Type::field()); builder.terminate_with_return(vec![v2]); - let ssa = builder.finish().mem2reg(); + let ssa = builder.finish().mem2reg().fold_constants(); + + println!("{ssa}"); let func = ssa.main(); let block_id = func.entry_block(); diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/mod.rs index 41d523c3b33..fcb30f09ae5 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/mod.rs @@ -3,6 +3,7 @@ //! Each pass is generally expected to mutate the SSA IR into a gradually //! simpler form until the IR only has a single function remaining with 1 block within it. //! Generally, these passes are also expected to minimize the final amount of instructions. +mod constant_folding; mod flatten_cfg; mod inlining; mod mem2reg; From 5d4be5c93262f6a1ec04bc0a3009702fc6ae8bbe Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 6 Jun 2023 15:20:26 -0500 Subject: [PATCH 03/21] Implement assign with immutable arrays --- crates/noirc_evaluator/src/ssa_refactor.rs | 2 + .../src/ssa_refactor/ssa_gen/context.rs | 157 +++++++++++++++++- .../src/ssa_refactor/ssa_gen/mod.rs | 49 +----- 3 files changed, 158 insertions(+), 50 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor.rs b/crates/noirc_evaluator/src/ssa_refactor.rs index 199f6328090..117f71e12c0 100644 --- a/crates/noirc_evaluator/src/ssa_refactor.rs +++ b/crates/noirc_evaluator/src/ssa_refactor.rs @@ -42,6 +42,8 @@ pub(crate) fn optimize_into_acir(program: Program) -> GeneratedAcir { .print("After Flattening:") .mem2reg() .print("After Mem2Reg:") + .fold_constants() + .print("After Constant Folding:") .into_acir(func_signature) } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs index cb1dc2eb855..b556e0f273b 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs @@ -284,8 +284,147 @@ impl<'a> FunctionContext<'a> { } } - /// Mutate lhs to equal rhs - pub(crate) fn assign(&mut self, lhs: Values, rhs: Values) { + /// Extract the given field of the tuple by reference. Panics if the given Values is not + /// a Tree::Branch or does not have enough fields. + pub(super) fn get_field_ref(tuple: &Values, field_index: usize) -> &Values { + match tuple { + Tree::Branch(trees) => &trees[field_index], + Tree::Leaf(value) => { + unreachable!("Tried to extract tuple index {field_index} from non-tuple {value:?}") + } + } + } + + /// Replace the given field of the tuple with a new one. Panics if the given Values is not + /// a Tree::Branch or does not have enough fields. + pub(super) fn replace_field(tuple: Values, field_index: usize, new_value: Values) -> Values { + match tuple { + Tree::Branch(mut trees) => { + trees[field_index] = new_value; + Tree::Branch(trees) + } + Tree::Leaf(value) => { + unreachable!("Tried to extract tuple index {field_index} from non-tuple {value:?}") + } + } + } + + /// Retrieves the given function, adding it to the function queue + /// if it is not yet compiled. + pub(super) fn get_or_queue_function(&mut self, id: FuncId) -> Values { + let function = self.shared_context.get_or_queue_function(id); + self.builder.import_function(function).into() + } + + /// Extracts the current value out of an LValue. + /// + /// Goal: Handle the case of assigning to nested expressions such as `foo.bar[i1].baz[i2] = e` + /// while also noting that assigning to arrays will create a new array rather than mutate + /// the original. + /// + /// Method: First `extract_current_value` must recurse on the lvalue to extract the current + /// value contained: + /// + /// v0 = foo.bar ; allocate instruction for bar + /// v1 = load v0 ; loading the bar array + /// v2 = add i1, baz_index ; field offset for index i1, field baz + /// v3 = array_get v1, index v2 ; foo.bar[i1].baz + /// + /// Method (part 2): Then, `assign_new_value` will recurse in the opposite direction to + /// construct the larger value as needed until we can `store` to the nearest + /// allocation. + /// + /// v4 = array_set v3, index i2, e ; finally create a new array setting the desired value + /// v5 = array_set v1, index v2, v4 ; now must also create the new bar array + /// store v5 in v0 ; and store the result in the only mutable reference + /// + /// The returned `LValueRef` tracks the current value at each step of the lvalue. + /// This is later used by `assign_new_value` to construct a new updated value that + /// can be assigned to an allocation within the LValueRef::Ident. + /// + /// This is operationally equivalent to extract_current_value_recursive, but splitting these + /// into two separate functions avoids cloning the outermost `Values` returned by the recursive + /// version, as it is only needed for recursion. + pub(super) fn extract_current_value(&mut self, lvalue: &ast::LValue) -> LValue { + match lvalue { + ast::LValue::Ident(ident) => LValue::Ident(self.ident_lvalue(ident)), + ast::LValue::Index { array, index, .. } => self.index_lvalue(array, index).2, + ast::LValue::MemberAccess { object, field_index } => { + let (old_object, object_lvalue) = self.extract_current_value_recursive(object); + let object_lvalue = Box::new(object_lvalue); + LValue::MemberAccess { old_object, object_lvalue, index: *field_index } + } + } + } + + /// Compile the given identifier as a reference - ie. avoid calling .eval() + fn ident_lvalue(&self, ident: &ast::Ident) -> Values { + match &ident.definition { + ast::Definition::Local(id) => self.lookup(*id), + other => panic!("Unexpected definition found for mutable value: {other}"), + } + } + + /// Compile the given `array[index]` expression as a reference. + /// This will return a triple of (array, index, lvalue_ref) where the lvalue_ref records the + /// structure of the lvalue expression for use by `assign_new_value`. + fn index_lvalue( + &mut self, + array: &ast::LValue, + index: &ast::Expression, + ) -> (ValueId, ValueId, LValue) { + let (old_array, array_lvalue) = self.extract_current_value_recursive(array); + let old_array = old_array.into_leaf().eval(self); + let array_lvalue = Box::new(array_lvalue); + let index = self.codegen_non_tuple_expression(index); + (old_array, index, LValue::Index { old_array, index, array_lvalue }) + } + + fn extract_current_value_recursive(&mut self, lvalue: &ast::LValue) -> (Values, LValue) { + match lvalue { + ast::LValue::Ident(ident) => { + let variable = self.ident_lvalue(ident); + (variable.clone(), LValue::Ident(variable)) + } + ast::LValue::Index { array, index, element_type, location: _ } => { + let (old_array, index, index_lvalue) = self.index_lvalue(array, index); + let element = self.codegen_array_index(old_array, index, element_type); + (element, index_lvalue) + } + ast::LValue::MemberAccess { object, field_index: index } => { + let (old_object, object_lvalue) = self.extract_current_value_recursive(object); + let object_lvalue = Box::new(object_lvalue); + let element = Self::get_field_ref(&old_object, *index).clone(); + (element, LValue::MemberAccess { old_object, object_lvalue, index: *index }) + } + } + } + + /// Assigns a new value to the given LValue. + /// The LValue can be created via a previous call to extract_current_value. + /// This method recurs on the given LValue to create a new value to assign an allocation + /// instruction within an LValue::Ident - see the comment on `extract_current_value` for more + /// details. + /// When first-class references are supported the nearest reference may be in any LValue + /// variant rather than just LValue::Ident. + pub(super) fn assign_new_value(&mut self, lvalue: LValue, new_value: Values) { + match lvalue { + LValue::Ident(references) => self.assign(references, new_value), + LValue::Index { old_array, index, array_lvalue } => { + let rvalue = new_value.into_leaf().eval(self); // TODO + let new_array = self.builder.insert_array_set(old_array, index, rvalue); + self.assign_new_value(*array_lvalue, new_array.into()); + } + LValue::MemberAccess { old_object, index, object_lvalue } => { + let new_object = Self::replace_field(old_object, index, new_value); + self.assign_new_value(*object_lvalue, new_object) + } + } + } + + /// Given an lhs containing only references, create a store instruction to store each value of + /// rhs into its corresponding value in lhs. + fn assign(&mut self, lhs: Values, rhs: Values) { match (lhs, rhs) { (Tree::Branch(lhs_branches), Tree::Branch(rhs_branches)) => { assert_eq!(lhs_branches.len(), rhs_branches.len()); @@ -305,13 +444,6 @@ impl<'a> FunctionContext<'a> { } } } - - /// Retrieves the given function, adding it to the function queue - /// if it is not yet compiled. - pub(super) fn get_or_queue_function(&mut self, id: FuncId) -> Values { - let function = self.shared_context.get_or_queue_function(id); - self.builder.import_function(function).into() - } } /// True if the given operator cannot be encoded directly and needs @@ -394,3 +526,10 @@ impl SharedContext { next_id } } + +/// Used to remember the results of each step of extracting a value from an ast::LValue +pub(super) enum LValue { + Ident(Values), + Index { old_array: ValueId, index: ValueId, array_lvalue: Box }, + MemberAccess { old_object: Values, index: usize, object_lvalue: Box }, +} diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs index 466151ff88f..586ec1f5ddb 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs @@ -169,7 +169,8 @@ impl<'a> FunctionContext<'a> { fn codegen_index(&mut self, index: &ast::Index) -> Values { let array = self.codegen_non_tuple_expression(&index.collection); - self.codegen_array_index(array, &index.index, &index.element_type, true) + let index_value = self.codegen_non_tuple_expression(&index.index); + self.codegen_array_index(array, index_value, &index.element_type) } /// This is broken off from codegen_index so that it can also be @@ -181,28 +182,19 @@ impl<'a> FunctionContext<'a> { fn codegen_array_index( &mut self, array: super::ir::value::ValueId, - index: &ast::Expression, + index: super::ir::value::ValueId, element_type: &ast::Type, - load_result: bool, ) -> Values { - let base_offset = self.codegen_non_tuple_expression(index); - - // base_index = base_offset * type_size + // base_index = index * type_size let type_size = Self::convert_type(element_type).size_of_type(); let type_size = self.builder.field_constant(type_size as u128); - let base_index = self.builder.insert_binary(base_offset, BinaryOp::Mul, type_size); + let base_index = self.builder.insert_binary(index, BinaryOp::Mul, type_size); let mut field_index = 0u128; Self::map_type(element_type, |typ| { let offset = self.make_offset(base_index, field_index); field_index += 1; - if load_result { - self.builder.insert_array_get(array, offset, typ) - } else { - // TODO - self.builder.insert_binary(array, BinaryOp::Add, offset) - } - .into() + self.builder.insert_array_get(array, offset, typ).into() }) } @@ -371,38 +363,13 @@ impl<'a> FunctionContext<'a> { } fn codegen_assign(&mut self, assign: &ast::Assign) -> Values { - let lhs = self.codegen_lvalue(&assign.lvalue); + let lhs = self.extract_current_value(&assign.lvalue); let rhs = self.codegen_expression(&assign.expression); - self.assign(lhs, rhs); + self.assign_new_value(lhs, rhs); self.unit_value() } - fn codegen_lvalue(&mut self, lvalue: &ast::LValue) -> Values { - match lvalue { - ast::LValue::Ident(ident) => { - // Do not .eval the Values here! We do not want to load from any references within - // since we want to return the references instead - match &ident.definition { - ast::Definition::Local(id) => self.lookup(*id), - other => panic!("Unexpected definition found for mutable value: {other}"), - } - } - ast::LValue::Index { array, index, element_type, location: _ } => { - // Note that unlike the Ident case, we're .eval'ing the array here. - // This is because arrays are already references and thus a mutable reference - // to an array would be a Value::Mutable( Value::Mutable ( address ) ), and we - // only need the inner mutable value. - let array = self.codegen_lvalue(array).into_leaf().eval(self); - self.codegen_array_index(array, index, element_type, false) - } - ast::LValue::MemberAccess { object, field_index } => { - let object = self.codegen_lvalue(object); - Self::get_field(object, *field_index) - } - } - } - fn codegen_semi(&mut self, expr: &Expression) -> Values { self.codegen_expression(expr); self.unit_value() From 584f9722cc1055561d295bdab2df1c19e9871bfa Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 6 Jun 2023 15:38:36 -0500 Subject: [PATCH 04/21] Add constant folding pass --- crates/noirc_evaluator/src/ssa_refactor.rs | 2 + .../src/ssa_refactor/opt/constant_folding.rs | 180 ++++++++++++++++++ .../src/ssa_refactor/opt/mod.rs | 1 + 3 files changed, 183 insertions(+) create mode 100644 crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs diff --git a/crates/noirc_evaluator/src/ssa_refactor.rs b/crates/noirc_evaluator/src/ssa_refactor.rs index b2f350e32ad..776f7c992ab 100644 --- a/crates/noirc_evaluator/src/ssa_refactor.rs +++ b/crates/noirc_evaluator/src/ssa_refactor.rs @@ -42,6 +42,8 @@ pub(crate) fn optimize_into_acir(program: Program, allow_log_ops: bool) -> Gener .print("After Flattening:") .mem2reg() .print("After Mem2Reg:") + .fold_constants() + .print("After Constant Folding:") .into_acir(func_signature, brillig, allow_log_ops) } diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs new file mode 100644 index 00000000000..9921174eb52 --- /dev/null +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs @@ -0,0 +1,180 @@ +use std::collections::{HashMap, HashSet}; + +use iter_extended::vecmap; + +use crate::ssa_refactor::{ + ir::{ + basic_block::BasicBlockId, dfg::InsertInstructionResult, function::Function, + instruction::InstructionId, value::ValueId, + }, + ssa_gen::Ssa, +}; + +impl Ssa { + /// Performs constant folding on each instruction. + /// + /// This is generally done automatically but this pass can become needed + /// if `DataFlowGraph::set_value` or `DataFlowGraph::set_value_from_id` are + /// used on a value which enables instructions dependent on the value to + /// now be simplified. + pub(crate) fn fold_constants(mut self) -> Ssa { + for function in self.functions.values_mut() { + constant_fold(function); + } + self + } +} + +fn constant_fold(function: &mut Function) { + let mut context = Context::default(); + context.block_queue.push(function.entry_block()); + + while let Some(block) = context.block_queue.pop() { + if context.visited_blocks.contains(&block) { + continue; + } + + context.fold_constants_in_block(function, block); + } +} + +#[derive(Default)] +struct Context { + /// Maps pre-unrolled ValueIds to unrolled ValueIds. + /// These will often be the exact same as before, unless the ValueId was + /// dependent on the loop induction variable which is changing on each iteration. + values: HashMap, + + visited_blocks: HashSet, + block_queue: Vec, +} + +impl Context { + fn fold_constants_in_block(&mut self, function: &mut Function, block: BasicBlockId) { + let instructions = std::mem::take(function.dfg[block].instructions_mut()); + + for instruction in instructions { + self.push_instruction(function, block, instruction); + } + + let terminator = + function.dfg[block].unwrap_terminator().map_values(|value| self.get_value(value)); + + function.dfg.set_block_terminator(block, terminator); + self.block_queue.extend(function.dfg[block].successors()); + } + + fn get_value(&self, value: ValueId) -> ValueId { + self.values.get(&value).copied().unwrap_or(value) + } + + fn push_instruction( + &mut self, + function: &mut Function, + block: BasicBlockId, + id: InstructionId, + ) { + let instruction = function.dfg[id].map_values(|id| self.get_value(id)); + let results = function.dfg.instruction_results(id).to_vec(); + + let ctrl_typevars = instruction + .requires_ctrl_typevars() + .then(|| vecmap(&results, |result| function.dfg.type_of_value(*result))); + + let new_results = + function.dfg.insert_instruction_and_results(instruction, block, ctrl_typevars); + + Self::insert_new_instruction_results(&mut self.values, &results, new_results); + } + + /// Modify the values HashMap to remember the mapping between an instruction result's previous + /// ValueId (from the source_function) and its new ValueId in the destination function. + fn insert_new_instruction_results( + values: &mut HashMap, + old_results: &[ValueId], + new_results: InsertInstructionResult, + ) { + assert_eq!(old_results.len(), new_results.len()); + + match new_results { + InsertInstructionResult::SimplifiedTo(new_result) => { + values.insert(old_results[0], new_result); + } + InsertInstructionResult::Results(new_results) => { + for (old_result, new_result) in old_results.iter().zip(new_results) { + values.insert(*old_result, *new_result); + } + } + InsertInstructionResult::InstructionRemoved => (), + } + } +} + +#[cfg(test)] +mod test { + use crate::ssa_refactor::{ + ir::{ + function::RuntimeType, + instruction::{BinaryOp, TerminatorInstruction}, + map::Id, + types::Type, + }, + ssa_builder::FunctionBuilder, + }; + + #[test] + fn simple_constant_fold() { + // fn main f0 { + // b0(v0: Field): + // v1 = add v0, Field 1 + // v2 = mul v1, Field 3 + // return v2 + // } + // + // After constructing this IR, we set the value of v0 to 2. + // The expected return afterwards should be 9. + let main_id = Id::test_new(0); + + // Compiling main + let mut builder = FunctionBuilder::new("main".into(), main_id, RuntimeType::Acir); + let v0 = builder.add_parameter(Type::field()); + + let one = builder.field_constant(1u128); + let two = builder.field_constant(2u128); + let three = builder.field_constant(3u128); + + let v1 = builder.insert_binary(v0, BinaryOp::Add, one); + let v2 = builder.insert_binary(v1, BinaryOp::Mul, three); + builder.terminate_with_return(vec![v2]); + + let mut ssa = builder.finish(); + let main = ssa.main_mut(); + let instructions = main.dfg[main.entry_block()].instructions(); + assert_eq!(instructions.len(), 2); // The final return is not counted + + // Expected output: + // + // fn main f0 { + // b0(v0: Field): + // return Field 9 + // } + main.dfg.set_value_from_id(v0, two); + + let ssa = ssa.fold_constants(); + let main = ssa.main(); + let block = &main.dfg[main.entry_block()]; + assert_eq!(block.instructions().len(), 0); + + match block.terminator() { + Some(TerminatorInstruction::Return { return_values }) => { + let value = main + .dfg + .get_numeric_constant(return_values[0]) + .expect("Expected constant 9") + .to_u128(); + assert_eq!(value, 9); + } + _ => unreachable!("b0 should have a return terminator"), + } + } +} diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/mod.rs index 41d523c3b33..fcb30f09ae5 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/mod.rs @@ -3,6 +3,7 @@ //! Each pass is generally expected to mutate the SSA IR into a gradually //! simpler form until the IR only has a single function remaining with 1 block within it. //! Generally, these passes are also expected to minimize the final amount of instructions. +mod constant_folding; mod flatten_cfg; mod inlining; mod mem2reg; From 399b708d8f64d06a63dc324fab3a6e0c8ff53c65 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 6 Jun 2023 15:43:03 -0500 Subject: [PATCH 05/21] Update comments --- .../src/ssa_refactor/opt/constant_folding.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs index 9921174eb52..94a420c9b3f 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/constant_folding.rs @@ -25,6 +25,8 @@ impl Ssa { } } +/// The structure of this pass is simple: +/// Go through each block and re-insert all instructions. fn constant_fold(function: &mut Function) { let mut context = Context::default(); context.block_queue.push(function.entry_block()); @@ -40,11 +42,8 @@ fn constant_fold(function: &mut Function) { #[derive(Default)] struct Context { - /// Maps pre-unrolled ValueIds to unrolled ValueIds. - /// These will often be the exact same as before, unless the ValueId was - /// dependent on the loop induction variable which is changing on each iteration. + /// Maps pre-folded ValueIds to the new ValueIds obtained by re-inserting the instruction. values: HashMap, - visited_blocks: HashSet, block_queue: Vec, } From 3e806987f11d8b91e4206dc711423bd9f20bb7d1 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 6 Jun 2023 16:20:59 -0500 Subject: [PATCH 06/21] Clippy --- .../noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs | 12 +++++++++--- .../src/ssa_refactor/ssa_gen/context.rs | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs index 3c258885085..7274458d744 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs @@ -408,9 +408,15 @@ impl Context { match (lhs_type, rhs_type) { // Function type should not be possible, since all functions // have been inlined. - (_, Type::Function) | (Type::Function, _) => unreachable!("all functions should be inlined"), - (_, Type::Reference) | (Type::Reference, _) => unreachable!("References are invalid in binary operations"), - (_, Type::Array) | (Type::Array, _) => unreachable!("Arrays are invalid in binary operations"), + (_, Type::Function) | (Type::Function, _) => { + unreachable!("all functions should be inlined") + } + (_, Type::Reference) | (Type::Reference, _) => { + unreachable!("References are invalid in binary operations") + } + (_, Type::Array) | (Type::Array, _) => { + unreachable!("Arrays are invalid in binary operations") + } // Unit type currently can mean a 0 constant, so we return the // other type. (typ, Type::Unit) | (Type::Unit, typ) => typ, diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs index 8507611cb60..ed4b1972465 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs @@ -422,7 +422,7 @@ impl<'a> FunctionContext<'a> { } LValue::MemberAccess { old_object, index, object_lvalue } => { let new_object = Self::replace_field(old_object, index, new_value); - self.assign_new_value(*object_lvalue, new_object) + self.assign_new_value(*object_lvalue, new_object); } } } From e6be42114e736b6f0b6d4e8e5508a4216f9972e2 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 6 Jun 2023 16:28:37 -0500 Subject: [PATCH 07/21] Update comment --- crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs index a15422b8b80..ed51f3397cc 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs @@ -172,8 +172,7 @@ impl DataFlowGraph { id } - /// Returns the field element represented by this value if it is a numeric constant. - /// Returns None if the given value is not a numeric constant. + /// Create a new constant array value from the given elements pub(crate) fn make_array(&mut self, elements: im::Vector) -> ValueId { self.make_value(Value::Array(elements)) } From fc296ab25e86e872c24f76e022399bde90c9b769 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 6 Jun 2023 16:32:25 -0500 Subject: [PATCH 08/21] Update type of array --- crates/noirc_evaluator/src/ssa_refactor/ir/value.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs index b83e761dc89..36b83e65f8a 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs @@ -57,7 +57,7 @@ impl Value { Value::Instruction { typ, .. } => *typ, Value::Param { typ, .. } => *typ, Value::NumericConstant { typ, .. } => *typ, - Value::Array(_) => Type::Reference, + Value::Array(_) => Type::Array, Value::Function { .. } => Type::Function, Value::Intrinsic { .. } => Type::Function, } From 910700553a11e4d159212bd708f31867ca8e1e7a Mon Sep 17 00:00:00 2001 From: jfecher Date: Wed, 7 Jun 2023 08:59:40 -0500 Subject: [PATCH 09/21] Update crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs index 9664cf56a8e..8c5a79a0541 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs @@ -218,7 +218,9 @@ impl Instruction { let index = index.try_to_u64().expect("Expected array index to fit in u64") as usize; assert!(index < array.len()); - return SimplifiedTo(array[index]); + SimplifiedTo(array[index]) + } else { + None } } Instruction::ArraySet { array, index, value } => { @@ -227,7 +229,9 @@ impl Instruction { let index = index.try_to_u64().expect("Expected array index to fit in u64") as usize; assert!(index < array.len()); - return SimplifiedTo(dfg.make_array(array.update(index, *value))); + SimplifiedTo(dfg.make_array(array.update(index, *value))) + } else { + None } } Instruction::Truncate { .. } => (), From 3aaa9784cb82a03bda625b5b1f78c0bb8e5f2b5f Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 7 Jun 2023 09:02:12 -0500 Subject: [PATCH 10/21] Undo formatting changes in instruction.rs --- .../src/ssa_refactor/ir/instruction.rs | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs index 8c5a79a0541..e20657c6625 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs @@ -184,7 +184,9 @@ impl Instruction { Instruction::Binary(binary) => return binary.simplify(dfg), Instruction::Cast(value, typ) => { if let Some(value) = (*typ == dfg.type_of_value(*value)).then_some(*value) { - return SimplifiedTo(value); + SimplifiedTo(value) + } else { + None } } Instruction::Not(value) => { @@ -194,15 +196,17 @@ impl Instruction { // would be incorrect however since the extra bits on the field would not be flipped. Value::NumericConstant { constant, typ } if *typ == Type::bool() => { let value = constant.is_zero() as u128; - return SimplifiedTo(dfg.make_constant(value.into(), Type::bool())); + SimplifiedTo(dfg.make_constant(value.into(), Type::bool())) } Value::Instruction { instruction, .. } => { // !!v => v if let Instruction::Not(value) = &dfg[*instruction] { - return SimplifiedTo(*value); + SimplifiedTo(*value) + } else { + None } } - _ => (), + _ => None, } } Instruction::Constrain(value) => { @@ -211,6 +215,7 @@ impl Instruction { return Remove; } } + None } Instruction::ArrayGet { array, index } => { let array = dfg.get_array_constant(*array); @@ -234,13 +239,12 @@ impl Instruction { None } } - Instruction::Truncate { .. } => (), - Instruction::Call { .. } => (), - Instruction::Allocate { .. } => (), - Instruction::Load { .. } => (), - Instruction::Store { .. } => (), + Instruction::Truncate { .. } => None, + Instruction::Call { .. } => None, + Instruction::Allocate { .. } => None, + Instruction::Load { .. } => None, + Instruction::Store { .. } => None, } - None } } From 65f5b0c5cc0e6477c274ef1a6b9be78c2fb0f742 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 7 Jun 2023 12:47:42 -0500 Subject: [PATCH 11/21] Massive acir_gen update --- crates/noirc_evaluator/src/ssa_refactor.rs | 3 +- .../src/ssa_refactor/abi_gen/mod.rs | 87 +--- .../src/ssa_refactor/acir_gen/acir_ir.rs | 1 - .../acir_gen/acir_ir/acir_variable.rs | 256 ++++------ .../ssa_refactor/acir_gen/acir_ir/errors.rs | 6 +- .../ssa_refactor/acir_gen/acir_ir/memory.rs | 122 ----- .../src/ssa_refactor/acir_gen/mod.rs | 444 ++++++++---------- .../src/ssa_refactor/ir/dfg.rs | 23 +- .../src/ssa_refactor/ir/instruction.rs | 16 +- .../src/ssa_refactor/ir/printer.rs | 4 +- .../src/ssa_refactor/ir/types.rs | 25 +- .../src/ssa_refactor/ir/value.rs | 14 +- .../src/ssa_refactor/opt/inlining.rs | 6 +- .../src/ssa_refactor/opt/mem2reg.rs | 8 +- .../src/ssa_refactor/ssa_builder/mod.rs | 13 +- .../src/ssa_refactor/ssa_gen/mod.rs | 20 +- .../src/ssa_refactor/ssa_gen/value.rs | 2 +- 17 files changed, 378 insertions(+), 672 deletions(-) delete mode 100644 crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/memory.rs diff --git a/crates/noirc_evaluator/src/ssa_refactor.rs b/crates/noirc_evaluator/src/ssa_refactor.rs index 776f7c992ab..827a221b15b 100644 --- a/crates/noirc_evaluator/src/ssa_refactor.rs +++ b/crates/noirc_evaluator/src/ssa_refactor.rs @@ -29,7 +29,6 @@ pub mod ssa_gen; /// form and performing optimizations there. When finished, /// convert the final SSA into ACIR and return it. pub(crate) fn optimize_into_acir(program: Program, allow_log_ops: bool) -> GeneratedAcir { - let func_signature = program.main_function_signature.clone(); let ssa = ssa_gen::generate_ssa(program).print("Initial SSA:"); let brillig = ssa.to_brillig(); ssa.inline_functions() @@ -44,7 +43,7 @@ pub(crate) fn optimize_into_acir(program: Program, allow_log_ops: bool) -> Gener .print("After Mem2Reg:") .fold_constants() .print("After Constant Folding:") - .into_acir(func_signature, brillig, allow_log_ops) + .into_acir(brillig, allow_log_ops) } /// Compiles the Program into ACIR and applies optimizations to the arithmetic gates diff --git a/crates/noirc_evaluator/src/ssa_refactor/abi_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/abi_gen/mod.rs index 400744fda11..db39b1c8110 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/abi_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/abi_gen/mod.rs @@ -2,92 +2,7 @@ use std::collections::BTreeMap; use acvm::acir::native_types::Witness; use iter_extended::{btree_map, vecmap}; -use noirc_abi::{Abi, AbiParameter, AbiType, FunctionSignature}; - -/// Traverses the parameters to the program to infer the lengths of any arrays that occur. -/// -/// This is needed for the acir_gen pass, because while the SSA representation of the program -/// knows the positions at which any arrays occur in the parameters to main, it does not know the -/// lengths of said arrays. -/// -/// This function returns the lengths ordered such as to correspond to the ordering used by the -/// SSA representation. This allows the lengths to be consumed as array params are encountered in -/// the SSA. -pub(crate) fn collate_array_info(abi_params: &[AbiParameter]) -> Vec<(usize, AbiType)> { - let mut acc = Vec::new(); - for abi_param in abi_params { - collate_array_info_recursive(&mut acc, &abi_param.typ); - } - acc -} - -/// The underlying recursive implementation of `collate_array_info` -/// -/// This does a depth-first traversal of the abi until an array (or string) is encountered, at -/// which point arrays are handled differently depending on the element type: -/// - arrays of fields, integers or booleans produce an array of the specified length -/// - arrays of structs produce an array of the specified length for each field of the flatten -/// struct (which reflects a simplification made during monomorphization) -fn collate_array_info_recursive(acc: &mut Vec<(usize, AbiType)>, abi_type: &AbiType) { - match abi_type { - AbiType::Array { length, typ: elem_type } => { - let elem_type = elem_type.as_ref(); - match elem_type { - AbiType::Array { .. } => { - unreachable!("2D arrays are not supported"); - } - AbiType::Struct { .. } => { - // monomorphization converts arrays of structs into an array per flattened - // struct field. - let mut destructured_array_types = Vec::new(); - flatten_abi_type_recursive(&mut destructured_array_types, elem_type); - for abi_type in destructured_array_types { - acc.push((*length as usize, abi_type)); - } - } - AbiType::String { .. } => { - unreachable!("Arrays of strings are not supported"); - } - AbiType::Boolean | AbiType::Field | AbiType::Integer { .. } => { - // Simple 1D array - acc.push((*length as usize, elem_type.clone())); - } - } - } - AbiType::Struct { fields } => { - for (_, field_type) in fields { - collate_array_info_recursive(acc, field_type); - } - } - AbiType::String { length } => { - // Strings are implemented as u8 arrays - let element_type = AbiType::Integer { sign: noirc_abi::Sign::Unsigned, width: 8 }; - acc.push((*length as usize, element_type)); - } - AbiType::Boolean | AbiType::Field | AbiType::Integer { .. } => { - // Do not produce arrays - } - } -} - -/// Used for flattening a struct into its ordered constituent field types. This is needed for -/// informing knowing the bit widths of any array sets that were destructured from an array of -/// structs. For this reason, any array encountered within this function are considered to be -/// nested within a struct and are therefore disallowed. This is acceptable because this function -/// will only be applied to structs which have been found in an array. -fn flatten_abi_type_recursive(acc: &mut Vec, abi_type: &AbiType) { - match abi_type { - AbiType::Array { .. } | AbiType::String { .. } => { - unreachable!("2D arrays are unsupported") - } - AbiType::Boolean | AbiType::Integer { .. } | AbiType::Field => acc.push(abi_type.clone()), - AbiType::Struct { fields } => { - for (_, field_type) in fields { - flatten_abi_type_recursive(acc, field_type); - } - } - } -} +use noirc_abi::{Abi, AbiParameter, FunctionSignature}; /// Arranges a function signature and a generated circuit's return witnesses into a /// `noirc_abi::Abi`. diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir.rs index 7a755210e2b..ae183aa962f 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir.rs @@ -1,4 +1,3 @@ pub(crate) mod acir_variable; pub(crate) mod errors; pub(crate) mod generated_acir; -pub(crate) mod memory; diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs index 7a77671c676..62bc09bb888 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs @@ -1,12 +1,9 @@ +use crate::ssa_refactor::acir_gen::AcirValue; use crate::ssa_refactor::ir::types::Type as SsaType; use crate::ssa_refactor::ir::{instruction::Endian, map::TwoWayMap, types::NumericType}; use acvm::acir::brillig_vm::Opcode as BrilligOpcode; -use super::{ - errors::AcirGenError, - generated_acir::GeneratedAcir, - memory::{ArrayId, Memory}, -}; +use super::{errors::AcirGenError, generated_acir::GeneratedAcir}; use acvm::{ acir::{ circuit::opcodes::FunctionInput, @@ -16,7 +13,7 @@ use acvm::{ FieldElement, }; use iter_extended::vecmap; -use std::{borrow::Cow, collections::HashMap, hash::Hash}; +use std::{borrow::Cow, hash::Hash}; #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] /// High level Type descriptor for Variables. @@ -72,11 +69,6 @@ pub(crate) struct AcirContext { /// then the `acir_ir` will be populated to assert this /// addition. acir_ir: GeneratedAcir, - - /// Maps an `AcirVar` to its type. - variables_to_types: HashMap, - /// Maps the elements of virtual arrays to their `AcirVar` elements - memory: Memory, } impl AcirContext { @@ -157,54 +149,36 @@ impl AcirContext { } /// Returns an `AcirVar` that is the XOR result of `lhs` & `rhs`. - pub(crate) fn xor_var(&mut self, lhs: AcirVar, rhs: AcirVar) -> Result { - let lhs_bit_size = *self - .variables_to_types - .get(&lhs) - .expect("ICE: XOR applied to field type, this should be caught by the type system"); - let rhs_bit_size = *self - .variables_to_types - .get(&lhs) - .expect("ICE: XOR applied to field type, this should be caught by the type system"); - assert_eq!(lhs_bit_size, rhs_bit_size, "ICE: Operands to XOR require equal bit size"); - - let outputs = self.black_box_function(BlackBoxFunc::XOR, vec![lhs, rhs])?; - let result = outputs[0]; - self.variables_to_types.insert(result, lhs_bit_size); - Ok(result) + pub(crate) fn xor_var( + &mut self, + lhs: AcirVar, + rhs: AcirVar, + bit_size: u32, + ) -> Result { + let inputs = vec![(AcirValue::Var(lhs), bit_size), (AcirValue::Var(rhs), bit_size)]; + let mut outputs = self.black_box_function(BlackBoxFunc::XOR, inputs)?; + Ok(outputs.swap_remove(0).into_var()) } /// Returns an `AcirVar` that is the AND result of `lhs` & `rhs`. - pub(crate) fn and_var(&mut self, lhs: AcirVar, rhs: AcirVar) -> Result { - let lhs_bit_size = *self - .variables_to_types - .get(&lhs) - .expect("ICE: AND applied to field type, this should be caught by the type system"); - let rhs_bit_size = *self - .variables_to_types - .get(&lhs) - .expect("ICE: AND applied to field type, this should be caught by the type system"); - assert_eq!(lhs_bit_size, rhs_bit_size, "ICE: Operands to AND require equal bit size"); - - let outputs = self.black_box_function(BlackBoxFunc::AND, vec![lhs, rhs])?; - let result = outputs[0]; - self.variables_to_types.insert(result, lhs_bit_size); - Ok(result) + pub(crate) fn and_var( + &mut self, + lhs: AcirVar, + rhs: AcirVar, + bit_size: u32, + ) -> Result { + let inputs = vec![(AcirValue::Var(lhs), bit_size), (AcirValue::Var(rhs), bit_size)]; + let mut outputs = self.black_box_function(BlackBoxFunc::AND, inputs)?; + Ok(outputs.swap_remove(0).into_var()) } /// Returns an `AcirVar` that is the OR result of `lhs` & `rhs`. - pub(crate) fn or_var(&mut self, lhs: AcirVar, rhs: AcirVar) -> Result { - let lhs_type = *self - .variables_to_types - .get(&lhs) - .expect("all variables should have a type attached to them"); - let rhs_type = *self - .variables_to_types - .get(&lhs) - .expect("all variables should have a type attached to them"); - assert_eq!(lhs_type, rhs_type, "types in or expressions should be the same"); - let bit_size = lhs_type.bit_size(); - + pub(crate) fn or_var( + &mut self, + lhs: AcirVar, + rhs: AcirVar, + bit_size: u32, + ) -> Result { let result = if bit_size == 1 { // Operands are booleans // a + b - ab @@ -219,14 +193,10 @@ impl AcirContext { let max = self.add_constant(FieldElement::from((1_u128 << bit_size) - 1)); let a = self.sub_var(max, lhs); let b = self.sub_var(max, rhs); - // We track the bit sizes of these intermediaries so that blackbox input generation - // infers them correctly. - self.variables_to_types.insert(a, lhs_type); - self.variables_to_types.insert(b, lhs_type); - let output = self.black_box_function(BlackBoxFunc::AND, vec![a, b])?; - self.sub_var(max, output[0]) + let inputs = vec![(AcirValue::Var(a), bit_size), (AcirValue::Var(b), bit_size)]; + let mut output = self.black_box_function(BlackBoxFunc::AND, inputs)?; + self.sub_var(max, output.swap_remove(0).into_var()) }; - self.variables_to_types.insert(result, lhs_type); Ok(result) } @@ -363,14 +333,8 @@ impl AcirContext { /// /// `x` must be a 1-bit integer (i.e. a boolean) pub(crate) fn not_var(&mut self, x: AcirVar) -> AcirVar { - assert_eq!( - self.variables_to_types.get(&x), - Some(&AcirType::boolean()), - "ICE: NOT op applied to non-boolean type" - ); - let data = &self.vars[x]; // Since `x` can only be 0 or 1, we can derive NOT as 1 - x - match data { + match &self.vars[x] { AcirVarData::Const(constant) => { self.add_data(AcirVarData::Expr(&Expression::one() - &Expression::from(*constant))) } @@ -406,6 +370,7 @@ impl AcirContext { &mut self, lhs: AcirVar, rhs: AcirVar, + bit_size: u32, ) -> Result<(AcirVar, AcirVar), AcirGenError> { let predicate = Expression::one(); @@ -415,18 +380,8 @@ impl AcirContext { let lhs_expr = lhs_data.to_expression(); let rhs_expr = rhs_data.to_expression(); - let lhs_bit_size = self.variables_to_types.get(&lhs).expect("euclidean division cannot be made on variables with no known bit size. This should have been caught by the frontend").bit_size(); - let rhs_bit_size = self.variables_to_types.get(&rhs).expect("euclidean division cannot be made on variables with no known bit size. This should have been caught by the frontend").bit_size(); - - assert_eq!( - lhs_bit_size, rhs_bit_size, - // This makes the assumption that the bit size is the last known integer - // type for this variable and that we are not getting the smallest range for example. - "Euclidean division can only be applied to variables of the same type" - ); - let (quotient, remainder) = - self.acir_ir.euclidean_division(&lhs_expr, &rhs_expr, lhs_bit_size, &predicate)?; + self.acir_ir.euclidean_division(&lhs_expr, &rhs_expr, bit_size, &predicate)?; let quotient_var = self.add_data(AcirVarData::Witness(quotient)); let remainder_var = self.add_data(AcirVarData::Witness(remainder)); @@ -439,10 +394,12 @@ impl AcirContext { &mut self, lhs: AcirVar, rhs: AcirVar, + bit_size: u32, ) -> Result { - let (_, remainder) = self.euclidean_division_var(lhs, rhs)?; + let (_, remainder) = self.euclidean_division_var(lhs, rhs, bit_size)?; Ok(remainder) } + /// Returns an `AcirVar` that is constrained to be `lhs >> rhs`. /// /// We convert right shifts to divisions, so this is equivalent to @@ -502,7 +459,6 @@ impl AcirContext { // integer, but a function requires the parameter to be a Field. } } - self.variables_to_types.insert(variable, AcirType(*numeric_type)); Ok(variable) } @@ -520,24 +476,25 @@ impl AcirContext { Ok(self.add_data(AcirVarData::Expr(result_expr))) } + /// Returns an `AcirVar` which will be `1` if lhs >= rhs /// and `0` otherwise. - fn more_than_eq_var(&mut self, lhs: AcirVar, rhs: AcirVar) -> Result { + fn more_than_eq_var( + &mut self, + lhs: AcirVar, + rhs: AcirVar, + bit_size: u32, + ) -> Result { let lhs_data = &self.vars[lhs]; let rhs_data = &self.vars[rhs]; let lhs_expr = lhs_data.to_expression(); let rhs_expr = rhs_data.to_expression(); - let lhs_type = self.variables_to_types.get(&lhs).expect("comparisons cannot be made on variables with no known max bit size. This should have been caught by the frontend"); - let rhs_type = self.variables_to_types.get(&rhs).expect("comparisons cannot be made on variables with no known max bit size. This should have been caught by the frontend"); - // TODO: check what happens when we do (a as u8) >= (b as u32) // TODO: The frontend should shout in this case - assert_eq!(lhs_type, rhs_type, "types in a more than eq comparison should be the same"); - let is_greater_than_eq = - self.acir_ir.more_than_eq_comparison(&lhs_expr, &rhs_expr, lhs_type.bit_size())?; + self.acir_ir.more_than_eq_comparison(&lhs_expr, &rhs_expr, bit_size)?; Ok(self.add_data(AcirVarData::Witness(is_greater_than_eq))) } @@ -548,10 +505,11 @@ impl AcirContext { &mut self, lhs: AcirVar, rhs: AcirVar, + bit_size: u32, ) -> Result { // Flip the result of calling more than equal method to // compute less than. - let comparison = self.more_than_eq_var(lhs, rhs)?; + let comparison = self.more_than_eq_var(lhs, rhs, bit_size)?; let one = self.add_constant(FieldElement::one()); let comparison_negated = self.sub_var(one, comparison); @@ -564,24 +522,29 @@ impl AcirContext { pub(crate) fn black_box_function( &mut self, name: BlackBoxFunc, - mut inputs: Vec, - ) -> Result, AcirGenError> { + mut inputs: Vec<(AcirValue, /*bit_size:*/ u32)>, + ) -> Result, AcirGenError> { // Separate out any arguments that should be constants let constants = match name { BlackBoxFunc::Pedersen => { // The last argument of pedersen is the domain separator, which must be a constant - let domain_var = - inputs.pop().expect("ICE: Pedersen call requires domain separator"); + let domain_var = inputs + .pop() + .expect("ICE: Pedersen call requires domain separator") + .0 + .into_var(); + let domain_constant = self.vars[domain_var] .as_constant() .expect("ICE: Domain separator must be a constant"); + vec![domain_constant] } _ => vec![], }; // Convert `AcirVar` to `FunctionInput` - let inputs = self.prepare_inputs_for_black_box_func_call(&inputs)?; + let inputs = self.prepare_inputs_for_black_box_func_call(inputs)?; // Call Black box with `FunctionInput` let outputs = self.acir_ir.call_black_box(name, inputs, constants); @@ -591,10 +554,9 @@ impl AcirContext { // // We do not apply range information on the output of the black box function. // See issue #1439 - let outputs_var = - vecmap(&outputs, |witness_index| self.add_data(AcirVarData::Witness(*witness_index))); - - Ok(outputs_var) + Ok(vecmap(&outputs, |witness_index| { + AcirValue::Var(self.add_data(AcirVarData::Witness(*witness_index))) + })) } /// Black box function calls expect their inputs to be in a specific data structure (FunctionInput). @@ -602,50 +564,17 @@ impl AcirContext { /// This function will convert `AcirVar` into `FunctionInput` for a blackbox function call. fn prepare_inputs_for_black_box_func_call( &mut self, - inputs: &[AcirVar], + inputs: Vec<(AcirValue, /*bit_size:*/ u32)>, ) -> Result, AcirGenError> { let mut witnesses = Vec::new(); - for input in inputs { - let var_data = &self.vars[input]; + for (input, num_bits) in inputs { + let var_data = &self.vars[input.into_var()]; // Intrinsics only accept Witnesses. This is not a limitation of the // intrinsics, its just how we have defined things. Ideally, we allow // constants too. let expr = var_data.to_expression(); let witness = self.acir_ir.get_or_create_witness(&expr); - - // Fetch the number of bits for this variable - // If it has never been constrained before, then we will - // encounter None, and so we take the max number of bits for a - // field element. - let num_bits = match self.variables_to_types.get(input) { - Some(typ) => { - // In Noir, we specify the number of bits to take from the input - // by doing the following: - // - // ``` - // call_intrinsic(x as u8) - // ``` - // - // The `as u8` specifies that we want to take 8 bits from the `x` - // variable. - // - // There were discussions about the SSA IR optimizing out range - // constraints. We would want to be careful with it here. For example: - // - // ``` - // let x : u32 = y as u32 - // call_intrinsic(x as u64) - // ``` - // The `x as u64` is redundant since we know that `x` fits within a u32. - // However, since the `x as u64` line is being used to tell the intrinsic - // to take 64 bits, we cannot remove it. - - typ.bit_size() - } - None => FieldElement::max_num_bits(), - }; - witnesses.push(FunctionInput { witness, num_bits }); } Ok(witnesses) @@ -663,7 +592,7 @@ impl AcirContext { input_var: AcirVar, radix_var: AcirVar, limb_count_var: AcirVar, - ) -> Result, AcirGenError> { + ) -> Result, AcirGenError> { let radix = self.vars[&radix_var].as_constant().expect("ICE: radix should be a constant").to_u128() as u32; @@ -677,7 +606,8 @@ impl AcirContext { let limbs = self.acir_ir.radix_le_decompose(input_expr, radix, limb_count)?; - let mut limb_vars = vecmap(limbs, |witness| self.add_data(AcirVarData::Witness(witness))); + let mut limb_vars = + vecmap(limbs, |witness| AcirValue::Var(self.add_data(AcirVarData::Witness(witness)))); if endian == Endian::Big { limb_vars.reverse(); @@ -692,13 +622,15 @@ impl AcirContext { endian: Endian, input_var: AcirVar, limb_count_var: AcirVar, - ) -> Result, AcirGenError> { + ) -> Result, AcirGenError> { let two_var = self.add_constant(FieldElement::from(2_u128)); self.radix_decompose(endian, input_var, two_var, limb_count_var) } /// Prints the given `AcirVar`s as witnesses. - pub(crate) fn print(&mut self, input: Vec) -> Result<(), AcirGenError> { + pub(crate) fn print(&mut self, input: Vec) -> Result<(), AcirGenError> { + let input = Self::flatten_values(input); + let witnesses = vecmap(input, |acir_var| { let var_data = &self.vars[acir_var]; let expr = var_data.to_expression(); @@ -708,42 +640,28 @@ impl AcirContext { Ok(()) } - /// Terminates the context and takes the resulting `GeneratedAcir` - pub(crate) fn finish(self) -> GeneratedAcir { - self.acir_ir - } - - /// Allocates an array of size `size` and returns a pointer to the array in memory. - pub(crate) fn allocate_array(&mut self, size: usize) -> ArrayId { - self.memory.allocate(size) - } - - /// Stores the given `AcirVar` at the specified address in memory - pub(crate) fn array_store( - &mut self, - array_id: ArrayId, - index: usize, - element: AcirVar, - ) -> Result<(), AcirGenError> { - self.memory.constant_set(array_id, index, element) + fn flatten_values(values: Vec) -> Vec { + let mut acir_vars = Vec::with_capacity(values.len()); + for value in values { + Self::flatten_value(&mut acir_vars, value); + } + acir_vars } - /// Gets the last stored `AcirVar` at the specified address in memory. - /// - /// This errors if nothing was previously stored at the address. - pub(crate) fn array_load( - &mut self, - array_id: ArrayId, - index: usize, - ) -> Result { - self.memory.constant_get(array_id, index) + pub(crate) fn flatten_value(acir_vars: &mut Vec, value: AcirValue) { + match value { + AcirValue::Var(acir_var) => acir_vars.push(acir_var), + AcirValue::Array(array) => { + for value in array { + Self::flatten_value(acir_vars, value); + } + } + } } - /// Gets all `AcirVar` elements currently stored at the array. - /// - /// This errors if nothing was previously stored any element in the array. - pub(crate) fn array_load_all(&self, array_id: ArrayId) -> Result, AcirGenError> { - self.memory.constant_get_all(array_id) + /// Terminates the context and takes the resulting `GeneratedAcir` + pub(crate) fn finish(self) -> GeneratedAcir { + self.acir_ir } /// Adds `Data` into the context and assigns it a Variable. diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/errors.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/errors.rs index 810208a6e83..fe9aba43339 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/errors.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/errors.rs @@ -1,10 +1,10 @@ -use super::memory::ArrayId; +use crate::ssa_refactor::ir::value::ValueId; #[derive(Debug, PartialEq, Eq, Clone)] pub(crate) enum AcirGenError { InvalidRangeConstraint { num_bits: u32 }, IndexOutOfBounds { index: usize, array_size: usize }, - UninitializedElementInArray { index: usize, array_id: ArrayId }, + UninitializedElementInArray { index: usize, array_id: ValueId }, } impl AcirGenError { @@ -19,7 +19,7 @@ impl AcirGenError { format!("Index out of bounds, array has size {array_size}, but index was {index}") } AcirGenError::UninitializedElementInArray { index, array_id } => { - format!("The element at index {index} was never set in array {array_id:?}") + format!("The element at index {index} was never set in array {array_id}") } } } diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/memory.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/memory.rs deleted file mode 100644 index 3cbf70bfdea..00000000000 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/memory.rs +++ /dev/null @@ -1,122 +0,0 @@ -use crate::ssa_refactor::ir::map::{DenseMap, Id}; - -use super::{acir_variable::AcirVar, errors::AcirGenError}; - -#[derive(Debug, Default)] -pub(crate) struct Array { - /// Elements in the array. - /// None represents the element not being set yet - /// - /// The size of this vector cannot be changed once set. - /// This follows the behavior of an Array as opposed to a Vector. - elements: Vec>, -} - -impl Array { - pub(crate) fn size(&self) -> usize { - self.elements.len() - } - - pub(crate) fn new(size: usize) -> Array { - Self { elements: vec![None; size] } - } -} - -#[derive(Debug, Default)] -/// Memory used to represent Arrays -pub(crate) struct Memory { - arrays: DenseMap, -} - -impl Memory { - /// Allocates an array of size `size`. - /// The elements in the array are not zero initialized - pub(crate) fn allocate(&mut self, size: usize) -> ArrayId { - let array = Array::new(size); - self.arrays.insert(array) - } - - /// Sets an element at the array that `ArrayId` points to. - /// The index must be constant in the Noir program. - pub(crate) fn constant_set( - &mut self, - array_id: ArrayId, - index: usize, - element: AcirVar, - ) -> Result<(), AcirGenError> { - let array = &mut self.arrays[array_id]; - Self::check_bounds(index, array.size())?; - - array.elements[index] = Some(element); - - Ok(()) - } - - /// Gets an element at the array that `ArrayId` points to. - /// The index must be constant in the Noir program. - pub(crate) fn constant_get( - &self, - array_id: ArrayId, - index: usize, - ) -> Result { - let array = &self.arrays[array_id]; - Self::check_bounds(index, array.size())?; - - array.elements[index].ok_or(AcirGenError::UninitializedElementInArray { index, array_id }) - } - - /// Gets all elements at the array that `ArrayId` points to. - /// - /// This returns an error if any of the array's elements have not been initialized. - pub(crate) fn constant_get_all(&self, array_id: ArrayId) -> Result, AcirGenError> { - let array = &self.arrays[array_id]; - let mut elements = Vec::new(); - for index in 0..array.size() { - elements.push(self.constant_get(array_id, index)?); - } - Ok(elements) - } - - /// Check if the index is larger than the array size - fn check_bounds(index: usize, array_size: usize) -> Result<(), AcirGenError> { - if index < array_size { - Ok(()) - } else { - Err(AcirGenError::IndexOutOfBounds { index, array_size }) - } - } -} - -/// Pointer to an allocated `Array` -pub(crate) type ArrayId = Id; - -#[cfg(test)] -mod tests { - use crate::ssa_refactor::acir_gen::acir_ir::{errors::AcirGenError, memory::Memory}; - - #[test] - fn smoke_api_get_uninitialized_element_out() { - let mut memory = Memory::default(); - - let array_size = 10; - let index = 0; - - let array_id = memory.allocate(array_size); - - let element = memory.constant_get(array_id, index); - // Should get an error because we are trying to get an element which has not been initialized - // yet. - assert_eq!(element, Err(AcirGenError::UninitializedElementInArray { index, array_id })); - } - #[test] - fn smoke_api_out_of_bounds() { - let mut memory = Memory::default(); - - let array_size = 10; - let array_id = memory.allocate(array_size); - - let element = memory.constant_get(array_id, array_size); - // Should get an index out of bounds error - assert_eq!(element, Err(AcirGenError::IndexOutOfBounds { index: array_size, array_size })); - } -} diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs index 7274458d744..0dce57f42ad 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs @@ -2,13 +2,8 @@ use std::collections::HashMap; -use self::acir_ir::{ - acir_variable::{AcirContext, AcirVar}, - errors::AcirGenError, - memory::ArrayId, -}; +use self::acir_ir::acir_variable::{AcirContext, AcirVar}; use super::{ - abi_gen::collate_array_info, ir::{ dfg::DataFlowGraph, function::RuntimeType, @@ -22,7 +17,8 @@ use super::{ ssa_gen::Ssa, }; use crate::brillig::{artifact::BrilligArtifact, Brillig}; -use noirc_abi::{AbiType, FunctionSignature, Sign}; +use acvm::FieldElement; +use iter_extended::vecmap; pub(crate) use acir_ir::generated_acir::GeneratedAcir; @@ -38,58 +34,37 @@ struct Context { /// AcirVar per SSA value. Before creating an `AcirVar` /// for an SSA value, we check this map. If an `AcirVar` /// already exists for this Value, we return the `AcirVar`. - ssa_value_to_acir_var: HashMap, AcirVar>, - /// Maps SSA values to array addresses (including index offset). - /// - /// When converting parameters the of main, `ArrayId`s are gathered and stored with an offset - /// of 0. When the use of these stored values are detected for address arithmetic, the results - /// of such instructions are stored, in effect capturing any further values that refer to - /// addresses. - ssa_value_to_array_address: HashMap, + ssa_values: HashMap, AcirValue>, + /// Manages and builds the `AcirVar`s to which the converted SSA values refer. acir_context: AcirContext, } -impl Ssa { - pub(crate) fn into_acir( - self, - main_function_signature: FunctionSignature, - brillig: Brillig, - allow_log_ops: bool, - ) -> GeneratedAcir { - let param_arrays_info: Vec<_> = collate_array_info(&main_function_signature.0) - .iter() - .map(|(size, abi_type)| (*size, numeric_type_for_abi_array_element_type(abi_type))) - .collect(); +#[derive(Debug, Clone)] +pub(crate) enum AcirValue { + Var(AcirVar), + Array(im::Vector), +} - let context = Context::default(); - context.convert_ssa(self, ¶m_arrays_info, brillig, allow_log_ops) +impl AcirValue { + fn into_var(self) -> AcirVar { + match self { + AcirValue::Var(var) => var, + AcirValue::Array(_) => panic!("Called AcirValue::into_var on an array"), + } } } -/// Gives the equivalent ssa numeric type for the given abi type. We are dealing in the context of -/// arrays - hence why only numerics are supported. -fn numeric_type_for_abi_array_element_type(abi_type: &AbiType) -> NumericType { - match abi_type { - AbiType::Boolean => NumericType::Unsigned { bit_size: 1 }, - AbiType::Integer { sign, width } => match sign { - Sign::Signed => NumericType::Signed { bit_size: *width }, - Sign::Unsigned => NumericType::Unsigned { bit_size: *width }, - }, - AbiType::Field => NumericType::NativeField, - _ => unreachable!("Non-numeric cannot be array element"), +impl Ssa { + pub(crate) fn into_acir(self, brillig: Brillig, allow_log_ops: bool) -> GeneratedAcir { + let context = Context::default(); + context.convert_ssa(self, brillig, allow_log_ops) } } impl Context { /// Converts SSA into ACIR - fn convert_ssa( - mut self, - ssa: Ssa, - param_array_info: &[(usize, NumericType)], - brillig: Brillig, - allow_log_ops: bool, - ) -> GeneratedAcir { + fn convert_ssa(mut self, ssa: Ssa, brillig: Brillig, allow_log_ops: bool) -> GeneratedAcir { assert_eq!( ssa.functions.len(), 1, @@ -99,7 +74,7 @@ impl Context { let dfg = &main_func.dfg; let entry_block = &dfg[main_func.entry_block()]; - self.convert_ssa_block_params(entry_block.parameters(), dfg, param_array_info); + self.convert_ssa_block_params(entry_block.parameters(), dfg); for instruction_id in entry_block.instructions() { self.convert_ssa_instruction(*instruction_id, dfg, &ssa, &brillig, allow_log_ops); @@ -110,51 +85,31 @@ impl Context { self.acir_context.finish() } - /// Adds and binds `AcirVar`s for each numeric block parameter or block parameter array - /// element. At the same time `ArrayId`s are bound for any references within the params. - fn convert_ssa_block_params( - &mut self, - params: &[ValueId], - dfg: &DataFlowGraph, - param_arrays_info: &[(usize, NumericType)], - ) { - let mut param_arrays_info_iter = param_arrays_info.iter(); + /// Adds and binds `AcirVar`s for each numeric block parameter or block parameter array element. + fn convert_ssa_block_params(&mut self, params: &[ValueId], dfg: &DataFlowGraph) { for param_id in params { - let value = &dfg[*param_id]; - let param_type = match value { - Value::Param { typ, .. } => typ, - _ => unreachable!("ICE: Only Param type values should appear in block parameters"), - }; - match param_type { - Type::Numeric(numeric_type) => { - let acir_var = self.add_numeric_input_var(numeric_type); - self.ssa_value_to_acir_var.insert(*param_id, acir_var); - } - Type::Reference => { - let (array_length, numeric_type) = param_arrays_info_iter - .next() - .expect("ICE: fewer arrays in abi than in block params"); - let array_id = self.acir_context.allocate_array(*array_length); - self.ssa_value_to_array_address.insert(*param_id, (array_id, 0)); - for index in 0..*array_length { - let acir_var = self.add_numeric_input_var(numeric_type); - self.acir_context - .array_store(array_id, index, acir_var) - .expect("invalid array store"); + let typ = dfg.type_of_value(*param_id); + let value = self.convert_ssa_block_param(&typ); + self.ssa_values.insert(*param_id, value); + } + } + + fn convert_ssa_block_param(&mut self, param_type: &Type) -> AcirValue { + match param_type { + Type::Numeric(numeric_type) => AcirValue::Var(self.add_numeric_input_var(numeric_type)), + Type::Array(element_types, length) => { + let mut elements = im::Vector::new(); + + for _ in 0..*length { + for element in element_types.iter() { + elements.push_back(self.convert_ssa_block_param(element)); } } - _ => { - unreachable!( - "ICE: Params to the program should only contains numerics and arrays" - ) - } + + AcirValue::Array(elements) } + _ => unreachable!("ICE: Params to the program should only contains numbers and arrays"), } - assert_eq!( - param_arrays_info_iter.next(), - None, - "ICE: more arrays in abi than in block params" - ); } /// Creates an `AcirVar` corresponding to a parameter witness to appears in the abi. A range @@ -183,26 +138,18 @@ impl Context { ) { let instruction = &dfg[instruction_id]; - let (results_id, results_vars) = match instruction { + match instruction { Instruction::Binary(binary) => { - let result_ids = dfg.instruction_results(instruction_id); - if Self::value_is_array_address(result_ids[0], dfg) { - self.track_array_address(result_ids[0], binary, dfg); - (Vec::new(), Vec::new()) - } else { - let result_acir_var = self.convert_ssa_binary(binary, dfg); - (vec![result_ids[0]], vec![result_acir_var]) - } + let result_acir_var = self.convert_ssa_binary(binary, dfg); + self.define_result_var(dfg, instruction_id, result_acir_var); } Instruction::Constrain(value_id) => { - let constrain_condition = self.convert_ssa_value(*value_id, dfg); + let constrain_condition = self.convert_numeric_value(*value_id, dfg); self.acir_context.assert_eq_one(constrain_condition); - (Vec::new(), Vec::new()) } Instruction::Cast(value_id, typ) => { let result_acir_var = self.convert_ssa_cast(value_id, typ, dfg); - let result_ids = dfg.instruction_results(instruction_id); - (vec![result_ids[0]], vec![result_acir_var]) + self.define_result_var(dfg, instruction_id, result_acir_var); } Instruction::Call { func, arguments } => { let result_ids = dfg.instruction_results(instruction_id); @@ -217,7 +164,7 @@ impl Context { // Generate the brillig code of the function let code = BrilligArtifact::default().link(&brillig[*id]); self.acir_context.brillig(code); - (result_ids.to_vec(), Vec::new()) + // TODO: Set result values } } } @@ -228,69 +175,87 @@ impl Context { dfg, allow_log_ops, ); - if Self::value_is_array_address(result_ids[0], dfg) { - // Some intrinsics return arrays - these require an allocation - if result_ids.len() != 1 { - todo!("Complex return type encountered. Restructuring required to provide info on how to repackage result"); - } - let array_id = self.acir_context.allocate_array(outputs.len()); - self.ssa_value_to_array_address.insert(result_ids[0], (array_id, 0)); - for (index, element) in outputs.iter().enumerate() { - self.acir_context - .array_store(array_id, index, *element) - .expect("add Result types to all methods so errors bubble up"); - } - (Vec::new(), Vec::new()) - } else { - (result_ids.to_vec(), outputs) + + assert_eq!(result_ids.len(), outputs.len()); + for (result, output) in result_ids.iter().zip(outputs) { + self.ssa_values.insert(*result, output); } } _ => unreachable!("expected calling a function"), } } Instruction::Not(value_id) => { - let boolean_var = self.convert_ssa_value(*value_id, dfg); + let boolean_var = self.convert_numeric_value(*value_id, dfg); let result_acir_var = self.acir_context.not_var(boolean_var); - - let result_ids = dfg.instruction_results(instruction_id); - (vec![result_ids[0]], vec![result_acir_var]) - } - Instruction::Allocate => { - let array_id = self.acir_context.allocate_array(1); - let result_ids = dfg.instruction_results(instruction_id); - self.ssa_value_to_array_address.insert(result_ids[0], (array_id, 0)); - (Vec::new(), Vec::new()) - } - Instruction::Store { address, value } => { - self.convert_ssa_store(address, value, dfg); - (Vec::new(), Vec::new()) - } - Instruction::Load { address } => { - let result_acir_var = self.convert_ssa_load(address); - let result_ids = dfg.instruction_results(instruction_id); - (vec![result_ids[0]], vec![result_acir_var]) + self.define_result_var(dfg, instruction_id, result_acir_var); } Instruction::Truncate { value, bit_size, max_bit_size } => { - let var = self.convert_ssa_value(*value, dfg); - let result_ids = dfg.instruction_results(instruction_id); + let var = self.convert_numeric_value(*value, dfg); let result_acir_var = self .acir_context .truncate_var(var, *bit_size, *max_bit_size) .expect("add Result types to all methods so errors bubble up"); - (vec![result_ids[0]], vec![result_acir_var]) + self.define_result_var(dfg, instruction_id, result_acir_var); + } + Instruction::ArrayGet { array, index } => { + let array = self.convert_array_value(*array, dfg); + let index = dfg + .get_numeric_constant(*index) + .expect("Expected array index to be a known constant") + .try_to_u64() + .expect("Expected array index to fit into a u64") + as usize; + + self.define_result(dfg, instruction_id, array[index].clone()); + } + Instruction::ArraySet { array, index, value } => { + let array = self.convert_array_value(*array, dfg); + let index = dfg + .get_numeric_constant(*index) + .expect("Expected array index to be a known constant") + .try_to_u64() + .expect("Expected array index to fit into a u64") + as usize; + + let value = self.convert_value(*value, dfg); + let new_array = AcirValue::Array(array.update(index, value)); + self.define_result(dfg, instruction_id, new_array); + } + Instruction::Allocate => { + unreachable!("Expected all allocate instructions to be removed before acir_gen") + } + Instruction::Store { .. } => { + unreachable!("Expected all store instructions to be removed before acir_gen") + } + Instruction::Load { .. } => { + unreachable!("Expected all load instructions to be removed before acir_gen") } - Instruction::ArrayGet { .. } => todo!(), - Instruction::ArraySet { .. } => todo!(), - }; - - // Map the results of the instructions to Acir variables - for (result_id, result_var) in results_id.into_iter().zip(results_vars) { - self.ssa_value_to_acir_var.insert(result_id, result_var); } } + /// Remember the result of an instruction returning a single value + fn define_result( + &mut self, + dfg: &DataFlowGraph, + instruction: InstructionId, + result: AcirValue, + ) { + let result_ids = dfg.instruction_results(instruction); + self.ssa_values.insert(result_ids[0], result); + } + + /// Remember the result of instruction returning a single numeric value + fn define_result_var( + &mut self, + dfg: &DataFlowGraph, + instruction: InstructionId, + result: AcirVar, + ) { + self.define_result(dfg, instruction, AcirValue::Var(result)); + } + /// Converts an SSA terminator's return values into their ACIR representations fn convert_ssa_return(&mut self, terminator: &TerminatorInstruction, dfg: &DataFlowGraph) { let return_values = match terminator { @@ -308,9 +273,7 @@ impl Context { // The return value may or may not be an array reference. Calling `flatten_value_list` // will expand the array if there is one. - let return_acir_vars = self - .flatten_value_list(return_values, dfg) - .expect("add Result types to all methods so errors bubble up"); + let return_acir_vars = self.flatten_value_list(return_values, dfg); for acir_var in return_acir_vars { self.acir_context.return_var(acir_var); @@ -330,28 +293,53 @@ impl Context { /// It is not safe to call this function on value ids that represent addresses. Instructions /// involving such values are evaluated via a separate path and stored in /// `ssa_value_to_array_address` instead. - fn convert_ssa_value(&mut self, value_id: ValueId, dfg: &DataFlowGraph) -> AcirVar { + fn convert_value(&mut self, value_id: ValueId, dfg: &DataFlowGraph) -> AcirValue { let value = &dfg[value_id]; - if let Some(acir_var) = self.ssa_value_to_acir_var.get(&value_id) { - return *acir_var; + if let Some(acir_value) = self.ssa_values.get(&value_id) { + return acir_value.clone(); } - let acir_var = match value { - Value::NumericConstant { constant, .. } => self.acir_context.add_constant(*constant), + + let acir_value = match value { + Value::NumericConstant { constant, .. } => { + AcirValue::Var(self.acir_context.add_constant(*constant)) + } + Value::Array { array, .. } => { + let elements = array.iter().map(|element| self.convert_value(*element, dfg)); + AcirValue::Array(elements.collect()) + } Value::Intrinsic(..) => todo!(), - Value::Array(..) => todo!(), Value::Function(..) => unreachable!("ICE: All functions should have been inlined"), Value::Instruction { .. } | Value::Param { .. } => { unreachable!("ICE: Should have been in cache {value:?}") } }; - self.ssa_value_to_acir_var.insert(value_id, acir_var); - acir_var + self.ssa_values.insert(value_id, acir_value.clone()); + acir_value + } + + fn convert_array_value( + &mut self, + value_id: ValueId, + dfg: &DataFlowGraph, + ) -> im::Vector { + match self.convert_value(value_id, dfg) { + AcirValue::Var(acir_var) => panic!("Expected an array value, found: {acir_var:?}"), + AcirValue::Array(array) => array, + } + } + + fn convert_numeric_value(&mut self, value_id: ValueId, dfg: &DataFlowGraph) -> AcirVar { + match self.convert_value(value_id, dfg) { + AcirValue::Var(acir_var) => acir_var, + AcirValue::Array(array) => panic!("Expected a numeric value, found: {array:?}"), + } } /// Processes a binary operation and converts the result into an `AcirVar` fn convert_ssa_binary(&mut self, binary: &Binary, dfg: &DataFlowGraph) -> AcirVar { - let lhs = self.convert_ssa_value(binary.lhs, dfg); - let rhs = self.convert_ssa_value(binary.rhs, dfg); + let lhs = self.convert_numeric_value(binary.lhs, dfg); + let rhs = self.convert_numeric_value(binary.rhs, dfg); + let bits = self.bit_count(binary.lhs, dfg); let binary_type = self.type_of_binary_operation(binary, dfg); @@ -365,25 +353,25 @@ impl Context { BinaryOp::Eq => self.acir_context.eq_var(lhs, rhs), BinaryOp::Lt => self .acir_context - .less_than_var(lhs, rhs) + .less_than_var(lhs, rhs, bits) .expect("add Result types to all methods so errors bubble up"), BinaryOp::Shl => self.acir_context.shift_left_var(lhs, rhs, binary_type.into()), BinaryOp::Shr => self.acir_context.shift_right_var(lhs, rhs, binary_type.into()), BinaryOp::Xor => self .acir_context - .xor_var(lhs, rhs) + .xor_var(lhs, rhs, bits) .expect("add Result types to all methods so errors bubble up"), BinaryOp::And => self .acir_context - .and_var(lhs, rhs) + .and_var(lhs, rhs, bits) .expect("add Result types to all methods so errors bubble up"), BinaryOp::Or => self .acir_context - .or_var(lhs, rhs) + .or_var(lhs, rhs, bits) .expect("add Result types to all methods so errors bubble up"), BinaryOp::Mod => self .acir_context - .modulo_var(lhs, rhs) + .modulo_var(lhs, rhs, bits) .expect("add Result types to all methods so errors bubble up"), } } @@ -414,7 +402,7 @@ impl Context { (_, Type::Reference) | (Type::Reference, _) => { unreachable!("References are invalid in binary operations") } - (_, Type::Array) | (Type::Array, _) => { + (_, Type::Array(..)) | (Type::Array(..), _) => { unreachable!("Arrays are invalid in binary operations") } // Unit type currently can mean a 0 constant, so we return the @@ -438,7 +426,7 @@ impl Context { /// Returns an `AcirVar` that is constrained to be fn convert_ssa_cast(&mut self, value_id: &ValueId, typ: &Type, dfg: &DataFlowGraph) -> AcirVar { - let variable = self.convert_ssa_value(*value_id, dfg); + let variable = self.convert_numeric_value(*value_id, dfg); match typ { Type::Numeric(numeric_type) => self @@ -458,28 +446,35 @@ impl Context { arguments: &[ValueId], dfg: &DataFlowGraph, allow_log_ops: bool, - ) -> Vec { - let inputs = self - .flatten_value_list(arguments, dfg) - .expect("add Result types to all methods so errors bubble up"); + ) -> Vec { match intrinsic { - Intrinsic::BlackBox(black_box) => self - .acir_context - .black_box_function(black_box, inputs) - .expect("add Result types to all methods so errors bubble up"), + Intrinsic::BlackBox(black_box) => { + let inputs = vecmap(arguments, |arg| { + let bit_count = self.bit_count(*arg, dfg); + (self.convert_value(*arg, dfg), bit_count) + }); + + self.acir_context + .black_box_function(black_box, inputs) + .expect("add Result types to all methods so errors bubble up") + } Intrinsic::ToRadix(endian) => { - // inputs = [field, radix, limb_size]; (see noir_stdlib/src/field.nr) + let field = self.convert_value(arguments[0], dfg).into_var(); + let radix = self.convert_value(arguments[1], dfg).into_var(); + let limb_size = self.convert_value(arguments[2], dfg).into_var(); self.acir_context - .radix_decompose(endian, inputs[0], inputs[1], inputs[2]) + .radix_decompose(endian, field, radix, limb_size) .expect("add Result types to all methods so errors bubble up") } Intrinsic::ToBits(endian) => { - // inputs = [field, bit_size]; (see noir_stdlib/src/field.nr) + let field = self.convert_value(arguments[0], dfg).into_var(); + let bit_size = self.convert_value(arguments[1], dfg).into_var(); self.acir_context - .bit_decompose(endian, inputs[0], inputs[1]) + .bit_decompose(endian, field, bit_size) .expect("add Result types to all methods so errors bubble up") } Intrinsic::Println => { + let inputs = vecmap(arguments, |arg| self.convert_value(*arg, dfg)); if allow_log_ops { self.acir_context .print(inputs) @@ -494,78 +489,29 @@ impl Context { /// Maps an ssa value list, for which some values may be references to arrays, by inlining /// the `AcirVar`s corresponding to the contents of each array into the list of `AcirVar`s /// that correspond to other values. - fn flatten_value_list( - &mut self, - arguments: &[ValueId], - dfg: &DataFlowGraph, - ) -> Result, AcirGenError> { - let mut acir_vars = Vec::new(); + fn flatten_value_list(&mut self, arguments: &[ValueId], dfg: &DataFlowGraph) -> Vec { + let mut acir_vars = Vec::with_capacity(arguments.len()); for value_id in arguments { - if Self::value_is_array_address(*value_id, dfg) { - let (array_id, index) = self - .ssa_value_to_array_address - .get(value_id) - .expect("ICE: Call argument of undeclared array"); - assert_eq!(index, &0, "ICE: Call arguments only accept arrays in their entirety"); - let elements = self.acir_context.array_load_all(*array_id)?; - acir_vars.extend(elements); - } else { - acir_vars.push(self.convert_ssa_value(*value_id, dfg)); - } + let value = self.convert_value(*value_id, dfg); + AcirContext::flatten_value(&mut acir_vars, value); } - Ok(acir_vars) - } - - /// Stores the `AcirVar` corresponding to `value` at the `ArrayId` and index corresponding to - /// `address`. - fn convert_ssa_store(&mut self, address: &ValueId, value: &ValueId, dfg: &DataFlowGraph) { - let element_var = self.convert_ssa_value(*value, dfg); - let (array_id, index) = - self.ssa_value_to_array_address.get(address).expect("ICE: Load from undeclared array"); - self.acir_context.array_store(*array_id, *index, element_var).expect("invalid array load"); - } - - /// Returns the `AcirVar` that was previously stored at the given address. - fn convert_ssa_load(&mut self, address: &ValueId) -> AcirVar { - let (array_id, index) = - self.ssa_value_to_array_address.get(address).expect("ICE: Load from undeclared array"); - self.acir_context.array_load(*array_id, *index).expect("invalid array load") + acir_vars } - /// Returns true if the value has been declared as an array address - fn value_is_array_address(value_id: ValueId, dfg: &DataFlowGraph) -> bool { - dfg.type_of_value(value_id) == Type::Reference - } - - /// Takes a binary instruction describing array address arithmetic and stores the result. - fn track_array_address(&mut self, value_id: ValueId, binary: &Binary, dfg: &DataFlowGraph) { - if binary.operator != BinaryOp::Add { - unreachable!("ICE: Array address arithmetic only supports Add"); + fn bit_count(&self, lhs: ValueId, dfg: &DataFlowGraph) -> u32 { + match dfg.type_of_value(lhs) { + Type::Numeric(NumericType::Signed { bit_size }) => bit_size, + Type::Numeric(NumericType::Unsigned { bit_size }) => bit_size, + Type::Numeric(NumericType::NativeField) => FieldElement::max_num_bits(), + _ => 0, } - let lhs_address = self.ssa_value_to_array_address.get(&binary.lhs); - let rhs_address = self.ssa_value_to_array_address.get(&binary.rhs); - let ((array_id, offset), other_value_id) = match (lhs_address, rhs_address) { - (Some(address), None) => (address, binary.rhs), - (None, Some(address)) => (address, binary.lhs), - (Some(_), Some(_)) => unreachable!("ICE: Addresses cannot be added"), - (None, None) => unreachable!("ICE: One operand must be an address"), - }; - let other_value = &dfg[other_value_id]; - let new_offset = match other_value { - Value::NumericConstant { constant, .. } => { - let further_offset = - constant.try_to_u64().expect("ICE: array arithmetic doesn't fit in u64") - as usize; - offset + further_offset - } - _ => unreachable!("Invalid array address arithmetic operand"), - }; - self.ssa_value_to_array_address.insert(value_id, (*array_id, new_offset)); } } #[cfg(test)] mod tests { + use std::rc::Rc; + use acvm::{ acir::{ circuit::Opcode, @@ -577,7 +523,7 @@ mod tests { use crate::{ brillig::Brillig, ssa_refactor::{ - ir::{function::RuntimeType, map::Id}, + ir::{function::RuntimeType, map::Id, types::Type}, ssa_builder::FunctionBuilder, }, }; @@ -588,20 +534,22 @@ mod tests { fn returns_body_scoped_arrays() { // fn main { // b0(): - // v0 = allocate - // store v0, Field 1 - // return v0 + // return [Field 1] // } let func_id = Id::test_new(0); let mut builder = FunctionBuilder::new("func".into(), func_id, RuntimeType::Acir); - let v0 = builder.insert_allocate(); - let const_one = builder.field_constant(FieldElement::one()); - builder.insert_store(v0, const_one); - builder.terminate_with_return(vec![v0]); + + let one = builder.field_constant(FieldElement::one()); + + let element_type = Rc::new(vec![Type::field()]); + let array = builder.array_constant(im::Vector::unit(one), element_type); + + builder.terminate_with_return(vec![array]); + let ssa = builder.finish(); let context = Context::default(); - let acir = context.convert_ssa(ssa, &[], Brillig::default(), false); + let acir = context.convert_ssa(ssa, Brillig::default(), false); let expected_opcodes = vec![Opcode::Arithmetic(&Expression::one() - &Expression::from(Witness(1)))]; diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs index ed51f3397cc..652cf293bdc 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::{collections::HashMap, rc::Rc}; use crate::ssa_refactor::ir::instruction::SimplifyResult; @@ -9,7 +9,7 @@ use super::{ Instruction, InstructionId, InstructionResultType, Intrinsic, TerminatorInstruction, }, map::DenseMap, - types::Type, + types::{CompositeType, Type}, value::{Value, ValueId}, }; @@ -173,8 +173,12 @@ impl DataFlowGraph { } /// Create a new constant array value from the given elements - pub(crate) fn make_array(&mut self, elements: im::Vector) -> ValueId { - self.make_value(Value::Array(elements)) + pub(crate) fn make_array( + &mut self, + array: im::Vector, + element_type: Rc, + ) -> ValueId { + self.make_value(Value::Array { array, element_type }) } /// Gets or creates a ValueId for the given FunctionId. @@ -290,18 +294,21 @@ impl DataFlowGraph { &self, value: ValueId, ) -> Option<(FieldElement, Type)> { - match self.values[value] { - Value::NumericConstant { constant, typ } => Some((constant, typ)), + match &self.values[value] { + Value::NumericConstant { constant, typ } => Some((*constant, typ.clone())), _ => None, } } /// Returns the Value::Array associated with this ValueId if it refers to an array constant. /// Otherwise, this returns None. - pub(crate) fn get_array_constant(&self, value: ValueId) -> Option> { + pub(crate) fn get_array_constant( + &self, + value: ValueId, + ) -> Option<(im::Vector, Rc)> { match &self.values[value] { // Vectors are shared, so cloning them is cheap - Value::Array(array) => Some(array.clone()), + Value::Array { array, element_type } => Some((array.clone(), element_type.clone())), _ => None, } } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs index e20657c6625..a9767bc3777 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs @@ -121,7 +121,7 @@ impl Instruction { pub(crate) fn result_type(&self) -> InstructionResultType { match self { Instruction::Binary(binary) => binary.result_type(), - Instruction::Cast(_, typ) => InstructionResultType::Known(*typ), + Instruction::Cast(_, typ) => InstructionResultType::Known(typ.clone()), Instruction::Allocate { .. } => InstructionResultType::Known(Type::Reference), Instruction::Not(value) | Instruction::Truncate { value, .. } => { InstructionResultType::Operand(*value) @@ -150,7 +150,7 @@ impl Instruction { rhs: f(binary.rhs), operator: binary.operator, }), - Instruction::Cast(value, typ) => Instruction::Cast(f(*value), *typ), + Instruction::Cast(value, typ) => Instruction::Cast(f(*value), typ.clone()), Instruction::Not(value) => Instruction::Not(f(*value)), Instruction::Truncate { value, bit_size, max_bit_size } => Instruction::Truncate { value: f(*value), @@ -181,7 +181,7 @@ impl Instruction { pub(crate) fn simplify(&self, dfg: &mut DataFlowGraph) -> SimplifyResult { use SimplifyResult::*; match self { - Instruction::Binary(binary) => return binary.simplify(dfg), + Instruction::Binary(binary) => binary.simplify(dfg), Instruction::Cast(value, typ) => { if let Some(value) = (*typ == dfg.type_of_value(*value)).then_some(*value) { SimplifiedTo(value) @@ -219,7 +219,9 @@ impl Instruction { } Instruction::ArrayGet { array, index } => { let array = dfg.get_array_constant(*array); - if let (Some(array), Some(index)) = (array, dfg.get_numeric_constant(*index)) { + let index = dfg.get_numeric_constant(*index); + + if let (Some((array, _)), Some(index)) = (array, index) { let index = index.try_to_u64().expect("Expected array index to fit in u64") as usize; assert!(index < array.len()); @@ -230,11 +232,13 @@ impl Instruction { } Instruction::ArraySet { array, index, value } => { let array = dfg.get_array_constant(*array); - if let (Some(array), Some(index)) = (array, dfg.get_numeric_constant(*index)) { + let index = dfg.get_numeric_constant(*index); + + if let (Some((array, element_type)), Some(index)) = (array, index) { let index = index.try_to_u64().expect("Expected array index to fit in u64") as usize; assert!(index < array.len()); - SimplifiedTo(dfg.make_array(array.update(index, *value))) + SimplifiedTo(dfg.make_array(array.update(index, *value), element_type)) } else { None } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs index 37c4177e7dd..2829c6768b8 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs @@ -67,8 +67,8 @@ fn value(function: &Function, id: ValueId) -> String { } Value::Function(id) => id.to_string(), Value::Intrinsic(intrinsic) => intrinsic.to_string(), - Value::Array(elements) => { - let elements = vecmap(elements, |element| value(function, *element)); + Value::Array { array, .. } => { + let elements = vecmap(array, |element| value(function, *element)); format!("[{}]", elements.join(", ")) } Value::Param { .. } | Value::Instruction { .. } => id.to_string(), diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/types.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/types.rs index 2d911e9000e..fe9705c8e00 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/types.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/types.rs @@ -1,3 +1,7 @@ +use std::rc::Rc; + +use iter_extended::vecmap; + /// A numeric type in the Intermediate representation /// Note: we class NativeField as a numeric type /// though we also apply limitations to it, such as not @@ -14,7 +18,7 @@ pub(crate) enum NumericType { } /// All types representable in the IR. -#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +#[derive(Clone, Debug, PartialEq, Eq, Hash)] pub(crate) enum Type { /// Represents numeric types in the IR, including field elements Numeric(NumericType), @@ -22,8 +26,8 @@ pub(crate) enum Type { /// A reference to some value, such as an array Reference, - /// An immutable array value - Array, + /// An immutable array value with the given element type and length + Array(Rc, usize), /// A function that may be called directly Function, @@ -48,18 +52,31 @@ impl Type { Type::unsigned(1) } + /// Creates the char type, represented as just a field to avoid extra range constraints. + pub(crate) fn char() -> Type { + Type::field() + } + /// Creates the native field type. pub(crate) fn field() -> Type { Type::Numeric(NumericType::NativeField) } } +/// Composite Types are essentially flattened struct or tuple types. +/// Array types may have these as elements where each flattened field is +/// included in the array sequentially. +pub(crate) type CompositeType = Vec; + impl std::fmt::Display for Type { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { Type::Numeric(numeric) => numeric.fmt(f), Type::Reference => write!(f, "reference"), - Type::Array => write!(f, "array"), + Type::Array(element, length) => { + let elements = vecmap(element.iter(), |element| element.to_string()); + write!(f, "[{}; {length}]", elements.join(", ")) + } Type::Function => write!(f, "function"), Type::Unit => write!(f, "unit"), } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs index 36b83e65f8a..30468a0a669 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs @@ -1,3 +1,5 @@ +use std::rc::Rc; + use acvm::FieldElement; use crate::ssa_refactor::ir::basic_block::BasicBlockId; @@ -6,7 +8,7 @@ use super::{ function::FunctionId, instruction::{InstructionId, Intrinsic}, map::Id, - types::Type, + types::{CompositeType, Type}, }; pub(crate) type ValueId = Id; @@ -36,7 +38,7 @@ pub(crate) enum Value { NumericConstant { constant: FieldElement, typ: Type }, /// Represents a constant array value - Array(im::Vector), + Array { array: im::Vector, element_type: Rc }, /// This Value refers to a function in the IR. /// Functions always have the type Type::Function. @@ -54,10 +56,10 @@ impl Value { /// Retrieves the type of this Value pub(crate) fn get_type(&self) -> Type { match self { - Value::Instruction { typ, .. } => *typ, - Value::Param { typ, .. } => *typ, - Value::NumericConstant { typ, .. } => *typ, - Value::Array(_) => Type::Array, + Value::Instruction { typ, .. } => typ.clone(), + Value::Param { typ, .. } => typ.clone(), + Value::NumericConstant { typ, .. } => typ.clone(), + Value::Array { element_type, array } => Type::Array(element_type.clone(), array.len()), Value::Function { .. } => Type::Function, Value::Intrinsic { .. } => Type::Function, } diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs index 2f4316ccbaa..0346dcee166 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs @@ -203,13 +203,13 @@ impl<'function> PerFunctionContext<'function> { unreachable!("All Value::Params should already be known from previous calls to translate_block. Unknown value {id} = {value:?}") } Value::NumericConstant { constant, typ } => { - self.context.builder.numeric_constant(*constant, *typ) + self.context.builder.numeric_constant(*constant, typ.clone()) } Value::Function(function) => self.context.builder.import_function(*function), Value::Intrinsic(intrinsic) => self.context.builder.import_intrinsic_id(*intrinsic), - Value::Array(array) => { + Value::Array { array, element_type } => { let elements = array.iter().map(|value| self.translate_value(*value)).collect(); - self.context.builder.array_constant(elements) + self.context.builder.array_constant(elements, element_type.clone()) } }; diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs index 9e68d40834c..4166512f695 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs @@ -227,6 +227,8 @@ impl PerBlockContext { #[cfg(test)] mod tests { + use std::rc::Rc; + use acvm::FieldElement; use im::vector; @@ -258,10 +260,12 @@ mod tests { let v0 = builder.insert_allocate(); let one = builder.field_constant(FieldElement::one()); let two = builder.field_constant(FieldElement::one()); - let array = builder.array_constant(vector![one, two]); + + let element_type = Rc::new(vec![Type::field()]); + let array = builder.array_constant(vector![one, two], element_type.clone()); builder.insert_store(v0, array); - let v1 = builder.insert_load(v0, Type::Array); + let v1 = builder.insert_load(v0, Type::Array(element_type, 2)); let v2 = builder.insert_array_get(v1, one, Type::field()); builder.terminate_with_return(vec![v2]); diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_builder/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_builder/mod.rs index c1867e05cf2..902278e286a 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_builder/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_builder/mod.rs @@ -1,3 +1,5 @@ +use std::rc::Rc; + use acvm::FieldElement; use crate::ssa_refactor::ir::{ @@ -14,6 +16,7 @@ use super::{ dfg::InsertInstructionResult, function::RuntimeType, instruction::{InstructionId, Intrinsic}, + types::CompositeType, }, ssa_gen::Ssa, }; @@ -104,9 +107,13 @@ impl FunctionBuilder { self.numeric_constant(value.into(), Type::field()) } - /// Insert an array constant into the current function with the given element values - pub(crate) fn array_constant(&mut self, elements: im::Vector) -> ValueId { - self.current_function.dfg.make_array(elements) + /// Insert an array constant into the current function with the given element values. + pub(crate) fn array_constant( + &mut self, + elements: im::Vector, + element_types: Rc, + ) -> ValueId { + self.current_function.dfg.make_array(elements, element_types) } /// Returns the type of the given value. diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs index 1e01054c508..386648c87f0 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs @@ -2,6 +2,8 @@ mod context; mod program; mod value; +use std::rc::Rc; + pub(crate) use program::Ssa; use context::SharedContext; @@ -14,7 +16,12 @@ use self::{ value::{Tree, Values}, }; -use super::ir::{function::RuntimeType, instruction::BinaryOp, types::Type, value::ValueId}; +use super::ir::{ + function::RuntimeType, + instruction::BinaryOp, + types::{CompositeType, Type}, + value::ValueId, +}; /// Generates SSA for the given monomorphized program. /// @@ -102,7 +109,8 @@ impl<'a> FunctionContext<'a> { match literal { ast::Literal::Array(array) => { let elements = vecmap(&array.contents, |element| self.codegen_expression(element)); - self.codegen_array(elements) + let element_types = Self::convert_type(&array.element_type).flatten(); + self.codegen_array(elements, element_types) } ast::Literal::Integer(value, typ) => { let typ = Self::convert_non_tuple_type(typ); @@ -115,7 +123,7 @@ impl<'a> FunctionContext<'a> { let elements = vecmap(string.as_bytes(), |byte| { self.builder.numeric_constant(*byte as u128, Type::field()).into() }); - self.codegen_array(elements) + self.codegen_array(elements, vec![Type::char()]) } } } @@ -129,7 +137,7 @@ impl<'a> FunctionContext<'a> { /// stored the same as the array [1, 2, 3, 4]. /// /// The value returned from this function is always that of the allocate instruction. - fn codegen_array(&mut self, elements: Vec) -> Values { + fn codegen_array(&mut self, elements: Vec, element_types: CompositeType) -> Values { let mut array = im::Vector::new(); for element in elements { @@ -139,7 +147,7 @@ impl<'a> FunctionContext<'a> { }); } - self.builder.array_constant(array).into() + self.builder.array_constant(array, Rc::new(element_types)).into() } fn codegen_block(&mut self, block: &[Expression]) -> Values { @@ -347,7 +355,7 @@ impl<'a> FunctionContext<'a> { let mut values = self.codegen_expression(&let_expr.expression); if let_expr.mutable { - values.map_mut(|value| { + values = values.map(|value| { let value = value.eval(self); Tree::Leaf(self.new_mutable_variable(value)) }); diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs index 8904c84594b..0637641528e 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs @@ -26,7 +26,7 @@ pub(super) enum Tree { /// used internally by functions in the ssa ir and should thus be isolated /// to a given function. If used outisde their function of origin, the IDs /// would be invalid. -#[derive(Debug, Copy, Clone)] +#[derive(Debug, Clone)] pub(super) enum Value { Normal(IrValueId), From 1116881e7cd2118d2ecf3a667b6ec89cc9515705 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 7 Jun 2023 13:25:00 -0500 Subject: [PATCH 12/21] Refactor acir array operations into a shared function --- .../ssa_refactor/acir_gen/acir_ir/errors.rs | 6 --- .../src/ssa_refactor/acir_gen/mod.rs | 51 +++++++++++-------- 2 files changed, 31 insertions(+), 26 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/errors.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/errors.rs index fe9aba43339..1373a54fe65 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/errors.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/errors.rs @@ -1,10 +1,7 @@ -use crate::ssa_refactor::ir::value::ValueId; - #[derive(Debug, PartialEq, Eq, Clone)] pub(crate) enum AcirGenError { InvalidRangeConstraint { num_bits: u32 }, IndexOutOfBounds { index: usize, array_size: usize }, - UninitializedElementInArray { index: usize, array_id: ValueId }, } impl AcirGenError { @@ -18,9 +15,6 @@ impl AcirGenError { AcirGenError::IndexOutOfBounds { index, array_size } => { format!("Index out of bounds, array has size {array_size}, but index was {index}") } - AcirGenError::UninitializedElementInArray { index, array_id } => { - format!("The element at index {index} was never set in array {array_id}") - } } } } diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs index 0dce57f42ad..1916f2b9a0b 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs @@ -200,28 +200,10 @@ impl Context { self.define_result_var(dfg, instruction_id, result_acir_var); } Instruction::ArrayGet { array, index } => { - let array = self.convert_array_value(*array, dfg); - let index = dfg - .get_numeric_constant(*index) - .expect("Expected array index to be a known constant") - .try_to_u64() - .expect("Expected array index to fit into a u64") - as usize; - - self.define_result(dfg, instruction_id, array[index].clone()); + self.handle_array_operation(instruction_id, *array, *index, None, dfg) } Instruction::ArraySet { array, index, value } => { - let array = self.convert_array_value(*array, dfg); - let index = dfg - .get_numeric_constant(*index) - .expect("Expected array index to be a known constant") - .try_to_u64() - .expect("Expected array index to fit into a u64") - as usize; - - let value = self.convert_value(*value, dfg); - let new_array = AcirValue::Array(array.update(index, value)); - self.define_result(dfg, instruction_id, new_array); + self.handle_array_operation(instruction_id, *array, *index, Some(*value), dfg) } Instruction::Allocate => { unreachable!("Expected all allocate instructions to be removed before acir_gen") @@ -235,6 +217,35 @@ impl Context { } } + /// Handles an ArrayGet or ArraySet instruction. + /// To set an index of the array (and create a new array in doing so), pass Some(value) for + /// store_value. To just retrieve an index of the array, pass None for store_value. + fn handle_array_operation( + &mut self, + instruction: InstructionId, + array: ValueId, + index: ValueId, + store_value: Option, + dfg: &DataFlowGraph, + ) { + let array = self.convert_array_value(array, dfg); + let index = dfg + .get_numeric_constant(index) + .expect("Expected array index to be a known constant") + .try_to_u64() + .expect("Expected array index to fit into a u64") as usize; + + let value = match store_value { + Some(store_value) => { + let store_value = self.convert_value(store_value, dfg); + AcirValue::Array(array.update(index, store_value)) + } + None => array[index].clone(), + }; + + self.define_result(dfg, instruction, value); + } + /// Remember the result of an instruction returning a single value fn define_result( &mut self, From d94444cfc65751c0948ebab21f62de6f63e3800d Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 7 Jun 2023 15:00:07 -0500 Subject: [PATCH 13/21] Appease clippy --- crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs index 1916f2b9a0b..0d0f74f3c24 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs @@ -200,10 +200,10 @@ impl Context { self.define_result_var(dfg, instruction_id, result_acir_var); } Instruction::ArrayGet { array, index } => { - self.handle_array_operation(instruction_id, *array, *index, None, dfg) + self.handle_array_operation(instruction_id, *array, *index, None, dfg); } Instruction::ArraySet { array, index, value } => { - self.handle_array_operation(instruction_id, *array, *index, Some(*value), dfg) + self.handle_array_operation(instruction_id, *array, *index, Some(*value), dfg); } Instruction::Allocate => { unreachable!("Expected all allocate instructions to be removed before acir_gen") From e6c3cd0ac932618f347d3350974c2ccd10def792 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 7 Jun 2023 15:05:29 -0500 Subject: [PATCH 14/21] Update to_radix and to_bits in acir_gen to return arrays --- .../src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs index 62bc09bb888..f8bb7939833 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs @@ -613,7 +613,7 @@ impl AcirContext { limb_vars.reverse(); } - Ok(limb_vars) + Ok(vec![AcirValue::Array(limb_vars.into())]) } /// Returns `AcirVar`s constrained to be the bit decomposition of the provided input From faa991c0c3927da43c74436d4e9c71d5b4fa3ecf Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 8 Jun 2023 09:22:46 -0500 Subject: [PATCH 15/21] Disable assert --- crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs index 0d0f74f3c24..65167b98a48 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs @@ -176,7 +176,10 @@ impl Context { allow_log_ops, ); - assert_eq!(result_ids.len(), outputs.len()); + // Issue #1438 causes this check to fail with intrinsics that return 0 + // results but the ssa form instead creates 1 unit result value. + // assert_eq!(result_ids.len(), outputs.len()); + for (result, output) in result_ids.iter().zip(outputs) { self.ssa_values.insert(*result, output); } From 5e7c1dddca236d23d84aee4634579cc5e5266ce9 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 8 Jun 2023 10:44:59 -0500 Subject: [PATCH 16/21] Fix convert_type for arrays --- crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs index ed4b1972465..e20a54ba8cc 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +use std::rc::Rc; use std::sync::{Mutex, RwLock}; use iter_extended::vecmap; @@ -186,7 +187,10 @@ impl<'a> FunctionContext<'a> { pub(super) fn convert_non_tuple_type(typ: &ast::Type) -> Type { match typ { ast::Type::Field => Type::field(), - ast::Type::Array(_, _) => Type::Reference, + ast::Type::Array(len, element) => { + let element_types = Self::convert_type(element).flatten(); + Type::Array(Rc::new(element_types), *len as usize) + } ast::Type::Integer(Signedness::Signed, bits) => Type::signed(*bits), ast::Type::Integer(Signedness::Unsigned, bits) => Type::unsigned(*bits), ast::Type::Bool => Type::unsigned(1), From 81f1e051ea562fafe5454d1d376576495ddfa122 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 8 Jun 2023 12:24:04 -0500 Subject: [PATCH 17/21] Include AcirType in AcirValue::Var variant --- .../acir_gen/acir_ir/acir_variable.rs | 78 ++++++++++++------- .../src/ssa_refactor/acir_gen/mod.rs | 59 +++++++++----- 2 files changed, 88 insertions(+), 49 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs index beda35ebccb..8b8aa6a0f64 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs @@ -28,6 +28,10 @@ use std::{borrow::Cow, hash::Hash}; pub(crate) struct AcirType(NumericType); impl AcirType { + pub(crate) fn new(typ: NumericType) -> Self { + Self(typ) + } + /// Returns the bit size of the underlying type pub(crate) fn bit_size(&self) -> u32 { match self.0 { @@ -45,8 +49,14 @@ impl AcirType { impl From for AcirType { fn from(value: SsaType) -> Self { + AcirType::from(&value) + } +} + +impl<'a> From<&'a SsaType> for AcirType { + fn from(value: &SsaType) -> Self { match value { - SsaType::Numeric(numeric_type) => AcirType(numeric_type), + SsaType::Numeric(numeric_type) => AcirType(*numeric_type), _ => unreachable!("The type {value} cannot be represented in ACIR"), } } @@ -156,11 +166,11 @@ impl AcirContext { &mut self, lhs: AcirVar, rhs: AcirVar, - bit_count: u32, + typ: AcirType, ) -> Result { - let inputs = vec![(AcirValue::Var(lhs), bit_count), (AcirValue::Var(rhs), bit_count)]; - let mut outputs = self.black_box_function(BlackBoxFunc::XOR, inputs)?; - Ok(outputs.swap_remove(0).into_var()) + let inputs = vec![AcirValue::Var(lhs, typ), AcirValue::Var(rhs, typ)]; + let outputs = self.black_box_function(BlackBoxFunc::XOR, inputs)?; + Ok(outputs[0]) } /// Returns an `AcirVar` that is the AND result of `lhs` & `rhs`. @@ -168,11 +178,11 @@ impl AcirContext { &mut self, lhs: AcirVar, rhs: AcirVar, - bit_count: u32, + typ: AcirType, ) -> Result { - let inputs = vec![(AcirValue::Var(lhs), bit_count), (AcirValue::Var(rhs), bit_count)]; - let mut outputs = self.black_box_function(BlackBoxFunc::AND, inputs)?; - Ok(outputs.swap_remove(0).into_var()) + let inputs = vec![AcirValue::Var(lhs, typ), AcirValue::Var(rhs, typ)]; + let outputs = self.black_box_function(BlackBoxFunc::AND, inputs)?; + Ok(outputs[0]) } /// Returns an `AcirVar` that is the OR result of `lhs` & `rhs`. @@ -180,8 +190,9 @@ impl AcirContext { &mut self, lhs: AcirVar, rhs: AcirVar, - bit_size: u32, + typ: AcirType, ) -> Result { + let bit_size = typ.bit_size(); if bit_size == 1 { // Operands are booleans // a + b - ab @@ -196,9 +207,9 @@ impl AcirContext { let max = self.add_constant(FieldElement::from((1_u128 << bit_size) - 1)); let a = self.sub_var(max, lhs)?; let b = self.sub_var(max, rhs)?; - let inputs = vec![(AcirValue::Var(a), bit_size), (AcirValue::Var(b), bit_size)]; - let mut output = self.black_box_function(BlackBoxFunc::AND, inputs)?; - self.sub_var(max, output.swap_remove(0).into_var()) + let inputs = vec![AcirValue::Var(a, typ), AcirValue::Var(b, typ)]; + let outputs = self.black_box_function(BlackBoxFunc::AND, inputs)?; + self.sub_var(max, outputs[0]) } } @@ -537,8 +548,8 @@ impl AcirContext { pub(crate) fn black_box_function( &mut self, name: BlackBoxFunc, - mut inputs: Vec<(AcirValue, /*bit_count*/ u32)>, - ) -> Result, AcirGenError> { + mut inputs: Vec, + ) -> Result, AcirGenError> { // Separate out any arguments that should be constants let constants = match name { BlackBoxFunc::Pedersen => { @@ -546,7 +557,6 @@ impl AcirContext { let domain_var = inputs .pop() .expect("ICE: Pedersen call requires domain separator") - .0 .into_var(); let domain_constant = self.vars[domain_var] @@ -570,7 +580,7 @@ impl AcirContext { // We do not apply range information on the output of the black box function. // See issue #1439 Ok(vecmap(&outputs, |witness_index| { - AcirValue::Var(self.add_data(AcirVarData::Witness(*witness_index))) + self.add_data(AcirVarData::Witness(*witness_index)) })) } @@ -579,18 +589,21 @@ impl AcirContext { /// This function will convert `AcirVar` into `FunctionInput` for a blackbox function call. fn prepare_inputs_for_black_box_func_call( &mut self, - inputs: Vec<(AcirValue, /*bit_count*/ u32)>, + inputs: Vec, ) -> Result, AcirGenError> { let mut witnesses = Vec::new(); - for (input, num_bits) in inputs { - let var_data = &self.vars[input.into_var()]; - - // Intrinsics only accept Witnesses. This is not a limitation of the - // intrinsics, its just how we have defined things. Ideally, we allow - // constants too. - let expr = var_data.to_expression(); - let witness = self.acir_ir.get_or_create_witness(&expr); - witnesses.push(FunctionInput { witness, num_bits }); + for input in inputs { + for (input, typ) in input.flatten() { + let var_data = &self.vars[input]; + + // Intrinsics only accept Witnesses. This is not a limitation of the + // intrinsics, its just how we have defined things. Ideally, we allow + // constants too. + let expr = var_data.to_expression(); + let witness = self.acir_ir.get_or_create_witness(&expr); + let num_bits = typ.bit_size(); + witnesses.push(FunctionInput { witness, num_bits }); + } } Ok(witnesses) } @@ -607,6 +620,7 @@ impl AcirContext { input_var: AcirVar, radix_var: AcirVar, limb_count_var: AcirVar, + result_element_type: AcirType, ) -> Result, AcirGenError> { let radix = self.vars[&radix_var].as_constant().expect("ICE: radix should be a constant").to_u128() @@ -622,7 +636,10 @@ impl AcirContext { let limbs = self.acir_ir.radix_le_decompose(input_expr, radix, limb_count)?; let mut limb_vars = - vecmap(limbs, |witness| AcirValue::Var(self.add_data(AcirVarData::Witness(witness)))); + vecmap(limbs, |witness| { + let witness = self.add_data(AcirVarData::Witness(witness)); + AcirValue::Var(witness, result_element_type) + }); if endian == Endian::Big { limb_vars.reverse(); @@ -637,9 +654,10 @@ impl AcirContext { endian: Endian, input_var: AcirVar, limb_count_var: AcirVar, + result_element_type: AcirType, ) -> Result, AcirGenError> { let two_var = self.add_constant(FieldElement::from(2_u128)); - self.radix_decompose(endian, input_var, two_var, limb_count_var) + self.radix_decompose(endian, input_var, two_var, limb_count_var, result_element_type) } /// Prints the given `AcirVar`s as witnesses. @@ -665,7 +683,7 @@ impl AcirContext { pub(crate) fn flatten_value(acir_vars: &mut Vec, value: AcirValue) { match value { - AcirValue::Var(acir_var) => acir_vars.push(acir_var), + AcirValue::Var(acir_var, _) => acir_vars.push(acir_var), AcirValue::Array(array) => { for value in array { Self::flatten_value(acir_vars, value); diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs index 3f62ad83da9..b9eb91d6de9 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs @@ -45,17 +45,24 @@ struct Context { #[derive(Debug, Clone)] pub(crate) enum AcirValue { - Var(AcirVar), + Var(AcirVar, AcirType), Array(im::Vector), } impl AcirValue { fn into_var(self) -> AcirVar { match self { - AcirValue::Var(var) => var, + AcirValue::Var(var, _) => var, AcirValue::Array(_) => panic!("Called AcirValue::into_var on an array"), } } + + fn flatten(self) -> Vec<(AcirVar, AcirType)> { + match self { + AcirValue::Var(var, typ) => vec![(var, typ)], + AcirValue::Array(array) => array.into_iter().flat_map(AcirValue::flatten).collect(), + } + } } impl Ssa { @@ -99,7 +106,10 @@ impl Context { fn convert_ssa_block_param(&mut self, param_type: &Type) -> AcirValue { match param_type { - Type::Numeric(numeric_type) => AcirValue::Var(self.add_numeric_input_var(numeric_type)), + Type::Numeric(numeric_type) => { + let typ = AcirType::new(*numeric_type); + AcirValue::Var(self.add_numeric_input_var(numeric_type), typ) + } Type::Array(element_types, length) => { let mut elements = im::Vector::new(); @@ -179,6 +189,7 @@ impl Context { arguments, dfg, allow_log_ops, + result_ids, ); // Issue #1438 causes this check to fail with intrinsics that return 0 @@ -272,7 +283,9 @@ impl Context { instruction: InstructionId, result: AcirVar, ) { - self.define_result(dfg, instruction, AcirValue::Var(result)); + let result_ids = dfg.instruction_results(instruction); + let typ = dfg.type_of_value(result_ids[0]).into(); + self.define_result(dfg, instruction, AcirValue::Var(result, typ)); } /// Converts an SSA terminator's return values into their ACIR representations @@ -319,8 +332,8 @@ impl Context { } let acir_value = match value { - Value::NumericConstant { constant, .. } => { - AcirValue::Var(self.acir_context.add_constant(*constant)) + Value::NumericConstant { constant, typ } => { + AcirValue::Var(self.acir_context.add_constant(*constant), typ.into()) } Value::Array { array, .. } => { let elements = array.iter().map(|element| self.convert_value(*element, dfg)); @@ -342,14 +355,14 @@ impl Context { dfg: &DataFlowGraph, ) -> im::Vector { match self.convert_value(value_id, dfg) { - AcirValue::Var(acir_var) => panic!("Expected an array value, found: {acir_var:?}"), + AcirValue::Var(acir_var, _) => panic!("Expected an array value, found: {acir_var:?}"), AcirValue::Array(array) => array, } } fn convert_numeric_value(&mut self, value_id: ValueId, dfg: &DataFlowGraph) -> AcirVar { match self.convert_value(value_id, dfg) { - AcirValue::Var(acir_var) => acir_var, + AcirValue::Var(acir_var, _) => acir_var, AcirValue::Array(array) => panic!("Expected a numeric value, found: {array:?}"), } } @@ -395,9 +408,9 @@ impl Context { BinaryOp::Lt => self.acir_context.less_than_var(lhs, rhs, bit_count), BinaryOp::Shl => self.acir_context.shift_left_var(lhs, rhs, binary_type), BinaryOp::Shr => self.acir_context.shift_right_var(lhs, rhs, binary_type), - BinaryOp::Xor => self.acir_context.xor_var(lhs, rhs, bit_count), - BinaryOp::And => self.acir_context.and_var(lhs, rhs, bit_count), - BinaryOp::Or => self.acir_context.or_var(lhs, rhs, bit_count), + BinaryOp::Xor => self.acir_context.xor_var(lhs, rhs, binary_type), + BinaryOp::And => self.acir_context.and_var(lhs, rhs, binary_type), + BinaryOp::Or => self.acir_context.or_var(lhs, rhs, binary_type), BinaryOp::Mod => self.acir_context.modulo_var(lhs, rhs, bit_count), } } @@ -472,31 +485,35 @@ impl Context { arguments: &[ValueId], dfg: &DataFlowGraph, allow_log_ops: bool, + result_ids: &[ValueId] ) -> Vec { match intrinsic { Intrinsic::BlackBox(black_box) => { - let inputs = vecmap(arguments, |arg| { - let bit_count = self.bit_count(*arg, dfg); - (self.convert_value(*arg, dfg), bit_count) - }); + let inputs = vecmap(arguments, |arg| self.convert_value(*arg, dfg)); - self.acir_context + let vars = self.acir_context .black_box_function(black_box, inputs) - .expect("add Result types to all methods so errors bubble up") + .expect("add Result types to all methods so errors bubble up"); + + self.convert_vars_to_values(vars, dfg, result_ids) } Intrinsic::ToRadix(endian) => { let field = self.convert_value(arguments[0], dfg).into_var(); let radix = self.convert_value(arguments[1], dfg).into_var(); let limb_size = self.convert_value(arguments[2], dfg).into_var(); + let result_type = dfg.type_of_value(result_ids[0]).into(); + self.acir_context - .radix_decompose(endian, field, radix, limb_size) + .radix_decompose(endian, field, radix, limb_size, result_type) .expect("add Result types to all methods so errors bubble up") } Intrinsic::ToBits(endian) => { let field = self.convert_value(arguments[0], dfg).into_var(); let bit_size = self.convert_value(arguments[1], dfg).into_var(); + let result_type = dfg.type_of_value(result_ids[0]).into(); + self.acir_context - .bit_decompose(endian, field, bit_size) + .bit_decompose(endian, field, bit_size, result_type) .expect("add Result types to all methods so errors bubble up") } Intrinsic::Println => { @@ -532,6 +549,10 @@ impl Context { _ => 0, } } + + fn convert_vars_to_values(&self, vars: Vec, dfg: &DataFlowGraph, result_ids: &[ValueId]) -> Vec { + let mut values = vec![]; + } } #[cfg(test)] From 1478a355cf9619b712ac51ddd0296f0450360370 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 8 Jun 2023 13:33:24 -0500 Subject: [PATCH 18/21] Fix black box functions --- crates/nargo_cli/src/cli/prove_cmd.rs | 2 +- .../acir_gen/acir_ir/acir_variable.rs | 19 +++---- .../src/ssa_refactor/acir_gen/mod.rs | 49 +++++++++++++++++-- .../src/ssa_refactor/ir/types.rs | 4 +- 4 files changed, 55 insertions(+), 19 deletions(-) diff --git a/crates/nargo_cli/src/cli/prove_cmd.rs b/crates/nargo_cli/src/cli/prove_cmd.rs index 601bacd278b..25948ba1784 100644 --- a/crates/nargo_cli/src/cli/prove_cmd.rs +++ b/crates/nargo_cli/src/cli/prove_cmd.rs @@ -44,7 +44,7 @@ pub(crate) struct ProveCommand { verifier_name: String, /// Verify proof after proving - #[arg(short, long)] + #[arg(long)] verify: bool, #[clap(flatten)] diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs index 8b8aa6a0f64..51f5b40232d 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs @@ -554,10 +554,8 @@ impl AcirContext { let constants = match name { BlackBoxFunc::Pedersen => { // The last argument of pedersen is the domain separator, which must be a constant - let domain_var = inputs - .pop() - .expect("ICE: Pedersen call requires domain separator") - .into_var(); + let domain_var = + inputs.pop().expect("ICE: Pedersen call requires domain separator").into_var(); let domain_constant = self.vars[domain_var] .as_constant() @@ -579,9 +577,7 @@ impl AcirContext { // // We do not apply range information on the output of the black box function. // See issue #1439 - Ok(vecmap(&outputs, |witness_index| { - self.add_data(AcirVarData::Witness(*witness_index)) - })) + Ok(vecmap(&outputs, |witness_index| self.add_data(AcirVarData::Witness(*witness_index)))) } /// Black box function calls expect their inputs to be in a specific data structure (FunctionInput). @@ -635,11 +631,10 @@ impl AcirContext { let limbs = self.acir_ir.radix_le_decompose(input_expr, radix, limb_count)?; - let mut limb_vars = - vecmap(limbs, |witness| { - let witness = self.add_data(AcirVarData::Witness(witness)); - AcirValue::Var(witness, result_element_type) - }); + let mut limb_vars = vecmap(limbs, |witness| { + let witness = self.add_data(AcirVarData::Witness(witness)); + AcirValue::Var(witness, result_element_type) + }); if endian == Endian::Big { limb_vars.reverse(); diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs index b9eb91d6de9..0086a98e56d 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs @@ -485,17 +485,18 @@ impl Context { arguments: &[ValueId], dfg: &DataFlowGraph, allow_log_ops: bool, - result_ids: &[ValueId] + result_ids: &[ValueId], ) -> Vec { match intrinsic { Intrinsic::BlackBox(black_box) => { let inputs = vecmap(arguments, |arg| self.convert_value(*arg, dfg)); - let vars = self.acir_context + let vars = self + .acir_context .black_box_function(black_box, inputs) .expect("add Result types to all methods so errors bubble up"); - self.convert_vars_to_values(vars, dfg, result_ids) + Self::convert_vars_to_values(vars, dfg, result_ids) } Intrinsic::ToRadix(endian) => { let field = self.convert_value(arguments[0], dfg).into_var(); @@ -550,8 +551,48 @@ impl Context { } } - fn convert_vars_to_values(&self, vars: Vec, dfg: &DataFlowGraph, result_ids: &[ValueId]) -> Vec { + /// Convert a Vec into a Vec using the given result ids. + /// If the type of a result id is an array, several acirvars are collected into + /// a single AcirValue::Array of the same length. + fn convert_vars_to_values( + vars: Vec, + dfg: &DataFlowGraph, + result_ids: &[ValueId], + ) -> Vec { let mut values = vec![]; + let mut vars = vars.into_iter(); + + for result in result_ids { + Self::convert_var_type_to_values( + dfg, + &dfg.type_of_value(*result), + &mut values, + &mut vars, + ); + } + + values + } + + fn convert_var_type_to_values( + dfg: &DataFlowGraph, + result_type: &Type, + values: &mut Vec, + vars: &mut impl Iterator, + ) { + match result_type { + Type::Array(elements, size) => { + for _ in 0..*size { + for element_type in elements.iter() { + Self::convert_var_type_to_values(dfg, element_type, values, vars); + } + } + } + typ => { + let var = vars.next().unwrap(); + values.push(AcirValue::Var(var, typ.into())); + } + } } } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/types.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/types.rs index fe9705c8e00..d4a3946375a 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/types.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/types.rs @@ -52,9 +52,9 @@ impl Type { Type::unsigned(1) } - /// Creates the char type, represented as just a field to avoid extra range constraints. + /// Creates the char type, represented as u8. pub(crate) fn char() -> Type { - Type::field() + Type::unsigned(8) } /// Creates the native field type. From 70137b84122eab70cf9befbb04b5d11bf8534c14 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 8 Jun 2023 13:54:35 -0500 Subject: [PATCH 19/21] Appease clippy --- .../noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs index 0086a98e56d..efdd54de5a2 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs @@ -563,19 +563,13 @@ impl Context { let mut vars = vars.into_iter(); for result in result_ids { - Self::convert_var_type_to_values( - dfg, - &dfg.type_of_value(*result), - &mut values, - &mut vars, - ); + Self::convert_var_type_to_values(&dfg.type_of_value(*result), &mut values, &mut vars); } values } fn convert_var_type_to_values( - dfg: &DataFlowGraph, result_type: &Type, values: &mut Vec, vars: &mut impl Iterator, @@ -584,7 +578,7 @@ impl Context { Type::Array(elements, size) => { for _ in 0..*size { for element_type in elements.iter() { - Self::convert_var_type_to_values(dfg, element_type, values, vars); + Self::convert_var_type_to_values(element_type, values, vars); } } } From 471585104410be8d4da410b6be2117210d51a4a3 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 8 Jun 2023 14:12:53 -0500 Subject: [PATCH 20/21] Fix simple_radix --- .../src/ssa_refactor/acir_gen/mod.rs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs index efdd54de5a2..c8be4ff105d 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs @@ -502,7 +502,7 @@ impl Context { let field = self.convert_value(arguments[0], dfg).into_var(); let radix = self.convert_value(arguments[1], dfg).into_var(); let limb_size = self.convert_value(arguments[2], dfg).into_var(); - let result_type = dfg.type_of_value(result_ids[0]).into(); + let result_type = Self::array_element_type(dfg, result_ids[0]); self.acir_context .radix_decompose(endian, field, radix, limb_size, result_type) @@ -511,7 +511,7 @@ impl Context { Intrinsic::ToBits(endian) => { let field = self.convert_value(arguments[0], dfg).into_var(); let bit_size = self.convert_value(arguments[1], dfg).into_var(); - let result_type = dfg.type_of_value(result_ids[0]).into(); + let result_type = Self::array_element_type(dfg, result_ids[0]); self.acir_context .bit_decompose(endian, field, bit_size, result_type) @@ -530,6 +530,18 @@ impl Context { } } + /// Given an array value, return the numerical type of its element. + /// Panics if the given value is not an array or has a non-numeric element type. + fn array_element_type(dfg: &DataFlowGraph, value: ValueId) -> AcirType { + match dfg.type_of_value(value) { + Type::Array(elements, _) => { + assert_eq!(elements.len(), 1); + (&elements[0]).into() + } + _ => unreachable!("Expected array type"), + } + } + /// Maps an ssa value list, for which some values may be references to arrays, by inlining /// the `AcirVar`s corresponding to the contents of each array into the list of `AcirVar`s /// that correspond to other values. From 9b0748ea3426d2b8b9399ce0f277ecf9ff42f898 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 8 Jun 2023 14:48:24 -0500 Subject: [PATCH 21/21] Add doc comments --- .../acir_gen/acir_ir/acir_variable.rs | 4 +++ .../src/ssa_refactor/acir_gen/mod.rs | 25 +++++++++++-------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs index 51f5b40232d..a6d7ae157d3 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs @@ -668,6 +668,9 @@ impl AcirContext { Ok(()) } + /// Flatten the given Vector of AcirValues into a single vector of only variables. + /// Each AcirValue::Array in the vector is recursively flattened, so each element + /// will flattened into the resulting Vec. E.g. flatten_values([1, [2, 3]) == [1, 2, 3]. fn flatten_values(values: Vec) -> Vec { let mut acir_vars = Vec::with_capacity(values.len()); for value in values { @@ -676,6 +679,7 @@ impl AcirContext { acir_vars } + /// Recursive helper for flatten_values to flatten a single AcirValue into the result vector. pub(crate) fn flatten_value(acir_vars: &mut Vec, value: AcirValue) { match value { AcirValue::Var(acir_var, _) => acir_vars.push(acir_var), diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs index c8be4ff105d..537b08ddd93 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs @@ -571,32 +571,35 @@ impl Context { dfg: &DataFlowGraph, result_ids: &[ValueId], ) -> Vec { - let mut values = vec![]; let mut vars = vars.into_iter(); - - for result in result_ids { - Self::convert_var_type_to_values(&dfg.type_of_value(*result), &mut values, &mut vars); - } - - values + vecmap(result_ids, |result| { + let result_type = dfg.type_of_value(*result); + Self::convert_var_type_to_values(&result_type, &mut vars) + }) } + /// Recursive helper for convert_vars_to_values. + /// If the given result_type is an array of length N, this will create an AcirValue::Array with + /// the first N elements of the given iterator. Otherwise, the result is a single + /// AcirValue::Var wrapping the first element of the iterator. fn convert_var_type_to_values( result_type: &Type, - values: &mut Vec, vars: &mut impl Iterator, - ) { + ) -> AcirValue { match result_type { Type::Array(elements, size) => { + let mut element_values = im::Vector::new(); for _ in 0..*size { for element_type in elements.iter() { - Self::convert_var_type_to_values(element_type, values, vars); + let element = Self::convert_var_type_to_values(element_type, vars); + element_values.push_back(element); } } + AcirValue::Array(element_values) } typ => { let var = vars.next().unwrap(); - values.push(AcirValue::Var(var, typ.into())); + AcirValue::Var(var, typ.into()) } } }