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: Remove Value::Array in favor of Instruction::MakeArray #2494

Closed
wants to merge 37 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
dcb55df
fix: Implement new mem2reg pass (#2420)
jfecher Aug 25, 2023
c4a14f2
Support aliasing arrays in mem2reg
jfecher Aug 25, 2023
5467e56
Move last_stores in Block struct
jfecher Aug 25, 2023
f7e9374
Satisfy clippy
jfecher Aug 25, 2023
58f1187
Constant fold during mem2reg pass
jfecher Aug 28, 2023
fcc6df9
Formatting
jfecher Aug 28, 2023
a34319c
Add regression test
jfecher Aug 28, 2023
51762c4
Formatting
jfecher Aug 28, 2023
410bc26
Move last_stores in Block struct
jfecher Aug 25, 2023
1fc27a3
Apply fix from #2466
jfecher Aug 28, 2023
7e32383
Apply fix from #2466
jfecher Aug 28, 2023
be82fec
Avoid mapping ids to the same id
jfecher Aug 28, 2023
21cdfd0
Clear last stores after cloning predecessor block
jfecher Aug 28, 2023
61be2a9
Add last stores fix
jfecher Aug 28, 2023
0bccde7
Clippy
jfecher Aug 28, 2023
2d52b60
Merge branch 'master' into jf/mem2reg-array-aliasing
jfecher Aug 29, 2023
6634038
Merge branch 'jf/mem2reg-array-aliasing' into jf/mem2reg-constant-fol…
jfecher Aug 29, 2023
62cbc1a
Add make_array instruction
jfecher Aug 29, 2023
e284814
Progress on make_array
jfecher Aug 29, 2023
859cebb
More make_array updates
jfecher Aug 30, 2023
502ec06
Merge branch 'master' into jf/make-array
jfecher Aug 30, 2023
f6a7493
cargo fmt
jfecher Aug 30, 2023
8db7031
Update test
jfecher Aug 30, 2023
872626f
Merge branch 'master' into jf/make-array
jfecher Sep 1, 2023
84024e8
Merge branch 'master' into jf/make-array
jfecher Sep 1, 2023
79e1205
Cargo fmt
jfecher Sep 1, 2023
73e4757
Add typ argument to MakeArray
jfecher Sep 1, 2023
732e90d
Fix really weird error that I'm not sure why isn't failing on master
jfecher Sep 1, 2023
dddb9c8
Merge branch 'master' into jf/make-array
jfecher Sep 5, 2023
b4b4d8f
Use new collect_all_aliases function from master merge
jfecher Sep 5, 2023
76b77f8
Merge branch 'master' into jf/make-array
jfecher Sep 5, 2023
198e33b
Spent way too long debugging a make_block call in update_slice insert…
jfecher Sep 6, 2023
444828a
Merge branch 'master' into jf/make-array
jfecher Sep 6, 2023
752dca0
Revert changes to slices test
jfecher Sep 6, 2023
f61048b
Remove println
jfecher Sep 6, 2023
15815a4
Alter handle_make_array
jfecher Sep 6, 2023
a9bc0dd
Clippy caught errors in other crates
jfecher Sep 6, 2023
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
2 changes: 1 addition & 1 deletion crates/acvm_backend_barretenberg/src/proof_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ fn prepend_public_inputs(proof: Vec<u8>, public_inputs: Vec<FieldElement>) -> Ve
let public_inputs_bytes =
public_inputs.into_iter().flat_map(|assignment| assignment.to_be_bytes());

public_inputs_bytes.chain(proof.into_iter()).collect()
public_inputs_bytes.chain(proof).collect()
}

// TODO: See nargo/src/artifacts/mod.rs
Expand Down
6 changes: 3 additions & 3 deletions crates/nargo_cli/src/cli/backend_cmd/uninstall_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,14 @@ pub(crate) fn run(args: UninstallCommand) -> Result<(), CliError> {
};

if let Some(backend) = new_active_backend {
set_active_backend(backend)
set_active_backend(backend);
} else {
// We've deleted the last backend. Clear the active backend file to be recreated once we install a new one.
clear_active_backend()
clear_active_backend();
}
}

std::fs::remove_dir_all(&backends_directory().join(args.backend))
std::fs::remove_dir_all(backends_directory().join(args.backend))
.expect("backend directory should be deleted");

Ok(())
Expand Down
93 changes: 46 additions & 47 deletions crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,48 @@ impl<'block> BrilligBlock<'block> {
&dfg.type_of_value(*value),
);
}
Instruction::MakeArray { elements, typ: _ } => {
let result = dfg.instruction_results(instruction_id)[0];

let new_variable =
self.function_context.get_or_create_variable(self.brillig_context, result, dfg);

// Initialize the variable
let pointer = match new_variable {
RegisterOrMemory::HeapArray(heap_array) => {
self.brillig_context
.allocate_fixed_length_array(heap_array.pointer, elements.len());

heap_array.pointer
}
RegisterOrMemory::HeapVector(heap_vector) => {
self.brillig_context
.const_instruction(heap_vector.size, elements.len().into());
self.brillig_context
.allocate_array_instruction(heap_vector.pointer, heap_vector.size);

heap_vector.pointer
}
_ => unreachable!(
"ICE: Cannot initialize array value created as {new_variable:?}"
),
};

// Write the items

// Allocate a register for the iterator
let iterator_register = self.brillig_context.make_constant(0_usize.into());

for element_id in elements.iter() {
let element_variable = self.convert_ssa_value(*element_id, dfg);
// Store the item in memory
self.store_variable_in_array(pointer, iterator_register, element_variable);
// Increment the iterator
self.brillig_context.usize_op_in_place(iterator_register, BinaryIntOp::Add, 1);
}

self.brillig_context.deallocate_register(iterator_register);
}
Instruction::ArrayGet { array, index } => {
let result_ids = dfg.instruction_results(instruction_id);
let destination_variable =
Expand Down Expand Up @@ -508,7 +550,9 @@ impl<'block> BrilligBlock<'block> {
value_variable,
);
}
_ => todo!("ICE: Instruction not supported {instruction:?}"),
Instruction::EnableSideEffects { .. } => {
todo!("Unsupported instruction: {instruction:?}")
}
};

self.brillig_context.set_call_stack(CallStack::new());
Expand Down Expand Up @@ -979,52 +1023,7 @@ impl<'block> BrilligBlock<'block> {
self.brillig_context.const_instruction(register_index, (*constant).into());
new_variable
}
Value::Array { array, .. } => {
let new_variable = self.function_context.get_or_create_variable(
self.brillig_context,
value_id,
dfg,
);

// Initialize the variable
let pointer = match new_variable {
RegisterOrMemory::HeapArray(heap_array) => {
self.brillig_context
.allocate_fixed_length_array(heap_array.pointer, array.len());

heap_array.pointer
}
RegisterOrMemory::HeapVector(heap_vector) => {
self.brillig_context
.const_instruction(heap_vector.size, array.len().into());
self.brillig_context
.allocate_array_instruction(heap_vector.pointer, heap_vector.size);

heap_vector.pointer
}
_ => unreachable!(
"ICE: Cannot initialize array value created as {new_variable:?}"
),
};

// Write the items

// Allocate a register for the iterator
let iterator_register = self.brillig_context.make_constant(0_usize.into());

for element_id in array.iter() {
let element_variable = self.convert_ssa_value(*element_id, dfg);
// Store the item in memory
self.store_variable_in_array(pointer, iterator_register, element_variable);
// Increment the iterator
self.brillig_context.usize_op_in_place(iterator_register, BinaryIntOp::Add, 1);
}

self.brillig_context.deallocate_register(iterator_register);

new_variable
}
_ => {
Value::Function(_) | Value::Intrinsic(_) | Value::ForeignFunction(_) => {
todo!("ICE: Cannot convert value {value:?}")
}
}
Expand Down
69 changes: 33 additions & 36 deletions crates/noirc_evaluator/src/ssa/acir_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,21 @@ impl Context {
let acir_var = self.convert_numeric_value(*condition, dfg)?;
self.current_side_effects_enabled_var = acir_var;
}
Instruction::MakeArray { elements, typ: _ } => {
let elements = elements
.iter()
.map(|element| self.convert_value(*element, dfg))
.collect::<im::Vector<_>>();

let array = AcirValue::Array(elements.clone());

let elements: Vec<_> = elements.into_iter().collect();
let result = dfg.instruction_results(instruction_id)[0];
let block_id = self.block_id(&result);

self.initialize_array(block_id, elements.len(), Some(&elements))?;
self.define_result(dfg, instruction_id, array);
}
Instruction::ArrayGet { array, index } => {
self.handle_array_operation(
instruction_id,
Expand Down Expand Up @@ -607,19 +622,10 @@ impl Context {
let array = dfg.resolve(array);
let block_id = self.block_id(&array);
if !self.initialized_arrays.contains(&block_id) {
match &dfg[array] {
Value::Array { array, .. } => {
let values: Vec<AcirValue> =
array.iter().map(|i| self.convert_value(*i, dfg)).collect();
self.initialize_array(block_id, array.len(), Some(&values))?;
}
_ => {
return Err(RuntimeError::UnInitialized {
name: "array".to_string(),
call_stack: self.acir_context.get_call_stack(),
})
}
}
return Err(RuntimeError::UnInitialized {
name: "array".to_string(),
call_stack: self.acir_context.get_call_stack(),
});
}

let index_var = self.convert_value(index, dfg).into_var()?;
Expand Down Expand Up @@ -669,19 +675,10 @@ impl Context {
// if not, we initialize it using the values from SSA
let already_initialized = self.initialized_arrays.contains(&block_id);
if !already_initialized {
match &dfg[array] {
Value::Array { array, .. } => {
let values: Vec<AcirValue> =
array.iter().map(|i| self.convert_value(*i, dfg)).collect();
self.initialize_array(block_id, array.len(), Some(&values))?;
}
_ => {
return Err(InternalError::General {
message: format!("Array {array} should be initialized"),
call_stack: self.acir_context.get_call_stack(),
})
}
}
return Err(InternalError::General {
message: format!("Array {array} should be initialized"),
call_stack: self.acir_context.get_call_stack(),
});
}

// Since array_set creates a new array, we create a new block ID for this
Expand Down Expand Up @@ -801,10 +798,6 @@ impl Context {
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(..) => unreachable!("ICE: All functions should have been inlined"),
Value::ForeignFunction(_) => unimplemented!(
Expand Down Expand Up @@ -1213,7 +1206,7 @@ mod tests {

use acvm::{
acir::{
circuit::Opcode,
circuit::{opcodes::BlockId, Opcode},
native_types::{Expression, Witness},
},
FieldElement,
Expand All @@ -1233,7 +1226,8 @@ mod tests {
fn returns_body_scoped_arrays() {
// fn main {
// b0():
// return [Field 1]
// v1 = make_array [Field 1]
// return v1
// }
let func_id = Id::test_new(0);
let mut builder = FunctionBuilder::new("func".into(), func_id, RuntimeType::Acir);
Expand All @@ -1242,7 +1236,7 @@ mod tests {

let element_type = Rc::new(vec![Type::field()]);
let array_type = Type::Array(element_type, 1);
let array = builder.array_constant(im::Vector::unit(one), array_type);
let array = builder.insert_make_array(im::Vector::unit(one), array_type);

builder.terminate_with_return(vec![array]);

Expand All @@ -1251,9 +1245,12 @@ mod tests {
let context = Context::new();
let mut acir = context.convert_ssa(ssa, Brillig::default(), &HashMap::default()).unwrap();

let expected_opcodes =
vec![Opcode::Arithmetic(&Expression::one() - &Expression::from(Witness(1)))];
let expected_opcodes = vec![
Opcode::Arithmetic(&Expression::one() - &Expression::from(Witness(1))),
Opcode::MemoryInit { block_id: BlockId(0), init: vec![Witness(1)] },
Opcode::Arithmetic(&Expression::one() - &Expression::from(Witness(2))),
Comment on lines +1248 to +1251
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure why this memory init is needed now. Did we need it before as well? Neither array is used dynamically.

];
assert_eq!(acir.take_opcodes(), expected_opcodes);
assert_eq!(acir.return_witnesses, vec![Witness(1)]);
assert_eq!(acir.return_witnesses, vec![Witness(2)]);
}
}
20 changes: 11 additions & 9 deletions crates/noirc_evaluator/src/ssa/function_builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,6 @@ 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>, 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 @@ -258,6 +253,15 @@ impl FunctionBuilder {
self.insert_instruction(Instruction::Call { func, arguments }, Some(result_types)).results()
}

/// Insert an instruction to create a new array of the given type
pub(crate) fn insert_make_array(
&mut self,
elements: im::Vector<ValueId>,
typ: Type,
) -> ValueId {
self.insert_instruction(Instruction::MakeArray { elements, typ }, None).first()
}

/// Insert an instruction to extract an element from an array
pub(crate) fn insert_array_get(
&mut self,
Expand Down Expand Up @@ -411,10 +415,8 @@ mod tests {
};
assert_eq!(slice_len, FieldElement::from(8_u128));

let slice = match &builder.current_function.dfg[call_results[1]] {
Value::Array { array, .. } => array,
_ => panic!(),
};
let slice = builder.current_function.dfg.get_array_constant(call_results[1]).unwrap().0;

assert_eq!(slice[0], one);
assert_eq!(slice[1], one);
assert_eq!(slice[2], one);
Expand Down
21 changes: 11 additions & 10 deletions crates/noirc_evaluator/src/ssa/ir/dfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,12 +233,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 @@ -378,12 +372,19 @@ impl DataFlowGraph {
}
}

/// Returns the Value::Array associated with this ValueId if it refers to an array constant.
/// Otherwise, this returns None.
/// Returns the array associated with this ValueId if it refers to the result of a make_array
/// instruction. Otherwise, this returns None.
pub(crate) fn get_array_constant(&self, value: ValueId) -> Option<(im::Vector<ValueId>, Type)> {
match &self.values[self.resolve(value)] {
// Arrays are shared, so cloning them is cheap
Value::Array { array, typ } => Some((array.clone(), typ.clone())),
Value::Instruction { instruction, .. } => {
match &self.instructions[*instruction] {
// Arrays are shared, so cloning them is cheap
Instruction::MakeArray { elements, typ } => {
Some((elements.clone(), typ.clone()))
}
_ => None,
}
}
_ => None,
}
}
Expand Down
24 changes: 3 additions & 21 deletions crates/noirc_evaluator/src/ssa/ir/function_inserter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,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 = array.iter().map(|id| self.resolve(*id)).collect();
let new_id = self.function.dfg.make_array(new_array, typ);
self.values.insert(value, new_id);
new_id
}
_ => value,
},
None => value,
}
}

Expand Down Expand Up @@ -81,17 +71,9 @@ impl<'f> FunctionInserter<'f> {

/// Push a new instruction to the given block and return its new InstructionId.
/// If the instruction was simplified out of the program, None is returned.
pub(crate) fn push_instruction(
&mut self,
id: InstructionId,
block: BasicBlockId,
) -> Option<InstructionId> {
pub(crate) fn push_instruction(&mut self, id: InstructionId, block: BasicBlockId) {
let (instruction, location) = self.map_instruction(id);

match self.push_instruction_value(instruction, id, block, location) {
InsertInstructionResult::Results(new_id, _) => Some(new_id),
_ => None,
}
self.push_instruction_value(instruction, id, block, location);
}

pub(crate) fn push_instruction_value(
Expand Down
Loading