From 1382d811b8e9edf9df64bd8e394ccffae01e65e5 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 13 Jan 2023 15:48:35 -0500 Subject: [PATCH] Revert "Remove returned_arrays and ArraySetId tracking" This reverts commit ee0c4e76ca4cf123f28b061e92b932cb5d95fd7b. --- crates/noirc_evaluator/src/ssa/context.rs | 75 +++++++++++++++- crates/noirc_evaluator/src/ssa/function.rs | 100 ++++++++++----------- crates/noirc_evaluator/src/ssa/node.rs | 7 +- 3 files changed, 126 insertions(+), 56 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/context.rs b/crates/noirc_evaluator/src/ssa/context.rs index 8b922752740..c2a9b6a3776 100644 --- a/crates/noirc_evaluator/src/ssa/context.rs +++ b/crates/noirc_evaluator/src/ssa/context.rs @@ -3,7 +3,9 @@ use super::conditional::{DecisionTree, TreeBuilder}; use super::function::{FuncIndex, SSAFunction}; use super::inline::StackFrame; use super::mem::{ArrayId, Memory}; -use super::node::{BinaryOp, FunctionKind, Instruction, NodeId, NodeObj, ObjectType, Operation}; +use super::node::{ + ArrayIdSet, BinaryOp, FunctionKind, Instruction, NodeId, NodeObj, ObjectType, Operation, +}; use super::{block, builtin, flatten, inline, integer, node, optim}; use std::collections::{HashMap, HashSet}; @@ -34,6 +36,9 @@ pub struct SsaContext { pub functions: HashMap, pub opcode_ids: HashMap, + /// Maps ArrayIdSet -> Vec<(ArrayId, u32)>, + pub function_returned_arrays: Vec>, + //Adjacency Matrix of the call graph; list of rows where each row indicates the functions called by the function whose FuncIndex is the row number pub call_graph: Vec>, dummy_store: HashMap, @@ -51,6 +56,7 @@ impl SsaContext { sealed_blocks: HashSet::new(), mem: Memory::default(), functions: HashMap::new(), + function_returned_arrays: Vec::new(), opcode_ids: HashMap::new(), call_graph: Vec::new(), dummy_store: HashMap::new(), @@ -1030,7 +1036,8 @@ impl SsaContext { let block1 = self[exit_block].predecessor[0]; let block2 = self[exit_block].predecessor[1]; - let a_type = self.get_object_type(a); + let mut a_type = self.get_object_type(a); + let b_type = self.get_object_type(b); let name = format!("if_{}_ret{c}", exit_block.0.into_raw_parts().0); *c += 1; @@ -1055,6 +1062,11 @@ impl SsaContext { } id } else { + if let (ObjectType::Function(a), ObjectType::Function(b)) = (a_type, b_type) { + let new_array_set_id = self.union_array_sets(a, b); + a_type = ObjectType::Function(new_array_set_id); + } + let new_var = node::Variable::new(a_type, name, None, exit_block); let v = self.add_variable(new_var, None); let operation = Operation::Phi { root: v, block_args: vec![(a, block1), (b, block2)] }; @@ -1074,6 +1086,10 @@ impl SsaContext { NodeId(index) } + pub fn set_function_type(&mut self, func_id: FuncId, node_id: NodeId, typ: ObjectType) { + self[node_id] = NodeObj::Function(FunctionKind::Normal(func_id), node_id, typ); + } + /// Return the standard NodeId for this FuncId. /// The 'standard' NodeId is just the NodeId assigned to the function when it /// is first compiled so that repeated NodeObjs are not made for the same function. @@ -1091,8 +1107,18 @@ impl SsaContext { return *id; } + let (len, elem_type) = opcode.get_result_type(); + let array_set = if len > 1 { + let array = self.new_array(&format!("{}_result", opcode), elem_type, len, None).1; + self.push_array_set(vec![(array, 0)]) + } else { + self.push_array_set(vec![]) + }; + let index = self.nodes.insert_with(|index| { - NodeObj::Function(FunctionKind::Builtin(opcode), NodeId(index), ObjectType::Function) + let node_id = NodeId(index); + let typ = ObjectType::Function(array_set); + NodeObj::Function(FunctionKind::Builtin(opcode), node_id, typ) }); self.opcode_ids.insert(opcode, NodeId(index)); NodeId(index) @@ -1105,6 +1131,33 @@ impl SsaContext { } } + pub fn push_array_set(&mut self, arrays: Vec<(ArrayId, u32)>) -> ArrayIdSet { + let next_id = self.function_returned_arrays.len(); + self.function_returned_arrays.push(arrays); + ArrayIdSet(next_id as u32) + } + + pub fn union_array_sets(&mut self, first: ArrayIdSet, second: ArrayIdSet) -> ArrayIdSet { + let mut union_set = self.function_returned_arrays[first.0 as usize].clone(); + union_set.extend_from_slice(&self.function_returned_arrays[second.0 as usize]); + // The sort + dedup here is expected to be cheap as the length of this set matches the number + // of array parameters of both functions, and the average function only uses roughly 0-2 array parameters. + // `union_array_sets` as a whole is also called fairly infrequently - only when function + // values are unified in a Phi instruction, e.g. from being returned by an if expression. + union_set.sort(); + union_set.dedup(); + self.push_array_set(union_set) + } + + pub fn get_returned_arrays(&self, func: NodeId) -> Vec<(ArrayId, u32)> { + match self[func].get_type() { + ObjectType::Function(arrays) => { + self.function_returned_arrays[arrays.0 as usize].clone() + } + other => unreachable!("get_returned_arrays: Expected function type, found {:?}", other), + } + } + pub fn convert_type(&mut self, t: &noirc_frontend::monomorphisation::ast::Type) -> ObjectType { use noirc_frontend::monomorphisation::ast::Type; use noirc_frontend::Signedness; @@ -1123,7 +1176,13 @@ impl SsaContext { } Type::Array(..) => panic!("Cannot convert an array type {} into an ObjectType since it is unknown which array it refers to", t), Type::Unit => ObjectType::NotAnObject, - Type::Function(..) => ObjectType::Function, + Type::Function(..) => { + // TODO #612: We should not track arrays through ObjectTypes. + // This is incorrect, we cannot track arrays through function types in this case + // since we do not know which arrays the function uses at this point. + let id = self.push_array_set(vec![]); + ObjectType::Function(id) + } Type::Tuple(_) => todo!("Conversion to ObjectType is unimplemented for tuples"), } } @@ -1207,3 +1266,11 @@ impl std::ops::IndexMut for SsaContext { &mut self.nodes[index.0] } } + +impl std::ops::Index for SsaContext { + type Output = Vec<(ArrayId, u32)>; + + fn index(&self, index: ArrayIdSet) -> &Self::Output { + &self.function_returned_arrays[index.0 as usize] + } +} diff --git a/crates/noirc_evaluator/src/ssa/function.rs b/crates/noirc_evaluator/src/ssa/function.rs index 0b9fac64619..c3ad41db67e 100644 --- a/crates/noirc_evaluator/src/ssa/function.rs +++ b/crates/noirc_evaluator/src/ssa/function.rs @@ -7,7 +7,7 @@ use noirc_frontend::monomorphisation::ast::{Call, Definition, FuncId, LocalId, T use super::builtin; use super::conditional::{AssumptionId, DecisionTree, TreeBuilder}; -use super::node::{Node, Operation}; +use super::node::{ArrayIdSet, Node, Operation}; use super::{ block, block::BlockId, @@ -48,10 +48,12 @@ impl SSAFunction { idx: FuncIndex, ctx: &mut SsaContext, ) -> SSAFunction { + // Temporary ArrayIdSet - the actual arrays returned by this function are not yet known. + let typ = ObjectType::Function(ArrayIdSet(u32::MAX)); SSAFunction { entry_block: block_id, id, - node_id: ctx.push_function_id(id, ObjectType::Function), + node_id: ctx.push_function_id(id, typ), name: name.to_string(), arguments: Vec::new(), result_types: Vec::new(), @@ -179,32 +181,46 @@ impl IRGenerator { let function_body = self.program.take_function_body(func_id); let last_value = self.codegen_expression(&function_body)?; let return_values = last_value.to_node_ids(); + let mut returned_arrays = vec![]; func.result_types.clear(); - let return_values = try_vecmap(return_values, |mut return_id| { - let node_opt = self.context.try_get_node(return_id); - let typ = node_opt.map_or(ObjectType::NotAnObject, |node| node.get_type()); - - if let Some(ins) = self.context.try_get_instruction(return_id) { - if ins.operation.opcode() == Opcode::Results { - // n.b. this required for result instructions, but won't hurt if done for all i - let new_var = node::Variable { - id: NodeId::dummy(), - obj_type: typ, - name: format!("return_{}", return_id.0.into_raw_parts().0), - root: None, - def: None, - witness: None, - parent_block: self.context.current_block, - }; - let b_id = self.context.add_variable(new_var, None); - let b_id1 = self.context.handle_assign(b_id, None, return_id)?; - return_id = ssa_form::get_current_value(&mut self.context, b_id1); + let return_values = + try_vecmap(return_values.into_iter().enumerate(), |(i, mut return_id)| { + let node_opt = self.context.try_get_node(return_id); + let typ = node_opt.map_or(ObjectType::NotAnObject, |node| { + let typ = node.get_type(); + if let ObjectType::Pointer(array_id) = typ { + returned_arrays.push((array_id, i as u32)); + } + typ + }); + + if let Some(ins) = self.context.try_get_instruction(return_id) { + if ins.operation.opcode() == Opcode::Results { + // n.b. this required for result instructions, but won't hurt if done for all i + let new_var = node::Variable { + id: NodeId::dummy(), + obj_type: typ, + name: format!("return_{}", return_id.0.into_raw_parts().0), + root: None, + def: None, + witness: None, + parent_block: self.context.current_block, + }; + let b_id = self.context.add_variable(new_var, None); + let b_id1 = self.context.handle_assign(b_id, None, return_id)?; + return_id = ssa_form::get_current_value(&mut self.context, b_id1); + } } - } - func.result_types.push(typ); - Ok::(return_id) - })?; + func.result_types.push(typ); + Ok::(return_id) + })?; + + let array_set_id = self.context.push_array_set(returned_arrays); + let function_type = ObjectType::Function(array_set_id); + + // Push a standard NodeId for this FuncId + self.context.set_function_type(func_id, func.node_id, function_type); self.context.new_instruction( node::Operation::Return(return_values), @@ -216,7 +232,7 @@ impl IRGenerator { self.context.current_block = current_block; self.function_context = current_function; - Ok(ObjectType::Function) + Ok(function_type) } fn create_function_parameter(&mut self, id: LocalId, typ: &Type, name: &str) -> Vec { @@ -238,46 +254,30 @@ impl IRGenerator { self.call_low_level(opcode, arguments) } else { let return_types = call.return_type.flatten().into_iter().enumerate(); + let returned_arrays = self.context.get_returned_arrays(func); let predicate = AssumptionId::dummy(); let location = call.location; - let call = Operation::Call { func, - arguments: arguments.clone(), - returned_arrays: vec![], + arguments, + returned_arrays: returned_arrays.clone(), predicate, location, }; - let call_instruction = self.context.new_instruction(call, ObjectType::NotAnObject)?; - let mut returned_arrays = vec![]; - let result_ids = try_vecmap(return_types, |(i, typ)| { + try_vecmap(return_types, |(i, typ)| { let result = Operation::Result { call_instruction, index: i as u32 }; let typ = match typ { - Type::Array(_, elem_type) => { - let elem_type = self.context.convert_type(&elem_type); - - // FIXME: We crash in anchor.rs if the user tries to use an index - // above this temporary length. - let max_array_size = 100_000; - - let array_id = - self.context.new_array("", elem_type, max_array_size, None).1; - returned_arrays.push((array_id, i as u32)); - ObjectType::Pointer(array_id) + Type::Array(_, _) => { + let id = returned_arrays.iter().find(|(_, index)| *index == i as u32); + ObjectType::Pointer(id.unwrap().0) } other => self.context.convert_type(&other), }; self.context.new_instruction(result, typ) - }); - - // Update the call instruction with the fresh array ids in returned_arrays - self.context.get_mut_instruction(call_instruction).operation = - Operation::Call { func, arguments, returned_arrays, predicate, location }; - - result_ids + }) } } diff --git a/crates/noirc_evaluator/src/ssa/node.rs b/crates/noirc_evaluator/src/ssa/node.rs index a77f2674daf..ae4a2641e4a 100644 --- a/crates/noirc_evaluator/src/ssa/node.rs +++ b/crates/noirc_evaluator/src/ssa/node.rs @@ -191,12 +191,15 @@ pub enum ObjectType { Signed(u32), //bit size Pointer(ArrayId), - Function, + Function(ArrayIdSet), // Function with a set of arrays that may be returned //TODO big_int //TODO floats NotAnObject, //not an object } +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +pub struct ArrayIdSet(pub u32); + #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum NumericType { Signed(u32), @@ -224,7 +227,7 @@ impl ObjectType { ObjectType::Signed(c) => *c, ObjectType::Unsigned(c) => *c, ObjectType::Pointer(_) => 0, - ObjectType::Function => 0, + ObjectType::Function(_) => 0, } }