Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Add Instruction::MakeArray to SSA #6071

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
dcb90c3
Remove Value::Array, add Instruction::MakeArray
jfecher Sep 11, 2024
76d036d
Updates
jfecher Sep 13, 2024
2141f47
Merge branch 'master' into jf/make-array
jfecher Sep 16, 2024
49bd056
Fix rust errors
jfecher Sep 16, 2024
1199b6f
Remove acir-gen TODO
jfecher Sep 17, 2024
734bee7
Fix SSA tests
jfecher Sep 17, 2024
9bc18aa
Merge branch 'master' into jf/make-array
TomAFrench Sep 25, 2024
58473ba
Merge branch 'master' into jf/make-array
TomAFrench Sep 30, 2024
5d47a43
chore: replace creation of array constants with `insert_make_array`
TomAFrench Sep 30, 2024
96c9a25
.
TomAFrench Sep 30, 2024
fd3e43a
Merge branch 'master' into jf/make-array
TomAFrench Oct 6, 2024
6df633a
chore: fix merge
TomAFrench Oct 7, 2024
a264dda
chore: update test to use make_array
TomAFrench Oct 7, 2024
06ada86
Merge branch 'master' into jf/make-array
jfecher Oct 8, 2024
94ed8cc
Fix die & cache make_array in unrolling
jfecher Oct 9, 2024
5b84a27
Resolve ValueIds in make array cache
jfecher Oct 9, 2024
0e1aec8
Fix evaluator tests
jfecher Oct 9, 2024
ecfe668
Merge branch 'master' into jf/make-array
jfecher Oct 9, 2024
45c1202
Fix test from master merge
jfecher Oct 9, 2024
eefc344
Fix test
jfecher Oct 9, 2024
c335fb6
chore: fix updates to `constant_array_deduplication` test
TomAFrench Oct 9, 2024
65c2e38
Update compiler/noirc_evaluator/src/ssa/ir/instruction.rs
jfecher Oct 9, 2024
0dd3b83
Remove comments
jfecher Oct 9, 2024
3f2e4ef
Update compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs
jfecher Oct 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 37 additions & 40 deletions compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -759,6 +759,43 @@ impl<'block> BrilligBlock<'block> {
Instruction::IfElse { .. } => {
unreachable!("IfElse instructions should not be possible in brillig")
}
Instruction::MakeArray { elements: array, typ } => {
let value_id = dfg.instruction_results(instruction_id)[0];
if !self.variables.is_allocated(&value_id) {
let new_variable = self.variables.define_variable(
self.function_context,
self.brillig_context,
value_id,
dfg,
);

// Initialize the variable
match new_variable {
BrilligVariable::BrilligArray(brillig_array) => {
self.brillig_context.codegen_initialize_array(brillig_array);
}
BrilligVariable::BrilligVector(vector) => {
let size = self
.brillig_context
.make_usize_constant_instruction(array.len().into());
self.brillig_context.codegen_initialize_vector(vector, size);
self.brillig_context.deallocate_single_addr(size);
}
_ => unreachable!(
"ICE: Cannot initialize array value created as {new_variable:?}"
),
};

// Write the items
let items_pointer = self
.brillig_context
.codegen_make_array_or_vector_items_pointer(new_variable);

self.initialize_constant_array(array, typ, dfg, items_pointer);

self.brillig_context.deallocate_register(items_pointer);
}
}
};

let dead_variables = self
Expand Down Expand Up @@ -1566,46 +1603,6 @@ impl<'block> BrilligBlock<'block> {
new_variable
}
}
Value::Array { array, typ } => {
if self.variables.is_allocated(&value_id) {
self.variables.get_allocation(self.function_context, value_id, dfg)
} else {
let new_variable = self.variables.define_variable(
self.function_context,
self.brillig_context,
value_id,
dfg,
);

// Initialize the variable
match new_variable {
BrilligVariable::BrilligArray(brillig_array) => {
self.brillig_context.codegen_initialize_array(brillig_array);
}
BrilligVariable::BrilligVector(vector) => {
let size = self
.brillig_context
.make_usize_constant_instruction(array.len().into());
self.brillig_context.codegen_initialize_vector(vector, size);
self.brillig_context.deallocate_single_addr(size);
}
_ => unreachable!(
"ICE: Cannot initialize array value created as {new_variable:?}"
),
};

// Write the items
let items_pointer = self
.brillig_context
.codegen_make_array_or_vector_items_pointer(new_variable);

self.initialize_constant_array(array, typ, dfg, items_pointer);

self.brillig_context.deallocate_register(items_pointer);

new_variable
}
}
Value::Function(_) => {
// For the debugger instrumentation we want to allow passing
// around values representing function pointers, even though
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,7 @@ impl ConstantAllocation {
}
if let Some(terminator_instruction) = block.terminator() {
terminator_instruction.for_each_value(|value_id| {
let variables = collect_variables_of_value(value_id, &func.dfg);
for variable in variables {
if let Some(variable) = collect_variables_of_value(value_id, &func.dfg) {
record_if_constant(block_id, variable, InstructionLocation::Terminator);
}
});
Expand Down Expand Up @@ -166,7 +165,7 @@ impl ConstantAllocation {
}

pub(crate) fn is_constant_value(id: ValueId, dfg: &DataFlowGraph) -> bool {
matches!(&dfg[dfg.resolve(id)], Value::NumericConstant { .. } | Value::Array { .. })
matches!(&dfg[dfg.resolve(id)], Value::NumericConstant { .. })
}

/// For a given function, finds all the blocks that are within loops
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,32 +45,19 @@ fn find_back_edges(
}

/// Collects the underlying variables inside a value id. It might be more than one, for example in constant arrays that are constructed with multiple vars.
jfecher marked this conversation as resolved.
Show resolved Hide resolved
pub(crate) fn collect_variables_of_value(value_id: ValueId, dfg: &DataFlowGraph) -> Vec<ValueId> {
pub(crate) fn collect_variables_of_value(
value_id: ValueId,
dfg: &DataFlowGraph,
) -> Option<ValueId> {
let value_id = dfg.resolve(value_id);
let value = &dfg[value_id];

match value {
Value::Instruction { .. } | Value::Param { .. } => {
vec![value_id]
}
// Literal arrays are constants, but might use variable values to initialize.
Value::Array { array, .. } => {
let mut value_ids = vec![value_id];

array.iter().for_each(|item_id| {
let underlying_ids = collect_variables_of_value(*item_id, dfg);
value_ids.extend(underlying_ids);
});

value_ids
}
Value::NumericConstant { .. } => {
vec![value_id]
Value::Instruction { .. } | Value::Param { .. } | Value::NumericConstant { .. } => {
Some(value_id)
}
// Functions are not variables in a defunctionalized SSA. Only constant function values should appear.
Value::ForeignFunction(_) | Value::Function(_) | Value::Intrinsic(..) => {
vec![]
}
Value::ForeignFunction(_) | Value::Function(_) | Value::Intrinsic(..) => None,
}
}

Expand Down
40 changes: 21 additions & 19 deletions compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -766,6 +766,12 @@ impl<'a> Context<'a> {
Instruction::IfElse { .. } => {
unreachable!("IfElse instruction remaining in acir-gen")
}
Instruction::MakeArray { elements, typ: _ } => {
let elements = elements.iter().map(|element| self.convert_value(*element, dfg));
let value = AcirValue::Array(elements.collect());
let result = dfg.instruction_results(instruction_id)[0];
self.ssa_values.insert(result, value);
}
}

self.acir_context.set_call_stack(CallStack::new());
Expand Down Expand Up @@ -1547,7 +1553,7 @@ impl<'a> Context<'a> {
if !already_initialized {
let value = &dfg[array];
match value {
Value::Array { .. } | Value::Instruction { .. } => {
Value::Instruction { .. } => {
let value = self.convert_value(array, dfg);
let array_typ = dfg.type_of_value(array);
let len = if !array_typ.contains_slice_element() {
Expand Down Expand Up @@ -1590,13 +1596,13 @@ impl<'a> Context<'a> {
match array_typ {
Type::Array(_, _) | Type::Slice(_) => {
match &dfg[array_id] {
Value::Array { array, .. } => {
for (i, value) in array.iter().enumerate() {
flat_elem_type_sizes.push(
self.flattened_slice_size(*value, dfg) + flat_elem_type_sizes[i],
);
}
}
// Value::Array { array, .. } => {
jfecher marked this conversation as resolved.
Show resolved Hide resolved
// for (i, value) in array.iter().enumerate() {
// flat_elem_type_sizes.push(
// self.flattened_slice_size(*value, dfg) + flat_elem_type_sizes[i],
// );
// }
// }
Value::Instruction { .. } | Value::Param { .. } => {
// An instruction representing the slice means it has been processed previously during ACIR gen.
// Use the previously defined result of an array operation to fetch the internal type information.
Expand Down Expand Up @@ -1729,13 +1735,13 @@ impl<'a> Context<'a> {
fn flattened_slice_size(&mut self, array_id: ValueId, dfg: &DataFlowGraph) -> usize {
let mut size = 0;
match &dfg[array_id] {
Value::Array { array, .. } => {
// The array is going to be the flattened outer array
// Flattened slice size from SSA value does not need to be multiplied by the len
for value in array {
size += self.flattened_slice_size(*value, dfg);
}
}
// Value::Array { array, .. } => {
jfecher marked this conversation as resolved.
Show resolved Hide resolved
// // The array is going to be the flattened outer array
// // Flattened slice size from SSA value does not need to be multiplied by the len
// for value in array {
// size += self.flattened_slice_size(*value, dfg);
// }
// }
Value::NumericConstant { .. } => {
size += 1;
}
Expand Down Expand Up @@ -1899,10 +1905,6 @@ impl<'a> Context<'a> {
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));
AcirValue::Array(elements.collect())
}
Value::Intrinsic(..) => todo!(),
Value::Function(function_id) => {
// This conversion is for debugging support only, to allow the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,8 @@ impl Context {
| Instruction::Load { .. }
| Instruction::Not(..)
| Instruction::Store { .. }
| Instruction::Truncate { .. } => {
| Instruction::Truncate { .. }
| Instruction::MakeArray { .. } => {
self.value_sets.push(instruction_arguments_and_results);
}

Expand Down Expand Up @@ -246,8 +247,7 @@ impl Context {
Value::ForeignFunction(..) => {
panic!("Should not be able to reach foreign function from non-brillig functions, {func_id} in function {}", function.name());
}
Value::Array { .. }
| Value::Instruction { .. }
Value::Instruction { .. }
| Value::NumericConstant { .. }
| Value::Param { .. } => {
panic!("At the point we are running disconnect there shouldn't be any other values as arguments")
Expand Down
11 changes: 4 additions & 7 deletions compiler/noirc_evaluator/src/ssa/function_builder/data_bus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,13 +161,10 @@ impl FunctionBuilder {
}
let len = databus.values.len();

let array = if len > 0 {
let array = self
.array_constant(databus.values, Type::Array(Arc::new(vec![Type::field()]), len));
Some(array)
} else {
None
};
let array = (len > 0).then(|| {
let array_type = Type::Array(Arc::new(vec![Type::field()]), len);
self.insert_make_array(databus.values, array_type)
});

DataBusBuilder {
index: 0,
Expand Down
22 changes: 12 additions & 10 deletions compiler/noirc_evaluator/src/ssa/function_builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,6 @@ impl FunctionBuilder {
self.numeric_constant(value.into(), Type::length_type())
}

/// Insert an array constant into the current function with the given element values.
pub(crate) fn array_constant(&mut self, elements: im::Vector<ValueId>, typ: Type) -> ValueId {
self.current_function.dfg.make_array(elements, typ)
}

/// 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)
Expand Down Expand Up @@ -340,6 +335,17 @@ impl FunctionBuilder {
self.insert_instruction(Instruction::EnableSideEffectsIf { condition }, None);
}

/// Insert a `make_array` instruction to create a new array or slice.
/// Returns the new array value. Expects `typ` to be an array or slice type.
pub(crate) fn insert_make_array(
&mut self,
elements: im::Vector<ValueId>,
typ: Type,
) -> ValueId {
assert!(matches!(typ, Type::Array(..) | Type::Slice(_)));
self.insert_instruction(Instruction::MakeArray { elements, typ }, None).first()
}

/// Terminates the current block with the given terminator instruction
/// if the current block does not already have a terminator instruction.
fn terminate_block_with(&mut self, terminator: TerminatorInstruction) {
Expand Down Expand Up @@ -495,7 +501,6 @@ mod tests {
instruction::{Endian, Intrinsic},
map::Id,
types::Type,
value::Value,
};

use super::FunctionBuilder;
Expand All @@ -517,10 +522,7 @@ mod tests {
let call_results =
builder.insert_call(to_bits_id, vec![input, length], result_types).into_owned();

let slice = match &builder.current_function.dfg[call_results[0]] {
Value::Array { array, .. } => array,
_ => panic!(),
};
let slice = builder.current_function.dfg.get_array_constant(call_results[0]).unwrap().0;
assert_eq!(slice[0], one);
assert_eq!(slice[1], one);
assert_eq!(slice[2], one);
Expand Down
20 changes: 11 additions & 9 deletions compiler/noirc_evaluator/src/ssa/ir/dfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -265,12 +265,6 @@ impl DataFlowGraph {
id
}

/// Create a new constant array value from the given elements
pub(crate) fn make_array(&mut self, array: im::Vector<ValueId>, typ: Type) -> ValueId {
assert!(matches!(typ, Type::Array(..) | Type::Slice(_)));
self.make_value(Value::Array { array, typ })
}

/// 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) {
Expand Down Expand Up @@ -457,8 +451,11 @@ impl DataFlowGraph {
/// Otherwise, this returns None.
pub(crate) fn get_array_constant(&self, value: ValueId) -> Option<(im::Vector<ValueId>, Type)> {
match &self.values[self.resolve(value)] {
Value::Instruction { instruction, .. } => match &self.instructions[*instruction] {
Instruction::MakeArray { elements, typ } => Some((elements.clone(), typ.clone())),
_ => None,
},
// Arrays are shared, so cloning them is cheap
Value::Array { array, typ } => Some((array.clone(), typ.clone())),
_ => None,
}
}
Expand Down Expand Up @@ -521,8 +518,13 @@ impl DataFlowGraph {
/// True if the given ValueId refers to a (recursively) constant value
pub(crate) fn is_constant(&self, argument: ValueId) -> bool {
match &self[self.resolve(argument)] {
Value::Instruction { .. } | Value::Param { .. } => false,
Value::Array { array, .. } => array.iter().all(|element| self.is_constant(*element)),
Value::Param { .. } => false,
Value::Instruction { instruction, .. } => match &self[*instruction] {
Instruction::MakeArray { elements, .. } => {
elements.iter().all(|element| self.is_constant(*element))
}
_ => false,
},
_ => true,
}
}
Expand Down
22 changes: 1 addition & 21 deletions compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,27 +36,7 @@ impl<'f> FunctionInserter<'f> {
value = self.function.dfg.resolve(value);
match self.values.get(&value) {
Some(value) => self.resolve(*value),
None => match &self.function.dfg[value] {
super::value::Value::Array { array, typ } => {
let array = array.clone();
let typ = typ.clone();
let new_array: im::Vector<ValueId> =
array.iter().map(|id| self.resolve(*id)).collect();

if let Some(fetched_value) =
self.const_arrays.get(&(new_array.clone(), typ.clone()))
{
return *fetched_value;
};

let new_array_clone = new_array.clone();
let new_id = self.function.dfg.make_array(new_array, typ.clone());
self.values.insert(value, new_id);
self.const_arrays.insert((new_array_clone, typ), new_id);
new_id
}
_ => value,
},
None => value,
}
}

Expand Down
Loading
Loading